[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Philipp Zabel]

Hi Russell,

Am Montag, den 27.10.2014, 23:57 + schrieb Russell King - ARM Linux:
> On Mon, Oct 27, 2014 at 11:26:30PM +0100, Daniel Vetter wrote:
> > Looking at the of_drm_find_panel function I actually wonder how that
> > works - the drm_panel doesn't really need to stick around afaics.
> > After all panel_list is global so some other driver can unload.
> > Russell's of support for possible_crtcs code works differently since
> > it only looks at per-drm_device lists.
> > 
> > This bridge code here though suffers from the same. So to me this
> > looks rather fishy.
> > 
> > It doesn't help that we have drm_of.[hc] around but not all the of
> > code is in there. Adding Russell too.
> 
> I rather dropped the ball with imx-drm when things became difficult with
> asking Greg to pull my git tree - and as a result, I decided that I would
> no longer be helping with patch submission for imx-drm, nor trying to get
> it out of the staging tree anymore.

In that case I'd like to take up the ball.

In my opinion the driver is now in a state in which it could be moved to
gpu/drm first and then fixed up there (all the blockers in
drivers/staging/imx-drm/TODO are gone). That would also give us a single
path for all imx-drm/ipu-v3 related patches again. My main motivation to
put it into staging in the first place was that the device tree bindings
had to be worked out, but now it only results in a coordination
nightmare.

regards
Philipp

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Ajay kumar]

On Mon, Nov 3, 2014 at 1:31 PM, Thierry Reding  
wrote:
> On Fri, Oct 31, 2014 at 04:51:43PM +0100, Daniel Vetter wrote:
>> On Wed, Oct 29, 2014 at 10:14:37AM +0100, Thierry Reding wrote:
>> > On Wed, Oct 29, 2014 at 09:57:02AM +0100, Daniel Vetter wrote:
>> > > That makes the entire thing a bit non-trivial, which is why I think it
>> > > would be better as some generic helper. Which then gets embedded or
>> > > instantiated for specific cases, like dt&drm_panel or dt&drm_bridge.
>> > > Or maybe even acpi&drm_bridge, who knows ;-)
>> >
>> > I worry a little about type safety. How will this generic helper know
>> > what registry to look in for? Or conversely, if all these resources are
>> > added to a single registry how do you know that they're of the correct
>> > type? Failing to ensure this could cause situations where you're asking
>> > for a panel and get a bridge in return because you've wrongly wired it
>> > up in device tree for example.
>> >
>> > But perhaps if both the registry and the device parts are turned into
>> > helpers we could still have a single core implementation and then
>> > instantiate that for each type, something roughly like this:
>> >
>> > struct registry {
>> > struct list_head list;
>> > struct mutex lock;
>> > };
>> >
>> > struct registry_record {
>> > struct list_head list;
>> > struct module *owner;
>> > struct kref *ref;
>> >
>> > struct device *dev;
>> > };
>> >
>> > int registry_add(struct registry *registry, struct registry_record 
>> > *record)
>> > {
>> > ...
>> > try_module_get(record->owner);
>> > ...
>> > }
>> >
>> > struct registry_record *registry_find_by_of_node(struct registry 
>> > *registry,
>> >  struct device_node 
>> > *np)
>> > {
>> > ...
>> > kref_get(...);
>> > ...
>> > }
>> >
>> > That way it should be possible to embed these into other structures,
>> > like so:
>> >
>> > struct drm_panel {
>> > struct registry_record record;
>> > ...
>> > };
>> >
>> > static struct registry drm_panels;
>> >
>> > int drm_panel_add(struct drm_panel *panel)
>> > {
>> > return registry_add(&drm_panels, &panel->record);
>> > }
>> >
>> > struct drm_panel *of_drm_panel_find(struct device_node *np)
>> > {
>> > struct registry_record *record;
>> >
>> > record = registry_find_by_of_node(&drm_panels, np);
>> >
>> > return container_of(record, struct drm_panel, record);
>> > }
>> >
>> > Is that what you had in mind?
>>
>> Yeah I've thought that we should instantiate using macros even, so that we
>> have per-type registries. So you'd smash the usual set of
>> DECLARE/DEFINE_AUX_DEV_REGISTRY into headers/source files, and they'd take
>> a (name, key, value) tripled. For the example here(of_drm_panel, struct
>> device_node *, struct drm_panel *) or similar. I might be hand-waving over
>> a few too many details though ;-)
>
> Okay, I'll take a stab at this and see if I can convert DRM panel to it.
It would be great if you can do this soon. I would anyhow need a reference
for converting bridge framework as per Daniel's requirement :)

Ajay

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Daniel Vetter]

On Mon, Nov 03, 2014 at 09:06:04AM +0100, Thierry Reding wrote:
> On Fri, Oct 31, 2014 at 04:59:40PM +0100, Daniel Vetter wrote:
> > On Wed, Oct 29, 2014 at 10:16:49AM +0100, Thierry Reding wrote:
> > > On Wed, Oct 29, 2014 at 08:51:27AM +0100, Daniel Vetter wrote:
> > > > On Tue, Oct 28, 2014 at 03:29:47PM +0100, Thierry Reding wrote:
> > > > > On Mon, Oct 27, 2014 at 11:20:31PM +0100, Daniel Vetter wrote:
> > > > > > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul  > > > > > chromium.org> wrote:
> > > > > > >>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
> > > > > > >>>   * @driver_private: pointer to the bridge driver's internal 
> > > > > > >>> context
> > > > > > >>>   */
> > > > > > >>>  struct drm_bridge {
> > > > > > >>> - struct drm_device *dev;
> > > > > > >>> + struct device *dev;
> > > > > > >>
> > > > > > >> Please don't rename the ->dev pointer into drm. Because _all_ 
> > > > > > >> the other
> > > > > > >> drm structures still call it ->dev. Also, can't we use struct 
> > > > > > >> device_node
> > > > > > >> here like we do in the of helpers Russell added? See 
> > > > > > >> 7e435aad38083
> > > > > > >>
> > > > > > >
> > > > > > > I think this is modeled after the naming in drm_panel, FWIW. 
> > > > > > > However,
> > > > > > > seems reasonable to keep the device_node instead.
> > > > > > 
> > > > > > Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like 
> > > > > > with
> > > > > > drm_crtc drop the struct device and go directly to a struct
> > > > > > device_node. Since we don't really need the sturct device, the only
> > > > > > thing we care about is the of_node. For added bonus wrap an #ifdef
> > > > > > CONFIG_OF around all the various struct device_node in drm_foo.h.
> > > > > > Should be all fairly simple to pull off with cocci.
> > > > > > 
> > > > > > Thierry?
> > > > > 
> > > > > The struct device * is in DRM panel because there's nothing device 
> > > > > tree
> > > > > specific about the concept. Having a struct device_node * instead 
> > > > > would
> > > > > indicate that it can only be used with a device tree, whereas the
> > > > > framework doesn't care the tiniest bit what type of device we have.
> > > > > 
> > > > > While the trend clearly is to use more device tree, I don't think we
> > > > > should make it impossible for anybody else to use these frameworks.
> > > > > 
> > > > > There are other advantages to keeping a struct device *, like having
> > > > > access to the proper device and its name. Also you get access to the
> > > > > device_node * via dev->of_node anyway. I don't see any advantage in
> > > > > switching to just a struct device_node *, only disadvantages.
> > > > 
> > > > Well the idea is to make the lookup key specific, and conditional on
> > > > #CONFIG_OF. If there's going to be another neat way to enumerate 
> > > > platform
> > > > devices then I think we should add that, too. Or maybe we should have a
> > > > void *platform_data or so.
> > > > 
> > > > The reason I really don't want a struct device * in core drm structures 
> > > > is
> > > > that two releases down the road people will have found tons of really
> > > > great ways to abuse them and re-create a midlayer. DRM core really 
> > > > should
> > > > only care about the sw objects and not be hw specific at all. Heck 
> > > > there's
> > > > not even an requirement to have any piece of actual hw, you could write 
> > > > a
> > > > completely fake drm driver (for e.g. testing like the new v4l driver).
> > > > 
> > > > Tbh I wonder a bit why we even have this registery embedded into the 
> > > > core
> > > > drm objects. Essentially the only thing you're doing is a list that maps
> > > > some platform specific key onto some subsystem specific driver structure
> > > > or fails the lookup. So instead of putting all these low-level details
> > > > into drm core structures can't we just have a generic hashtable/list for
> > > > this, plus some static inline helpers that cast the void * you get into
> > > > the one you want?
> > > > 
> > > > I also get the feeling that this really should be in the driver core 
> > > > (like
> > > > the component helpers), and that we should think a lot harder about
> > > > lifetimes and refcounting (see my other reply on that).
> > > 
> > > Yes, that sounds very useful indeed. Also see my reply to yours. =)
> > 
> > Just replying here with some of the irc discussions we've had. Since
> > drm_bridge/panel isn't a core drm interface object exposed to userspace
> > it's less critical. I still think that wasting a few brain cycles to have
> > a clear separation between the abstract interface object and how to bind
> > and unbind the pieces together is worthwhile, even though the cost when
> > getting it wrong is much less severe than in the case of a mandatory piece
> > of core infrastructure.
> > 
> > I think in general the recent armsoc motivated drm infrastructure lacks a
> > bit in attention to these details. E.g. the cma helpers are built into

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Thierry Reding]

On Fri, Oct 31, 2014 at 04:59:40PM +0100, Daniel Vetter wrote:
> On Wed, Oct 29, 2014 at 10:16:49AM +0100, Thierry Reding wrote:
> > On Wed, Oct 29, 2014 at 08:51:27AM +0100, Daniel Vetter wrote:
> > > On Tue, Oct 28, 2014 at 03:29:47PM +0100, Thierry Reding wrote:
> > > > On Mon, Oct 27, 2014 at 11:20:31PM +0100, Daniel Vetter wrote:
> > > > > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul  
> > > > > wrote:
> > > > > >>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
> > > > > >>>   * @driver_private: pointer to the bridge driver's internal 
> > > > > >>> context
> > > > > >>>   */
> > > > > >>>  struct drm_bridge {
> > > > > >>> - struct drm_device *dev;
> > > > > >>> + struct device *dev;
> > > > > >>
> > > > > >> Please don't rename the ->dev pointer into drm. Because _all_ the 
> > > > > >> other
> > > > > >> drm structures still call it ->dev. Also, can't we use struct 
> > > > > >> device_node
> > > > > >> here like we do in the of helpers Russell added? See 7e435aad38083
> > > > > >>
> > > > > >
> > > > > > I think this is modeled after the naming in drm_panel, FWIW. 
> > > > > > However,
> > > > > > seems reasonable to keep the device_node instead.
> > > > > 
> > > > > Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with
> > > > > drm_crtc drop the struct device and go directly to a struct
> > > > > device_node. Since we don't really need the sturct device, the only
> > > > > thing we care about is the of_node. For added bonus wrap an #ifdef
> > > > > CONFIG_OF around all the various struct device_node in drm_foo.h.
> > > > > Should be all fairly simple to pull off with cocci.
> > > > > 
> > > > > Thierry?
> > > > 
> > > > The struct device * is in DRM panel because there's nothing device tree
> > > > specific about the concept. Having a struct device_node * instead would
> > > > indicate that it can only be used with a device tree, whereas the
> > > > framework doesn't care the tiniest bit what type of device we have.
> > > > 
> > > > While the trend clearly is to use more device tree, I don't think we
> > > > should make it impossible for anybody else to use these frameworks.
> > > > 
> > > > There are other advantages to keeping a struct device *, like having
> > > > access to the proper device and its name. Also you get access to the
> > > > device_node * via dev->of_node anyway. I don't see any advantage in
> > > > switching to just a struct device_node *, only disadvantages.
> > > 
> > > Well the idea is to make the lookup key specific, and conditional on
> > > #CONFIG_OF. If there's going to be another neat way to enumerate platform
> > > devices then I think we should add that, too. Or maybe we should have a
> > > void *platform_data or so.
> > > 
> > > The reason I really don't want a struct device * in core drm structures is
> > > that two releases down the road people will have found tons of really
> > > great ways to abuse them and re-create a midlayer. DRM core really should
> > > only care about the sw objects and not be hw specific at all. Heck there's
> > > not even an requirement to have any piece of actual hw, you could write a
> > > completely fake drm driver (for e.g. testing like the new v4l driver).
> > > 
> > > Tbh I wonder a bit why we even have this registery embedded into the core
> > > drm objects. Essentially the only thing you're doing is a list that maps
> > > some platform specific key onto some subsystem specific driver structure
> > > or fails the lookup. So instead of putting all these low-level details
> > > into drm core structures can't we just have a generic hashtable/list for
> > > this, plus some static inline helpers that cast the void * you get into
> > > the one you want?
> > > 
> > > I also get the feeling that this really should be in the driver core (like
> > > the component helpers), and that we should think a lot harder about
> > > lifetimes and refcounting (see my other reply on that).
> > 
> > Yes, that sounds very useful indeed. Also see my reply to yours. =)
> 
> Just replying here with some of the irc discussions we've had. Since
> drm_bridge/panel isn't a core drm interface object exposed to userspace
> it's less critical. I still think that wasting a few brain cycles to have
> a clear separation between the abstract interface object and how to bind
> and unbind the pieces together is worthwhile, even though the cost when
> getting it wrong is much less severe than in the case of a mandatory piece
> of core infrastructure.
> 
> I think in general the recent armsoc motivated drm infrastructure lacks a
> bit in attention to these details. E.g. the cma helpers are built into the
> drm.ko module, but clearly are auxilliary library code. So they should be
> pulled out and the headers clean up, so that we have a clear separation
> between core and helpers. Otherwise someone will sooner or later screw up
> and insert a helper depency into the core, and then we've started with the
> midlayer mess. Same goes with drm_bridge/panel,

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Thierry Reding]

On Fri, Oct 31, 2014 at 04:51:43PM +0100, Daniel Vetter wrote:
> On Wed, Oct 29, 2014 at 10:14:37AM +0100, Thierry Reding wrote:
> > On Wed, Oct 29, 2014 at 09:57:02AM +0100, Daniel Vetter wrote:
> > > That makes the entire thing a bit non-trivial, which is why I think it
> > > would be better as some generic helper. Which then gets embedded or
> > > instantiated for specific cases, like dt&drm_panel or dt&drm_bridge.
> > > Or maybe even acpi&drm_bridge, who knows ;-)
> > 
> > I worry a little about type safety. How will this generic helper know
> > what registry to look in for? Or conversely, if all these resources are
> > added to a single registry how do you know that they're of the correct
> > type? Failing to ensure this could cause situations where you're asking
> > for a panel and get a bridge in return because you've wrongly wired it
> > up in device tree for example.
> > 
> > But perhaps if both the registry and the device parts are turned into
> > helpers we could still have a single core implementation and then
> > instantiate that for each type, something roughly like this:
> > 
> > struct registry {
> > struct list_head list;
> > struct mutex lock;
> > };
> > 
> > struct registry_record {
> > struct list_head list;
> > struct module *owner;
> > struct kref *ref;
> > 
> > struct device *dev;
> > };
> > 
> > int registry_add(struct registry *registry, struct registry_record 
> > *record)
> > {
> > ...
> > try_module_get(record->owner);
> > ...
> > }
> > 
> > struct registry_record *registry_find_by_of_node(struct registry 
> > *registry,
> >  struct device_node *np)
> > {
> > ...
> > kref_get(...);
> > ...
> > }
> > 
> > That way it should be possible to embed these into other structures,
> > like so:
> > 
> > struct drm_panel {
> > struct registry_record record;
> > ...
> > };
> > 
> > static struct registry drm_panels;
> > 
> > int drm_panel_add(struct drm_panel *panel)
> > {
> > return registry_add(&drm_panels, &panel->record);
> > }
> > 
> > struct drm_panel *of_drm_panel_find(struct device_node *np)
> > {
> > struct registry_record *record;
> > 
> > record = registry_find_by_of_node(&drm_panels, np);
> > 
> > return container_of(record, struct drm_panel, record);
> > }
> > 
> > Is that what you had in mind?
> 
> Yeah I've thought that we should instantiate using macros even, so that we
> have per-type registries. So you'd smash the usual set of
> DECLARE/DEFINE_AUX_DEV_REGISTRY into headers/source files, and they'd take
> a (name, key, value) tripled. For the example here(of_drm_panel, struct
> device_node *, struct drm_panel *) or similar. I might be hand-waving over
> a few too many details though ;-)

Okay, I'll take a stab at this and see if I can convert DRM panel to it.

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

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Daniel Vetter]

On Wed, Oct 29, 2014 at 10:09:04AM +0100, Andrzej Hajda wrote:
> On 10/29/2014 08:58 AM, Daniel Vetter wrote:
> > On Tue, Oct 28, 2014 at 04:05:34PM +0100, Thierry Reding wrote:
> >> On Tue, Oct 28, 2014 at 08:16:44PM +0530, Ajay kumar wrote:
> >>> On Tue, Oct 28, 2014 at 8:11 PM, Thierry Reding
> >>>  wrote:
>  On Tue, Oct 28, 2014 at 03:19:36PM +0100, Daniel Vetter wrote:
> > On Tue, Oct 28, 2014 at 1:28 PM, Ajay kumar  
> > wrote:
> >> On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter  
> >> wrote:
>  [...]
> >>> Hm, if you do this can you pls also update drm_panel accordingly? It
> >>> shouldn't be a lot of fuzz and would make things around drm+dt more
> >>> consistent.
> >> Are you talking about using struct device_node instead of struct 
> >> device?
> >> I guess you have misplaced the comment under the wrong section!
> >
> > Yeah, that should have been one up ;-)
> 
>  Like I said earlier, I don't think dropping struct device * in favour of
>  struct device_node * is a good idea.
> >>> I am not sure about drm_panel.
> >>> But, I am not really doing anything with the struct device pointer in
> >>> case of bridge.
> >>> So, just wondering if it is really needed?
> >>
> >> I think it's useful to have it just to send the right message. DRM panel
> >> and DRM bridge aren't specific to device tree. They are completely
> >> generic and can work with any type of device, whether it was
> >> instantiated from the device tree or some other infrastructure. Dropping
> >> struct device * will make it effectively useless on anything but DT. I
> >> don't think we should strive for that, even if only DT-enabled platforms
> >> currently use them.
> > 
> > See my other reply, but I now think we should put neither into drm
> > structures. This "find me the driver structures for this device" problem
> > looks really generic, and it has nothing to do with the drm structures and
> > concepts like bridges/panels at all. It shouldn't be in there at all.
> > 
> > Adding it looks very much like reintroducing the drm midlayer that we just
> > finally made obsolete, just not at the top-level (struct drm_device) but
> > at a bunch of leaf nodes. I expect all the same issues though. And I'm
> > definitely not looking to de-midlayer more stuff that we're just adding.
> > 
> > Imo this should be solved as a separate helper thing, maybe in the driver
> > core akin to the component helpers from Russell.
> > -Daniel
> > 
> 
> As I understand you want something generic to look for panels, bridges,
> whatever and, like components, it should allow to safe hot plug/unplug.
> I have proposed such thing few months ago [1]. Have you looked at it?
> 
> [1]: https://lkml.org/lkml/2014/4/30/345

Yeah I think I've looked but wasn't convinced. I do agree though that we
should definitely aim for something in the driver core. Since if Greg
doesn't like it it's not suddenly better if we just hide it in the drm
subsystem. This really smells like a generic issue - after all we already
have a two implementations with bridges&panels already.

So maybe we need to augment the component framework with the possibility
to add additional devices later on at runtime, or something similar. Not
really sure since I don't have actual practice with these issues since
i915 doesn't (yet) have these problems.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Daniel Vetter]

On Wed, Oct 29, 2014 at 10:16:49AM +0100, Thierry Reding wrote:
> On Wed, Oct 29, 2014 at 08:51:27AM +0100, Daniel Vetter wrote:
> > On Tue, Oct 28, 2014 at 03:29:47PM +0100, Thierry Reding wrote:
> > > On Mon, Oct 27, 2014 at 11:20:31PM +0100, Daniel Vetter wrote:
> > > > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul  
> > > > wrote:
> > > > >>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
> > > > >>>   * @driver_private: pointer to the bridge driver's internal context
> > > > >>>   */
> > > > >>>  struct drm_bridge {
> > > > >>> - struct drm_device *dev;
> > > > >>> + struct device *dev;
> > > > >>
> > > > >> Please don't rename the ->dev pointer into drm. Because _all_ the 
> > > > >> other
> > > > >> drm structures still call it ->dev. Also, can't we use struct 
> > > > >> device_node
> > > > >> here like we do in the of helpers Russell added? See 7e435aad38083
> > > > >>
> > > > >
> > > > > I think this is modeled after the naming in drm_panel, FWIW. However,
> > > > > seems reasonable to keep the device_node instead.
> > > > 
> > > > Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with
> > > > drm_crtc drop the struct device and go directly to a struct
> > > > device_node. Since we don't really need the sturct device, the only
> > > > thing we care about is the of_node. For added bonus wrap an #ifdef
> > > > CONFIG_OF around all the various struct device_node in drm_foo.h.
> > > > Should be all fairly simple to pull off with cocci.
> > > > 
> > > > Thierry?
> > > 
> > > The struct device * is in DRM panel because there's nothing device tree
> > > specific about the concept. Having a struct device_node * instead would
> > > indicate that it can only be used with a device tree, whereas the
> > > framework doesn't care the tiniest bit what type of device we have.
> > > 
> > > While the trend clearly is to use more device tree, I don't think we
> > > should make it impossible for anybody else to use these frameworks.
> > > 
> > > There are other advantages to keeping a struct device *, like having
> > > access to the proper device and its name. Also you get access to the
> > > device_node * via dev->of_node anyway. I don't see any advantage in
> > > switching to just a struct device_node *, only disadvantages.
> > 
> > Well the idea is to make the lookup key specific, and conditional on
> > #CONFIG_OF. If there's going to be another neat way to enumerate platform
> > devices then I think we should add that, too. Or maybe we should have a
> > void *platform_data or so.
> > 
> > The reason I really don't want a struct device * in core drm structures is
> > that two releases down the road people will have found tons of really
> > great ways to abuse them and re-create a midlayer. DRM core really should
> > only care about the sw objects and not be hw specific at all. Heck there's
> > not even an requirement to have any piece of actual hw, you could write a
> > completely fake drm driver (for e.g. testing like the new v4l driver).
> > 
> > Tbh I wonder a bit why we even have this registery embedded into the core
> > drm objects. Essentially the only thing you're doing is a list that maps
> > some platform specific key onto some subsystem specific driver structure
> > or fails the lookup. So instead of putting all these low-level details
> > into drm core structures can't we just have a generic hashtable/list for
> > this, plus some static inline helpers that cast the void * you get into
> > the one you want?
> > 
> > I also get the feeling that this really should be in the driver core (like
> > the component helpers), and that we should think a lot harder about
> > lifetimes and refcounting (see my other reply on that).
> 
> Yes, that sounds very useful indeed. Also see my reply to yours. =)

Just replying here with some of the irc discussions we've had. Since
drm_bridge/panel isn't a core drm interface object exposed to userspace
it's less critical. I still think that wasting a few brain cycles to have
a clear separation between the abstract interface object and how to bind
and unbind the pieces together is worthwhile, even though the cost when
getting it wrong is much less severe than in the case of a mandatory piece
of core infrastructure.

I think in general the recent armsoc motivated drm infrastructure lacks a
bit in attention to these details. E.g. the cma helpers are built into the
drm.ko module, but clearly are auxilliary library code. So they should be
pulled out and the headers clean up, so that we have a clear separation
between core and helpers. Otherwise someone will sooner or later screw up
and insert a helper depency into the core, and then we've started with the
midlayer mess. Same goes with drm_bridge/panel, which didn't even bother
to have separate headers from the core modeset header drm_crtc.h.

So would be great if someone could invest a bit of time into cleaning this
up. Writing proper api docs also helps a lot with achieving a clean and
sensible split ;-)
-Dan

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Daniel Vetter]

On Thu, Oct 30, 2014 at 10:09:28AM +, Russell King - ARM Linux wrote:
> On Thu, Oct 30, 2014 at 11:01:02AM +0100, Andrzej Hajda wrote:
> > On 10/29/2014 10:14 AM, Thierry Reding wrote:
> > > On Wed, Oct 29, 2014 at 09:57:02AM +0100, Daniel Vetter wrote:
> > >> I think we nee try_get_module for the code and kref on the actual data
> > >> structures.
> > > 
> > > Agreed, that should do the trick. We'd probably need some sort of logic
> > > to also make operations return something like -ENODEV when the
> > > underlying device has disappeared. I think David had introduced
> > > something similar for DRM device not so long ago?
> > 
> > If the underlying device disappears it would be good to receive
> > notification anyway to trigger DRM HPD event. And if we have the
> > notification, we can release references to the device smoothly. We do
> > not need to play tricky games with krefs, zombie data and module
> > refcounting.
> 
> Any solution which thinks it needs to lock modules into the core is
> fundamentally broken.  It totally misses the point.
> 
> While you can lock a module into the core using try_get_module(), that
> doesn't stop the device itself being unbound from a driver.  Soo many
> people have fallen into that trap.  They write their device driver,
> along with some kind of framework which they make use try_get_module().
> They think its safe.  When you then echo the device name into the
> driver's unbind sysfs file, all hell breaks loose and the kernel oopses.
> 
> try_get_module is /totally/ useless for ensuring that things stick around.
> 
> The reality is that you can't make devices stick around.  Once that
> remove callback from the driver layer is called, that's it, the device
> _is_ going away whether you like it or not.  You can't stop it.  It's
> no good returning -EBUSY, because the remove return code is ignored.
> 
> What's more scarey is when you consider that in a real hotplug
> situation, when the remove callback is called, the device has
> /already/ gone.
> 
> So please, stop thinking that try_get_module() has some magic solution.
> Any "solution" to device lifetimes using try_get_module() totally misses
> the problem, and is just mere obfuscation and actually a bug in itself.

We need proper refcounting ofc, but we also need to make sure that as long
as the thing is around the text section for the final unref (at least
that) doesn't go poof. I'd prefer if the framework takes care of that
detail and drivers could just supply a THIS_MODULE at the right place.

But fully agree on your overall point that try_get_module alone is pure
snake oil.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Daniel Vetter]

On Wed, Oct 29, 2014 at 10:14:37AM +0100, Thierry Reding wrote:
> On Wed, Oct 29, 2014 at 09:57:02AM +0100, Daniel Vetter wrote:
> > That makes the entire thing a bit non-trivial, which is why I think it
> > would be better as some generic helper. Which then gets embedded or
> > instantiated for specific cases, like dt&drm_panel or dt&drm_bridge.
> > Or maybe even acpi&drm_bridge, who knows ;-)
> 
> I worry a little about type safety. How will this generic helper know
> what registry to look in for? Or conversely, if all these resources are
> added to a single registry how do you know that they're of the correct
> type? Failing to ensure this could cause situations where you're asking
> for a panel and get a bridge in return because you've wrongly wired it
> up in device tree for example.
> 
> But perhaps if both the registry and the device parts are turned into
> helpers we could still have a single core implementation and then
> instantiate that for each type, something roughly like this:
> 
>   struct registry {
>   struct list_head list;
>   struct mutex lock;
>   };
> 
>   struct registry_record {
>   struct list_head list;
>   struct module *owner;
>   struct kref *ref;
> 
>   struct device *dev;
>   };
> 
>   int registry_add(struct registry *registry, struct registry_record 
> *record)
>   {
>   ...
>   try_module_get(record->owner);
>   ...
>   }
> 
>   struct registry_record *registry_find_by_of_node(struct registry 
> *registry,
>struct device_node *np)
>   {
>   ...
>   kref_get(...);
>   ...
>   }
> 
> That way it should be possible to embed these into other structures,
> like so:
> 
>   struct drm_panel {
>   struct registry_record record;
>   ...
>   };
> 
>   static struct registry drm_panels;
> 
>   int drm_panel_add(struct drm_panel *panel)
>   {
>   return registry_add(&drm_panels, &panel->record);
>   }
> 
>   struct drm_panel *of_drm_panel_find(struct device_node *np)
>   {
>   struct registry_record *record;
> 
>   record = registry_find_by_of_node(&drm_panels, np);
> 
>   return container_of(record, struct drm_panel, record);
>   }
> 
> Is that what you had in mind?

Yeah I've thought that we should instantiate using macros even, so that we
have per-type registries. So you'd smash the usual set of
DECLARE/DEFINE_AUX_DEV_REGISTRY into headers/source files, and they'd take
a (name, key, value) tripled. For the example here(of_drm_panel, struct
device_node *, struct drm_panel *) or similar. I might be hand-waving over
a few too many details though ;-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Andrzej Hajda]

On 10/29/2014 10:14 AM, Thierry Reding wrote:
> On Wed, Oct 29, 2014 at 09:57:02AM +0100, Daniel Vetter wrote:
>> On Wed, Oct 29, 2014 at 09:38:23AM +0100, Thierry Reding wrote:
>>> On Wed, Oct 29, 2014 at 08:43:14AM +0100, Daniel Vetter wrote:
 On Tue, Oct 28, 2014 at 03:35:50PM +0100, Thierry Reding wrote:
> On Mon, Oct 27, 2014 at 11:26:30PM +0100, Daniel Vetter wrote:
>> On Mon, Oct 27, 2014 at 11:20 PM, Daniel Vetter  
>> wrote:
>>> On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul  
>>> wrote:
>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
>>   * @driver_private: pointer to the bridge driver's internal context
>>   */
>>  struct drm_bridge {
>> - struct drm_device *dev;
>> + struct device *dev;
>
> Please don't rename the ->dev pointer into drm. Because _all_ the 
> other
> drm structures still call it ->dev. Also, can't we use struct 
> device_node
> here like we do in the of helpers Russell added? See 7e435aad38083
>

 I think this is modeled after the naming in drm_panel, FWIW. However,
 seems reasonable to keep the device_node instead.
>>>
>>> Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with
>>> drm_crtc drop the struct device and go directly to a struct
>>> device_node. Since we don't really need the sturct device, the only
>>> thing we care about is the of_node. For added bonus wrap an #ifdef
>>> CONFIG_OF around all the various struct device_node in drm_foo.h.
>>> Should be all fairly simple to pull off with cocci.
>>>
>>> Thierry?
>>
>> Looking at the of_drm_find_panel function I actually wonder how that
>> works - the drm_panel doesn't really need to stick around afaics.
>> After all panel_list is global so some other driver can unload.
>> Russell's of support for possible_crtcs code works differently since
>> it only looks at per-drm_device lists.
>
> I don't understand. Panels are global resources that get registered by
> some driver that isn't tied to the DRM device until attached. It can't
> be in a per-DRM device list, because it's external to the device.
>
> And yes, they can go away when a driver is unloaded, though it's not the
> typical use-case.
>
>> This bridge code here though suffers from the same. So to me this
>> looks rather fishy.
>
> Well, this version of the DRM bridge support was written to be close to
> DRM panel, so it isn't really surprising that it's similar =), but like
> I said, I don't really understand what you think is wrong with it.

 You have a mutex to protect the global list of bridges/panels. But if you
 do a lookup you don't grab a reference count on the panel, so the moment
 you drop the list mutex the panel/bridge can go away.

 Yes usually you don't unload a driver on a soc but soc isn't everything
 and designing new stuff to not be hotunplug save isn't great either.
>>>
>>> Yeah, I certainly agree that adding proper reference counting would be a
>>> good thing. I think perhaps we could just take a reference on the struct
>>> device * to prevent it from disappearing.
>>>
>>> Although perhaps I misunderstand what you mean by "go away".
>>
>> I meant the drm_panel/bridge could go away any second, since nothing
>> prevents a module unload of the panel/bridge driver. So in theory you
>> could get the unregister call right after you've done the lookup. Which
>> means the bridge/panel pointer you've just returned is freed memory.
> 
> Ah yes, I see now.
> 
>> I think we nee try_get_module for the code and kref on the actual data
>> structures.
> 
> Agreed, that should do the trick. We'd probably need some sort of logic
> to also make operations return something like -ENODEV when the
> underlying device has disappeared. I think David had introduced
> something similar for DRM device not so long ago?

If the underlying device disappears it would be good to receive
notification anyway to trigger DRM HPD event. And if we have the
notification, we can release references to the device smoothly. We do
not need to play tricky games with krefs, zombie data and module
refcounting.
On the other side component framework uses notification callbacks
bind/unbind for master and components to smoothly attach/release
devices, why should it be done differently in this case.

Again, look at interface_tracker [1] it does exactly what you need.

[1]: https://lkml.org/lkml/2014/4/30/345

Regards
Andrzej

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Russell King - ARM Linux]

On Thu, Oct 30, 2014 at 11:01:02AM +0100, Andrzej Hajda wrote:
> On 10/29/2014 10:14 AM, Thierry Reding wrote:
> > On Wed, Oct 29, 2014 at 09:57:02AM +0100, Daniel Vetter wrote:
> >> I think we nee try_get_module for the code and kref on the actual data
> >> structures.
> > 
> > Agreed, that should do the trick. We'd probably need some sort of logic
> > to also make operations return something like -ENODEV when the
> > underlying device has disappeared. I think David had introduced
> > something similar for DRM device not so long ago?
> 
> If the underlying device disappears it would be good to receive
> notification anyway to trigger DRM HPD event. And if we have the
> notification, we can release references to the device smoothly. We do
> not need to play tricky games with krefs, zombie data and module
> refcounting.

Any solution which thinks it needs to lock modules into the core is
fundamentally broken.  It totally misses the point.

While you can lock a module into the core using try_get_module(), that
doesn't stop the device itself being unbound from a driver.  Soo many
people have fallen into that trap.  They write their device driver,
along with some kind of framework which they make use try_get_module().
They think its safe.  When you then echo the device name into the
driver's unbind sysfs file, all hell breaks loose and the kernel oopses.

try_get_module is /totally/ useless for ensuring that things stick around.

The reality is that you can't make devices stick around.  Once that
remove callback from the driver layer is called, that's it, the device
_is_ going away whether you like it or not.  You can't stop it.  It's
no good returning -EBUSY, because the remove return code is ignored.

What's more scarey is when you consider that in a real hotplug
situation, when the remove callback is called, the device has
/already/ gone.

So please, stop thinking that try_get_module() has some magic solution.
Any "solution" to device lifetimes using try_get_module() totally misses
the problem, and is just mere obfuscation and actually a bug in itself.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Thierry Reding]

On Wed, Oct 29, 2014 at 08:51:27AM +0100, Daniel Vetter wrote:
> On Tue, Oct 28, 2014 at 03:29:47PM +0100, Thierry Reding wrote:
> > On Mon, Oct 27, 2014 at 11:20:31PM +0100, Daniel Vetter wrote:
> > > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul  
> > > wrote:
> > > >>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
> > > >>>   * @driver_private: pointer to the bridge driver's internal context
> > > >>>   */
> > > >>>  struct drm_bridge {
> > > >>> - struct drm_device *dev;
> > > >>> + struct device *dev;
> > > >>
> > > >> Please don't rename the ->dev pointer into drm. Because _all_ the other
> > > >> drm structures still call it ->dev. Also, can't we use struct 
> > > >> device_node
> > > >> here like we do in the of helpers Russell added? See 7e435aad38083
> > > >>
> > > >
> > > > I think this is modeled after the naming in drm_panel, FWIW. However,
> > > > seems reasonable to keep the device_node instead.
> > > 
> > > Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with
> > > drm_crtc drop the struct device and go directly to a struct
> > > device_node. Since we don't really need the sturct device, the only
> > > thing we care about is the of_node. For added bonus wrap an #ifdef
> > > CONFIG_OF around all the various struct device_node in drm_foo.h.
> > > Should be all fairly simple to pull off with cocci.
> > > 
> > > Thierry?
> > 
> > The struct device * is in DRM panel because there's nothing device tree
> > specific about the concept. Having a struct device_node * instead would
> > indicate that it can only be used with a device tree, whereas the
> > framework doesn't care the tiniest bit what type of device we have.
> > 
> > While the trend clearly is to use more device tree, I don't think we
> > should make it impossible for anybody else to use these frameworks.
> > 
> > There are other advantages to keeping a struct device *, like having
> > access to the proper device and its name. Also you get access to the
> > device_node * via dev->of_node anyway. I don't see any advantage in
> > switching to just a struct device_node *, only disadvantages.
> 
> Well the idea is to make the lookup key specific, and conditional on
> #CONFIG_OF. If there's going to be another neat way to enumerate platform
> devices then I think we should add that, too. Or maybe we should have a
> void *platform_data or so.
> 
> The reason I really don't want a struct device * in core drm structures is
> that two releases down the road people will have found tons of really
> great ways to abuse them and re-create a midlayer. DRM core really should
> only care about the sw objects and not be hw specific at all. Heck there's
> not even an requirement to have any piece of actual hw, you could write a
> completely fake drm driver (for e.g. testing like the new v4l driver).
> 
> Tbh I wonder a bit why we even have this registery embedded into the core
> drm objects. Essentially the only thing you're doing is a list that maps
> some platform specific key onto some subsystem specific driver structure
> or fails the lookup. So instead of putting all these low-level details
> into drm core structures can't we just have a generic hashtable/list for
> this, plus some static inline helpers that cast the void * you get into
> the one you want?
> 
> I also get the feeling that this really should be in the driver core (like
> the component helpers), and that we should think a lot harder about
> lifetimes and refcounting (see my other reply on that).

Yes, that sounds very useful indeed. Also see my reply to yours. =)

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

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Thierry Reding]

On Wed, Oct 29, 2014 at 09:57:02AM +0100, Daniel Vetter wrote:
> On Wed, Oct 29, 2014 at 09:38:23AM +0100, Thierry Reding wrote:
> > On Wed, Oct 29, 2014 at 08:43:14AM +0100, Daniel Vetter wrote:
> > > On Tue, Oct 28, 2014 at 03:35:50PM +0100, Thierry Reding wrote:
> > > > On Mon, Oct 27, 2014 at 11:26:30PM +0100, Daniel Vetter wrote:
> > > > > On Mon, Oct 27, 2014 at 11:20 PM, Daniel Vetter  
> > > > > wrote:
> > > > > > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul  > > > > > chromium.org> wrote:
> > > > >  @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
> > > > >    * @driver_private: pointer to the bridge driver's internal 
> > > > >  context
> > > > >    */
> > > > >   struct drm_bridge {
> > > > >  - struct drm_device *dev;
> > > > >  + struct device *dev;
> > > > > >>>
> > > > > >>> Please don't rename the ->dev pointer into drm. Because _all_ the 
> > > > > >>> other
> > > > > >>> drm structures still call it ->dev. Also, can't we use struct 
> > > > > >>> device_node
> > > > > >>> here like we do in the of helpers Russell added? See 7e435aad38083
> > > > > >>>
> > > > > >>
> > > > > >> I think this is modeled after the naming in drm_panel, FWIW. 
> > > > > >> However,
> > > > > >> seems reasonable to keep the device_node instead.
> > > > > >
> > > > > > Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like 
> > > > > > with
> > > > > > drm_crtc drop the struct device and go directly to a struct
> > > > > > device_node. Since we don't really need the sturct device, the only
> > > > > > thing we care about is the of_node. For added bonus wrap an #ifdef
> > > > > > CONFIG_OF around all the various struct device_node in drm_foo.h.
> > > > > > Should be all fairly simple to pull off with cocci.
> > > > > >
> > > > > > Thierry?
> > > > > 
> > > > > Looking at the of_drm_find_panel function I actually wonder how that
> > > > > works - the drm_panel doesn't really need to stick around afaics.
> > > > > After all panel_list is global so some other driver can unload.
> > > > > Russell's of support for possible_crtcs code works differently since
> > > > > it only looks at per-drm_device lists.
> > > > 
> > > > I don't understand. Panels are global resources that get registered by
> > > > some driver that isn't tied to the DRM device until attached. It can't
> > > > be in a per-DRM device list, because it's external to the device.
> > > > 
> > > > And yes, they can go away when a driver is unloaded, though it's not the
> > > > typical use-case.
> > > > 
> > > > > This bridge code here though suffers from the same. So to me this
> > > > > looks rather fishy.
> > > > 
> > > > Well, this version of the DRM bridge support was written to be close to
> > > > DRM panel, so it isn't really surprising that it's similar =), but like
> > > > I said, I don't really understand what you think is wrong with it.
> > > 
> > > You have a mutex to protect the global list of bridges/panels. But if you
> > > do a lookup you don't grab a reference count on the panel, so the moment
> > > you drop the list mutex the panel/bridge can go away.
> > > 
> > > Yes usually you don't unload a driver on a soc but soc isn't everything
> > > and designing new stuff to not be hotunplug save isn't great either.
> > 
> > Yeah, I certainly agree that adding proper reference counting would be a
> > good thing. I think perhaps we could just take a reference on the struct
> > device * to prevent it from disappearing.
> > 
> > Although perhaps I misunderstand what you mean by "go away".
> 
> I meant the drm_panel/bridge could go away any second, since nothing
> prevents a module unload of the panel/bridge driver. So in theory you
> could get the unregister call right after you've done the lookup. Which
> means the bridge/panel pointer you've just returned is freed memory.

Ah yes, I see now.

> I think we nee try_get_module for the code and kref on the actual data
> structures.

Agreed, that should do the trick. We'd probably need some sort of logic
to also make operations return something like -ENODEV when the
underlying device has disappeared. I think David had introduced
something similar for DRM device not so long ago?

> That makes the entire thing a bit non-trivial, which is why I think it
> would be better as some generic helper. Which then gets embedded or
> instantiated for specific cases, like dt&drm_panel or dt&drm_bridge.
> Or maybe even acpi&drm_bridge, who knows ;-)

I worry a little about type safety. How will this generic helper know
what registry to look in for? Or conversely, if all these resources are
added to a single registry how do you know that they're of the correct
type? Failing to ensure this could cause situations where you're asking
for a panel and get a bridge in return because you've wrongly wired it
up in device tree for example.

But perhaps if both the registry and the device parts are turned into
helpers we could still have a single core implementation and then
instantiate t

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Andrzej Hajda]

On 10/29/2014 08:58 AM, Daniel Vetter wrote:
> On Tue, Oct 28, 2014 at 04:05:34PM +0100, Thierry Reding wrote:
>> On Tue, Oct 28, 2014 at 08:16:44PM +0530, Ajay kumar wrote:
>>> On Tue, Oct 28, 2014 at 8:11 PM, Thierry Reding
>>>  wrote:
 On Tue, Oct 28, 2014 at 03:19:36PM +0100, Daniel Vetter wrote:
> On Tue, Oct 28, 2014 at 1:28 PM, Ajay kumar  wrote:
>> On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter  
>> wrote:
 [...]
>>> Hm, if you do this can you pls also update drm_panel accordingly? It
>>> shouldn't be a lot of fuzz and would make things around drm+dt more
>>> consistent.
>> Are you talking about using struct device_node instead of struct device?
>> I guess you have misplaced the comment under the wrong section!
>
> Yeah, that should have been one up ;-)

 Like I said earlier, I don't think dropping struct device * in favour of
 struct device_node * is a good idea.
