Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge.

[Laurent Pinchart]

Hi Daniel,

On Monday 07 Aug 2017 16:59:39 Daniel Vetter wrote:
> On Mon, Aug 07, 2017 at 01:22:23PM +0300, Laurent Pinchart wrote:
> > On Monday 07 Aug 2017 11:25:07 Daniel Vetter wrote:
> >> On Sat, Aug 05, 2017 at 12:59:07PM +0200, Noralf Trønnes wrote:
> >>> Den 05.08.2017 00.19, skrev Ilia Mirkin:
> >>>> On Fri, Aug 4, 2017 at 4:43 PM, Eric Anholt <eric@xxxxxxxxxx> wrote:
> >>>>> Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> writes:
> >>>>>> On Tuesday 18 Jul 2017 14:05:06 Eric Anholt wrote:
> >>>>>>> This will let drivers reduce the error cleanup they need, in
> >>>>>>> particular the "is_panel_bridge" flag.
> >>>>>>> 
> >>>>>>> v2: Slight cleanup of remove function by Andrzej
> >>>>>> 
> >>>>>> I just want to point out that, in the context of Daniel's work on
> >>>>>> hot-unplug, 90% of the devm_* allocations are wrong and will get in
> >>>>>> the way. All DRM core objects that are accessible one way or
> >>>>>> another from userspace will need to be properly reference-counted
> >>>>>> and freed only when the last reference disappears, which could be
> >>>>>> well after the corresponding device is removed. I believe this
> >>>>>> could be one such objects :-/
> >>>>> 
> >>>>> Sure, if you're hotplugging, your life is pain.  For
> >>>>> non-hotpluggable devices, like our SOC platform devices (current
> >>>>> panel-bridge consumers), this still seems like an excellent
> >>>>> simplification of memory management.
> >>>> 
> >>>> At that point you may as well make your module non-unloadable, and
> >>>> return failure when trying to remove a device from management by the
> >>>> driver (whatever the opposite of "probe" is, I forget). Hotplugging
> >>>> doesn't only happen when physically removing, it can happen for all
> >>>> kinds of reasons... and userspace may still hold references in some
> >>>> of those cases.
> >>> 
> >>> If drm_open() gets a ref on dev->dev and puts it in drm_release(),
> >>> won't that delay devm_* cleanup until userspace is done?
> >> 
> >> No. drm_device is the thing that is refcounted for userspace references
> >> like open FD (we're not perfect about it, e.g. sysfs and dma-buf/fence
> >> don't).
> >> 
> >> devm_ otoh is tied to the lifetime of the underlying device, and that
> >> one can get outlived by drm_device. Or at least afaiui, devm_ stuff is
> >> nuked on unplug, and not when the final sw reference of the struct device
> >> disappears.
> >> 
> >> Not sure tough, it's complicated.
> > 
> > It's complicated when you have to understand the behaviour by reading the
> > code, but the behaviour isn't that complex. devm resources are released
> > both
> > 
> > 1. right after the driver's .remove() operation returns
> > 2. when the device is deleted (last reference released)
> 
> Right, I had vague memories when reading the code ... But iirc there's
> also some way to delay cleanup until it's deleted, maybe we could exploit
> that (and wrap it up into a drm_devm_add wrapper)?
> 
> Plan B, but that is a lot more ugly, would be to create a fake struct
> device that we never bind (hence remove can't be called) and use to track
> the lifetime of drm_device stuff. Would avoid re-inventing all the devm_
> functions, but would also result in a dummy device in sysfs.

If we wanted to go that way, we should decouple devm_* from struct device (or 
even struct kobject) and tie it to a new resource tracker structure. The 
current devm_* API would remain the same, embedding that resource tracker 
object in struct device, but we could also use the machinery with the resource 
tracker object directly.

However, I'm not convince we should do so. We don't have any automatic memory 
allocation system for DRM objects (such as framebuffers or planes). Instead, 
drivers must implement a destroy operation for the object, and the core calls 
it when the last reference to the object goes away. This works fine, and I 
don't see what would prevent use from implementing the same mechanism for 
bridges or other similar structures.

> Why is this stuff tied to struct device and not struct kobject anyway. Or
> at least something we could embed in random cool place (like drm_device).
> 
> Greg, any ideas how we could simplify management of stuff that's created
> at driver load, but its lifetime tied to something which isn't directly a
> struct device?
> 
> > It's the first one that makes devm_* allocation unsuitable for any
> > structure that is accessible from userspace (such as in file operation
> > handlers).
> 
> Yeah, if we could release at 2. it would be perfect I think ...

It wouldn't, as unbind/rebind cycles would allocate new objects each time 
without freeing the previous ones until the device is deleted.

-- 
Regards,

Laurent Pinchart