[RFC V2 0/3] drm/bridge: panel and chaining

[Ajay kumar]

On Thu, May 15, 2014 at 6:04 PM, Daniel Vetter  wrote:
> On Thu, May 15, 2014 at 12:32:58PM +0200, Thierry Reding wrote:
>> On Fri, May 09, 2014 at 09:59:34AM -0400, Rob Clark wrote:
>> > On Fri, May 9, 2014 at 5:08 AM, Andrzej Hajda  
>> > wrote:
>> [...]
>> > > 4. And the last thing, it is more about the concept/design. drm_bridge,
>> > > drm_hw_block suggests that those interfaces describes the whole device:
>> > > bridge, panel, whatever.
>> >
>> > hmm, I don't think this is the case.  I can easily see things like:
>> >
>> >   struct foo_panel {
>> > struct drm_panel base;
>> > struct drm_bridge bridge;
>> > ...
>> >   }
>> >
>> > where a panel implementation implements both panel and bridge.  In
>> > fact that is kinda what I was encouraging.
>>
>> For lack of a better entry point into the discussion, let me pick this
>> example. It seems like in general we all agree that the basic structure
>> would have to be something like this:
>>
>>   CRTC -> encoder -> bridge [ -> bridge ... ] -> panel
>>
>> Where panel could optionally be a bridge/panel hybrid as Rob pointed
>> out. I'm not sure if there's panels like this, but I suspect the use
>> case would be where a panel had built-in controls to modify the image in
>> some fashion (brightness, saturation, ...). I think those things exist
>> in DCS for example.
>>
>> What's missing here, and if I understand correctly what Sean said about
>> the connector type, what we need to solve is how to wire up the
>> connector so that it reflects the correct type. So the above would have
>> to become something like this:
>>
>>   CRTC -> encoder -> bridge [ -> bridge ... ] -> panel
>>   connector -^
>
> This is kinda always how I've thought it should play out: The last item in
> the chain actually implements the drm connector, everyone else just
> ignores it.
The last bridge or the panel?


>> One of the problems with that approach is that panel is more than just a
>> video sink. It also controls the panel (and optionally backlight) power.
>> The standard DRM connector helpers don't work well in that case, because
>> drm_helper_connector_dpms() forwards changes to the CRTC and encoder,
>> though that could possibly be solved by wrapping it in a small custom
>> implementation that also controls the panel. Anyway, that's really just
>> an implementation detail.
Why did this discussion come up? Are we going to call panel controls
from a common layer?


