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

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

[Chris Wilson]

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);
 
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index c4fc49583dc0..554104ccb939 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -71,6 +71,14 @@ struct drm_driver {
 	void (*postclose) (struct drm_device *, struct drm_file *);
 	void (*lastclose) (struct drm_device *);
 	int (*unload) (struct drm_device *);
+	/**
+	 * @release:
+	 *
+	 * Optional callback for destroying device state after the final
+	 * reference is released, i.e. the device is being destroyed.
+	 */
+	void (*release) (struct drm_device *);
+
 	int (*dma_ioctl) (struct drm_device *dev, void *data, struct drm_file *file_priv);
 	int (*dma_quiescent) (struct drm_device *);
 	int (*context_dtor) (struct drm_device *dev, int context);
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Thu, Dec 08, 2016 at 08:18:40AM +0000, 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);
>  
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index c4fc49583dc0..554104ccb939 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -71,6 +71,14 @@ struct drm_driver {
>  	void (*postclose) (struct drm_device *, struct drm_file *);
>  	void (*lastclose) (struct drm_device *);
>  	int (*unload) (struct drm_device *);
> +	/**
> +	 * @release:
> +	 *
> +	 * Optional callback for destroying device state after the final
> +	 * reference is released, i.e. the device is being destroyed.
> +	 */
> +	void (*release) (struct drm_device *);

I think sprinkling a reference to this hook into the documentation for
drm_dev_put would be real good. There's also a pile of text citing
drm_dev_unref(), especially in the overview section. I think that should
be udated to explain that release memory should only happen in ->release.
And I think with this change we can remove the cautious note that drm_dev_ref/unref are busted, too.

And while reviewing this entire mess I've noticed that we probably want to
move the call for drm_vblank_cleanup() from _unregister to
drm_dev_release?
-Daniel
> +
>  	int (*dma_ioctl) (struct drm_device *dev, void *data, struct drm_file *file_priv);
>  	int (*dma_quiescent) (struct drm_device *);
>  	int (*context_dtor) (struct drm_device *dev, int context);
> -- 
> 2.11.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Laurent Pinchart]

Hi Chris,

Thank you for the patch.

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. Similarly, the kfree() at the end 
should be made conditional. Or, better, you could implement a release function 
that just wraps kfree(), and set it as the release handler in drm_dev_alloc(). 
Drivers using drm_dev_init() would need to provide their own release handler.

> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index c4fc49583dc0..554104ccb939 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -71,6 +71,14 @@ struct drm_driver {
>  	void (*postclose) (struct drm_device *, struct drm_file *);
>  	void (*lastclose) (struct drm_device *);
>  	int (*unload) (struct drm_device *);
> +	/**
> +	 * @release:
> +	 *
> +	 * Optional callback for destroying device state after the final
> +	 * reference is released, i.e. the device is being destroyed.
> +	 */
> +	void (*release) (struct drm_device *);
> +
>  	int (*dma_ioctl) (struct drm_device *dev, void *data, struct drm_file
> *file_priv);
> 	int (*dma_quiescent) (struct drm_device *);
>  	int (*context_dtor) (struct drm_device *dev, int context);

-- 
Regards,

Laurent Pinchart

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

On Wed, Dec 14, 2016 at 12:04:38AM +0200, Laurent Pinchart wrote:
> Hi Chris,
> 
> Thank you for the patch.
> 
> 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.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Laurent Pinchart]

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