Re: [PATCH] drm: Provide a driver hook for drm_dev_release()

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Chris,

On Thursday 19 Jan 2017 11:20:37 Chris Wilson wrote:
> On Wed, Dec 14, 2016 at 12:04:38AM +0200, Laurent Pinchart wrote:
> > On Thursday 08 Dec 2016 08:18:40 Chris Wilson wrote:
> >> Some state is coupled into the device lifetime outside of the
> >> load/unload timeframe and requires teardown during final unreference
> >> from drm_dev_release(). For example, dmabufs hold both a device and
> >> module reference and may live longer than expected (i.e. the current
> >> pattern of the driver tearing down its state and then releasing a
> >> reference to the drm device) and yet touch driver private state when
> >> destroyed.
> >> 
> >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >> ---
> >> 
> >>  drivers/gpu/drm/drm_drv.c | 3 +++
> >>  include/drm/drm_drv.h     | 8 ++++++++
> >>  2 files changed, 11 insertions(+)
> >> 
> >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> >> index f74b7d06ec01..f945bbcc8eb3 100644
> >> --- a/drivers/gpu/drm/drm_drv.c
> >> +++ b/drivers/gpu/drm/drm_drv.c
> >> @@ -595,6 +595,9 @@ static void drm_dev_release(struct kref *ref)
> >>  {
> >>  	struct drm_device *dev = container_of(ref, struct drm_device, ref);
> >> 
> >> +	if (dev->driver->release)
> >> +		dev->driver->release(dev);
> >> +
> >>  	if (drm_core_check_feature(dev, DRIVER_GEM))
> >>  		drm_gem_destroy(dev);
> > 
> > For drivers embedding the drm_device structure, you should only call
> > .release() at the very end of this function, as the callback will free
> > memory, including the embedded struct drm_device.
> 
> Not quite. The layering is wrong as the driver needs to release its
> state prior to e.g. drm_gem_destroy. It is not for freeing the memory.

drm_dev_release() is the drm_device kref's release function. It is the last 
function called on a drm_device when the last reference goes away. It's 
responsible for performing the last cleanups, and turning the lights off.

Turning the lights off involves freeing the drm_device memory. This is 
currently done by drm_dev_release() unconditionally. When drivers allocate the 
drm_device structure dynamically with drm_dev_alloc() the existing code is 
fine. However, when drivers embed the drm_device structure in a driver-
specific structure, the kfree() at the end of drm_dev_release() only does the 
right thing if the driver-specific structure embeds the drm_device as the very 
first field. This is a hack, and should eventually be fixed, by propagating 
the release call to the driver and having the driver free the memory it has 
allocated. This can only be done as the very last operation in 
drm_dev_release().

When drm_dev_release() is called all GEM objects should have been destroyed 
already. The ones exported through dmabuf should hold a reference to the 
drm_device instance, which is dropped when the dmabufs are destroyed. It 
should thus be safe to call drm_gem_destroy() before the driver's release() 
callback.

-- 
Regards,

Laurent Pinchart

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