> I don't see an issue with the dpms really. At worst the overall kms driver
> (which provides the crtc and encoder implementation) needs to pass a
> suitable dpms implementation for the drm_bridge to use in the connector.
> Or we could just move this vfunc into the core kms funtion table since
> everyone uses the same (well except i915, but that's a different story).
>
> dpms changes would then still be driven through the crtc -> encoder ->
> bridge ... chain.
>
>> So once a complete chain from encoder to panel has been created, we need
>> a way to find the appropriate connector type. Perhaps to achieve that it
>> would be useful to instantiate the connector by the bridge that's
>> connected to the panel. So essentially something like this:
>>
>>   struct drm_bridge {
>>   /* to allow chaining */
>>   struct drm_bridge *next;
>>   /* for bridges directly connected to a panel or monitor */
>>   struct drm_connector *connector;
>>   /* for bridges directly connected to a panel */
>>   struct drm_panel *panel;
>
> Imo these two shouldn't be here, but instead should be embedded into the
> overall structure the drm_bridge driver is using.
>>
>>   ...
>>   };
>>
>> To make this work in a generic way, I think we're missing one bridge
>> instance. Currently the bridge in an encoder is completely optional, so
>> if there is no bridge in the system this needs to be special cased. One
>> way to unify this would be to make drm_encoder implement drm_bridge, or
>> an alternative would be to make drm_panel implement a bridge. Both can
>> possibly be dummies stubbed out with simple helpers.
>
> Yeah, imo if the panel isn't just a dumb thing but needs to actively take
> part in the modeset dance, it simply needs to implement itself as a
> drm_bridge+panel combo and do the magic in the various hooks provided.

And, what does one mean when a real panel should implement both
drm_panel and drm_bridge?
who will call drm_bridge->funcs for the panel?, and
who will call drm_panel->funcs for the panel?
Can someone please elaborate this with existing implementation details
for bridge and panel? I am really getting confused.

>> I wonder if this would have any consequences on Dave's DP MST work,
>> since presumably an MST hub could be considered a bridge. In that case I
>> guess there would need to be support for multiple "next" bridges,
>> perhaps a simple doubly-linked list instead of a next 

[RFC V2 0/3] drm/bridge: panel and chaining

[Daniel Vetter]

On Thu, May 15, 2014 at 12:32:58PM +0200, Thierry Reding wrote:
> On Fri, May 09, 2014 at 09:59:34AM -0400, Rob Clark wrote:
> > On Fri, May 9, 2014 at 5:08 AM, Andrzej Hajda  
> > wrote:
> [...]
> > > 4. And the last thing, it is more about the concept/design. drm_bridge,
> > > drm_hw_block suggests that those interfaces describes the whole device:
> > > bridge, panel, whatever.
> > 
> > hmm, I don't think this is the case.  I can easily see things like:
> > 
> >   struct foo_panel {
> > struct drm_panel base;
> > struct drm_bridge bridge;
> > ...
> >   }
> > 
> > where a panel implementation implements both panel and bridge.  In
> > fact that is kinda what I was encouraging.
> 
> For lack of a better entry point into the discussion, let me pick this
> example. It seems like in general we all agree that the basic structure
> would have to be something like this:
> 
>   CRTC -> encoder -> bridge [ -> bridge ... ] -> panel
> 
> Where panel could optionally be a bridge/panel hybrid as Rob pointed
> out. I'm not sure if there's panels like this, but I suspect the use
> case would be where a panel had built-in controls to modify the image in
> some fashion (brightness, saturation, ...). I think those things exist
> in DCS for example.
> 
> What's missing here, and if I understand correctly what Sean said about
> the connector type, what we need to solve is how to wire up the
> connector so that it reflects the correct type. So the above would have
> to become something like this:
> 
>   CRTC -> encoder -> bridge [ -> bridge ... ] -> panel
>   connector -^

This is kinda always how I've thought it should play out: The last item in
the chain actually implements the drm connector, everyone else just
ignores it.

> One of the problems with that approach is that panel is more than just a
> video sink. It also controls the panel (and optionally backlight) power.
> The standard DRM connector helpers don't work well in that case, because
> drm_helper_connector_dpms() forwards changes to the CRTC and encoder,
> though that could possibly be solved by wrapping it in a small custom
> implementation that also controls the panel. Anyway, that's really just
> an implementation detail.

I don't see an issue with the dpms really. At worst the overall kms driver
(which provides the crtc and encoder implementation) needs to pass a
suitable dpms implementation for the drm_bridge to use in the connector.
Or we could just move this vfunc into the core kms funtion table since
everyone uses the same (well except i915, but that's a different story).

dpms changes would then still be driven through the crtc -> encoder ->
bridge ... chain.

> So once a complete chain from encoder to panel has been created, we need
> a way to find the appropriate connector type. Perhaps to achieve that it
> would be useful to instantiate the connector by the bridge that's
> connected to the panel. So essentially something like this:
> 
>   struct drm_bridge {
>   /* to allow chaining */
>   struct drm_bridge *next;
>   /* for bridges directly connected to a panel or monitor */
>   struct drm_connector *connector;
>   /* for bridges directly connected to a panel */
>   struct drm_panel *panel;

Imo these two shouldn't be here, but instead should be embedded into the
overall structure the drm_bridge driver is using.
> 
>   ...
>   };
> 
> To make this work in a generic way, I think we're missing one bridge
> instance. Currently the bridge in an encoder is completely optional, so
> if there is no bridge in the system this needs to be special cased. One
> way to unify this would be to make drm_encoder implement drm_bridge, or
> an alternative would be to make drm_panel implement a bridge. Both can
> possibly be dummies stubbed out with simple helpers.

Yeah, imo if the panel isn't just a dumb thing but needs to actively take
part in the modeset dance, it simply needs to implement itself as a
drm_bridge+panel combo and do the magic in the various hooks provided.

> I wonder if this would have any consequences on Dave's DP MST work,
> since presumably an MST hub could be considered a bridge. In that case I
> guess there would need to be support for multiple "next" bridges,
> perhaps a simple doubly-linked list instead of a next pointer. The first
> bridge would then be used to model the hub's input and child bridges
> would be used to model each
> of the outputs.

DP is standardize. No need to wrestle the complexity through a drm_bridge,
since code reuse anywhere else (non-DP) will be know to be zero. And for
all the guys implementing a DP output can simply use the helpers. So I
don't see an upside of this idea.

Imo we should restrict drm_bridge to all the crazy transcoder chips/blocks
commonly found on SoCs which all do non-standard stuff and where we
actually have an established or anticipated need for 

[RFC V2 0/3] drm/bridge: panel and chaining

[Thierry Reding]

On Fri, May 09, 2014 at 09:59:34AM -0400, Rob Clark wrote:
> On Fri, May 9, 2014 at 5:08 AM, Andrzej Hajda  wrote:
[...]
> > 4. And the last thing, it is more about the concept/design. drm_bridge,
> > drm_hw_block suggests that those interfaces describes the whole device:
> > bridge, panel, whatever.
> 
> hmm, I don't think this is the case.  I can easily see things like:
> 
>   struct foo_panel {
> struct drm_panel base;
> struct drm_bridge bridge;
> ...
>   }
> 
> where a panel implementation implements both panel and bridge.  In
> fact that is kinda what I was encouraging.

For lack of a better entry point into the discussion, let me pick this
example. It seems like in general we all agree that the basic structure
would have to be something like this:

CRTC -> encoder -> bridge [ -> bridge ... ] -> panel

Where panel could optionally be a bridge/panel hybrid as Rob pointed
out. I'm not sure if there's panels like this, but I suspect the use
case would be where a panel had built-in controls to modify the image in
some fashion (brightness, saturation, ...). I think those things exist
in DCS for example.

What's missing here, and if I understand correctly what Sean said about
the connector type, what we need to solve is how to wire up the
connector so that it reflects the correct type. So the above would have
to become something like this:

CRTC -> encoder -> bridge [ -> bridge ... ] -> panel
connector -^

One of the problems with that approach is that panel is more than just a
video sink. It also controls the panel (and optionally backlight) power.
The standard DRM connector helpers don't work well in that case, because
drm_helper_connector_dpms() forwards changes to the CRTC and encoder,
though that could possibly be solved by wrapping it in a small custom
implementation that also controls the panel. Anyway, that's really just
an implementation detail.

So once a complete chain from encoder to panel has been created, we need
a way to find the appropriate connector type. Perhaps to achieve that it
would be useful to instantiate the connector by the bridge that's
connected to the panel. So essentially something like this:

struct drm_bridge {
/* to allow chaining */
struct drm_bridge *next;
/* for bridges directly connected to a panel or monitor */
struct drm_connector *connector;
/* for bridges directly connected to a panel */
struct drm_panel *panel;

...
};

To make this work in a generic way, I think we're missing one bridge
instance. Currently the bridge in an encoder is completely optional, so
if there is no bridge in the system this needs to be special cased. One
way to unify this would be to make drm_encoder implement drm_bridge, or
an alternative would be to make drm_panel implement a bridge. Both can
possibly be dummies stubbed out with simple helpers.

I wonder if this would have any consequences on Dave's DP MST work,
since presumably an MST hub could be considered a bridge. In that case I
guess there would need to be support for multiple "next" bridges,
perhaps a simple doubly-linked list instead of a next pointer. The first
bridge would then be used to model the hub's input and child bridges
would be used to model each
of the outputs.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

[RFC V2 0/3] drm/bridge: panel and chaining

[Thierry Reding]

On Thu, May 08, 2014 at 02:28:12PM -0400, Rob Clark wrote:
> On Thu, May 8, 2014 at 7:57 AM, Inki Dae  wrote:
[...]
> > And more thing, we would need to consider image enhancer device placed
> > between crtc and connector/encoder devices. And it'd better to rename
> > drm_hw_block to drm_bridge, and existing drm_bridge relevant codes
> > should be removed.
> 
> I don't object to changing the name to hw_block or something else.
> Although to avoid introducing too much confusion in this discussion,
> for now I am calling it bridge/drm_bridge_funcs/etc for now ;-)

FWIW, I think the name drm_bridge is fine. It describes a device that
takes a video signal as an input and outputs a video signal, possibly
using a different encoding.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

[RFC V2 0/3] drm/bridge: panel and chaining

[Andrzej Hajda]

On 05/12/2014 06:00 PM, Sean Paul wrote:
> On Mon, May 12, 2014 at 3:06 AM, Andrzej Hajda  wrote:
>> On 05/09/2014 05:05 PM, Ajay kumar wrote:
>>> On Fri, May 9, 2014 at 7:29 PM, Rob Clark  wrote:
 On Fri, May 9, 2014 at 5:08 AM, Andrzej Hajda  
 wrote:
> On 05/08/2014 08:24 PM, Rob Clark wrote:
>> On Thu, May 8, 2014 at 2:41 AM, Andrzej Hajda  
>> wrote:
>>> On 05/05/2014 09:52 PM, Ajay Kumar wrote:
 This patchset is based on exynos-drm-next-todo branch of Inki Dae's 
 tree at:
 git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git

 I have just put up Rob's and Sean's idea of chaining up the bridges
 in code, and have implemented basic panel controls as a chained bridge.
 This works well with ptn3460 bridge chip on exynos5250-snow board.

 Still need to make use of standard list calls and figure out proper way
 of deleting the bridge chain. So, this is just a rough version.
>>> As I understand this patchset tries to solve two things:
>>> 1. Implement panel as drm_bridge, to ease support for hardware chains:
>>> Crtc -> Encoder -> Bridge -> Panel
>>> 2. Add support to drm_bridge chaining, to allow software chains:
>>> drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
>>>
>>> It is done using Russian doll schema, ops from the bridge calls the same
>>> ops from the next bridge and the next bridge ops can do the same.
>>>
>>> This schema means that all the bridges including the last one are seen
>>> from the drm core point of view as a one big drm_bridge. Additionally in
>>> this particular case, the first bridge (ptn3460) implements connector
>>> so it is hard to guess what is the location of the 2nd bridge in video
>>> stream chain, sometimes it is after the connector, sometimes before.
>>> All this is quite confusing.
>>>
>>> But if you look at the bridge from upstream video interface point of
>>> view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
>>> video input side acts as a panel. On the output side it expects a panel,
>>> lvds panel in this case.
>> tbh, this is mostly about what we call it.  Perhaps "bridge" isn't the
>> best name.. I wouldn't object to changing it.
>>
>> But my thinking was to leave in drm_panel_funcs things that are just
>> needed by the connector (get_modes().. and maybe some day we need
>> detect/etc).  Then leave everything else in drm_bridge_funcs.  A panel
>> could (if needed) implement both interfaces.
>>
>> That is basically the same as what you are proposing, but without
>> renaming bridge to panel ;-)
> Good to hear that. However there are points which are not clear for me.
> But first lets clarify names, I will use panel and bridge words
> to describe the hardware, and drm_panel, drm_bridge to describe the
> software interfaces.
>
> What bothers me:
> 1. You want to leave connector related callbacks in drm_panel and
> the rest in drm_bridge. In case of ptn3460 it does not work, ptn3460
> must implement connector internally because of this limitation. I guess
> it is quite typical bridge. This problem does not exists in case
> of using drm_panel for ptn3460.
>
> 2. drm_bridge is attached to the encoder, this and the callback order
> suggests the video data flow should be as below:
> drm_crtc -> drm_encoder [-> drm_bridge] -> drm_connector [-> drm_panel]
>
> ptn3460 implements drm_bridge and drm_connector so it suggests its
> drm_bridge should be the last one, so there should be no place to add
> lvds panel implemented as a drm_bridge after it, as it is done in this
> patchset.
>
> Additionally it clearly shows that there should be two categories of
> drm_bridges - non-terminal and terminal.
>
> 3. drm_dev uses all-or-nothing approach, ie. it will start only when all
> its components are up. It simplifies synchronization but is quite
> fragile - the whole drm will be down due to error in some of its 
> components.
> For this reason I prefer drm_panel as it is not real drm component
> it can be attached/detached to/from drm_connector anytime. I am not
> really sure but drm_bridge does not allow for that.
 So I do think we need to stick to this all-or-nothing approach for
 anything that is visible to userspace
 (drm_{plane,crtc,encoder,connector}).  We don't currently have a way
 to "hotplug" those so I don't see a real smooth upgrade path to add
 that in a backwards compatible way that won't cause problems with old
 userspace.

 But, that said, we have more flexibility with things not visible to
 userspace (drm_{panel,bridge}).  I'm not sure how much we want to
 allow things to be completely dynamic (we already have some hard
 enough locking 

[RFC V2 0/3] drm/bridge: panel and chaining

[Andrzej Hajda]

On 05/12/2014 02:45 PM, Rob Clark wrote:
> On Mon, May 12, 2014 at 3:06 AM, Andrzej Hajda  wrote:
>> On 05/09/2014 05:05 PM, Ajay kumar wrote:
>>> On Fri, May 9, 2014 at 7:29 PM, Rob Clark  wrote:
 On Fri, May 9, 2014 at 5:08 AM, Andrzej Hajda  
 wrote:
> On 05/08/2014 08:24 PM, Rob Clark wrote:
>> On Thu, May 8, 2014 at 2:41 AM, Andrzej Hajda  
>> wrote:
>>> On 05/05/2014 09:52 PM, Ajay Kumar wrote:
 This patchset is based on exynos-drm-next-todo branch of Inki Dae's 
 tree at:
 git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git

 I have just put up Rob's and Sean's idea of chaining up the bridges
 in code, and have implemented basic panel controls as a chained bridge.
 This works well with ptn3460 bridge chip on exynos5250-snow board.

 Still need to make use of standard list calls and figure out proper way
 of deleting the bridge chain. So, this is just a rough version.
>>> As I understand this patchset tries to solve two things:
>>> 1. Implement panel as drm_bridge, to ease support for hardware chains:
>>> Crtc -> Encoder -> Bridge -> Panel
>>> 2. Add support to drm_bridge chaining, to allow software chains:
>>> drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
>>>
>>> It is done using Russian doll schema, ops from the bridge calls the same
>>> ops from the next bridge and the next bridge ops can do the same.
>>>
>>> This schema means that all the bridges including the last one are seen
>>> from the drm core point of view as a one big drm_bridge. Additionally in
>>> this particular case, the first bridge (ptn3460) implements connector
>>> so it is hard to guess what is the location of the 2nd bridge in video
>>> stream chain, sometimes it is after the connector, sometimes before.
>>> All this is quite confusing.
>>>
>>> But if you look at the bridge from upstream video interface point of
>>> view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
>>> video input side acts as a panel. On the output side it expects a panel,
>>> lvds panel in this case.
>> tbh, this is mostly about what we call it.  Perhaps "bridge" isn't the
>> best name.. I wouldn't object to changing it.
>>
>> But my thinking was to leave in drm_panel_funcs things that are just
>> needed by the connector (get_modes().. and maybe some day we need
>> detect/etc).  Then leave everything else in drm_bridge_funcs.  A panel
>> could (if needed) implement both interfaces.
>>
>> That is basically the same as what you are proposing, but without
>> renaming bridge to panel ;-)
> Good to hear that. However there are points which are not clear for me.
> But first lets clarify names, I will use panel and bridge words
> to describe the hardware, and drm_panel, drm_bridge to describe the
> software interfaces.
>
> What bothers me:
> 1. You want to leave connector related callbacks in drm_panel and
> the rest in drm_bridge. In case of ptn3460 it does not work, ptn3460
> must implement connector internally because of this limitation. I guess
> it is quite typical bridge. This problem does not exists in case
> of using drm_panel for ptn3460.
>
> 2. drm_bridge is attached to the encoder, this and the callback order
> suggests the video data flow should be as below:
> drm_crtc -> drm_encoder [-> drm_bridge] -> drm_connector [-> drm_panel]
>
> ptn3460 implements drm_bridge and drm_connector so it suggests its
> drm_bridge should be the last one, so there should be no place to add
> lvds panel implemented as a drm_bridge after it, as it is done in this
> patchset.
>
> Additionally it clearly shows that there should be two categories of
> drm_bridges - non-terminal and terminal.
>
> 3. drm_dev uses all-or-nothing approach, ie. it will start only when all
> its components are up. It simplifies synchronization but is quite
> fragile - the whole drm will be down due to error in some of its 
> components.
> For this reason I prefer drm_panel as it is not real drm component
> it can be attached/detached to/from drm_connector anytime. I am not
> really sure but drm_bridge does not allow for that.
 So I do think we need to stick to this all-or-nothing approach for
 anything that is visible to userspace
 (drm_{plane,crtc,encoder,connector}).  We don't currently have a way
 to "hotplug" those so I don't see a real smooth upgrade path to add
 that in a backwards compatible way that won't cause problems with old
 userspace.

 But, that said, we have more flexibility with things not visible to
 userspace (drm_{panel,bridge}).  I'm not sure how much we want to
 allow things to be completely dynamic (we already have some hard
 enough locking 

[RFC V2 0/3] drm/bridge: panel and chaining

[Sean Paul]

On Mon, May 12, 2014 at 3:06 AM, Andrzej Hajda  wrote:
> On 05/09/2014 05:05 PM, Ajay kumar wrote:
>> On Fri, May 9, 2014 at 7:29 PM, Rob Clark  wrote:
>>> On Fri, May 9, 2014 at 5:08 AM, Andrzej Hajda  
>>> wrote:
 On 05/08/2014 08:24 PM, Rob Clark wrote:
> On Thu, May 8, 2014 at 2:41 AM, Andrzej Hajda  
> wrote:
>> On 05/05/2014 09:52 PM, Ajay Kumar wrote:
>>> This patchset is based on exynos-drm-next-todo branch of Inki Dae's 
>>> tree at:
>>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
>>>
>>> I have just put up Rob's and Sean's idea of chaining up the bridges
>>> in code, and have implemented basic panel controls as a chained bridge.
>>> This works well with ptn3460 bridge chip on exynos5250-snow board.
>>>
>>> Still need to make use of standard list calls and figure out proper way
>>> of deleting the bridge chain. So, this is just a rough version.
>> As I understand this patchset tries to solve two things:
>> 1. Implement panel as drm_bridge, to ease support for hardware chains:
>> Crtc -> Encoder -> Bridge -> Panel
>> 2. Add support to drm_bridge chaining, to allow software chains:
>> drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
>>
>> It is done using Russian doll schema, ops from the bridge calls the same
>> ops from the next bridge and the next bridge ops can do the same.
>>
>> This schema means that all the bridges including the last one are seen
>> from the drm core point of view as a one big drm_bridge. Additionally in
>> this particular case, the first bridge (ptn3460) implements connector
>> so it is hard to guess what is the location of the 2nd bridge in video
>> stream chain, sometimes it is after the connector, sometimes before.
>> All this is quite confusing.
>>
>> But if you look at the bridge from upstream video interface point of
>> view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
>> video input side acts as a panel. On the output side it expects a panel,
>> lvds panel in this case.
> tbh, this is mostly about what we call it.  Perhaps "bridge" isn't the
> best name.. I wouldn't object to changing it.
>
> But my thinking was to leave in drm_panel_funcs things that are just
> needed by the connector (get_modes().. and maybe some day we need
> detect/etc).  Then leave everything else in drm_bridge_funcs.  A panel
> could (if needed) implement both interfaces.
>
> That is basically the same as what you are proposing, but without
> renaming bridge to panel ;-)
 Good to hear that. However there are points which are not clear for me.
 But first lets clarify names, I will use panel and bridge words
 to describe the hardware, and drm_panel, drm_bridge to describe the
 software interfaces.

 What bothers me:
 1. You want to leave connector related callbacks in drm_panel and
 the rest in drm_bridge. In case of ptn3460 it does not work, ptn3460
 must implement connector internally because of this limitation. I guess
 it is quite typical bridge. This problem does not exists in case
 of using drm_panel for ptn3460.

 2. drm_bridge is attached to the encoder, this and the callback order
 suggests the video data flow should be as below:
 drm_crtc -> drm_encoder [-> drm_bridge] -> drm_connector [-> drm_panel]

 ptn3460 implements drm_bridge and drm_connector so it suggests its
 drm_bridge should be the last one, so there should be no place to add
 lvds panel implemented as a drm_bridge after it, as it is done in this
 patchset.

 Additionally it clearly shows that there should be two categories of
 drm_bridges - non-terminal and terminal.

 3. drm_dev uses all-or-nothing approach, ie. it will start only when all
 its components are up. It simplifies synchronization but is quite
 fragile - the whole drm will be down due to error in some of its 
 components.
 For this reason I prefer drm_panel as it is not real drm component
 it can be attached/detached to/from drm_connector anytime. I am not
 really sure but drm_bridge does not allow for that.
>>> So I do think we need to stick to this all-or-nothing approach for
>>> anything that is visible to userspace
>>> (drm_{plane,crtc,encoder,connector}).  We don't currently have a way
>>> to "hotplug" those so I don't see a real smooth upgrade path to add
>>> that in a backwards compatible way that won't cause problems with old
>>> userspace.
>>>
>>> But, that said, we have more flexibility with things not visible to
>>> userspace (drm_{panel,bridge}).  I'm not sure how much we want to
>>> allow things to be completely dynamic (we already have some hard
>>> enough locking fun).  But proposals/rfcs/etc welcome.
>>>
>>> I guess I'm not completely familiar w/ ptn3460, but the fact that it
>>> needs to 

[RFC V2 0/3] drm/bridge: panel and chaining

[Andrzej Hajda]

On 05/09/2014 05:05 PM, Ajay kumar wrote:
> On Fri, May 9, 2014 at 7:29 PM, Rob Clark  wrote:
>> On Fri, May 9, 2014 at 5:08 AM, Andrzej Hajda  wrote:
>>> On 05/08/2014 08:24 PM, Rob Clark wrote:
 On Thu, May 8, 2014 at 2:41 AM, Andrzej Hajda  
 wrote:
> On 05/05/2014 09:52 PM, Ajay Kumar wrote:
>> This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree 
>> at:
>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
>>
>> I have just put up Rob's and Sean's idea of chaining up the bridges
>> in code, and have implemented basic panel controls as a chained bridge.
>> This works well with ptn3460 bridge chip on exynos5250-snow board.
>>
>> Still need to make use of standard list calls and figure out proper way
>> of deleting the bridge chain. So, this is just a rough version.
> As I understand this patchset tries to solve two things:
> 1. Implement panel as drm_bridge, to ease support for hardware chains:
> Crtc -> Encoder -> Bridge -> Panel
> 2. Add support to drm_bridge chaining, to allow software chains:
> drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
>
> It is done using Russian doll schema, ops from the bridge calls the same
> ops from the next bridge and the next bridge ops can do the same.
>
> This schema means that all the bridges including the last one are seen
> from the drm core point of view as a one big drm_bridge. Additionally in
> this particular case, the first bridge (ptn3460) implements connector
> so it is hard to guess what is the location of the 2nd bridge in video
> stream chain, sometimes it is after the connector, sometimes before.
> All this is quite confusing.
>
> But if you look at the bridge from upstream video interface point of
> view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
> video input side acts as a panel. On the output side it expects a panel,
> lvds panel in this case.
 tbh, this is mostly about what we call it.  Perhaps "bridge" isn't the
 best name.. I wouldn't object to changing it.

 But my thinking was to leave in drm_panel_funcs things that are just
 needed by the connector (get_modes().. and maybe some day we need
 detect/etc).  Then leave everything else in drm_bridge_funcs.  A panel
 could (if needed) implement both interfaces.

 That is basically the same as what you are proposing, but without
 renaming bridge to panel ;-)
>>> Good to hear that. However there are points which are not clear for me.
>>> But first lets clarify names, I will use panel and bridge words
>>> to describe the hardware, and drm_panel, drm_bridge to describe the
>>> software interfaces.
>>>
>>> What bothers me:
>>> 1. You want to leave connector related callbacks in drm_panel and
>>> the rest in drm_bridge. In case of ptn3460 it does not work, ptn3460
>>> must implement connector internally because of this limitation. I guess
>>> it is quite typical bridge. This problem does not exists in case
>>> of using drm_panel for ptn3460.
>>>
>>> 2. drm_bridge is attached to the encoder, this and the callback order
>>> suggests the video data flow should be as below:
>>> drm_crtc -> drm_encoder [-> drm_bridge] -> drm_connector [-> drm_panel]
>>>
>>> ptn3460 implements drm_bridge and drm_connector so it suggests its
>>> drm_bridge should be the last one, so there should be no place to add
>>> lvds panel implemented as a drm_bridge after it, as it is done in this
>>> patchset.
>>>
>>> Additionally it clearly shows that there should be two categories of
>>> drm_bridges - non-terminal and terminal.
>>>
>>> 3. drm_dev uses all-or-nothing approach, ie. it will start only when all
>>> its components are up. It simplifies synchronization but is quite
>>> fragile - the whole drm will be down due to error in some of its components.
>>> For this reason I prefer drm_panel as it is not real drm component
>>> it can be attached/detached to/from drm_connector anytime. I am not
>>> really sure but drm_bridge does not allow for that.
>> So I do think we need to stick to this all-or-nothing approach for
>> anything that is visible to userspace
>> (drm_{plane,crtc,encoder,connector}).  We don't currently have a way
>> to "hotplug" those so I don't see a real smooth upgrade path to add
>> that in a backwards compatible way that won't cause problems with old
>> userspace.
>>
>> But, that said, we have more flexibility with things not visible to
>> userspace (drm_{panel,bridge}).  I'm not sure how much we want to
>> allow things to be completely dynamic (we already have some hard
>> enough locking fun).  But proposals/rfcs/etc welcome.
>>
>> I guess I'm not completely familiar w/ ptn3460, but the fact that it
>> needs to implement drm_connector makes me a bit suspicious.  Seems
>> like a symptom of missing things in drm_panel_funcs.  It would be
>> better to always create the 

[RFC V2 0/3] drm/bridge: panel and chaining

[Rob Clark]

On Mon, May 12, 2014 at 3:06 AM, Andrzej Hajda  wrote:
> On 05/09/2014 05:05 PM, Ajay kumar wrote:
>> On Fri, May 9, 2014 at 7:29 PM, Rob Clark  wrote:
>>> On Fri, May 9, 2014 at 5:08 AM, Andrzej Hajda  
>>> wrote:
 On 05/08/2014 08:24 PM, Rob Clark wrote:
> On Thu, May 8, 2014 at 2:41 AM, Andrzej Hajda  
> wrote:
>> On 05/05/2014 09:52 PM, Ajay Kumar wrote:
>>> This patchset is based on exynos-drm-next-todo branch of Inki Dae's 
>>> tree at:
>>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
>>>
>>> I have just put up Rob's and Sean's idea of chaining up the bridges
>>> in code, and have implemented basic panel controls as a chained bridge.
>>> This works well with ptn3460 bridge chip on exynos5250-snow board.
>>>
>>> Still need to make use of standard list calls and figure out proper way
>>> of deleting the bridge chain. So, this is just a rough version.
>> As I understand this patchset tries to solve two things:
>> 1. Implement panel as drm_bridge, to ease support for hardware chains:
>> Crtc -> Encoder -> Bridge -> Panel
>> 2. Add support to drm_bridge chaining, to allow software chains:
>> drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
>>
>> It is done using Russian doll schema, ops from the bridge calls the same
>> ops from the next bridge and the next bridge ops can do the same.
>>
>> This schema means that all the bridges including the last one are seen
>> from the drm core point of view as a one big drm_bridge. Additionally in
>> this particular case, the first bridge (ptn3460) implements connector
>> so it is hard to guess what is the location of the 2nd bridge in video
>> stream chain, sometimes it is after the connector, sometimes before.
>> All this is quite confusing.
>>
>> But if you look at the bridge from upstream video interface point of
>> view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
>> video input side acts as a panel. On the output side it expects a panel,
>> lvds panel in this case.
> tbh, this is mostly about what we call it.  Perhaps "bridge" isn't the
> best name.. I wouldn't object to changing it.
>
> But my thinking was to leave in drm_panel_funcs things that are just
> needed by the connector (get_modes().. and maybe some day we need
> detect/etc).  Then leave everything else in drm_bridge_funcs.  A panel
> could (if needed) implement both interfaces.
>
> That is basically the same as what you are proposing, but without
> renaming bridge to panel ;-)
 Good to hear that. However there are points which are not clear for me.
 But first lets clarify names, I will use panel and bridge words
 to describe the hardware, and drm_panel, drm_bridge to describe the
 software interfaces.

 What bothers me:
 1. You want to leave connector related callbacks in drm_panel and
 the rest in drm_bridge. In case of ptn3460 it does not work, ptn3460
 must implement connector internally because of this limitation. I guess
 it is quite typical bridge. This problem does not exists in case
 of using drm_panel for ptn3460.

 2. drm_bridge is attached to the encoder, this and the callback order
 suggests the video data flow should be as below:
 drm_crtc -> drm_encoder [-> drm_bridge] -> drm_connector [-> drm_panel]

 ptn3460 implements drm_bridge and drm_connector so it suggests its
 drm_bridge should be the last one, so there should be no place to add
 lvds panel implemented as a drm_bridge after it, as it is done in this
 patchset.

 Additionally it clearly shows that there should be two categories of
 drm_bridges - non-terminal and terminal.

 3. drm_dev uses all-or-nothing approach, ie. it will start only when all
 its components are up. It simplifies synchronization but is quite
 fragile - the whole drm will be down due to error in some of its 
 components.
 For this reason I prefer drm_panel as it is not real drm component
 it can be attached/detached to/from drm_connector anytime. I am not
 really sure but drm_bridge does not allow for that.
