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

[Andrzej Hajda <a.hajda@samsung.com>]

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 <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".
>>
>> 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


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