>>> I am not sure about drm_panel.
>>> But, I am not really doing anything with the struct device pointer in
>>> case of bridge.
>>> So, just wondering if it is really needed?
>>
>> I think it's useful to have it just to send the right message. DRM panel
>> and DRM bridge aren't specific to device tree. They are completely
>> generic and can work with any type of device, whether it was
>> instantiated from the device tree or some other infrastructure. Dropping
>> struct device * will make it effectively useless on anything but DT. I
>> don't think we should strive for that, even if only DT-enabled platforms
>> currently use them.
> 
> See my other reply, but I now think we should put neither into drm
> structures. This "find me the driver structures for this device" problem
> looks really generic, and it has nothing to do with the drm structures and
> concepts like bridges/panels at all. It shouldn't be in there at all.
> 
> Adding it looks very much like reintroducing the drm midlayer that we just
> finally made obsolete, just not at the top-level (struct drm_device) but
> at a bunch of leaf nodes. I expect all the same issues though. And I'm
> definitely not looking to de-midlayer more stuff that we're just adding.
> 
> Imo this should be solved as a separate helper thing, maybe in the driver
> core akin to the component helpers from Russell.
> -Daniel
> 

As I understand you want something generic to look for panels, bridges,
whatever and, like components, it should allow to safe hot plug/unplug.
I have proposed such thing few months ago [1]. Have you looked at it?

[1]: https://lkml.org/lkml/2014/4/30/345

Regards
Andrzej

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Daniel Vetter]

On Wed, Oct 29, 2014 at 09:38:23AM +0100, Thierry Reding wrote:
> On Wed, Oct 29, 2014 at 08:43:14AM +0100, Daniel Vetter wrote:
> > On Tue, Oct 28, 2014 at 03:35:50PM +0100, Thierry Reding wrote:
> > > On Mon, Oct 27, 2014 at 11:26:30PM +0100, Daniel Vetter wrote:
> > > > On Mon, Oct 27, 2014 at 11:20 PM, Daniel Vetter  
> > > > wrote:
> > > > > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul  
> > > > > wrote:
> > > >  @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
> > > >    * @driver_private: pointer to the bridge driver's internal 
> > > >  context
> > > >    */
> > > >   struct drm_bridge {
> > > >  - struct drm_device *dev;
> > > >  + struct device *dev;
> > > > >>>
> > > > >>> Please don't rename the ->dev pointer into drm. Because _all_ the 
> > > > >>> other
> > > > >>> drm structures still call it ->dev. Also, can't we use struct 
> > > > >>> device_node
> > > > >>> here like we do in the of helpers Russell added? See 7e435aad38083
> > > > >>>
> > > > >>
> > > > >> I think this is modeled after the naming in drm_panel, FWIW. However,
> > > > >> seems reasonable to keep the device_node instead.
> > > > >
> > > > > Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with
> > > > > drm_crtc drop the struct device and go directly to a struct
> > > > > device_node. Since we don't really need the sturct device, the only
> > > > > thing we care about is the of_node. For added bonus wrap an #ifdef
> > > > > CONFIG_OF around all the various struct device_node in drm_foo.h.
> > > > > Should be all fairly simple to pull off with cocci.
> > > > >
> > > > > Thierry?
> > > > 
> > > > Looking at the of_drm_find_panel function I actually wonder how that
> > > > works - the drm_panel doesn't really need to stick around afaics.
> > > > After all panel_list is global so some other driver can unload.
> > > > Russell's of support for possible_crtcs code works differently since
> > > > it only looks at per-drm_device lists.
> > > 
> > > I don't understand. Panels are global resources that get registered by
> > > some driver that isn't tied to the DRM device until attached. It can't
> > > be in a per-DRM device list, because it's external to the device.
> > > 
> > > And yes, they can go away when a driver is unloaded, though it's not the
> > > typical use-case.
> > > 
> > > > This bridge code here though suffers from the same. So to me this
> > > > looks rather fishy.
> > > 
> > > Well, this version of the DRM bridge support was written to be close to
> > > DRM panel, so it isn't really surprising that it's similar =), but like
> > > I said, I don't really understand what you think is wrong with it.
> > 
> > You have a mutex to protect the global list of bridges/panels. But if you
> > do a lookup you don't grab a reference count on the panel, so the moment
> > you drop the list mutex the panel/bridge can go away.
> > 
> > Yes usually you don't unload a driver on a soc but soc isn't everything
> > and designing new stuff to not be hotunplug save isn't great either.
> 
> Yeah, I certainly agree that adding proper reference counting would be a
> good thing. I think perhaps we could just take a reference on the struct
> device * to prevent it from disappearing.
> 
> Although perhaps I misunderstand what you mean by "go away".

I meant the drm_panel/bridge could go away any second, since nothing
prevents a module unload of the panel/bridge driver. So in theory you
could get the unregister call right after you've done the lookup. Which
means the bridge/panel pointer you've just returned is freed memory.

I think we nee try_get_module for the code and kref on the actual data
structures. That makes the entire thing a bit non-trivial, which is why I
think it would be better as some generic helper. Which then gets embedded
or instantiated for specific cases, like dt&drm_panel or dt&drm_bridge. Or
maybe even acpi&drm_bridge, who knows ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Thierry Reding]

On Wed, Oct 29, 2014 at 08:43:14AM +0100, Daniel Vetter wrote:
> On Tue, Oct 28, 2014 at 03:35:50PM +0100, Thierry Reding wrote:
> > On Mon, Oct 27, 2014 at 11:26:30PM +0100, Daniel Vetter wrote:
> > > On Mon, Oct 27, 2014 at 11:20 PM, Daniel Vetter  
> > > wrote:
> > > > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul  
> > > > wrote:
> > >  @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
> > >    * @driver_private: pointer to the bridge driver's internal context
> > >    */
> > >   struct drm_bridge {
> > >  - struct drm_device *dev;
> > >  + struct device *dev;
> > > >>>
> > > >>> Please don't rename the ->dev pointer into drm. Because _all_ the 
> > > >>> other
> > > >>> drm structures still call it ->dev. Also, can't we use struct 
> > > >>> device_node
> > > >>> here like we do in the of helpers Russell added? See 7e435aad38083
> > > >>>
> > > >>
> > > >> I think this is modeled after the naming in drm_panel, FWIW. However,
> > > >> seems reasonable to keep the device_node instead.
> > > >
> > > > Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with
> > > > drm_crtc drop the struct device and go directly to a struct
> > > > device_node. Since we don't really need the sturct device, the only
> > > > thing we care about is the of_node. For added bonus wrap an #ifdef
> > > > CONFIG_OF around all the various struct device_node in drm_foo.h.
> > > > Should be all fairly simple to pull off with cocci.
> > > >
> > > > Thierry?
> > > 
> > > Looking at the of_drm_find_panel function I actually wonder how that
> > > works - the drm_panel doesn't really need to stick around afaics.
> > > After all panel_list is global so some other driver can unload.
> > > Russell's of support for possible_crtcs code works differently since
> > > it only looks at per-drm_device lists.
> > 
> > I don't understand. Panels are global resources that get registered by
> > some driver that isn't tied to the DRM device until attached. It can't
> > be in a per-DRM device list, because it's external to the device.
> > 
> > And yes, they can go away when a driver is unloaded, though it's not the
> > typical use-case.
> > 
> > > This bridge code here though suffers from the same. So to me this
> > > looks rather fishy.
> > 
> > Well, this version of the DRM bridge support was written to be close to
> > DRM panel, so it isn't really surprising that it's similar =), but like
> > I said, I don't really understand what you think is wrong with it.
> 
> You have a mutex to protect the global list of bridges/panels. But if you
> do a lookup you don't grab a reference count on the panel, so the moment
> you drop the list mutex the panel/bridge can go away.
> 
> Yes usually you don't unload a driver on a soc but soc isn't everything
> and designing new stuff to not be hotunplug save isn't great either.

Yeah, I certainly agree that adding proper reference counting would be a
good thing. I think perhaps we could just take a reference on the struct
device * to prevent it from disappearing.

Although perhaps I misunderstand what you mean by "go away".

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

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Daniel Vetter]

On Tue, Oct 28, 2014 at 04:05:34PM +0100, Thierry Reding wrote:
> On Tue, Oct 28, 2014 at 08:16:44PM +0530, Ajay kumar wrote:
> > On Tue, Oct 28, 2014 at 8:11 PM, Thierry Reding
> >  wrote:
> > > On Tue, Oct 28, 2014 at 03:19:36PM +0100, Daniel Vetter wrote:
> > >> On Tue, Oct 28, 2014 at 1:28 PM, Ajay kumar  
> > >> wrote:
> > >> > On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter  
> > >> > wrote:
> > > [...]
> > >> >> Hm, if you do this can you pls also update drm_panel accordingly? It
> > >> >> shouldn't be a lot of fuzz and would make things around drm+dt more
> > >> >> consistent.
> > >> > Are you talking about using struct device_node instead of struct 
> > >> > device?
> > >> > I guess you have misplaced the comment under the wrong section!
> > >>
> > >> Yeah, that should have been one up ;-)
> > >
> > > Like I said earlier, I don't think dropping struct device * in favour of
> > > struct device_node * is a good idea.
> > I am not sure about drm_panel.
> > But, I am not really doing anything with the struct device pointer in
> > case of bridge.
> > So, just wondering if it is really needed?
> 
> I think it's useful to have it just to send the right message. DRM panel
> and DRM bridge aren't specific to device tree. They are completely
> generic and can work with any type of device, whether it was
> instantiated from the device tree or some other infrastructure. Dropping
> struct device * will make it effectively useless on anything but DT. I
> don't think we should strive for that, even if only DT-enabled platforms
> currently use them.

See my other reply, but I now think we should put neither into drm
structures. This "find me the driver structures for this device" problem
looks really generic, and it has nothing to do with the drm structures and
concepts like bridges/panels at all. It shouldn't be in there at all.

Adding it looks very much like reintroducing the drm midlayer that we just
finally made obsolete, just not at the top-level (struct drm_device) but
at a bunch of leaf nodes. I expect all the same issues though. And I'm
definitely not looking to de-midlayer more stuff that we're just adding.

Imo this should be solved as a separate helper thing, maybe in the driver
core akin to the component helpers from Russell.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Daniel Vetter]

On Tue, Oct 28, 2014 at 03:29:47PM +0100, Thierry Reding wrote:
> On Mon, Oct 27, 2014 at 11:20:31PM +0100, Daniel Vetter wrote:
> > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul  wrote:
> > >>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
> > >>>   * @driver_private: pointer to the bridge driver's internal context
> > >>>   */
> > >>>  struct drm_bridge {
> > >>> - struct drm_device *dev;
> > >>> + struct device *dev;
> > >>
> > >> Please don't rename the ->dev pointer into drm. Because _all_ the other
> > >> drm structures still call it ->dev. Also, can't we use struct device_node
> > >> here like we do in the of helpers Russell added? See 7e435aad38083
> > >>
> > >
> > > I think this is modeled after the naming in drm_panel, FWIW. However,
> > > seems reasonable to keep the device_node instead.
> > 
> > Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with
> > drm_crtc drop the struct device and go directly to a struct
> > device_node. Since we don't really need the sturct device, the only
> > thing we care about is the of_node. For added bonus wrap an #ifdef
> > CONFIG_OF around all the various struct device_node in drm_foo.h.
> > Should be all fairly simple to pull off with cocci.
> > 
> > Thierry?
> 
> The struct device * is in DRM panel because there's nothing device tree
> specific about the concept. Having a struct device_node * instead would
> indicate that it can only be used with a device tree, whereas the
> framework doesn't care the tiniest bit what type of device we have.
> 
> While the trend clearly is to use more device tree, I don't think we
> should make it impossible for anybody else to use these frameworks.
> 
> There are other advantages to keeping a struct device *, like having
> access to the proper device and its name. Also you get access to the
> device_node * via dev->of_node anyway. I don't see any advantage in
> switching to just a struct device_node *, only disadvantages.

Well the idea is to make the lookup key specific, and conditional on
#CONFIG_OF. If there's going to be another neat way to enumerate platform
devices then I think we should add that, too. Or maybe we should have a
void *platform_data or so.

The reason I really don't want a struct device * in core drm structures is
that two releases down the road people will have found tons of really
great ways to abuse them and re-create a midlayer. DRM core really should
only care about the sw objects and not be hw specific at all. Heck there's
not even an requirement to have any piece of actual hw, you could write a
completely fake drm driver (for e.g. testing like the new v4l driver).

Tbh I wonder a bit why we even have this registery embedded into the core
drm objects. Essentially the only thing you're doing is a list that maps
some platform specific key onto some subsystem specific driver structure
or fails the lookup. So instead of putting all these low-level details
into drm core structures can't we just have a generic hashtable/list for
this, plus some static inline helpers that cast the void * you get into
the one you want?

I also get the feeling that this really should be in the driver core (like
the component helpers), and that we should think a lot harder about
lifetimes and refcounting (see my other reply on that).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Daniel Vetter]

On Tue, Oct 28, 2014 at 03:35:50PM +0100, Thierry Reding wrote:
> On Mon, Oct 27, 2014 at 11:26:30PM +0100, Daniel Vetter wrote:
> > On Mon, Oct 27, 2014 at 11:20 PM, Daniel Vetter  wrote:
> > > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul  
> > > wrote:
> >  @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
> >    * @driver_private: pointer to the bridge driver's internal context
> >    */
> >   struct drm_bridge {
> >  - struct drm_device *dev;
> >  + struct device *dev;
> > >>>
> > >>> Please don't rename the ->dev pointer into drm. Because _all_ the other
> > >>> drm structures still call it ->dev. Also, can't we use struct 
> > >>> device_node
> > >>> here like we do in the of helpers Russell added? See 7e435aad38083
> > >>>
> > >>
> > >> I think this is modeled after the naming in drm_panel, FWIW. However,
> > >> seems reasonable to keep the device_node instead.
> > >
> > > Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with
> > > drm_crtc drop the struct device and go directly to a struct
> > > device_node. Since we don't really need the sturct device, the only
> > > thing we care about is the of_node. For added bonus wrap an #ifdef
> > > CONFIG_OF around all the various struct device_node in drm_foo.h.
> > > Should be all fairly simple to pull off with cocci.
> > >
> > > Thierry?
> > 
> > Looking at the of_drm_find_panel function I actually wonder how that
> > works - the drm_panel doesn't really need to stick around afaics.
> > After all panel_list is global so some other driver can unload.
> > Russell's of support for possible_crtcs code works differently since
> > it only looks at per-drm_device lists.
> 
> I don't understand. Panels are global resources that get registered by
> some driver that isn't tied to the DRM device until attached. It can't
> be in a per-DRM device list, because it's external to the device.
> 
> And yes, they can go away when a driver is unloaded, though it's not the
> typical use-case.
> 
> > This bridge code here though suffers from the same. So to me this
> > looks rather fishy.
> 
> Well, this version of the DRM bridge support was written to be close to
> DRM panel, so it isn't really surprising that it's similar =), but like
> I said, I don't really understand what you think is wrong with it.

You have a mutex to protect the global list of bridges/panels. But if you
do a lookup you don't grab a reference count on the panel, so the moment
you drop the list mutex the panel/bridge can go away.

Yes usually you don't unload a driver on a soc but soc isn't everything
and designing new stuff to not be hotunplug save isn't great either.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Ajay kumar]

On Tue, Oct 28, 2014 at 8:11 PM, Thierry Reding
 wrote:
> On Tue, Oct 28, 2014 at 03:19:36PM +0100, Daniel Vetter wrote:
>> On Tue, Oct 28, 2014 at 1:28 PM, Ajay kumar  wrote:
>> > On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter  wrote:
> [...]
>> >> Hm, if you do this can you pls also update drm_panel accordingly? It
>> >> shouldn't be a lot of fuzz and would make things around drm+dt more
>> >> consistent.
>> > Are you talking about using struct device_node instead of struct device?
>> > I guess you have misplaced the comment under the wrong section!
>>
>> Yeah, that should have been one up ;-)
>
> Like I said earlier, I don't think dropping struct device * in favour of
> struct device_node * is a good idea.
I am not sure about drm_panel.
But, I am not really doing anything with the struct device pointer in
case of bridge.
So, just wondering if it is really needed?

Ajay

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Ajay kumar]

On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter  wrote:
> On Tue, Oct 28, 2014 at 10:21 AM, Ajay kumar  wrote:
>> Hi Daniel and Sean,
>>
>> Thanks for the comments!
>>
>> On Tue, Oct 28, 2014 at 1:28 AM, Sean Paul  wrote:
>>> On Mon, Oct 27, 2014 at 3:01 PM, Daniel Vetter  wrote:
 So don't ask why but I accidentally ended up in a branch looking at this
 patch and didn't like it. So very quick&grumpy review.

 First, please make the patch subject more descriptive: I'd expect a helper
 function scaffolding like the various crtc/probe/dp ... helpers we already
 have. You instead add code to untangle the probe ordering. Please say so.
>> Sure. I will reword it properly.
>>
 More comments below.

 On Wed, Aug 27, 2014 at 07:59:37PM +0530, Ajay Kumar wrote:
> A set of helper functions are defined in this patch to make
> bridge driver probe independent of the drm flow.
>
> The bridge devices register themselves on a lookup table
> when they get probed by calling "drm_bridge_add".
>
> The parent encoder driver waits till the bridge is available
> in the lookup table(by calling "of_drm_find_bridge") and then
> continues with its initialization.
>
> The encoder driver should also call "drm_bridge_attach" to pass
> on the drm_device, encoder pointers to the bridge object.
>
> drm_bridge_attach inturn calls drm_bridge_init to register itself
> with the drm core. Later, it calls "bridge->funcs->attach" so that
> bridge can continue with other initializations.
>
> Signed-off-by: Ajay Kumar 

 [snip]

> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
>   * @driver_private: pointer to the bridge driver's internal context
>   */
>  struct drm_bridge {
> - struct drm_device *dev;
> + struct device *dev;

 Please don't rename the ->dev pointer into drm. Because _all_ the other
 drm structures still call it ->dev. Also, can't we use struct device_node
 here like we do in the of helpers Russell added? See 7e435aad38083

>>>
>>> I think this is modeled after the naming in drm_panel,
>> Right, The entire rework is based on how drm_panel framework is structured.
>>
>>> FWIW. However,
>>> seems reasonable to keep the device_node instead.
>> Yes, its visible that just device_node would be sufficient.
>> This will save us from renaming drm_device as well.
>>
> + struct drm_device *drm;
> + struct drm_encoder *encoder;

 This breaks bridge->bridge chaining (if we ever get there). It seems
 pretty much unused anyway except for an EBUSY check. Can't you use
 bridge->dev for that?

>>>
>>> Yeah, I'd prefer to pass drm_device directly into drm_bridge_attach
>>> and leave it up to the caller to establish the proper chain.
>> Ok. I will use drm_device pointer directly instead of passing encoder 
>> pointer.
>
> Hm, if you do this can you pls also update drm_panel accordingly? It
> shouldn't be a lot of fuzz and would make things around drm+dt more
> consistent.
Are you talking about using struct device_node instead of struct device?
I guess you have misplaced the comment under the wrong section!

>>
>   struct list_head head;
> + struct list_head list;

 These lists need better names. I know that the "head" is really awful,
 especially since it's actually not the head of the list but just an
 element.
>>>
>>> I think we can just rip bridge_list out of mode_config if we're going
>>> to keep track of bridges elsewhere. So we can nuke "head" and keep
>>> "list". This also means that bridge->destroy() goes away, but that's
>>> probably Ok if everything converts to the standalone driver model
>>> where we have driver->remove()
>>>
>>> Sean
>> Great! Thierry actually mentioned about this once, and we have the
>> confirmation now.
>>
>
>   struct drm_mode_object base;


 Aside: I've noticed all this trying to update the kerneldoc for struct
 drm_bridge, which just showed that this patch makes inconsistent changes.
 Trying to write kerneldoc is a really great way to come up with better
 interfaces imo.

 Cheers, Daniel
>> I din't get this actually. You want me to create Doc../drm_bridge.txt
>> or something similar?
>
> If you want to document drm_bridge then I recomment to sprinkle proper
> kerneldoc over drm_bridge.c and pull it all into the drm DocBook
> template. That way all the drm documentation is in one place. I've
> done that for drm_crtc.h in an unrelated patch series (but based upon
> a branch with your patch here included) and there's struct drm_bridge*
> in there. Hence why I've noticed.
Can you send a link for that?
And, is there any problem if the doc comes later?

Ajay

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Thierry Reding]

On Tue, Oct 28, 2014 at 08:16:44PM +0530, Ajay kumar wrote:
> On Tue, Oct 28, 2014 at 8:11 PM, Thierry Reding
>  wrote:
> > On Tue, Oct 28, 2014 at 03:19:36PM +0100, Daniel Vetter wrote:
> >> On Tue, Oct 28, 2014 at 1:28 PM, Ajay kumar  wrote:
> >> > On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter  
> >> > wrote:
> > [...]
> >> >> Hm, if you do this can you pls also update drm_panel accordingly? It
> >> >> shouldn't be a lot of fuzz and would make things around drm+dt more
> >> >> consistent.
> >> > Are you talking about using struct device_node instead of struct device?
> >> > I guess you have misplaced the comment under the wrong section!
> >>
> >> Yeah, that should have been one up ;-)
> >
> > Like I said earlier, I don't think dropping struct device * in favour of
> > struct device_node * is a good idea.
> I am not sure about drm_panel.
> But, I am not really doing anything with the struct device pointer in
> case of bridge.
> So, just wondering if it is really needed?

I think it's useful to have it just to send the right message. DRM panel
and DRM bridge aren't specific to device tree. They are completely
generic and can work with any type of device, whether it was
instantiated from the device tree or some other infrastructure. Dropping
struct device * will make it effectively useless on anything but DT. I
don't think we should strive for that, even if only DT-enabled platforms
currently use them.

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

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Thierry Reding]

On Tue, Oct 28, 2014 at 03:19:36PM +0100, Daniel Vetter wrote:
> On Tue, Oct 28, 2014 at 1:28 PM, Ajay kumar  wrote:
> > On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter  wrote:
[...]
> >> Hm, if you do this can you pls also update drm_panel accordingly? It
> >> shouldn't be a lot of fuzz and would make things around drm+dt more
> >> consistent.
> > Are you talking about using struct device_node instead of struct device?
> > I guess you have misplaced the comment under the wrong section!
> 
> Yeah, that should have been one up ;-)

Like I said earlier, I don't think dropping struct device * in favour of
struct device_node * is a good idea.

> >> If you want to document drm_bridge then I recomment to sprinkle proper
> >> kerneldoc over drm_bridge.c and pull it all into the drm DocBook
> >> template. That way all the drm documentation is in one place. I've
> >> done that for drm_crtc.h in an unrelated patch series (but based upon
> >> a branch with your patch here included) and there's struct drm_bridge*
> >> in there. Hence why I've noticed.
> > Can you send a link for that?
> > And, is there any problem if the doc comes later?
> 
> Since quite a while we've asked for the kerneldoc polish as part of
> each drm core patch series. It's just that drm_bridge/panel kinda have
> flown under the radar of the people usually asking for docs ;-)

FWIW, there's some kerneldoc in include/drm/drm_panel.h but I guess I
could write up something more complete and integrate it into DocBook.

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

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Thierry Reding]

On Mon, Oct 27, 2014 at 11:26:30PM +0100, Daniel Vetter wrote:
> On Mon, Oct 27, 2014 at 11:20 PM, Daniel Vetter  wrote:
> > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul  wrote:
>  @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
>    * @driver_private: pointer to the bridge driver's internal context
>    */
>   struct drm_bridge {
>  - struct drm_device *dev;
>  + struct device *dev;
> >>>
> >>> Please don't rename the ->dev pointer into drm. Because _all_ the other
> >>> drm structures still call it ->dev. Also, can't we use struct device_node
> >>> here like we do in the of helpers Russell added? See 7e435aad38083
> >>>
> >>
> >> I think this is modeled after the naming in drm_panel, FWIW. However,
> >> seems reasonable to keep the device_node instead.
> >
> > Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with
> > drm_crtc drop the struct device and go directly to a struct
> > device_node. Since we don't really need the sturct device, the only
> > thing we care about is the of_node. For added bonus wrap an #ifdef
> > CONFIG_OF around all the various struct device_node in drm_foo.h.
> > Should be all fairly simple to pull off with cocci.
> >
> > Thierry?
> 
> Looking at the of_drm_find_panel function I actually wonder how that
> works - the drm_panel doesn't really need to stick around afaics.
> After all panel_list is global so some other driver can unload.
> Russell's of support for possible_crtcs code works differently since
> it only looks at per-drm_device lists.

I don't understand. Panels are global resources that get registered by
some driver that isn't tied to the DRM device until attached. It can't
be in a per-DRM device list, because it's external to the device.

And yes, they can go away when a driver is unloaded, though it's not the
typical use-case.

> This bridge code here though suffers from the same. So to me this
> looks rather fishy.

Well, this version of the DRM bridge support was written to be close to
DRM panel, so it isn't really surprising that it's similar =), but like
I said, I don't really understand what you think is wrong with it.

> It doesn't help that we have drm_of.[hc] around but not all the of
> code is in there. Adding Russell too.

DRM panel and DRM bridge aren't just OF helpers. They can be used with
whatever type of device you want.

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

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Thierry Reding]

On Mon, Oct 27, 2014 at 11:20:31PM +0100, Daniel Vetter wrote:
> On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul  wrote:
> >>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
> >>>   * @driver_private: pointer to the bridge driver's internal context
> >>>   */
> >>>  struct drm_bridge {
> >>> - struct drm_device *dev;
> >>> + struct device *dev;
> >>
> >> Please don't rename the ->dev pointer into drm. Because _all_ the other
> >> drm structures still call it ->dev. Also, can't we use struct device_node
> >> here like we do in the of helpers Russell added? See 7e435aad38083
> >>
> >
> > I think this is modeled after the naming in drm_panel, FWIW. However,
> > seems reasonable to keep the device_node instead.
> 
> Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with
> drm_crtc drop the struct device and go directly to a struct
> device_node. Since we don't really need the sturct device, the only
> thing we care about is the of_node. For added bonus wrap an #ifdef
> CONFIG_OF around all the various struct device_node in drm_foo.h.
> Should be all fairly simple to pull off with cocci.
> 
> Thierry?

The struct device * is in DRM panel because there's nothing device tree
specific about the concept. Having a struct device_node * instead would
indicate that it can only be used with a device tree, whereas the
framework doesn't care the tiniest bit what type of device we have.

While the trend clearly is to use more device tree, I don't think we
should make it impossible for anybody else to use these frameworks.

There are other advantages to keeping a struct device *, like having
access to the proper device and its name. Also you get access to the
device_node * via dev->of_node anyway. I don't see any advantage in
switching to just a struct device_node *, only disadvantages.

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

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Daniel Vetter]

On Tue, Oct 28, 2014 at 1:28 PM, Ajay kumar  wrote:
> On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter  wrote:
>> On Tue, Oct 28, 2014 at 10:21 AM, Ajay kumar  wrote:
>>> Hi Daniel and Sean,
>>>
>>> Thanks for the comments!
>>>
>>> On Tue, Oct 28, 2014 at 1:28 AM, Sean Paul  wrote:
 On Mon, Oct 27, 2014 at 3:01 PM, Daniel Vetter  wrote:
> So don't ask why but I accidentally ended up in a branch looking at this
> patch and didn't like it. So very quick&grumpy review.
>
> First, please make the patch subject more descriptive: I'd expect a helper
> function scaffolding like the various crtc/probe/dp ... helpers we already
> have. You instead add code to untangle the probe ordering. Please say so.
>>> Sure. I will reword it properly.
>>>
> More comments below.
>
> On Wed, Aug 27, 2014 at 07:59:37PM +0530, Ajay Kumar wrote:
>> A set of helper functions are defined in this patch to make
>> bridge driver probe independent of the drm flow.
>>
>> The bridge devices register themselves on a lookup table
>> when they get probed by calling "drm_bridge_add".
>>
>> The parent encoder driver waits till the bridge is available
>> in the lookup table(by calling "of_drm_find_bridge") and then
>> continues with its initialization.
>>
>> The encoder driver should also call "drm_bridge_attach" to pass
>> on the drm_device, encoder pointers to the bridge object.
>>
>> drm_bridge_attach inturn calls drm_bridge_init to register itself
>> with the drm core. Later, it calls "bridge->funcs->attach" so that
>> bridge can continue with other initializations.
>>
>> Signed-off-by: Ajay Kumar 
>
> [snip]
>
>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
>>   * @driver_private: pointer to the bridge driver's internal context
>>   */
>>  struct drm_bridge {
>> - struct drm_device *dev;
>> + struct device *dev;
>
> Please don't rename the ->dev pointer into drm. Because _all_ the other
> drm structures still call it ->dev. Also, can't we use struct device_node
> here like we do in the of helpers Russell added? See 7e435aad38083
>

 I think this is modeled after the naming in drm_panel,
>>> Right, The entire rework is based on how drm_panel framework is structured.
>>>
 FWIW. However,
 seems reasonable to keep the device_node instead.
>>> Yes, its visible that just device_node would be sufficient.
>>> This will save us from renaming drm_device as well.
>>>
>> + struct drm_device *drm;
>> + struct drm_encoder *encoder;
>
> This breaks bridge->bridge chaining (if we ever get there). It seems
> pretty much unused anyway except for an EBUSY check. Can't you use
> bridge->dev for that?
>

 Yeah, I'd prefer to pass drm_device directly into drm_bridge_attach
 and leave it up to the caller to establish the proper chain.
>>> Ok. I will use drm_device pointer directly instead of passing encoder 
>>> pointer.
>>
>> Hm, if you do this can you pls also update drm_panel accordingly? It
>> shouldn't be a lot of fuzz and would make things around drm+dt more
>> consistent.
> Are you talking about using struct device_node instead of struct device?
> I guess you have misplaced the comment under the wrong section!

Yeah, that should have been one up ;-)

>>   struct list_head head;
>> + struct list_head list;
>
> These lists need better names. I know that the "head" is really awful,
> especially since it's actually not the head of the list but just an
> element.

 I think we can just rip bridge_list out of mode_config if we're going
 to keep track of bridges elsewhere. So we can nuke "head" and keep
 "list". This also means that bridge->destroy() goes away, but that's
 probably Ok if everything converts to the standalone driver model
 where we have driver->remove()

 Sean
>>> Great! Thierry actually mentioned about this once, and we have the
>>> confirmation now.
>>>
>>
>>   struct drm_mode_object base;
>
>
> Aside: I've noticed all this trying to update the kerneldoc for struct
> drm_bridge, which just showed that this patch makes inconsistent changes.
> Trying to write kerneldoc is a really great way to come up with better
> interfaces imo.
>
> Cheers, Daniel
>>> I din't get this actually. You want me to create Doc../drm_bridge.txt
>>> or something similar?
>>
>> If you want to document drm_bridge then I recomment to sprinkle proper
>> kerneldoc over drm_bridge.c and pull it all into the drm DocBook
>> template. That way all the drm documentation is in one place. I've
>> done that for drm_crtc.h in an unrelated patch series (but based upon
>> a branch with your patch here included) and there's struct drm_bridge*
>> in there. Hence why I've noticed.
> Can you send a link for that?
> And, is there any problem if the doc

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Ajay kumar]

On Tue, Oct 28, 2014 at 1:41 AM, Sean Paul  wrote:
> On Wed, Aug 27, 2014 at 10:29 AM, Ajay Kumar  
> wrote:
>> A set of helper functions are defined in this patch to make
>> bridge driver probe independent of the drm flow.
>>
>> The bridge devices register themselves on a lookup table
>> when they get probed by calling "drm_bridge_add".
>>
>> The parent encoder driver waits till the bridge is available
>> in the lookup table(by calling "of_drm_find_bridge") and then
>> continues with its initialization.
>>
>> The encoder driver should also call "drm_bridge_attach" to pass
>> on the drm_device, encoder pointers to the bridge object.
>>
>> drm_bridge_attach inturn calls drm_bridge_init to register itself
>> with the drm core. Later, it calls "bridge->funcs->attach" so that
>> bridge can continue with other initializations.
>>
>> Signed-off-by: Ajay Kumar 
>> ---
>>  drivers/gpu/drm/Makefile   |1 +
>>  drivers/gpu/drm/bridge/Kconfig |   15 -
>>  drivers/gpu/drm/drm_bridge.c   |  102 
>> 
>>  drivers/gpu/drm/drm_crtc.c |4 +-
>>  drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |4 +-
>>  include/drm/drm_crtc.h |   12 +++-
>>  6 files changed, 131 insertions(+), 7 deletions(-)
>>  create mode 100644 drivers/gpu/drm/drm_bridge.c
>>
>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>> index 4a55d59..bdbfb6f 100644
>> --- a/drivers/gpu/drm/Makefile
>> +++ b/drivers/gpu/drm/Makefile
>> @@ -19,6 +19,7 @@ drm-y   :=drm_auth.o drm_buffer.o drm_bufs.o 
>> drm_cache.o \
>>  drm-$(CONFIG_COMPAT) += drm_ioc32.o
>>  drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
>>  drm-$(CONFIG_PCI) += ati_pcigart.o
>> +drm-$(CONFIG_DRM_BRIDGE) += drm_bridge.o
>>  drm-$(CONFIG_DRM_PANEL) += drm_panel.o
>>  drm-$(CONFIG_OF) += drm_of.o
>>
>> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
>> index 884923f..5a8e907 100644
>> --- a/drivers/gpu/drm/bridge/Kconfig
>> +++ b/drivers/gpu/drm/bridge/Kconfig
>> @@ -1,5 +1,16 @@
>> -config DRM_PTN3460
>> -   tristate "PTN3460 DP/LVDS bridge"
>> +config DRM_BRIDGE
>
> I'm not convinced this adds any value, to be honest.
Hmm, then how to compile drm_bridge.c?

> In addition to
> whether or not it's useful, it seems like you'd need to stub the
> drm_bridge_* functions that are declared in drm_crtc.h or break them
> out into drm_bridge.h.
Well, Thierry mentioned about this long back. Again, we have the
confirmation now!

Ajay
[snip]

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Ajay kumar]

On Tue, Oct 28, 2014 at 1:41 AM, Sean Paul  wrote:
> On Wed, Aug 27, 2014 at 10:29 AM, Ajay Kumar  
> wrote:
>> A set of helper functions are defined in this patch to make
>> bridge driver probe independent of the drm flow.
>>
>> The bridge devices register themselves on a lookup table
>> when they get probed by calling "drm_bridge_add".
>>
>> The parent encoder driver waits till the bridge is available
>> in the lookup table(by calling "of_drm_find_bridge") and then
>> continues with its initialization.
>>
>> The encoder driver should also call "drm_bridge_attach" to pass
>> on the drm_device, encoder pointers to the bridge object.
>>
>> drm_bridge_attach inturn calls drm_bridge_init to register itself
>> with the drm core. Later, it calls "bridge->funcs->attach" so that
>> bridge can continue with other initializations.
>>
>> Signed-off-by: Ajay Kumar 
>> ---
>>  drivers/gpu/drm/Makefile   |1 +
>>  drivers/gpu/drm/bridge/Kconfig |   15 -
>>  drivers/gpu/drm/drm_bridge.c   |  102 
>> 
>>  drivers/gpu/drm/drm_crtc.c |4 +-
>>  drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |4 +-
>>  include/drm/drm_crtc.h |   12 +++-
>>  6 files changed, 131 insertions(+), 7 deletions(-)
>>  create mode 100644 drivers/gpu/drm/drm_bridge.c
>>
>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>> index 4a55d59..bdbfb6f 100644
>> --- a/drivers/gpu/drm/Makefile
>> +++ b/drivers/gpu/drm/Makefile
>> @@ -19,6 +19,7 @@ drm-y   :=drm_auth.o drm_buffer.o drm_bufs.o 
>> drm_cache.o \
>>  drm-$(CONFIG_COMPAT) += drm_ioc32.o
>>  drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
>>  drm-$(CONFIG_PCI) += ati_pcigart.o
>> +drm-$(CONFIG_DRM_BRIDGE) += drm_bridge.o
>>  drm-$(CONFIG_DRM_PANEL) += drm_panel.o
>>  drm-$(CONFIG_OF) += drm_of.o
>>
>> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
>> index 884923f..5a8e907 100644
>> --- a/drivers/gpu/drm/bridge/Kconfig
>> +++ b/drivers/gpu/drm/bridge/Kconfig
>> @@ -1,5 +1,16 @@
>> -config DRM_PTN3460
>> -   tristate "PTN3460 DP/LVDS bridge"
>> +config DRM_BRIDGE
>
> I'm not convinced this adds any value, to be honest. In addition to
> whether or not it's useful, it seems like you'd need to stub the
> drm_bridge_* functions that are declared in drm_crtc.h or break them
> out into drm_bridge.h.
>
> Sean
>
>> +   tristate
>> depends on DRM
>> select DRM_KMS_HELPER
>> +   help
>> + Bridge registration and lookup framework.
>> +
>> +menu "bridge chips"
>> +   depends on DRM_BRIDGE
>> +
>> +config DRM_PTN3460
>> +   tristate "PTN3460 DP/LVDS bridge"
>> +   depends on DRM_BRIDGE
>> ---help---
>> +
>> +endmenu
>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>> new file mode 100644
>> index 000..b2d43fd
>> --- /dev/null
>> +++ b/drivers/gpu/drm/drm_bridge.c
>> @@ -0,0 +1,102 @@
>> +/*
>> + * Copyright (c) 2014 Samsung Electronics Co., Ltd
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the 
>> "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sub license,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including the
>> + * next paragraph) shall be included in all copies or substantial portions
>> + * of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS 
>> OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR 
>> OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>> + * DEALINGS IN THE SOFTWARE.
>> + */
>> +
>> +#include 
>> +#include 
>> +
>> +#include 
>> +
>> +#include "drm/drmP.h"
>> +
>> +static DEFINE_MUTEX(bridge_lock);
>> +static LIST_HEAD(bridge_list);
>> +
>> +int drm_bridge_add(struct drm_bridge *bridge)
>> +{
>> +   mutex_lock(&bridge_lock);
>> +   list_add_tail(&bridge->list, &bridge_list);
>> +   mutex_unlock(&bridge_lock);
>> +
>> +   return 0;
>> +}
>> +EXPORT_SYMBOL(drm_bridge_add);
>> +
>> +void drm_bridge_remove(struct drm_bridge *bridge)
>> +{
>> +   mutex_lock(&bridge_lock);
>> +   list_del_init(&bridge->list);
>> +   mutex_unlock(&bridge_lock);
>> +}
>> +EXPORT_SYMBOL(drm_bridge_remove);
>> +
>> +int drm_bridge_attach(struct drm_bridge *bridge,
>> +   struct drm_enc

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Ajay kumar]

Hi Daniel and Sean,

Thanks for the comments!

On Tue, Oct 28, 2014 at 1:28 AM, Sean Paul  wrote:
> On Mon, Oct 27, 2014 at 3:01 PM, Daniel Vetter  wrote:
>> So don't ask why but I accidentally ended up in a branch looking at this
>> patch and didn't like it. So very quick&grumpy review.
>>
>> First, please make the patch subject more descriptive: I'd expect a helper
>> function scaffolding like the various crtc/probe/dp ... helpers we already
>> have. You instead add code to untangle the probe ordering. Please say so.
Sure. I will reword it properly.

>> More comments below.
>>
>> On Wed, Aug 27, 2014 at 07:59:37PM +0530, Ajay Kumar wrote:
>>> A set of helper functions are defined in this patch to make
>>> bridge driver probe independent of the drm flow.
>>>
>>> The bridge devices register themselves on a lookup table
>>> when they get probed by calling "drm_bridge_add".
>>>
>>> The parent encoder driver waits till the bridge is available
>>> in the lookup table(by calling "of_drm_find_bridge") and then
>>> continues with its initialization.
>>>
>>> The encoder driver should also call "drm_bridge_attach" to pass
>>> on the drm_device, encoder pointers to the bridge object.
>>>
>>> drm_bridge_attach inturn calls drm_bridge_init to register itself
>>> with the drm core. Later, it calls "bridge->funcs->attach" so that
>>> bridge can continue with other initializations.
>>>
>>> Signed-off-by: Ajay Kumar 
>>
>> [snip]
>>
>>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
>>>   * @driver_private: pointer to the bridge driver's internal context
>>>   */
>>>  struct drm_bridge {
>>> - struct drm_device *dev;
>>> + struct device *dev;
>>
>> Please don't rename the ->dev pointer into drm. Because _all_ the other
>> drm structures still call it ->dev. Also, can't we use struct device_node
>> here like we do in the of helpers Russell added? See 7e435aad38083
>>
>
> I think this is modeled after the naming in drm_panel,
Right, The entire rework is based on how drm_panel framework is structured.

> FWIW. However,
> seems reasonable to keep the device_node instead.
Yes, its visible that just device_node would be sufficient.
This will save us from renaming drm_device as well.

>>> + struct drm_device *drm;
>>> + struct drm_encoder *encoder;
>>
>> This breaks bridge->bridge chaining (if we ever get there). It seems
>> pretty much unused anyway except for an EBUSY check. Can't you use
>> bridge->dev for that?
>>
>
> Yeah, I'd prefer to pass drm_device directly into drm_bridge_attach
> and leave it up to the caller to establish the proper chain.
Ok. I will use drm_device pointer directly instead of passing encoder pointer.

>>>   struct list_head head;
>>> + struct list_head list;
>>
>> These lists need better names. I know that the "head" is really awful,
>> especially since it's actually not the head of the list but just an
>> element.
>
> I think we can just rip bridge_list out of mode_config if we're going
> to keep track of bridges elsewhere. So we can nuke "head" and keep
> "list". This also means that bridge->destroy() goes away, but that's
> probably Ok if everything converts to the standalone driver model
> where we have driver->remove()
>
> Sean
Great! Thierry actually mentioned about this once, and we have the
confirmation now.

>>>
>>>   struct drm_mode_object base;
>>
>>
>> Aside: I've noticed all this trying to update the kerneldoc for struct
>> drm_bridge, which just showed that this patch makes inconsistent changes.
>> Trying to write kerneldoc is a really great way to come up with better
>> interfaces imo.
>>
>> Cheers, Daniel
I din't get this actually. You want me to create Doc../drm_bridge.txt
or something similar?

Ajay

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Daniel Vetter]

On Tue, Oct 28, 2014 at 10:21 AM, Ajay kumar  wrote:
> Hi Daniel and Sean,
>
> Thanks for the comments!
>
> On Tue, Oct 28, 2014 at 1:28 AM, Sean Paul  wrote:
>> On Mon, Oct 27, 2014 at 3:01 PM, Daniel Vetter  wrote:
>>> So don't ask why but I accidentally ended up in a branch looking at this
>>> patch and didn't like it. So very quick&grumpy review.
>>>
>>> First, please make the patch subject more descriptive: I'd expect a helper
>>> function scaffolding like the various crtc/probe/dp ... helpers we already
>>> have. You instead add code to untangle the probe ordering. Please say so.
> Sure. I will reword it properly.
>
>>> More comments below.
>>>
>>> On Wed, Aug 27, 2014 at 07:59:37PM +0530, Ajay Kumar wrote:
 A set of helper functions are defined in this patch to make
 bridge driver probe independent of the drm flow.

 The bridge devices register themselves on a lookup table
 when they get probed by calling "drm_bridge_add".

 The parent encoder driver waits till the bridge is available
 in the lookup table(by calling "of_drm_find_bridge") and then
 continues with its initialization.

 The encoder driver should also call "drm_bridge_attach" to pass
 on the drm_device, encoder pointers to the bridge object.

 drm_bridge_attach inturn calls drm_bridge_init to register itself
 with the drm core. Later, it calls "bridge->funcs->attach" so that
 bridge can continue with other initializations.

 Signed-off-by: Ajay Kumar 
>>>
>>> [snip]
>>>
 @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
   * @driver_private: pointer to the bridge driver's internal context
   */
  struct drm_bridge {
 - struct drm_device *dev;
 + struct device *dev;
>>>
>>> Please don't rename the ->dev pointer into drm. Because _all_ the other
>>> drm structures still call it ->dev. Also, can't we use struct device_node
>>> here like we do in the of helpers Russell added? See 7e435aad38083
>>>
>>
>> I think this is modeled after the naming in drm_panel,
> Right, The entire rework is based on how drm_panel framework is structured.
>
>> FWIW. However,
>> seems reasonable to keep the device_node instead.
> Yes, its visible that just device_node would be sufficient.
> This will save us from renaming drm_device as well.
>
 + struct drm_device *drm;
 + struct drm_encoder *encoder;
>>>
>>> This breaks bridge->bridge chaining (if we ever get there). It seems
>>> pretty much unused anyway except for an EBUSY check. Can't you use
>>> bridge->dev for that?
>>>
>>
>> Yeah, I'd prefer to pass drm_device directly into drm_bridge_attach
>> and leave it up to the caller to establish the proper chain.
> Ok. I will use drm_device pointer directly instead of passing encoder pointer.

Hm, if you do this can you pls also update drm_panel accordingly? It
shouldn't be a lot of fuzz and would make things around drm+dt more
consistent.

>
   struct list_head head;
 + struct list_head list;
>>>
>>> These lists need better names. I know that the "head" is really awful,
>>> especially since it's actually not the head of the list but just an
>>> element.
>>
>> I think we can just rip bridge_list out of mode_config if we're going
>> to keep track of bridges elsewhere. So we can nuke "head" and keep
>> "list". This also means that bridge->destroy() goes away, but that's
>> probably Ok if everything converts to the standalone driver model
>> where we have driver->remove()
>>
>> Sean
> Great! Thierry actually mentioned about this once, and we have the
> confirmation now.
>

   struct drm_mode_object base;
>>>
>>>
>>> Aside: I've noticed all this trying to update the kerneldoc for struct
>>> drm_bridge, which just showed that this patch makes inconsistent changes.
>>> Trying to write kerneldoc is a really great way to come up with better
>>> interfaces imo.
>>>
>>> Cheers, Daniel
> I din't get this actually. You want me to create Doc../drm_bridge.txt
> or something similar?

If you want to document drm_bridge then I recomment to sprinkle proper
kerneldoc over drm_bridge.c and pull it all into the drm DocBook
template. That way all the drm documentation is in one place. I've
done that for drm_crtc.h in an unrelated patch series (but based upon
a branch with your patch here included) and there's struct drm_bridge*
in there. Hence why I've noticed.

If you do kerneldoc/DocBook integration for drm_bridge it's probably
best to also do it for drm_panel and use the opportunity to
review/rework the interfaces a bit for consistency. E.g. move dt
related stuff into drm_of.c and have that in a separate section in the
docs.
-Daniel


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Sean Paul]

On Tue, Oct 28, 2014 at 10:19 AM, Daniel Vetter  wrote:
> On Tue, Oct 28, 2014 at 1:28 PM, Ajay kumar  wrote:
>> On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter  wrote:
>>> On Tue, Oct 28, 2014 at 10:21 AM, Ajay kumar  wrote:
 Hi Daniel and Sean,

 Thanks for the comments!

 On Tue, Oct 28, 2014 at 1:28 AM, Sean Paul  
 wrote:
> On Mon, Oct 27, 2014 at 3:01 PM, Daniel Vetter  wrote:
>> So don't ask why but I accidentally ended up in a branch looking at this
>> patch and didn't like it. So very quick&grumpy review.
>>
>> First, please make the patch subject more descriptive: I'd expect a 
>> helper
>> function scaffolding like the various crtc/probe/dp ... helpers we 
>> already
>> have. You instead add code to untangle the probe ordering. Please say so.
 Sure. I will reword it properly.

>> More comments below.
>>
>> On Wed, Aug 27, 2014 at 07:59:37PM +0530, Ajay Kumar wrote:
>>> A set of helper functions are defined in this patch to make
>>> bridge driver probe independent of the drm flow.
>>>
>>> The bridge devices register themselves on a lookup table
>>> when they get probed by calling "drm_bridge_add".
>>>
>>> The parent encoder driver waits till the bridge is available
>>> in the lookup table(by calling "of_drm_find_bridge") and then
>>> continues with its initialization.
>>>
>>> The encoder driver should also call "drm_bridge_attach" to pass
>>> on the drm_device, encoder pointers to the bridge object.
>>>
>>> drm_bridge_attach inturn calls drm_bridge_init to register itself
>>> with the drm core. Later, it calls "bridge->funcs->attach" so that
>>> bridge can continue with other initializations.
>>>
>>> Signed-off-by: Ajay Kumar 
>>
>> [snip]
>>
>>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
>>>   * @driver_private: pointer to the bridge driver's internal context
>>>   */
>>>  struct drm_bridge {
>>> - struct drm_device *dev;
>>> + struct device *dev;
>>
>> Please don't rename the ->dev pointer into drm. Because _all_ the other
>> drm structures still call it ->dev. Also, can't we use struct device_node
>> here like we do in the of helpers Russell added? See 7e435aad38083
>>
>
> I think this is modeled after the naming in drm_panel,
 Right, The entire rework is based on how drm_panel framework is structured.

> FWIW. However,
> seems reasonable to keep the device_node instead.
 Yes, its visible that just device_node would be sufficient.
 This will save us from renaming drm_device as well.

>>> + struct drm_device *drm;
>>> + struct drm_encoder *encoder;
>>
>> This breaks bridge->bridge chaining (if we ever get there). It seems
>> pretty much unused anyway except for an EBUSY check. Can't you use
>> bridge->dev for that?
>>
>
> Yeah, I'd prefer to pass drm_device directly into drm_bridge_attach
> and leave it up to the caller to establish the proper chain.
 Ok. I will use drm_device pointer directly instead of passing encoder 
 pointer.
>>>
>>> Hm, if you do this can you pls also update drm_panel accordingly? It
>>> shouldn't be a lot of fuzz and would make things around drm+dt more
>>> consistent.
>> Are you talking about using struct device_node instead of struct device?
>> I guess you have misplaced the comment under the wrong section!
>
> Yeah, that should have been one up ;-)
>
>>>   struct list_head head;
>>> + struct list_head list;
>>
>> These lists need better names. I know that the "head" is really awful,
>> especially since it's actually not the head of the list but just an
>> element.
>
> I think we can just rip bridge_list out of mode_config if we're going
> to keep track of bridges elsewhere. So we can nuke "head" and keep
> "list". This also means that bridge->destroy() goes away, but that's
> probably Ok if everything converts to the standalone driver model
> where we have driver->remove()
>
> Sean
 Great! Thierry actually mentioned about this once, and we have the
 confirmation now.

>>>
>>>   struct drm_mode_object base;
>>
>>
>> Aside: I've noticed all this trying to update the kerneldoc for struct
>> drm_bridge, which just showed that this patch makes inconsistent changes.
>> Trying to write kerneldoc is a really great way to come up with better
>> interfaces imo.
>>
>> Cheers, Daniel
 I din't get this actually. You want me to create Doc../drm_bridge.txt
 or something similar?
>>>
>>> If you want to document drm_bridge then I recomment to sprinkle proper
>>> kerneldoc over drm_bridge.c and pull it all into the drm DocBook
>>> template. That way all the drm documentation is in one place. I've
>>> done that for drm_crtc.h in an unrelated patch series (

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Russell King - ARM Linux]

On Mon, Oct 27, 2014 at 11:26:30PM +0100, Daniel Vetter wrote:
> Looking at the of_drm_find_panel function I actually wonder how that
> works - the drm_panel doesn't really need to stick around afaics.
> After all panel_list is global so some other driver can unload.
> Russell's of support for possible_crtcs code works differently since
> it only looks at per-drm_device lists.
> 
> This bridge code here though suffers from the same. So to me this
> looks rather fishy.
> 
> It doesn't help that we have drm_of.[hc] around but not all the of
> code is in there. Adding Russell too.

I rather dropped the ball with imx-drm when things became difficult with
asking Greg to pull my git tree - and as a result, I decided that I would
no longer be helping with patch submission for imx-drm, nor trying to get
it out of the staging tree anymore.

I do still have the patch (dated from July) in my git tree which adds it
to imx-drm - see below.  Rebased to 3.18-rc2 today.

I also have a patch (from April) which creates a generic OF DDC connector
helper, but that remains unfinished, in the sense that it lives beside
imx-drm, pulling the DDC specific code out of imx-hdmi and imx-tve, even
though there's nothing imx-drm specific about it.

8<
From: Russell King 
Subject: [PATCH] imx-drm: convert imx-drm to use the generic DRM OF helper

Use the generic DRM OF helper to locate the possible CRTCs for the
encoder, thereby shrinking the imx-drm driver some more.

Acked-by: Philipp Zabel 
Signed-off-by: Russell King 
---
 drivers/staging/imx-drm/imx-drm-core.c | 72 ++
 1 file changed, 13 insertions(+), 59 deletions(-)

diff --git a/drivers/staging/imx-drm/imx-drm-core.c 
b/drivers/staging/imx-drm/imx-drm-core.c
index 9cb222e2996f..5e2c1f4b9165 100644
--- a/drivers/staging/imx-drm/imx-drm-core.c
+++ b/drivers/staging/imx-drm/imx-drm-core.c
@@ -24,6 +24,7 @@
 #include 
 #include 
 #include 
+#include 

 #include "imx-drm.h"

@@ -47,7 +48,6 @@ struct imx_drm_crtc {
struct drm_crtc *crtc;
int pipe;
struct imx_drm_crtc_helper_funcsimx_drm_helper_funcs;
-   struct device_node  *port;
 };

 static int legacyfb_depth = 16;
@@ -366,9 +366,10 @@ int imx_drm_add_crtc(struct drm_device *drm, struct 
drm_crtc *crtc,

imx_drm_crtc->imx_drm_helper_funcs = *imx_drm_helper_funcs;
imx_drm_crtc->pipe = imxdrm->pipes++;
-   imx_drm_crtc->port = port;
imx_drm_crtc->crtc = crtc;

+   crtc->port = port;
+
imxdrm->crtc[imx_drm_crtc->pipe] = imx_drm_crtc;

*new_crtc = imx_drm_crtc;
@@ -409,34 +410,6 @@ int imx_drm_remove_crtc(struct imx_drm_crtc *imx_drm_crtc)
 }
 EXPORT_SYMBOL_GPL(imx_drm_remove_crtc);

-/*
- * Find the DRM CRTC possible mask for the connected endpoint.
- *
- * The encoder possible masks are defined by their position in the
- * mode_config crtc_list.  This means that CRTCs must not be added
- * or removed once the DRM device has been fully initialised.
- */
-static uint32_t imx_drm_find_crtc_mask(struct imx_drm_device *imxdrm,
-   struct device_node *endpoint)
-{
-   struct device_node *port;
-   unsigned i;
-
-   port = of_graph_get_remote_port(endpoint);
-   if (!port)
-   return 0;
-   of_node_put(port);
-
-   for (i = 0; i < MAX_CRTC; i++) {
-   struct imx_drm_crtc *imx_drm_crtc = imxdrm->crtc[i];
-
-   if (imx_drm_crtc && imx_drm_crtc->port == port)
-   return drm_crtc_mask(imx_drm_crtc->crtc);
-   }
-
-   return 0;
-}
-
 static struct device_node *imx_drm_of_get_next_endpoint(
const struct device_node *parent, struct device_node *prev)
 {
@@ -449,35 +422,16 @@ static struct device_node *imx_drm_of_get_next_endpoint(
 int imx_drm_encoder_parse_of(struct drm_device *drm,
struct drm_encoder *encoder, struct device_node *np)
 {
-   struct imx_drm_device *imxdrm = drm->dev_private;
-   struct device_node *ep = NULL;
-   uint32_t crtc_mask = 0;
-   int i;
+   uint32_t crtc_mask = drm_of_find_possible_crtcs(drm, np);

-   for (i = 0; ; i++) {
-   u32 mask;
-
-   ep = imx_drm_of_get_next_endpoint(np, ep);
-   if (!ep)
-   break;
-
-   mask = imx_drm_find_crtc_mask(imxdrm, ep);
-
-   /*
-* If we failed to find the CRTC(s) which this encoder is
-* supposed to be connected to, it's because the CRTC has
-* not been registered yet.  Defer probing, and hope that
-* the required CRTC is added later.
-*/
-   if (mask == 0)
-   return -EPROBE_DEFER;
-
-   crtc_mask |= mask;
-   }
-
-   of_node_put(ep);
-   if (i == 0)
-   return -ENOENT;
+   /*
+* If we failed to find the CRTC(s) 

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Daniel Vetter]

On Mon, Oct 27, 2014 at 11:20 PM, Daniel Vetter  wrote:
> On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul  wrote:
 @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
   * @driver_private: pointer to the bridge driver's internal context
   */
  struct drm_bridge {
 - struct drm_device *dev;
 + struct device *dev;
>>>
>>> Please don't rename the ->dev pointer into drm. Because _all_ the other
>>> drm structures still call it ->dev. Also, can't we use struct device_node
>>> here like we do in the of helpers Russell added? See 7e435aad38083
>>>
>>
>> I think this is modeled after the naming in drm_panel, FWIW. However,
>> seems reasonable to keep the device_node instead.
>
> Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with
> drm_crtc drop the struct device and go directly to a struct
> device_node. Since we don't really need the sturct device, the only
> thing we care about is the of_node. For added bonus wrap an #ifdef
> CONFIG_OF around all the various struct device_node in drm_foo.h.
> Should be all fairly simple to pull off with cocci.
>
> Thierry?

Looking at the of_drm_find_panel function I actually wonder how that
works - the drm_panel doesn't really need to stick around afaics.
After all panel_list is global so some other driver can unload.
Russell's of support for possible_crtcs code works differently since
it only looks at per-drm_device lists.

This bridge code here though suffers from the same. So to me this
looks rather fishy.

It doesn't help that we have drm_of.[hc] around but not all the of
code is in there. Adding Russell too.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Daniel Vetter]

On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul  wrote:
>>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
>>>   * @driver_private: pointer to the bridge driver's internal context
>>>   */
>>>  struct drm_bridge {
>>> - struct drm_device *dev;
>>> + struct device *dev;
>>
>> Please don't rename the ->dev pointer into drm. Because _all_ the other
>> drm structures still call it ->dev. Also, can't we use struct device_node
>> here like we do in the of helpers Russell added? See 7e435aad38083
>>
>
> I think this is modeled after the naming in drm_panel, FWIW. However,
> seems reasonable to keep the device_node instead.

Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with
drm_crtc drop the struct device and go directly to a struct
device_node. Since we don't really need the sturct device, the only
thing we care about is the of_node. For added bonus wrap an #ifdef
CONFIG_OF around all the various struct device_node in drm_foo.h.
Should be all fairly simple to pull off with cocci.

Thierry?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Daniel Vetter]

So don't ask why but I accidentally ended up in a branch looking at this
patch and didn't like it. So very quick&grumpy review.

First, please make the patch subject more descriptive: I'd expect a helper
function scaffolding like the various crtc/probe/dp ... helpers we already
have. You instead add code to untangle the probe ordering. Please say so.

More comments below.

On Wed, Aug 27, 2014 at 07:59:37PM +0530, Ajay Kumar wrote:
> A set of helper functions are defined in this patch to make
> bridge driver probe independent of the drm flow.
> 
> The bridge devices register themselves on a lookup table
> when they get probed by calling "drm_bridge_add".
> 
> The parent encoder driver waits till the bridge is available
> in the lookup table(by calling "of_drm_find_bridge") and then
> continues with its initialization.
> 
> The encoder driver should also call "drm_bridge_attach" to pass
> on the drm_device, encoder pointers to the bridge object.
> 
> drm_bridge_attach inturn calls drm_bridge_init to register itself
> with the drm core. Later, it calls "bridge->funcs->attach" so that
> bridge can continue with other initializations.
> 
> Signed-off-by: Ajay Kumar 

[snip]

> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
>   * @driver_private: pointer to the bridge driver's internal context
>   */
>  struct drm_bridge {
> - struct drm_device *dev;
> + struct device *dev;

Please don't rename the ->dev pointer into drm. Because _all_ the other
drm structures still call it ->dev. Also, can't we use struct device_node
here like we do in the of helpers Russell added? See 7e435aad38083

> + struct drm_device *drm;
> + struct drm_encoder *encoder;

This breaks bridge->bridge chaining (if we ever get there). It seems
pretty much unused anyway except for an EBUSY check. Can't you use
bridge->dev for that?

>   struct list_head head;
> + struct list_head list;

These lists need better names. I know that the "head" is really awful,
especially since it's actually not the head of the list but just an
element.
>  
>   struct drm_mode_object base;


Aside: I've noticed all this trying to update the kerneldoc for struct
drm_bridge, which just showed that this patch makes inconsistent changes.
Trying to write kerneldoc is a really great way to come up with better
interfaces imo.

Cheers, Daniel

>  
> @@ -906,6 +911,11 @@ extern void drm_connector_cleanup(struct drm_connector 
> *connector);
>  /* helper to unplug all connectors from sysfs for device */
>  extern void drm_connector_unplug_all(struct drm_device *dev);
>  
> +extern int drm_bridge_add(struct drm_bridge *bridge);
> +extern void drm_bridge_remove(struct drm_bridge *bridge);
> +extern struct drm_bridge *of_drm_find_bridge(struct device_node *np);
> +extern int drm_bridge_attach(struct drm_bridge *bridge,
> + struct drm_encoder *encoder);
>  extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge 
> *bridge);
>  extern void drm_bridge_cleanup(struct drm_bridge *bridge);
>  
> -- 
> 1.7.9.5
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Sean Paul]

On Wed, Aug 27, 2014 at 10:29 AM, Ajay Kumar  
wrote:
> A set of helper functions are defined in this patch to make
> bridge driver probe independent of the drm flow.
>
> The bridge devices register themselves on a lookup table
> when they get probed by calling "drm_bridge_add".
>
> The parent encoder driver waits till the bridge is available
> in the lookup table(by calling "of_drm_find_bridge") and then
> continues with its initialization.
>
> The encoder driver should also call "drm_bridge_attach" to pass
> on the drm_device, encoder pointers to the bridge object.
>
> drm_bridge_attach inturn calls drm_bridge_init to register itself
> with the drm core. Later, it calls "bridge->funcs->attach" so that
> bridge can continue with other initializations.
>
> Signed-off-by: Ajay Kumar 
> ---
>  drivers/gpu/drm/Makefile   |1 +
>  drivers/gpu/drm/bridge/Kconfig |   15 -
>  drivers/gpu/drm/drm_bridge.c   |  102 
> 
>  drivers/gpu/drm/drm_crtc.c |4 +-
>  drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |4 +-
>  include/drm/drm_crtc.h |   12 +++-
>  6 files changed, 131 insertions(+), 7 deletions(-)
>  create mode 100644 drivers/gpu/drm/drm_bridge.c
>
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 4a55d59..bdbfb6f 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -19,6 +19,7 @@ drm-y   :=drm_auth.o drm_buffer.o drm_bufs.o 
> drm_cache.o \
>  drm-$(CONFIG_COMPAT) += drm_ioc32.o
>  drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
>  drm-$(CONFIG_PCI) += ati_pcigart.o
> +drm-$(CONFIG_DRM_BRIDGE) += drm_bridge.o
>  drm-$(CONFIG_DRM_PANEL) += drm_panel.o
>  drm-$(CONFIG_OF) += drm_of.o
>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 884923f..5a8e907 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -1,5 +1,16 @@
> -config DRM_PTN3460
> -   tristate "PTN3460 DP/LVDS bridge"
> +config DRM_BRIDGE

I'm not convinced this adds any value, to be honest. In addition to
whether or not it's useful, it seems like you'd need to stub the
drm_bridge_* functions that are declared in drm_crtc.h or break them
out into drm_bridge.h.

Sean

> +   tristate
> depends on DRM
> select DRM_KMS_HELPER
> +   help
> + Bridge registration and lookup framework.
> +
> +menu "bridge chips"
> +   depends on DRM_BRIDGE
> +
> +config DRM_PTN3460
> +   tristate "PTN3460 DP/LVDS bridge"
> +   depends on DRM_BRIDGE
> ---help---
> +
> +endmenu
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> new file mode 100644
> index 000..b2d43fd
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -0,0 +1,102 @@
> +/*
> + * Copyright (c) 2014 Samsung Electronics Co., Ltd
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sub license,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the
> + * next paragraph) shall be included in all copies or substantial portions
> + * of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + */
> +
> +#include 
> +#include 
> +
> +#include 
> +
> +#include "drm/drmP.h"
> +
> +static DEFINE_MUTEX(bridge_lock);
> +static LIST_HEAD(bridge_list);
> +
> +int drm_bridge_add(struct drm_bridge *bridge)
> +{
> +   mutex_lock(&bridge_lock);
> +   list_add_tail(&bridge->list, &bridge_list);
> +   mutex_unlock(&bridge_lock);
> +
> +   return 0;
> +}
> +EXPORT_SYMBOL(drm_bridge_add);
> +
> +void drm_bridge_remove(struct drm_bridge *bridge)
> +{
> +   mutex_lock(&bridge_lock);
> +   list_del_init(&bridge->list);
> +   mutex_unlock(&bridge_lock);
> +}
> +EXPORT_SYMBOL(drm_bridge_remove);
> +
> +int drm_bridge_attach(struct drm_bridge *bridge,
> +   struct drm_encoder *encoder)
> +{
> +   int ret;
> +
> +   if (!bridge || !encoder)
> +   return -EINVAL;
> +
> +   if (bridge->encoder)
> +   return -EBUSY;
> +
> +   encoder->b

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Sean Paul]

On Mon, Oct 27, 2014 at 3:01 PM, Daniel Vetter  wrote:
> So don't ask why but I accidentally ended up in a branch looking at this
> patch and didn't like it. So very quick&grumpy review.
>
> First, please make the patch subject more descriptive: I'd expect a helper
> function scaffolding like the various crtc/probe/dp ... helpers we already
> have. You instead add code to untangle the probe ordering. Please say so.
>
> More comments below.
>
> On Wed, Aug 27, 2014 at 07:59:37PM +0530, Ajay Kumar wrote:
>> A set of helper functions are defined in this patch to make
>> bridge driver probe independent of the drm flow.
>>
>> The bridge devices register themselves on a lookup table
>> when they get probed by calling "drm_bridge_add".
>>
>> The parent encoder driver waits till the bridge is available
>> in the lookup table(by calling "of_drm_find_bridge") and then
>> continues with its initialization.
>>
>> The encoder driver should also call "drm_bridge_attach" to pass
>> on the drm_device, encoder pointers to the bridge object.
>>
>> drm_bridge_attach inturn calls drm_bridge_init to register itself
>> with the drm core. Later, it calls "bridge->funcs->attach" so that
>> bridge can continue with other initializations.
>>
>> Signed-off-by: Ajay Kumar 
>
> [snip]
>
>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
>>   * @driver_private: pointer to the bridge driver's internal context
>>   */
>>  struct drm_bridge {
>> - struct drm_device *dev;
>> + struct device *dev;
>
> Please don't rename the ->dev pointer into drm. Because _all_ the other
> drm structures still call it ->dev. Also, can't we use struct device_node
> here like we do in the of helpers Russell added? See 7e435aad38083
>

I think this is modeled after the naming in drm_panel, FWIW. However,
seems reasonable to keep the device_node instead.

>> + struct drm_device *drm;
>> + struct drm_encoder *encoder;
>
> This breaks bridge->bridge chaining (if we ever get there). It seems
> pretty much unused anyway except for an EBUSY check. Can't you use
> bridge->dev for that?
>

Yeah, I'd prefer to pass drm_device directly into drm_bridge_attach
and leave it up to the caller to establish the proper chain.

>>   struct list_head head;
>> + struct list_head list;
>
> These lists need better names. I know that the "head" is really awful,
> especially since it's actually not the head of the list but just an
> element.

I think we can just rip bridge_list out of mode_config if we're going
to keep track of bridges elsewhere. So we can nuke "head" and keep
"list". This also means that bridge->destroy() goes away, but that's
probably Ok if everything converts to the standalone driver model
where we have driver->remove()

Sean

>>
>>   struct drm_mode_object base;
>
>
> Aside: I've noticed all this trying to update the kerneldoc for struct
> drm_bridge, which just showed that this patch makes inconsistent changes.
> Trying to write kerneldoc is a really great way to come up with better
> interfaces imo.
>
> Cheers, Daniel
>
>>
>> @@ -906,6 +911,11 @@ extern void drm_connector_cleanup(struct drm_connector 
>> *connector);
>>  /* helper to unplug all connectors from sysfs for device */
>>  extern void drm_connector_unplug_all(struct drm_device *dev);
>>
>> +extern int drm_bridge_add(struct drm_bridge *bridge);
>> +extern void drm_bridge_remove(struct drm_bridge *bridge);
>> +extern struct drm_bridge *of_drm_find_bridge(struct device_node *np);
>> +extern int drm_bridge_attach(struct drm_bridge *bridge,
>> + struct drm_encoder *encoder);
>>  extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge 
>> *bridge);
>>  extern void drm_bridge_cleanup(struct drm_bridge *bridge);
>>
>> --
>> 1.7.9.5
>>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Ajay Kumar]

A set of helper functions are defined in this patch to make
bridge driver probe independent of the drm flow.

The bridge devices register themselves on a lookup table
when they get probed by calling "drm_bridge_add".

The parent encoder driver waits till the bridge is available
in the lookup table(by calling "of_drm_find_bridge") and then
continues with its initialization.

The encoder driver should also call "drm_bridge_attach" to pass
on the drm_device, encoder pointers to the bridge object.

drm_bridge_attach inturn calls drm_bridge_init to register itself
with the drm core. Later, it calls "bridge->funcs->attach" so that
bridge can continue with other initializations.

Signed-off-by: Ajay Kumar 
---
 drivers/gpu/drm/Makefile   |1 +
 drivers/gpu/drm/bridge/Kconfig |   15 -
 drivers/gpu/drm/drm_bridge.c   |  102 
 drivers/gpu/drm/drm_crtc.c |4 +-
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |4 +-
 include/drm/drm_crtc.h |   12 +++-
 6 files changed, 131 insertions(+), 7 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_bridge.c

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 4a55d59..bdbfb6f 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -19,6 +19,7 @@ drm-y   :=drm_auth.o drm_buffer.o drm_bufs.o 
drm_cache.o \
 drm-$(CONFIG_COMPAT) += drm_ioc32.o
 drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
 drm-$(CONFIG_PCI) += ati_pcigart.o
+drm-$(CONFIG_DRM_BRIDGE) += drm_bridge.o
 drm-$(CONFIG_DRM_PANEL) += drm_panel.o
 drm-$(CONFIG_OF) += drm_of.o

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 884923f..5a8e907 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -1,5 +1,16 @@
-config DRM_PTN3460
-   tristate "PTN3460 DP/LVDS bridge"
+config DRM_BRIDGE
+   tristate
depends on DRM
select DRM_KMS_HELPER
+   help
+ Bridge registration and lookup framework.
+
+menu "bridge chips"
+   depends on DRM_BRIDGE
+
+config DRM_PTN3460
+   tristate "PTN3460 DP/LVDS bridge"
+   depends on DRM_BRIDGE
---help---
+
+endmenu
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
new file mode 100644
index 000..b2d43fd
--- /dev/null
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -0,0 +1,102 @@
+/*
+ * Copyright (c) 2014 Samsung Electronics Co., Ltd
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sub license,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the
+ * next paragraph) shall be included in all copies or substantial portions
+ * of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+#include 
+#include 
+
+#include 
+
+#include "drm/drmP.h"
+
+static DEFINE_MUTEX(bridge_lock);
+static LIST_HEAD(bridge_list);
+
+int drm_bridge_add(struct drm_bridge *bridge)
+{
+   mutex_lock(&bridge_lock);
+   list_add_tail(&bridge->list, &bridge_list);
+   mutex_unlock(&bridge_lock);
+
+   return 0;
+}
+EXPORT_SYMBOL(drm_bridge_add);
+
+void drm_bridge_remove(struct drm_bridge *bridge)
+{
+   mutex_lock(&bridge_lock);
+   list_del_init(&bridge->list);
+   mutex_unlock(&bridge_lock);
+}
+EXPORT_SYMBOL(drm_bridge_remove);
+
+int drm_bridge_attach(struct drm_bridge *bridge,
+   struct drm_encoder *encoder)
+{
+   int ret;
+
+   if (!bridge || !encoder)
+   return -EINVAL;
+
+   if (bridge->encoder)
+   return -EBUSY;
+
+   encoder->bridge = bridge;
+   bridge->encoder = encoder;
+   bridge->drm = encoder->dev;
+
+   ret = drm_bridge_init(bridge->drm, bridge);
+   if (ret) {
+   DRM_ERROR("Failed to register bridge with drm\n");
+   return ret;
+   }
+
+   if (bridge->funcs->attach)
+   return bridge->funcs->attach(bridge);
+
+   return 0;
+}
+EXPORT_SYMBOL(drm_bridge_attach);
+
+#ifdef CONFIG_OF
+struct drm_bridge *of_drm_find_bridge(struct device_node *np)
+{
+   struct drm_bridge *bridge;
+
+   mute