On Wed, Apr 30, 2014 at 8:25 PM, AJAY KUMAR RAMAKRISHNA SHYMALAMMA < ajaykumar.rs@samsung.com> wrote:
------- *Original Message* -------
*Sender* : Sean Paulseanpaul@chromium.org
*Date* : Apr 30, 2014 02:34 (GMT+05:30)
*Title* : Re: [RFC 0/2] drm/bridge: panel and chaining
On Tue, Apr 29, 2014 at 3:57 PM, Rob Clark wrote:
So I thought it would be easier to explain what I had in mind regarding Ajay's patchset (to add panel support) in patch form. Originally Thierry had some concerns with adding panel support in bridges ad-hoc. So this idea adds the support of chaining multiple bridges, one of which may be a panal adapter (maybe I should have called it drm_panel_adapter_bridge). There are a few rough edges and TODOs, it isn't trying to be complete yet.
So the one question is about that hunk I had to move in ptn3460 from pre_enable() to enable(). If that really needs to come before the encoder and after the panel, then we should go for a slightly different approach instead: add a 'struct drm_bridge *next' ptr in 'struct drm_bridge'. Then each bridge implementation is responsible to call the next in the chain (if not null) at the appropriate points. That gives a bit more flexibility to bridges to have something both pre and post the downstream bridge/panel in each of the pre/enable/disable/post steps.
Arbitrarily chaining bridges seems like a more robust solution to me than the composite bridge.
For the panel case, I wonder if we could change drm_bridge_init to accept a panel, then we could just chain the panel calls off the various places the bridge hooks are invoked in the drm layer.
This idea originated from Rob itself. He wanted to move out drm_panel calls from both ptn3460 and ps8622 drivers and he wanted them at a common place. But Daniel suggested that having a chain of bridges is good. That is how composite_bridge was formed.
I still think we are addressing a very simple problem in a complex manner. I tried testing this patchset on my board, with some tweaks(explained in PATCH 2/2]), and I could get it working. This code basically adds 3 bridge structures to handle the calls, but in actual hardware you can map them to only one bridge device. I am still not sure what's the problem in just having the panel calls around the bridge calls in drm core?
Feel free to ignore if this has already been explored on the other thread (which I haven't been following).
Sean
Rob Clark (2): drm/bridge: add composite and panel bridges drm/bridge/ptn3460: add panel support
drivers/gpu/drm/bridge/Makefile | 2 + drivers/gpu/drm/bridge/drm_bridge_util.c | 251
+++++++++++++++++++++++++++++++
drivers/gpu/drm/bridge/drm_bridge_util.h | 29 ++++ drivers/gpu/drm/bridge/ptn3460.c | 39 ++++- include/drm/bridge/ptn3460.h | 6 +- 5 files changed, 319 insertions(+), 8 deletions(-) create mode 100644 drivers/gpu/drm/bridge/drm_bridge_util.c create mode 100644 drivers/gpu/drm/bridge/drm_bridge_util.h
-- 1.9.0
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Apr 30, 2014 at 11:13 AM, Ajay kumar ajaynumb@gmail.com wrote:
On Wed, Apr 30, 2014 at 8:25 PM, AJAY KUMAR RAMAKRISHNA SHYMALAMMA ajaykumar.rs@samsung.com wrote:
------- Original Message -------
Sender : Sean Paulseanpaul@chromium.org
Date : Apr 30, 2014 02:34 (GMT+05:30)
Title : Re: [RFC 0/2] drm/bridge: panel and chaining
On Tue, Apr 29, 2014 at 3:57 PM, Rob Clark wrote:
So I thought it would be easier to explain what I had in mind regarding Ajay's patchset (to add panel support) in patch form. Originally Thierry had some concerns with adding panel support in bridges ad-hoc. So this idea adds the support of chaining multiple bridges, one of which may be a panal adapter (maybe I should have called it drm_panel_adapter_bridge). There are a few rough edges and TODOs, it isn't trying to be complete yet.
So the one question is about that hunk I had to move in ptn3460 from pre_enable() to enable(). If that really needs to come before the encoder and after the panel, then we should go for a slightly different approach instead: add a 'struct drm_bridge *next' ptr in 'struct drm_bridge'. Then each bridge implementation is responsible to call the next in the chain (if not null) at the appropriate points. That gives a bit more flexibility to bridges to have something both pre and post the downstream bridge/panel in each of the pre/enable/disable/post steps.
Arbitrarily chaining bridges seems like a more robust solution to me than the composite bridge.
For the panel case, I wonder if we could change drm_bridge_init to accept a panel, then we could just chain the panel calls off the various places the bridge hooks are invoked in the drm layer.
This idea originated from Rob itself. He wanted to move out drm_panel calls from both ptn3460 and ps8622 drivers and he wanted them at a common place. But Daniel suggested that having a chain of bridges is good. That is how composite_bridge was formed.
so I'm thinking that, given what Sean and others have said, that the chaining inside bridge implementation would give more flexibility. By which I mean:
struct drm_bridge { + struct drm_bridge *next; /* the next in the chain */ .... };
and then in each bridge implementation would do something like this for each fxn:
static void foo_bridge_pre_enable(...) { ... do stuff before ... + if (bridge->next) + bridge->next->pre_enable(...); ... do stuff after ... }
it would mean now all bridge fxns are now required, even if they only call next in chain.. we can toss in some helpers for that.
For dealing with panels, and this gets into Inki's proposal, I think we can just declare that panels themselves implement drm_bridge interface if needed. So drm_panel_funcs is for extra API's needed by connector (like get_modes()) and everything else is part of drm_bridge_funcs.
BR, -R
I still think we are addressing a very simple problem in a complex manner. I tried testing this patchset on my board, with some tweaks(explained in PATCH 2/2]), and I could get it working. This code basically adds 3 bridge structures to handle the calls, but in actual hardware you can map them to only one bridge device. I am still not sure what's the problem in just having the panel calls around the bridge calls in drm core?
Feel free to ignore if this has already been explored on the other thread (which I haven't been following).
Sean
Rob Clark (2): drm/bridge: add composite and panel bridges drm/bridge/ptn3460: add panel support
drivers/gpu/drm/bridge/Makefile | 2 + drivers/gpu/drm/bridge/drm_bridge_util.c | 251 +++++++++++++++++++++++++++++++ drivers/gpu/drm/bridge/drm_bridge_util.h | 29 ++++ drivers/gpu/drm/bridge/ptn3460.c | 39 ++++- include/drm/bridge/ptn3460.h | 6 +- 5 files changed, 319 insertions(+), 8 deletions(-) create mode 100644 drivers/gpu/drm/bridge/drm_bridge_util.c create mode 100644 drivers/gpu/drm/bridge/drm_bridge_util.h
-- 1.9.0
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Apr 30, 2014 at 11:38 AM, Rob Clark robdclark@gmail.com wrote:
On Wed, Apr 30, 2014 at 11:13 AM, Ajay kumar ajaynumb@gmail.com wrote:
On Wed, Apr 30, 2014 at 8:25 PM, AJAY KUMAR RAMAKRISHNA SHYMALAMMA ajaykumar.rs@samsung.com wrote:
------- Original Message -------
Sender : Sean Paulseanpaul@chromium.org
Date : Apr 30, 2014 02:34 (GMT+05:30)
Title : Re: [RFC 0/2] drm/bridge: panel and chaining
On Tue, Apr 29, 2014 at 3:57 PM, Rob Clark wrote:
So I thought it would be easier to explain what I had in mind regarding Ajay's patchset (to add panel support) in patch form. Originally Thierry had some concerns with adding panel support in bridges ad-hoc. So this idea adds the support of chaining multiple bridges, one of which may be a panal adapter (maybe I should have called it drm_panel_adapter_bridge). There are a few rough edges and TODOs, it isn't trying to be complete yet.
So the one question is about that hunk I had to move in ptn3460 from pre_enable() to enable(). If that really needs to come before the encoder and after the panel, then we should go for a slightly different approach instead: add a 'struct drm_bridge *next' ptr in 'struct drm_bridge'. Then each bridge implementation is responsible to call the next in the chain (if not null) at the appropriate points. That gives a bit more flexibility to bridges to have something both pre and post the downstream bridge/panel in each of the pre/enable/disable/post steps.
Arbitrarily chaining bridges seems like a more robust solution to me than the composite bridge.
For the panel case, I wonder if we could change drm_bridge_init to accept a panel, then we could just chain the panel calls off the various places the bridge hooks are invoked in the drm layer.
This idea originated from Rob itself. He wanted to move out drm_panel calls from both ptn3460 and ps8622 drivers and he wanted them at a common place. But Daniel suggested that having a chain of bridges is good. That is how composite_bridge was formed.
so I'm thinking that, given what Sean and others have said, that the chaining inside bridge implementation would give more flexibility. By which I mean:
struct drm_bridge {
- struct drm_bridge *next; /* the next in the chain */ ....
};
and then in each bridge implementation would do something like this for each fxn:
static void foo_bridge_pre_enable(...) { ... do stuff before ...
- if (bridge->next)
bridge->next->pre_enable(...);}
it would mean now all bridge fxns are now required, even if they only call next in chain.. we can toss in some helpers for that.
For dealing with panels, and this gets into Inki's proposal, I think we can just declare that panels themselves implement drm_bridge interface if needed. So drm_panel_funcs is for extra API's needed by connector (like get_modes()) and everything else is part of drm_bridge_funcs.
So if we do this, we can add panels off the end (or wherever) of the chain transparently, masquerading as bridges? That sounds like a pretty good solution to me.
Sean
BR, -R
I still think we are addressing a very simple problem in a complex manner. I tried testing this patchset on my board, with some tweaks(explained in PATCH 2/2]), and I could get it working. This code basically adds 3 bridge structures to handle the calls, but in actual hardware you can map them to only one bridge device. I am still not sure what's the problem in just having the panel calls around the bridge calls in drm core?
Feel free to ignore if this has already been explored on the other thread (which I haven't been following).
Sean
Rob Clark (2): drm/bridge: add composite and panel bridges drm/bridge/ptn3460: add panel support
drivers/gpu/drm/bridge/Makefile | 2 + drivers/gpu/drm/bridge/drm_bridge_util.c | 251 +++++++++++++++++++++++++++++++ drivers/gpu/drm/bridge/drm_bridge_util.h | 29 ++++ drivers/gpu/drm/bridge/ptn3460.c | 39 ++++- include/drm/bridge/ptn3460.h | 6 +- 5 files changed, 319 insertions(+), 8 deletions(-) create mode 100644 drivers/gpu/drm/bridge/drm_bridge_util.c create mode 100644 drivers/gpu/drm/bridge/drm_bridge_util.h
-- 1.9.0
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Apr 30, 2014 at 1:46 PM, Sean Paul seanpaul@chromium.org wrote:
On Wed, Apr 30, 2014 at 11:38 AM, Rob Clark robdclark@gmail.com wrote:
On Wed, Apr 30, 2014 at 11:13 AM, Ajay kumar ajaynumb@gmail.com wrote:
On Wed, Apr 30, 2014 at 8:25 PM, AJAY KUMAR RAMAKRISHNA SHYMALAMMA ajaykumar.rs@samsung.com wrote:
------- Original Message -------
Sender : Sean Paulseanpaul@chromium.org
Date : Apr 30, 2014 02:34 (GMT+05:30)
Title : Re: [RFC 0/2] drm/bridge: panel and chaining
On Tue, Apr 29, 2014 at 3:57 PM, Rob Clark wrote:
So I thought it would be easier to explain what I had in mind regarding Ajay's patchset (to add panel support) in patch form. Originally Thierry had some concerns with adding panel support in bridges ad-hoc. So this idea adds the support of chaining multiple bridges, one of which may be a panal adapter (maybe I should have called it drm_panel_adapter_bridge). There are a few rough edges and TODOs, it isn't trying to be complete yet.
So the one question is about that hunk I had to move in ptn3460 from pre_enable() to enable(). If that really needs to come before the encoder and after the panel, then we should go for a slightly different approach instead: add a 'struct drm_bridge *next' ptr in 'struct drm_bridge'. Then each bridge implementation is responsible to call the next in the chain (if not null) at the appropriate points. That gives a bit more flexibility to bridges to have something both pre and post the downstream bridge/panel in each of the pre/enable/disable/post steps.
Arbitrarily chaining bridges seems like a more robust solution to me than the composite bridge.
For the panel case, I wonder if we could change drm_bridge_init to accept a panel, then we could just chain the panel calls off the various places the bridge hooks are invoked in the drm layer.
This idea originated from Rob itself. He wanted to move out drm_panel calls from both ptn3460 and ps8622 drivers and he wanted them at a common place. But Daniel suggested that having a chain of bridges is good. That is how composite_bridge was formed.
so I'm thinking that, given what Sean and others have said, that the chaining inside bridge implementation would give more flexibility. By which I mean:
struct drm_bridge {
- struct drm_bridge *next; /* the next in the chain */ ....
};
and then in each bridge implementation would do something like this for each fxn:
static void foo_bridge_pre_enable(...) { ... do stuff before ...
- if (bridge->next)
bridge->next->pre_enable(...);}
it would mean now all bridge fxns are now required, even if they only call next in chain.. we can toss in some helpers for that.
For dealing with panels, and this gets into Inki's proposal, I think we can just declare that panels themselves implement drm_bridge interface if needed. So drm_panel_funcs is for extra API's needed by connector (like get_modes()) and everything else is part of drm_bridge_funcs.
So if we do this, we can add panels off the end (or wherever) of the chain transparently, masquerading as bridges? That sounds like a pretty good solution to me.
yup, that is pretty much what I'm thinking. Some difference in implementation details, but I think it covers what Inki wants too
BR, -R
Sean
BR, -R
I still think we are addressing a very simple problem in a complex manner. I tried testing this patchset on my board, with some tweaks(explained in PATCH 2/2]), and I could get it working. This code basically adds 3 bridge structures to handle the calls, but in actual hardware you can map them to only one bridge device. I am still not sure what's the problem in just having the panel calls around the bridge calls in drm core?
Feel free to ignore if this has already been explored on the other thread (which I haven't been following).
Sean
Rob Clark (2): drm/bridge: add composite and panel bridges drm/bridge/ptn3460: add panel support
drivers/gpu/drm/bridge/Makefile | 2 + drivers/gpu/drm/bridge/drm_bridge_util.c | 251 +++++++++++++++++++++++++++++++ drivers/gpu/drm/bridge/drm_bridge_util.h | 29 ++++ drivers/gpu/drm/bridge/ptn3460.c | 39 ++++- include/drm/bridge/ptn3460.h | 6 +- 5 files changed, 319 insertions(+), 8 deletions(-) create mode 100644 drivers/gpu/drm/bridge/drm_bridge_util.c create mode 100644 drivers/gpu/drm/bridge/drm_bridge_util.h
-- 1.9.0
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wednesday, April 30, 2014, Rob Clark robdclark@gmail.com wrote:
On Wed, Apr 30, 2014 at 11:13 AM, Ajay kumar <ajaynumb@gmail.comjavascript:;> wrote:
On Wed, Apr 30, 2014 at 8:25 PM, AJAY KUMAR RAMAKRISHNA SHYMALAMMA <
ajaykumar.rs@samsung.com javascript:;> wrote:
------- Original Message -------
Sender : Sean Paul<seanpaul@chromium.org javascript:;>
Date : Apr 30, 2014 02:34 (GMT+05:30)
Title : Re: [RFC 0/2] drm/bridge: panel and chaining
On Tue, Apr 29, 2014 at 3:57 PM, Rob Clark wrote:
So I thought it would be easier to explain what I had in mind
regarding
Ajay's patchset (to add panel support) in patch form. Originally
Thierry
had some concerns with adding panel support in bridges ad-hoc. So
this
idea adds the support of chaining multiple bridges, one of which may
be
a panal adapter (maybe I should have called it
drm_panel_adapter_bridge).
There are a few rough edges and TODOs, it isn't trying to be complete yet.
So the one question is about that hunk I had to move in ptn3460 from pre_enable() to enable(). If that really needs to come before the encoder and after the panel, then we should go for a slightly
different
approach instead: add a 'struct drm_bridge *next' ptr in 'struct drm_bridge'. Then each bridge implementation is responsible to call the next in the chain (if not null) at the appropriate points. That gives a bit more flexibility to bridges to have something both pre and post the downstream bridge/panel in each of the
pre/enable/disable/post
steps.
Arbitrarily chaining bridges seems like a more robust solution to me than the composite bridge.
For the panel case, I wonder if we could change drm_bridge_init to accept a panel, then we could just chain the panel calls off the various places the bridge hooks are invoked in the drm layer.
This idea originated from Rob itself. He wanted to move out drm_panel
calls
from both ptn3460 and ps8622 drivers and he wanted them at a common
place.
But Daniel suggested that having a chain of bridges is good. That is how composite_bridge was formed.
so I'm thinking that, given what Sean and others have said, that the chaining inside bridge implementation would give more flexibility. By which I mean:
struct drm_bridge {
- struct drm_bridge *next; /* the next in the chain */ ....
};
and then in each bridge implementation would do something like this for each fxn:
static void foo_bridge_pre_enable(...) { ... do stuff before ...
- if (bridge->next)
bridge->next->pre_enable(...);}
it would mean now all bridge fxns are now required, even if they only call next in chain.. we can toss in some helpers for that.
I don't think we would need new helpers or a struct bridge *next ptr.
This can be easily done with existing helpers itself. drm_bridge_init anyhow adds onto a common list of bridges - "bridge_list". We just need to stop calling bridge callbacks via encoder->bridge->funcs->xyz() and instead parse the bridge_list to get each of the bridge ptr in the list, and call their corresponding callbacks. The order of bridge chain would be the order in which drm_bridge_init was called.
For dealing with panels, and this gets into Inki's proposal, I think we can just declare that panels themselves implement drm_bridge interface if needed. So drm_panel_funcs is for extra API's needed by connector (like get_modes()) and everything else is part of drm_bridge_funcs.
BR, -R
I still think we are addressing a very simple problem in a complex
manner.
I tried testing this patchset on my board, with some tweaks(explained in
PATCH 2/2]),
and I could get it working. This code basically adds 3 bridge structures
to handle the calls,
but in actual hardware you can map them to only one bridge device. I am still not sure what's the problem in just having the panel calls
around
the bridge calls in drm core?
Feel free to ignore if this has already been explored on the other thread (which I haven't been following).
Sean
Rob Clark (2): drm/bridge: add composite and panel bridges drm/bridge/ptn3460: add panel support
drivers/gpu/drm/bridge/Makefile | 2 + drivers/gpu/drm/bridge/drm_bridge_util.c | 251
+++++++++++++++++++++++++++++++
drivers/gpu/drm/bridge/drm_bridge_util.h | 29 ++++ drivers/gpu/drm/bridge/ptn3460.c | 39 ++++- include/drm/bridge/ptn3460.h | 6 +- 5 files changed, 319 insertions(+), 8 deletions(-) create mode 100644 drivers/gpu/drm/bridge/drm_bridge_util.c create mode 100644 drivers/gpu/drm/bridge/drm_bridge_util.h
-- 1.9.0
dri-devel mailing list dri-devel@lists.freedesktop.org javascript:; http://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org
-
Ajay kumar -
Rob Clark -
Sean Paul