>>> So I do think we need to stick to this all-or-nothing approach for
>>> anything that is visible to userspace
>>> (drm_{plane,crtc,encoder,connector}).  We don't currently have a way
>>> to "hotplug" those so I don't see a real smooth upgrade path to add
>>> that in a backwards compatible way that won't cause problems with old
>>> userspace.
>>>
>>> But, that said, we have more flexibility with things not visible to
>>> userspace (drm_{panel,bridge}).  I'm not sure how much we want to
>>> allow things to be completely dynamic (we already have some hard
>>> enough locking fun).  But proposals/rfcs/etc welcome.
>>>
>>> I guess I'm not completely familiar w/ ptn3460, but the fact that it
>>> needs to 

[RFC V2 0/3] drm/bridge: panel and chaining

[Ajay kumar]

On Fri, May 9, 2014 at 7:29 PM, Rob Clark  wrote:
> On Fri, May 9, 2014 at 5:08 AM, Andrzej Hajda  wrote:
>> On 05/08/2014 08:24 PM, Rob Clark wrote:
>>> On Thu, May 8, 2014 at 2:41 AM, Andrzej Hajda  
>>> wrote:
 On 05/05/2014 09:52 PM, Ajay Kumar wrote:
> This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree 
> at:
> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
>
> I have just put up Rob's and Sean's idea of chaining up the bridges
> in code, and have implemented basic panel controls as a chained bridge.
> This works well with ptn3460 bridge chip on exynos5250-snow board.
>
> Still need to make use of standard list calls and figure out proper way
> of deleting the bridge chain. So, this is just a rough version.

 As I understand this patchset tries to solve two things:
 1. Implement panel as drm_bridge, to ease support for hardware chains:
 Crtc -> Encoder -> Bridge -> Panel
 2. Add support to drm_bridge chaining, to allow software chains:
 drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel

 It is done using Russian doll schema, ops from the bridge calls the same
 ops from the next bridge and the next bridge ops can do the same.

 This schema means that all the bridges including the last one are seen
 from the drm core point of view as a one big drm_bridge. Additionally in
 this particular case, the first bridge (ptn3460) implements connector
 so it is hard to guess what is the location of the 2nd bridge in video
 stream chain, sometimes it is after the connector, sometimes before.
 All this is quite confusing.

 But if you look at the bridge from upstream video interface point of
 view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
 video input side acts as a panel. On the output side it expects a panel,
 lvds panel in this case.
>>>
>>> tbh, this is mostly about what we call it.  Perhaps "bridge" isn't the
>>> best name.. I wouldn't object to changing it.
>>>
>>> But my thinking was to leave in drm_panel_funcs things that are just
>>> needed by the connector (get_modes().. and maybe some day we need
>>> detect/etc).  Then leave everything else in drm_bridge_funcs.  A panel
>>> could (if needed) implement both interfaces.
>>>
>>> That is basically the same as what you are proposing, but without
>>> renaming bridge to panel ;-)
>>
>> Good to hear that. However there are points which are not clear for me.
>> But first lets clarify names, I will use panel and bridge words
>> to describe the hardware, and drm_panel, drm_bridge to describe the
>> software interfaces.
>>
>> What bothers me:
>> 1. You want to leave connector related callbacks in drm_panel and
>> the rest in drm_bridge. In case of ptn3460 it does not work, ptn3460
>> must implement connector internally because of this limitation. I guess
>> it is quite typical bridge. This problem does not exists in case
>> of using drm_panel for ptn3460.
>>
>> 2. drm_bridge is attached to the encoder, this and the callback order
>> suggests the video data flow should be as below:
>> drm_crtc -> drm_encoder [-> drm_bridge] -> drm_connector [-> drm_panel]
>>
>> ptn3460 implements drm_bridge and drm_connector so it suggests its
>> drm_bridge should be the last one, so there should be no place to add
>> lvds panel implemented as a drm_bridge after it, as it is done in this
>> patchset.
>>
>> Additionally it clearly shows that there should be two categories of
>> drm_bridges - non-terminal and terminal.
>>
>> 3. drm_dev uses all-or-nothing approach, ie. it will start only when all
>> its components are up. It simplifies synchronization but is quite
>> fragile - the whole drm will be down due to error in some of its components.
>> For this reason I prefer drm_panel as it is not real drm component
>> it can be attached/detached to/from drm_connector anytime. I am not
>> really sure but drm_bridge does not allow for that.
>
> So I do think we need to stick to this all-or-nothing approach for
> anything that is visible to userspace
> (drm_{plane,crtc,encoder,connector}).  We don't currently have a way
> to "hotplug" those so I don't see a real smooth upgrade path to add
> that in a backwards compatible way that won't cause problems with old
> userspace.
>
> But, that said, we have more flexibility with things not visible to
> userspace (drm_{panel,bridge}).  I'm not sure how much we want to
> allow things to be completely dynamic (we already have some hard
> enough locking fun).  But proposals/rfcs/etc welcome.
>
> I guess I'm not completely familiar w/ ptn3460, but the fact that it
> needs to implement drm_connector makes me a bit suspicious.  Seems
> like a symptom of missing things in drm_panel_funcs.  It would be
> better to always create the connector statically, and just have
> _detect() -> disconnected if panel==NULL.
This is something which only Sean can answer!
I 

[RFC V2 0/3] drm/bridge: panel and chaining

[Andrzej Hajda]

On 05/08/2014 08:24 PM, Rob Clark wrote:
> On Thu, May 8, 2014 at 2:41 AM, Andrzej Hajda  wrote:
>> On 05/05/2014 09:52 PM, Ajay Kumar wrote:
>>> This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at:
>>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
>>>
>>> I have just put up Rob's and Sean's idea of chaining up the bridges
>>> in code, and have implemented basic panel controls as a chained bridge.
>>> This works well with ptn3460 bridge chip on exynos5250-snow board.
>>>
>>> Still need to make use of standard list calls and figure out proper way
>>> of deleting the bridge chain. So, this is just a rough version.
>>
>> As I understand this patchset tries to solve two things:
>> 1. Implement panel as drm_bridge, to ease support for hardware chains:
>> Crtc -> Encoder -> Bridge -> Panel
>> 2. Add support to drm_bridge chaining, to allow software chains:
>> drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
>>
>> It is done using Russian doll schema, ops from the bridge calls the same
>> ops from the next bridge and the next bridge ops can do the same.
>>
>> This schema means that all the bridges including the last one are seen
>> from the drm core point of view as a one big drm_bridge. Additionally in
>> this particular case, the first bridge (ptn3460) implements connector
>> so it is hard to guess what is the location of the 2nd bridge in video
>> stream chain, sometimes it is after the connector, sometimes before.
>> All this is quite confusing.
>>
>> But if you look at the bridge from upstream video interface point of
>> view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
>> video input side acts as a panel. On the output side it expects a panel,
>> lvds panel in this case.
> 
> tbh, this is mostly about what we call it.  Perhaps "bridge" isn't the
> best name.. I wouldn't object to changing it.
> 
> But my thinking was to leave in drm_panel_funcs things that are just
> needed by the connector (get_modes().. and maybe some day we need
> detect/etc).  Then leave everything else in drm_bridge_funcs.  A panel
> could (if needed) implement both interfaces.
> 
> That is basically the same as what you are proposing, but without
> renaming bridge to panel ;-)

Good to hear that. However there are points which are not clear for me.
But first lets clarify names, I will use panel and bridge words
to describe the hardware, and drm_panel, drm_bridge to describe the
software interfaces.

What bothers me:
1. You want to leave connector related callbacks in drm_panel and
the rest in drm_bridge. In case of ptn3460 it does not work, ptn3460
must implement connector internally because of this limitation. I guess
it is quite typical bridge. This problem does not exists in case
of using drm_panel for ptn3460.

2. drm_bridge is attached to the encoder, this and the callback order
suggests the video data flow should be as below:
drm_crtc -> drm_encoder [-> drm_bridge] -> drm_connector [-> drm_panel]

ptn3460 implements drm_bridge and drm_connector so it suggests its
drm_bridge should be the last one, so there should be no place to add
lvds panel implemented as a drm_bridge after it, as it is done in this
patchset.

Additionally it clearly shows that there should be two categories of
drm_bridges - non-terminal and terminal.

3. drm_dev uses all-or-nothing approach, ie. it will start only when all
its components are up. It simplifies synchronization but is quite
fragile - the whole drm will be down due to error in some of its components.
For this reason I prefer drm_panel as it is not real drm component
it can be attached/detached to/from drm_connector anytime. I am not
really sure but drm_bridge does not allow for that.

Real life example to show importance of it: I have a phone with MIPI-DSI
panel and HDMI. Due to initialization issues HDMI bridge driver
sometimes fails during probe and the drmdev do not start. Of course this
is development stage so I have serial console I can diagnose the
problem, disable HDMI, fix the problem, etc...
But what happens in case of end-user. He will see black screen - bricked
phone. In case the bridge will be implemented using drm_panel
he will have working phone with broken HDMI, much better.

4. And the last thing, it is more about the concept/design. drm_bridge,
drm_hw_block suggests that those interfaces describes the whole device:
bridge, panel, whatever. In my approach I have an interface
to describe only one video input port of the device. And drm_panel is
in fact misleading name, drm_sink may be better. So real panel
would implement drm_sink interface. Bridge would implement drm_sink
interface and it will request other drm_sink interface, to interact with
the panel which is after it.
This approach seems to me more flexible. Beside things I have described
above it will allow to implement also more complicated devices, dsi
hubs, video mixers, etc.


Regards
Andrzej

> 
> BR,
> -R
> 
>> So why not implement ptn3460 

[RFC V2 0/3] drm/bridge: panel and chaining

[Rob Clark]

On Fri, May 9, 2014 at 5:08 AM, Andrzej Hajda  wrote:
> On 05/08/2014 08:24 PM, Rob Clark wrote:
>> On Thu, May 8, 2014 at 2:41 AM, Andrzej Hajda  wrote:
>>> On 05/05/2014 09:52 PM, Ajay Kumar wrote:
 This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree 
 at:
 git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git

 I have just put up Rob's and Sean's idea of chaining up the bridges
 in code, and have implemented basic panel controls as a chained bridge.
 This works well with ptn3460 bridge chip on exynos5250-snow board.

 Still need to make use of standard list calls and figure out proper way
 of deleting the bridge chain. So, this is just a rough version.
>>>
>>> As I understand this patchset tries to solve two things:
>>> 1. Implement panel as drm_bridge, to ease support for hardware chains:
>>> Crtc -> Encoder -> Bridge -> Panel
>>> 2. Add support to drm_bridge chaining, to allow software chains:
>>> drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
>>>
>>> It is done using Russian doll schema, ops from the bridge calls the same
>>> ops from the next bridge and the next bridge ops can do the same.
>>>
>>> This schema means that all the bridges including the last one are seen
>>> from the drm core point of view as a one big drm_bridge. Additionally in
>>> this particular case, the first bridge (ptn3460) implements connector
>>> so it is hard to guess what is the location of the 2nd bridge in video
>>> stream chain, sometimes it is after the connector, sometimes before.
>>> All this is quite confusing.
>>>
>>> But if you look at the bridge from upstream video interface point of
>>> view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
>>> video input side acts as a panel. On the output side it expects a panel,
>>> lvds panel in this case.
>>
>> tbh, this is mostly about what we call it.  Perhaps "bridge" isn't the
>> best name.. I wouldn't object to changing it.
>>
>> But my thinking was to leave in drm_panel_funcs things that are just
>> needed by the connector (get_modes().. and maybe some day we need
>> detect/etc).  Then leave everything else in drm_bridge_funcs.  A panel
>> could (if needed) implement both interfaces.
>>
>> That is basically the same as what you are proposing, but without
>> renaming bridge to panel ;-)
>
> Good to hear that. However there are points which are not clear for me.
> But first lets clarify names, I will use panel and bridge words
> to describe the hardware, and drm_panel, drm_bridge to describe the
> software interfaces.
>
> What bothers me:
> 1. You want to leave connector related callbacks in drm_panel and
> the rest in drm_bridge. In case of ptn3460 it does not work, ptn3460
> must implement connector internally because of this limitation. I guess
> it is quite typical bridge. This problem does not exists in case
> of using drm_panel for ptn3460.
>
> 2. drm_bridge is attached to the encoder, this and the callback order
> suggests the video data flow should be as below:
> drm_crtc -> drm_encoder [-> drm_bridge] -> drm_connector [-> drm_panel]
>
> ptn3460 implements drm_bridge and drm_connector so it suggests its
> drm_bridge should be the last one, so there should be no place to add
> lvds panel implemented as a drm_bridge after it, as it is done in this
> patchset.
>
> Additionally it clearly shows that there should be two categories of
> drm_bridges - non-terminal and terminal.
>
> 3. drm_dev uses all-or-nothing approach, ie. it will start only when all
> its components are up. It simplifies synchronization but is quite
> fragile - the whole drm will be down due to error in some of its components.
> For this reason I prefer drm_panel as it is not real drm component
> it can be attached/detached to/from drm_connector anytime. I am not
> really sure but drm_bridge does not allow for that.

So I do think we need to stick to this all-or-nothing approach for
anything that is visible to userspace
(drm_{plane,crtc,encoder,connector}).  We don't currently have a way
to "hotplug" those so I don't see a real smooth upgrade path to add
that in a backwards compatible way that won't cause problems with old
userspace.

But, that said, we have more flexibility with things not visible to
userspace (drm_{panel,bridge}).  I'm not sure how much we want to
allow things to be completely dynamic (we already have some hard
enough locking fun).  But proposals/rfcs/etc welcome.

I guess I'm not completely familiar w/ ptn3460, but the fact that it
needs to implement drm_connector makes me a bit suspicious.  Seems
like a symptom of missing things in drm_panel_funcs.  It would be
better to always create the connector statically, and just have
_detect() -> disconnected if panel==NULL.

> Real life example to show importance of it: I have a phone with MIPI-DSI
> panel and HDMI. Due to initialization issues HDMI bridge driver
> sometimes fails during probe and the drmdev do not start. Of course 

[RFC V2 0/3] drm/bridge: panel and chaining

[Ajay kumar]

On Thu, May 8, 2014 at 6:29 PM, Andrzej Hajda  wrote:
> On 05/08/2014 12:26 PM, Ajay kumar wrote:
>> Hi Andrej,
>>
>> On Thu, May 8, 2014 at 12:11 PM, Andrzej Hajda  
>> wrote:
>>> On 05/05/2014 09:52 PM, Ajay Kumar wrote:
 This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree 
 at:
 git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git

 I have just put up Rob's and Sean's idea of chaining up the bridges
 in code, and have implemented basic panel controls as a chained bridge.
 This works well with ptn3460 bridge chip on exynos5250-snow board.

 Still need to make use of standard list calls and figure out proper way
 of deleting the bridge chain. So, this is just a rough version.
>>> As I understand this patchset tries to solve two things:
>>> 1. Implement panel as drm_bridge, to ease support for hardware chains:
>>> Crtc -> Encoder -> Bridge -> Panel
>>> 2. Add support to drm_bridge chaining, to allow software chains:
>>> drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
>>>
>>> It is done using Russian doll schema, ops from the bridge calls the same
>>> ops from the next bridge and the next bridge ops can do the same.
>>>
>>> This schema means that all the bridges including the last one are seen
>>> from the drm core point of view as a one big drm_bridge. Additionally in
>>> this particular case, the first bridge (ptn3460) implements connector
>>> so it is hard to guess what is the location of the 2nd bridge in video
>>> stream chain, sometimes it is after the connector, sometimes before.
>>> All this is quite confusing.
>>>
>>> But if you look at the bridge from upstream video interface point of
>>> view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
>>> video input side acts as a panel. On the output side it expects a panel,
>>> lvds panel in this case.
>>>
>>> So why not implement ptn3460 bridge as drm_panel which internally uses
>>> another drm_panel. With this approach everything fits much better.
>>> You do not need those (pre|post)_(enable|disable) calls, you do not need
>>> to implement connector in the bridge and you have a driver following
>>> linux driver model. And no single bit changed in drm core.
>> This discussion should have ideally happened when Sean added bridge
>> into drm-core!
>
> Yes, I agree with this :)
>
>> And, we do need (pre|post)_(enable|disable) calls for bridges, but the 
>> drm_panel
>> framework supports only enable/disable.
>
> For true bridges pre|post can be just implemented as a part of the call,
> for example:
>
> bridge_enable()
> {
> /* pre-enable stuff */
> panel_enable();
> /* post-enable stuff */
> }
>
It should ideally be like this:
1) panel enable - switch on the LCD unit.
2) bridge enable - switch on the bridge.
3) encoder->commit/dpms on - valid data starts coming out of DP/MIPI
4) switch on the backlight unit and enable BL_EN.

> And for your particular problem you have written:
>> The LVDS datasheet says at least 200ms delay is needed from "Valid
>> data" to "BL on".
>
> I am not sure what exactly means 'valid data' in this case, if it is
> after lcd_en gpio
> why not just use schedule_delayed_work. If it should be called later I
> guess it should
> be possible to find a proper callback to drm_panel.
Right. This was already discussed.
I suggested adding pre_enable/enable/disable and post_disable for drm_panel,
but Thierry was not ok with it.

Regards,
Ajay


>>> I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2].
>>> It was not accepted as Inki preferred drm_bridge but as I see the
>>> problems with drm_bridges I have decide to attract attention to much
>>> more cleaner solution.
>>> [1]: http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559
>>> [2]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/27044
>>>
>>> Regards
>>> Andrzej
>>>
>>>
 Ajay Kumar (3):
   [RFC V2 1/3] drm: implement chaining of drm bridges
   [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges
   [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining

  .../bindings/drm/bridge/bridge_panel.txt   |  45 
  drivers/gpu/drm/bridge/Kconfig |   6 +
  drivers/gpu/drm/bridge/Makefile|   1 +
  drivers/gpu/drm/bridge/bridge_panel.c  | 240 
 +
  drivers/gpu/drm/bridge/ptn3460.c   |  21 +-
  drivers/gpu/drm/drm_crtc.c |  13 +-
  include/drm/bridge/bridge_panel.h  |  37 
  include/drm/drm_crtc.h |   2 +
  8 files changed, 360 insertions(+), 5 deletions(-)
  create mode 100644 
 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
  create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c
  create mode 100644 include/drm/bridge/bridge_panel.h

>

[RFC V2 0/3] drm/bridge: panel and chaining

[Inki Dae]

On 2014? 05? 08? 19:52, Ajay kumar wrote:
> +Dave
> +Thierry
> 
> On Thu, May 8, 2014 at 1:14 PM, Inki Dae  wrote:
>>
>> Just re-sending with text mode. Sorry for this.
>>
>>
>> On 2014? 05? 08? 15:41, Andrzej Hajda wrote:
>>> On 05/05/2014 09:52 PM, Ajay Kumar wrote:
 This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree 
 at:
 git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git

 I have just put up Rob's and Sean's idea of chaining up the bridges
 in code, and have implemented basic panel controls as a chained bridge.
 This works well with ptn3460 bridge chip on exynos5250-snow board.

 Still need to make use of standard list calls and figure out proper way
 of deleting the bridge chain. So, this is just a rough version.
>>>
>>> As I understand this patchset tries to solve two things:
>>> 1. Implement panel as drm_bridge, to ease support for hardware chains:
>>> Crtc -> Encoder -> Bridge -> Panel
>>> 2. Add support to drm_bridge chaining, to allow software chains:
>>> drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
>>>
>>> It is done using Russian doll schema, ops from the bridge calls the same
>>> ops from the next bridge and the next bridge ops can do the same.
>>>
>>> This schema means that all the bridges including the last one are seen
>>> from the drm core point of view as a one big drm_bridge. Additionally in
>>> this particular case, the first bridge (ptn3460) implements connector
>>> so it is hard to guess what is the location of the 2nd bridge in video
>>> stream chain, sometimes it is after the connector, sometimes before.
>>> All this is quite confusing.
>>>
>>> But if you look at the bridge from upstream video interface point of
>>> view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
>>> video input side acts as a panel. On the output side it expects a panel,
>>> lvds panel in this case.
>>>
>>> So why not implement ptn3460 bridge as drm_panel which internally uses
>>> another drm_panel. With this approach everything fits much better.
>>> You do not need those (pre|post)_(enable|disable) calls, you do not need
>>> to implement connector in the bridge and you have a driver following
>>> linux driver model. And no single bit changed in drm core.
>>>
>>> I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2].
>>> It was not accepted as Inki preferred drm_bridge but as I see the
>>
>> Yes, in above email threads, I disagreed to using drm_panel framework
>> for bridge device, especially, to implement connector/encoder to crtc
>> driver.
>>
>> However, I thought that it'd be more generic way that lvds drivers use
>> driver-model, and the use of drm_panel infrastructure would be suitable
>> to doing this.
>>
>> So my intention in turn, was that LVDS driver uses integrated drm_bridge
>> based on drm_panel infrastructure[1], and RFC patch[2] for it. This way
>> is same as your proposal in the point that LVDS and Panel drivers use
>> driver-model. The only different point is that LVDS driver has some ops
>> specific to LVDS device, not using existing ops of drm_panel commonly:
>> we may need to consider the characteristic of LVDS device.
>>
>> [1]:http://www.spinics.net/lists/dri-devel/msg5.html
>> [2]:http://www.spinics.net/lists/dri-devel/msg55658.html
>>
>> Thanks,
>> Inki Dae
> I am just consolidating the discussion happening here.
> 1) This patchset started from a discussion wherein I tried to combine
> drm_panel with drm_bridge.
> https://www.mail-archive.com/linux-samsung-soc at 
> vger.kernel.org/msg28943.html
> 2) Sean and Rob suggested to implement a chain of bridges and then
> consider adding
> basic panel controls as a bridge.
> 3) Andrej's idea is to drop the existing bridge infrastructure and
> implement ptn3460 using drm_panel,
> the same way he has implemented
> http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559.
> 4) Inki's suggestion is to combine drm_bridge, drm_panel and
> drm_enhance into a single
> drm_hw_block.
> 

And more thing, we would need to consider image enhancer device placed
between crtc and connector/encoder devices. And it'd better to rename
drm_hw_block to drm_bridge, and existing drm_bridge relevant codes
should be removed.

Thanks,
Inki Dae

> I am currently trying to implement (2):chaining of bridges, and I
> think we have not yet
> reached to a consensus. So adding few other people from drm community
> to comment.
> 
> Regards,
> Ajay
> 
>>> problems with drm_bridges I have decide to attract attention to much
>>> more cleaner solution.
>>>
>>> [1]: http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559
>>> [2]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/27044
>>>
>>> Regards
>>> Andrzej
>>>
>>>

 Ajay Kumar (3):
   [RFC V2 1/3] drm: implement chaining of drm bridges
   [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges
   [RFC V2 3/3] 

[RFC V2 0/3] drm/bridge: panel and chaining

[Inki Dae]

Just re-sending with text mode. Sorry for this.


On 2014? 05? 08? 15:41, Andrzej Hajda wrote:
> On 05/05/2014 09:52 PM, Ajay Kumar wrote:
>> This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at:
>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
>>
>> I have just put up Rob's and Sean's idea of chaining up the bridges
>> in code, and have implemented basic panel controls as a chained bridge.
>> This works well with ptn3460 bridge chip on exynos5250-snow board.
>>
>> Still need to make use of standard list calls and figure out proper way
>> of deleting the bridge chain. So, this is just a rough version.
> 
> As I understand this patchset tries to solve two things:
> 1. Implement panel as drm_bridge, to ease support for hardware chains:
> Crtc -> Encoder -> Bridge -> Panel
> 2. Add support to drm_bridge chaining, to allow software chains:
> drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
> 
> It is done using Russian doll schema, ops from the bridge calls the same
> ops from the next bridge and the next bridge ops can do the same.
> 
> This schema means that all the bridges including the last one are seen
> from the drm core point of view as a one big drm_bridge. Additionally in
> this particular case, the first bridge (ptn3460) implements connector
> so it is hard to guess what is the location of the 2nd bridge in video
> stream chain, sometimes it is after the connector, sometimes before.
> All this is quite confusing.
> 
> But if you look at the bridge from upstream video interface point of
> view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
> video input side acts as a panel. On the output side it expects a panel,
> lvds panel in this case.
> 
> So why not implement ptn3460 bridge as drm_panel which internally uses
> another drm_panel. With this approach everything fits much better.
> You do not need those (pre|post)_(enable|disable) calls, you do not need
> to implement connector in the bridge and you have a driver following
> linux driver model. And no single bit changed in drm core.
> 
> I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2].
> It was not accepted as Inki preferred drm_bridge but as I see the

Yes, in above email threads, I disagreed to using drm_panel framework
for bridge device, especially, to implement connector/encoder to crtc
driver.

However, I thought that it'd be more generic way that lvds drivers use
driver-model, and the use of drm_panel infrastructure would be suitable
to doing this.

So my intention in turn, was that LVDS driver uses integrated drm_bridge
based on drm_panel infrastructure[1], and RFC patch[2] for it. This way
is same as your proposal in the point that LVDS and Panel drivers use
driver-model. The only different point is that LVDS driver has some ops
specific to LVDS device, not using existing ops of drm_panel commonly:
we may need to consider the characteristic of LVDS device.

[1]:http://www.spinics.net/lists/dri-devel/msg5.html
[2]:http://www.spinics.net/lists/dri-devel/msg55658.html

Thanks,
Inki Dae

> problems with drm_bridges I have decide to attract attention to much
> more cleaner solution.
> 
> [1]: http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559
> [2]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/27044
> 
> Regards
> Andrzej
> 
> 
>>
>> Ajay Kumar (3):
>>   [RFC V2 1/3] drm: implement chaining of drm bridges
>>   [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges
>>   [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
>>
>>  .../bindings/drm/bridge/bridge_panel.txt   |  45 
>>  drivers/gpu/drm/bridge/Kconfig |   6 +
>>  drivers/gpu/drm/bridge/Makefile|   1 +
>>  drivers/gpu/drm/bridge/bridge_panel.c  | 240 
>> +
>>  drivers/gpu/drm/bridge/ptn3460.c   |  21 +-
>>  drivers/gpu/drm/drm_crtc.c |  13 +-
>>  include/drm/bridge/bridge_panel.h  |  37 
>>  include/drm/drm_crtc.h |   2 +
>>  8 files changed, 360 insertions(+), 5 deletions(-)
>>  create mode 100644 
>> Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
>>  create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c
>>  create mode 100644 include/drm/bridge/bridge_panel.h
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 
> in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[RFC V2 0/3] drm/bridge: panel and chaining

[Inki Dae]

Hi Andrzej


On 2014? 05? 08? 15:41, Andrzej Hajda wrote:
> On 05/05/2014 09:52 PM, Ajay Kumar wrote:
>> This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at:
>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
>>
>> I have just put up Rob's and Sean's idea of chaining up the bridges
>> in code, and have implemented basic panel controls as a chained bridge.
>> This works well with ptn3460 bridge chip on exynos5250-snow board.
>>
>> Still need to make use of standard list calls and figure out proper way
>> of deleting the bridge chain. So, this is just a rough version.
>
> As I understand this patchset tries to solve two things:
> 1. Implement panel as drm_bridge, to ease support for hardware chains:
> Crtc -> Encoder -> Bridge -> Panel
> 2. Add support to drm_bridge chaining, to allow software chains:
> drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
>
> It is done using Russian doll schema, ops from the bridge calls the same
> ops from the next bridge and the next bridge ops can do the same.
>
> This schema means that all the bridges including the last one are seen
> from the drm core point of view as a one big drm_bridge. Additionally in
> this particular case, the first bridge (ptn3460) implements connector
> so it is hard to guess what is the location of the 2nd bridge in video
> stream chain, sometimes it is after the connector, sometimes before.
> All this is quite confusing.
>
> But if you look at the bridge from upstream video interface point of
> view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
> video input side acts as a panel. On the output side it expects a panel,
> lvds panel in this case.
>
> So why not implement ptn3460 bridge as drm_panel which internally uses
> another drm_panel. With this approach everything fits much better.
> You do not need those (pre|post)_(enable|disable) calls, you do not need
> to implement connector in the bridge and you have a driver following
> linux driver model. And no single bit changed in drm core.
>
> I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2].
> It was not accepted as Inki preferred drm_bridge but as I see the
> problems with drm_bridges I have decide to attract attention to much

Yes, in above email threads, I disagreed to using drm_panel framework 
for bridge device, especially, to implement connector/encoder to crtc 
driver.

However, I thought that it'd be more generic way that lvds drivers use 
driver-model, and the use of drm_panel infrastructure would be suitable 
to doing this.

So my intention in turn,  was that LVDS driver uses integrated 
drm_bridge based on drm_panel infrastructure[1], and RFC patch[2] for 
it. This way is same as your proposal in the point that LVDS and Panel 
drivers use driver-model. The only different point is that LVDS driver 
has some ops specific to LVDS device, not using existing ops of 
drm_panel commonly: we may need to consider the characteristic of LVDS 
device.

[1]:http://www.spinics.net/lists/dri-devel/msg5.html
[2]:http://www.spinics.net/lists/dri-devel/msg55658.html

Thanks,
Inki Dae

> more cleaner solution.
>
> [1]: http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559
> [2]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/27044
>
> Regards
> Andrzej
>
>
>>
>> Ajay Kumar (3):
>>[RFC V2 1/3] drm: implement chaining of drm bridges
>>[RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges
>>[RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
>>
>>   .../bindings/drm/bridge/bridge_panel.txt   |  45 
>>   drivers/gpu/drm/bridge/Kconfig |   6 +
>>   drivers/gpu/drm/bridge/Makefile|   1 +
>>   drivers/gpu/drm/bridge/bridge_panel.c  | 240 
>> +
>>   drivers/gpu/drm/bridge/ptn3460.c   |  21 +-
>>   drivers/gpu/drm/drm_crtc.c |  13 +-
>>   include/drm/bridge/bridge_panel.h  |  37 
>>   include/drm/drm_crtc.h |   2 +
>>   8 files changed, 360 insertions(+), 5 deletions(-)
>>   create mode 100644 
>> Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
>>   create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c
>>   create mode 100644 include/drm/bridge/bridge_panel.h
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 
> in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[RFC V2 0/3] drm/bridge: panel and chaining

[Ajay kumar]

+Dave
+Thierry

On Thu, May 8, 2014 at 1:14 PM, Inki Dae  wrote:
>
> Just re-sending with text mode. Sorry for this.
>
>
> On 2014? 05? 08? 15:41, Andrzej Hajda wrote:
>> On 05/05/2014 09:52 PM, Ajay Kumar wrote:
>>> This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at:
>>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
>>>
>>> I have just put up Rob's and Sean's idea of chaining up the bridges
>>> in code, and have implemented basic panel controls as a chained bridge.
>>> This works well with ptn3460 bridge chip on exynos5250-snow board.
>>>
>>> Still need to make use of standard list calls and figure out proper way
>>> of deleting the bridge chain. So, this is just a rough version.
>>
>> As I understand this patchset tries to solve two things:
>> 1. Implement panel as drm_bridge, to ease support for hardware chains:
>> Crtc -> Encoder -> Bridge -> Panel
>> 2. Add support to drm_bridge chaining, to allow software chains:
>> drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
>>
>> It is done using Russian doll schema, ops from the bridge calls the same
>> ops from the next bridge and the next bridge ops can do the same.
>>
>> This schema means that all the bridges including the last one are seen
>> from the drm core point of view as a one big drm_bridge. Additionally in
>> this particular case, the first bridge (ptn3460) implements connector
>> so it is hard to guess what is the location of the 2nd bridge in video
>> stream chain, sometimes it is after the connector, sometimes before.
>> All this is quite confusing.
>>
>> But if you look at the bridge from upstream video interface point of
>> view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
>> video input side acts as a panel. On the output side it expects a panel,
>> lvds panel in this case.
>>
>> So why not implement ptn3460 bridge as drm_panel which internally uses
>> another drm_panel. With this approach everything fits much better.
>> You do not need those (pre|post)_(enable|disable) calls, you do not need
>> to implement connector in the bridge and you have a driver following
>> linux driver model. And no single bit changed in drm core.
>>
>> I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2].
>> It was not accepted as Inki preferred drm_bridge but as I see the
>
> Yes, in above email threads, I disagreed to using drm_panel framework
> for bridge device, especially, to implement connector/encoder to crtc
> driver.
>
> However, I thought that it'd be more generic way that lvds drivers use
> driver-model, and the use of drm_panel infrastructure would be suitable
> to doing this.
>
> So my intention in turn, was that LVDS driver uses integrated drm_bridge
> based on drm_panel infrastructure[1], and RFC patch[2] for it. This way
> is same as your proposal in the point that LVDS and Panel drivers use
> driver-model. The only different point is that LVDS driver has some ops
> specific to LVDS device, not using existing ops of drm_panel commonly:
> we may need to consider the characteristic of LVDS device.
>
> [1]:http://www.spinics.net/lists/dri-devel/msg5.html
> [2]:http://www.spinics.net/lists/dri-devel/msg55658.html
>
> Thanks,
> Inki Dae
I am just consolidating the discussion happening here.
1) This patchset started from a discussion wherein I tried to combine
drm_panel with drm_bridge.
https://www.mail-archive.com/linux-samsung-soc at 
vger.kernel.org/msg28943.html
2) Sean and Rob suggested to implement a chain of bridges and then
consider adding
basic panel controls as a bridge.
3) Andrej's idea is to drop the existing bridge infrastructure and
implement ptn3460 using drm_panel,
the same way he has implemented
http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559.
4) Inki's suggestion is to combine drm_bridge, drm_panel and
drm_enhance into a single
drm_hw_block.

I am currently trying to implement (2):chaining of bridges, and I
think we have not yet
reached to a consensus. So adding few other people from drm community
to comment.

Regards,
Ajay

>> problems with drm_bridges I have decide to attract attention to much
>> more cleaner solution.
>>
>> [1]: http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559
>> [2]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/27044
>>
>> Regards
>> Andrzej
>>
>>
>>>
>>> Ajay Kumar (3):
>>>   [RFC V2 1/3] drm: implement chaining of drm bridges
>>>   [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges
>>>   [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
>>>
>>>  .../bindings/drm/bridge/bridge_panel.txt   |  45 
>>>  drivers/gpu/drm/bridge/Kconfig |   6 +
>>>  drivers/gpu/drm/bridge/Makefile|   1 +
>>>  drivers/gpu/drm/bridge/bridge_panel.c  | 240 
>>> +
>>>  drivers/gpu/drm/bridge/ptn3460.c   |  21 +-
>>>  drivers/gpu/drm/drm_crtc.c   

[RFC V2 0/3] drm/bridge: panel and chaining

[Ajay kumar]

Hi Andrej,

On Thu, May 8, 2014 at 12:11 PM, Andrzej Hajda  wrote:
> On 05/05/2014 09:52 PM, Ajay Kumar wrote:
>> This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at:
>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
>>
>> I have just put up Rob's and Sean's idea of chaining up the bridges
>> in code, and have implemented basic panel controls as a chained bridge.
>> This works well with ptn3460 bridge chip on exynos5250-snow board.
>>
>> Still need to make use of standard list calls and figure out proper way
>> of deleting the bridge chain. So, this is just a rough version.
>
> As I understand this patchset tries to solve two things:
> 1. Implement panel as drm_bridge, to ease support for hardware chains:
> Crtc -> Encoder -> Bridge -> Panel
> 2. Add support to drm_bridge chaining, to allow software chains:
> drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
>
> It is done using Russian doll schema, ops from the bridge calls the same
> ops from the next bridge and the next bridge ops can do the same.
>
> This schema means that all the bridges including the last one are seen
> from the drm core point of view as a one big drm_bridge. Additionally in
> this particular case, the first bridge (ptn3460) implements connector
> so it is hard to guess what is the location of the 2nd bridge in video
> stream chain, sometimes it is after the connector, sometimes before.
> All this is quite confusing.
>
> But if you look at the bridge from upstream video interface point of
> view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
> video input side acts as a panel. On the output side it expects a panel,
> lvds panel in this case.
>
> So why not implement ptn3460 bridge as drm_panel which internally uses
> another drm_panel. With this approach everything fits much better.
> You do not need those (pre|post)_(enable|disable) calls, you do not need
> to implement connector in the bridge and you have a driver following
> linux driver model. And no single bit changed in drm core.
This discussion should have ideally happened when Sean added bridge
into drm-core!
And, we do need (pre|post)_(enable|disable) calls for bridges, but the drm_panel
framework supports only enable/disable.

> I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2].
> It was not accepted as Inki preferred drm_bridge but as I see the
> problems with drm_bridges I have decide to attract attention to much
> more cleaner solution.
> [1]: http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559
> [2]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/27044
>
> Regards
> Andrzej
>
>
>>
>> Ajay Kumar (3):
>>   [RFC V2 1/3] drm: implement chaining of drm bridges
>>   [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges
>>   [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
>>
>>  .../bindings/drm/bridge/bridge_panel.txt   |  45 
>>  drivers/gpu/drm/bridge/Kconfig |   6 +
>>  drivers/gpu/drm/bridge/Makefile|   1 +
>>  drivers/gpu/drm/bridge/bridge_panel.c  | 240 
>> +
>>  drivers/gpu/drm/bridge/ptn3460.c   |  21 +-
>>  drivers/gpu/drm/drm_crtc.c |  13 +-
>>  include/drm/bridge/bridge_panel.h  |  37 
>>  include/drm/drm_crtc.h |   2 +
>>  8 files changed, 360 insertions(+), 5 deletions(-)
>>  create mode 100644 
>> Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
>>  create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c
>>  create mode 100644 include/drm/bridge/bridge_panel.h
>>
>

[RFC V2 0/3] drm/bridge: panel and chaining

[Andrzej Hajda]

On 05/08/2014 12:26 PM, Ajay kumar wrote:
> Hi Andrej,
>
> On Thu, May 8, 2014 at 12:11 PM, Andrzej Hajda  wrote:
>> On 05/05/2014 09:52 PM, Ajay Kumar wrote:
>>> This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at:
>>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
>>>
>>> I have just put up Rob's and Sean's idea of chaining up the bridges
>>> in code, and have implemented basic panel controls as a chained bridge.
>>> This works well with ptn3460 bridge chip on exynos5250-snow board.
>>>
>>> Still need to make use of standard list calls and figure out proper way
>>> of deleting the bridge chain. So, this is just a rough version.
>> As I understand this patchset tries to solve two things:
>> 1. Implement panel as drm_bridge, to ease support for hardware chains:
>> Crtc -> Encoder -> Bridge -> Panel
>> 2. Add support to drm_bridge chaining, to allow software chains:
>> drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
>>
>> It is done using Russian doll schema, ops from the bridge calls the same
>> ops from the next bridge and the next bridge ops can do the same.
>>
>> This schema means that all the bridges including the last one are seen
>> from the drm core point of view as a one big drm_bridge. Additionally in
>> this particular case, the first bridge (ptn3460) implements connector
>> so it is hard to guess what is the location of the 2nd bridge in video
>> stream chain, sometimes it is after the connector, sometimes before.
>> All this is quite confusing.
>>
>> But if you look at the bridge from upstream video interface point of
>> view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
>> video input side acts as a panel. On the output side it expects a panel,
>> lvds panel in this case.
>>
>> So why not implement ptn3460 bridge as drm_panel which internally uses
>> another drm_panel. With this approach everything fits much better.
>> You do not need those (pre|post)_(enable|disable) calls, you do not need
>> to implement connector in the bridge and you have a driver following
>> linux driver model. And no single bit changed in drm core.
> This discussion should have ideally happened when Sean added bridge
> into drm-core!

Yes, I agree with this :)

> And, we do need (pre|post)_(enable|disable) calls for bridges, but the 
> drm_panel
> framework supports only enable/disable.

For true bridges pre|post can be just implemented as a part of the call,
for example:

bridge_enable()
{
/* pre-enable stuff */
panel_enable();
/* post-enable stuff */
}

And for your particular problem you have written:
> The LVDS datasheet says at least 200ms delay is needed from "Valid
> data" to "BL on".

I am not sure what exactly means 'valid data' in this case, if it is
after lcd_en gpio
why not just use schedule_delayed_work. If it should be called later I
guess it should
be possible to find a proper callback to drm_panel.

Regards
Andrzej

>> I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2].
>> It was not accepted as Inki preferred drm_bridge but as I see the
>> problems with drm_bridges I have decide to attract attention to much
>> more cleaner solution.
>> [1]: http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559
>> [2]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/27044
>>
>> Regards
>> Andrzej
>>
>>
>>> Ajay Kumar (3):
>>>   [RFC V2 1/3] drm: implement chaining of drm bridges
>>>   [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges
>>>   [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
>>>
>>>  .../bindings/drm/bridge/bridge_panel.txt   |  45 
>>>  drivers/gpu/drm/bridge/Kconfig |   6 +
>>>  drivers/gpu/drm/bridge/Makefile|   1 +
>>>  drivers/gpu/drm/bridge/bridge_panel.c  | 240 
>>> +
>>>  drivers/gpu/drm/bridge/ptn3460.c   |  21 +-
>>>  drivers/gpu/drm/drm_crtc.c |  13 +-
>>>  include/drm/bridge/bridge_panel.h  |  37 
>>>  include/drm/drm_crtc.h |   2 +
>>>  8 files changed, 360 insertions(+), 5 deletions(-)
>>>  create mode 100644 
>>> Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
>>>  create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c
>>>  create mode 100644 include/drm/bridge/bridge_panel.h
>>>

[RFC V2 0/3] drm/bridge: panel and chaining

[Rob Clark]

On Thu, May 8, 2014 at 7:57 AM, Inki Dae  wrote:
> On 2014? 05? 08? 19:52, Ajay kumar wrote:
>> +Dave
>> +Thierry
>>
>> On Thu, May 8, 2014 at 1:14 PM, Inki Dae  wrote:
>>>
>>> Just re-sending with text mode. Sorry for this.
>>>
>>>
>>> On 2014? 05? 08? 15:41, Andrzej Hajda wrote:
 On 05/05/2014 09:52 PM, Ajay Kumar wrote:
> This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree 
> at:
> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
>
> I have just put up Rob's and Sean's idea of chaining up the bridges
> in code, and have implemented basic panel controls as a chained bridge.
> This works well with ptn3460 bridge chip on exynos5250-snow board.
>
> Still need to make use of standard list calls and figure out proper way
> of deleting the bridge chain. So, this is just a rough version.

 As I understand this patchset tries to solve two things:
 1. Implement panel as drm_bridge, to ease support for hardware chains:
 Crtc -> Encoder -> Bridge -> Panel
 2. Add support to drm_bridge chaining, to allow software chains:
 drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel

 It is done using Russian doll schema, ops from the bridge calls the same
 ops from the next bridge and the next bridge ops can do the same.

 This schema means that all the bridges including the last one are seen
 from the drm core point of view as a one big drm_bridge. Additionally in
 this particular case, the first bridge (ptn3460) implements connector
 so it is hard to guess what is the location of the 2nd bridge in video
 stream chain, sometimes it is after the connector, sometimes before.
 All this is quite confusing.

 But if you look at the bridge from upstream video interface point of
 view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
 video input side acts as a panel. On the output side it expects a panel,
 lvds panel in this case.

 So why not implement ptn3460 bridge as drm_panel which internally uses
 another drm_panel. With this approach everything fits much better.
 You do not need those (pre|post)_(enable|disable) calls, you do not need
 to implement connector in the bridge and you have a driver following
 linux driver model. And no single bit changed in drm core.

 I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2].
 It was not accepted as Inki preferred drm_bridge but as I see the
>>>
>>> Yes, in above email threads, I disagreed to using drm_panel framework
>>> for bridge device, especially, to implement connector/encoder to crtc
>>> driver.
>>>
>>> However, I thought that it'd be more generic way that lvds drivers use
>>> driver-model, and the use of drm_panel infrastructure would be suitable
>>> to doing this.
>>>
>>> So my intention in turn, was that LVDS driver uses integrated drm_bridge
>>> based on drm_panel infrastructure[1], and RFC patch[2] for it. This way
>>> is same as your proposal in the point that LVDS and Panel drivers use
>>> driver-model. The only different point is that LVDS driver has some ops
>>> specific to LVDS device, not using existing ops of drm_panel commonly:
>>> we may need to consider the characteristic of LVDS device.
>>>
>>> [1]:http://www.spinics.net/lists/dri-devel/msg5.html
>>> [2]:http://www.spinics.net/lists/dri-devel/msg55658.html
>>>
>>> Thanks,
>>> Inki Dae
>> I am just consolidating the discussion happening here.
>> 1) This patchset started from a discussion wherein I tried to combine
>> drm_panel with drm_bridge.
>> https://www.mail-archive.com/linux-samsung-soc at 
>> vger.kernel.org/msg28943.html
>> 2) Sean and Rob suggested to implement a chain of bridges and then
>> consider adding
>> basic panel controls as a bridge.
>> 3) Andrej's idea is to drop the existing bridge infrastructure and
>> implement ptn3460 using drm_panel,
>> the same way he has implemented
>> http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559.
>> 4) Inki's suggestion is to combine drm_bridge, drm_panel and
>> drm_enhance into a single
>> drm_hw_block.
>>
>
> And more thing, we would need to consider image enhancer device placed
> between crtc and connector/encoder devices. And it'd better to rename
> drm_hw_block to drm_bridge, and existing drm_bridge relevant codes
> should be removed.

I don't object to changing the name to hw_block or something else.
Although to avoid introducing too much confusion in this discussion,
for now I am calling it bridge/drm_bridge_funcs/etc for now ;-)

BR,
-R

> Thanks,
> Inki Dae
>
>> I am currently trying to implement (2):chaining of bridges, and I
>> think we have not yet
>> reached to a consensus. So adding few other people from drm community
>> to comment.
>>
>> Regards,
>> Ajay
>>
 problems with drm_bridges I have decide to attract attention to much
 more cleaner solution.

 

[RFC V2 0/3] drm/bridge: panel and chaining

[Rob Clark]

On Thu, May 8, 2014 at 2:41 AM, Andrzej Hajda  wrote:
> On 05/05/2014 09:52 PM, Ajay Kumar wrote:
>> This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at:
>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
>>
>> I have just put up Rob's and Sean's idea of chaining up the bridges
>> in code, and have implemented basic panel controls as a chained bridge.
>> This works well with ptn3460 bridge chip on exynos5250-snow board.
>>
>> Still need to make use of standard list calls and figure out proper way
>> of deleting the bridge chain. So, this is just a rough version.
>
> As I understand this patchset tries to solve two things:
> 1. Implement panel as drm_bridge, to ease support for hardware chains:
> Crtc -> Encoder -> Bridge -> Panel
> 2. Add support to drm_bridge chaining, to allow software chains:
> drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
>
> It is done using Russian doll schema, ops from the bridge calls the same
> ops from the next bridge and the next bridge ops can do the same.
>
> This schema means that all the bridges including the last one are seen
> from the drm core point of view as a one big drm_bridge. Additionally in
> this particular case, the first bridge (ptn3460) implements connector
> so it is hard to guess what is the location of the 2nd bridge in video
> stream chain, sometimes it is after the connector, sometimes before.
> All this is quite confusing.
>
> But if you look at the bridge from upstream video interface point of
> view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
> video input side acts as a panel. On the output side it expects a panel,
> lvds panel in this case.

tbh, this is mostly about what we call it.  Perhaps "bridge" isn't the
best name.. I wouldn't object to changing it.

But my thinking was to leave in drm_panel_funcs things that are just
needed by the connector (get_modes().. and maybe some day we need
detect/etc).  Then leave everything else in drm_bridge_funcs.  A panel
could (if needed) implement both interfaces.

That is basically the same as what you are proposing, but without
renaming bridge to panel ;-)

BR,
-R

> So why not implement ptn3460 bridge as drm_panel which internally uses
> another drm_panel. With this approach everything fits much better.
> You do not need those (pre|post)_(enable|disable) calls, you do not need
> to implement connector in the bridge and you have a driver following
> linux driver model. And no single bit changed in drm core.
>
> I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2].
> It was not accepted as Inki preferred drm_bridge but as I see the
> problems with drm_bridges I have decide to attract attention to much
> more cleaner solution.
>
> [1]: http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559
> [2]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/27044
>
> Regards
> Andrzej
>
>
>>
>> Ajay Kumar (3):
>>   [RFC V2 1/3] drm: implement chaining of drm bridges
>>   [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges
>>   [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
>>
>>  .../bindings/drm/bridge/bridge_panel.txt   |  45 
>>  drivers/gpu/drm/bridge/Kconfig |   6 +
>>  drivers/gpu/drm/bridge/Makefile|   1 +
>>  drivers/gpu/drm/bridge/bridge_panel.c  | 240 
>> +
>>  drivers/gpu/drm/bridge/ptn3460.c   |  21 +-
>>  drivers/gpu/drm/drm_crtc.c |  13 +-
>>  include/drm/bridge/bridge_panel.h  |  37 
>>  include/drm/drm_crtc.h |   2 +
>>  8 files changed, 360 insertions(+), 5 deletions(-)
>>  create mode 100644 
>> Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
>>  create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c
>>  create mode 100644 include/drm/bridge/bridge_panel.h
>>
>

[RFC V2 0/3] drm/bridge: panel and chaining

[Andrzej Hajda]

On 05/05/2014 09:52 PM, Ajay Kumar wrote:
> This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at:
> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
> 
> I have just put up Rob's and Sean's idea of chaining up the bridges
> in code, and have implemented basic panel controls as a chained bridge.
> This works well with ptn3460 bridge chip on exynos5250-snow board.
> 
> Still need to make use of standard list calls and figure out proper way
> of deleting the bridge chain. So, this is just a rough version.

As I understand this patchset tries to solve two things:
1. Implement panel as drm_bridge, to ease support for hardware chains:
Crtc -> Encoder -> Bridge -> Panel
2. Add support to drm_bridge chaining, to allow software chains:
drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel

It is done using Russian doll schema, ops from the bridge calls the same
ops from the next bridge and the next bridge ops can do the same.

This schema means that all the bridges including the last one are seen
from the drm core point of view as a one big drm_bridge. Additionally in
this particular case, the first bridge (ptn3460) implements connector
so it is hard to guess what is the location of the 2nd bridge in video
stream chain, sometimes it is after the connector, sometimes before.
All this is quite confusing.

But if you look at the bridge from upstream video interface point of
view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
video input side acts as a panel. On the output side it expects a panel,
lvds panel in this case.

So why not implement ptn3460 bridge as drm_panel which internally uses
another drm_panel. With this approach everything fits much better.
You do not need those (pre|post)_(enable|disable) calls, you do not need
to implement connector in the bridge and you have a driver following
linux driver model. And no single bit changed in drm core.

I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2].
It was not accepted as Inki preferred drm_bridge but as I see the
problems with drm_bridges I have decide to attract attention to much
more cleaner solution.

[1]: http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559
[2]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/27044

Regards
Andrzej


> 
> Ajay Kumar (3):
>   [RFC V2 1/3] drm: implement chaining of drm bridges
>   [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges
>   [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
> 
>  .../bindings/drm/bridge/bridge_panel.txt   |  45 
>  drivers/gpu/drm/bridge/Kconfig |   6 +
>  drivers/gpu/drm/bridge/Makefile|   1 +
>  drivers/gpu/drm/bridge/bridge_panel.c  | 240 
> +
>  drivers/gpu/drm/bridge/ptn3460.c   |  21 +-
>  drivers/gpu/drm/drm_crtc.c |  13 +-
>  include/drm/bridge/bridge_panel.h  |  37 
>  include/drm/drm_crtc.h |   2 +
>  8 files changed, 360 insertions(+), 5 deletions(-)
>  create mode 100644 
> Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
>  create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c
>  create mode 100644 include/drm/bridge/bridge_panel.h
> 

[RFC V2 0/3] drm/bridge: panel and chaining

[Ajay Kumar]

This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at:
git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git

I have just put up Rob's and Sean's idea of chaining up the bridges
in code, and have implemented basic panel controls as a chained bridge.
This works well with ptn3460 bridge chip on exynos5250-snow board.

Still need to make use of standard list calls and figure out proper way
of deleting the bridge chain. So, this is just a rough version.

Ajay Kumar (3):
  [RFC V2 1/3] drm: implement chaining of drm bridges
  [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges
  [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining

 .../bindings/drm/bridge/bridge_panel.txt   |  45 
 drivers/gpu/drm/bridge/Kconfig |   6 +
 drivers/gpu/drm/bridge/Makefile|   1 +
 drivers/gpu/drm/bridge/bridge_panel.c  | 240 +
 drivers/gpu/drm/bridge/ptn3460.c   |  21 +-
 drivers/gpu/drm/drm_crtc.c |  13 +-
 include/drm/bridge/bridge_panel.h  |  37 
 include/drm/drm_crtc.h |   2 +
 8 files changed, 360 insertions(+), 5 deletions(-)
 create mode 100644 
Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
 create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c
 create mode 100644 include/drm/bridge/bridge_panel.h

-- 
1.8.1.2