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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3181 bytes --]

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 <daniel@ffwll.ch> wrote:
> > > > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul <seanpaul@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".

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel