[RFC 0/2] drm/bridge: panel and chaining

[Sean Paul]

On Wed, Apr 30, 2014 at 11:38 AM, Rob Clark <robdclark at gmail.com> wrote:
> On Wed, Apr 30, 2014 at 11:13 AM, Ajay kumar <ajaynumb at gmail.com> wrote:
>>
>> On Wed, Apr 30, 2014 at 8:25 PM, AJAY KUMAR RAMAKRISHNA SHYMALAMMA <ajaykumar.rs at samsung.com> wrote:
>>>
>>>
>>>
>>>
>>>
>>> ------- Original Message -------
>>>
>>> Sender : Sean Paul<seanpaul at 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.
>

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 at lists.freedesktop.org
>>> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>
>>>
>>>
>>>
>>>
>>>
>>
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel