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

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

Hi Chris,

Thank you for the patch.

On Thursday 02 Feb 2017 09:08:20 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.
> 
> v2: Export drm_dev_fini() and move the responsibility for finalizing the
> drm_device and freeing it to the release callback. (If no callback is
> provided, the core will call drm_dev_fini() and kfree(dev) as before.)
> v3: Remember to add drm_dev_fini() to drm_drv.h
> v4: Tidy language for kerneldoc
> v5: Cross reference from drm_dev_init() to note that driver->release()
> allows for arbitrary embedding.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_drv.c | 65 ++++++++++++++++++++++++++++++--------------
>  include/drm/drm_drv.h     | 13 ++++++++++
>  2 files changed, 58 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index a8ce3179c07c..fe611d601916 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -465,7 +465,10 @@ static void drm_fs_inode_free(struct inode *inode)
>   * that do embed &struct drm_device it must be placed first in the overall
>   * structure, and the overall structure must be allocated using kmalloc():
> The * drm core's release function unconditionally calls kfree() on the @dev
> pointer - * when the final reference is released.
> + * when the final reference is released. To override this behaviour, and so
> + * allow embedding of the drm_device inside the driver's device struct at
> an + * arbitrary offset, you must supply a driver->release() callback and
> control + * the finalization explicitly.
>   *
>   * RETURNS:
>   * 0 on success, or error code on failure.
> @@ -553,6 +556,41 @@ int drm_dev_init(struct drm_device *dev,
>  EXPORT_SYMBOL(drm_dev_init);
> 
>  /**
> + * drm_dev_fini - Finalize a dead DRM device
> + * @dev: DRM device
> + *
> + * Finalize a dead DRM device. This is the converse to drm_dev_init() and
> + * frees up all state allocated by it. All driver state should be finalized
> + * first. Note that this function does not free the @dev, that is left to
> the + * caller.
> + *
> + * The ref-count of @dev must be zero, and drm_dev_fini() should only be
> called + * from a drm_driver->release() callback.
> + */
> +void drm_dev_fini(struct drm_device *dev)
> +{
> +	drm_vblank_cleanup(dev);
> +
> +	if (drm_core_check_feature(dev, DRIVER_GEM))
> +		drm_gem_destroy(dev);
> +
> +	drm_legacy_ctxbitmap_cleanup(dev);
> +	drm_ht_remove(&dev->map_hash);
> +	drm_fs_inode_free(dev->anon_inode);
> +
> +	drm_minor_free(dev, DRM_MINOR_PRIMARY);
> +	drm_minor_free(dev, DRM_MINOR_RENDER);
> +	drm_minor_free(dev, DRM_MINOR_CONTROL);
> +
> +	mutex_destroy(&dev->master_mutex);
> +	mutex_destroy(&dev->ctxlist_mutex);
> +	mutex_destroy(&dev->filelist_mutex);
> +	mutex_destroy(&dev->struct_mutex);
> +	kfree(dev->unique);
> +}
> +EXPORT_SYMBOL(drm_dev_fini);
> +
> +/**
>   * drm_dev_alloc - Allocate new DRM device
>   * @driver: DRM driver to allocate device for
>   * @parent: Parent device object
> @@ -598,25 +636,12 @@ static void drm_dev_release(struct kref *ref)
>  {
>  	struct drm_device *dev = container_of(ref, struct drm_device, ref);
> 
> -	drm_vblank_cleanup(dev);
> -
> -	if (drm_core_check_feature(dev, DRIVER_GEM))
> -		drm_gem_destroy(dev);
> -
> -	drm_legacy_ctxbitmap_cleanup(dev);
> -	drm_ht_remove(&dev->map_hash);
> -	drm_fs_inode_free(dev->anon_inode);
> -
> -	drm_minor_free(dev, DRM_MINOR_PRIMARY);
> -	drm_minor_free(dev, DRM_MINOR_RENDER);
> -	drm_minor_free(dev, DRM_MINOR_CONTROL);
> -
> -	mutex_destroy(&dev->master_mutex);
> -	mutex_destroy(&dev->ctxlist_mutex);
> -	mutex_destroy(&dev->filelist_mutex);
> -	mutex_destroy(&dev->struct_mutex);
> -	kfree(dev->unique);
> -	kfree(dev);
> +	if (dev->driver->release) {
> +		dev->driver->release(dev);
> +	} else {
> +		drm_dev_fini(dev);
> +		kfree(dev);
> +	}
>  }
> 
>  /**
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index 732e85652d1e..d0d2fa83d06c 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -102,6 +102,17 @@ struct drm_driver {
>  	 *
>  	 */
>  	void (*unload) (struct drm_device *);
> +
> +	/**
> +	 * @release:
> +	 *
> +	 * Optional callback for destroying device state after the final

Nitpicking, I'd talk about "device data" or "device memory" instead of "device 
state", as the latter could be confused with the device atomic state. Apart 
from that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	 * reference is released, i.e. the device is being destroyed. Drivers
> +	 * using this callback are responsible for calling drm_dev_fini()
> +	 * to finalize the device and then freeing the struct themselves.
> +	 */
> +	void (*release) (struct drm_device *);
> +
>  	int (*set_busid)(struct drm_device *dev, struct drm_master *master);
> 
>  	/**
> @@ -437,6 +448,8 @@ extern unsigned int drm_debug;
>  int drm_dev_init(struct drm_device *dev,
>  		 struct drm_driver *driver,
>  		 struct device *parent);
> +void drm_dev_fini(struct drm_device *dev);
> +
>  struct drm_device *drm_dev_alloc(struct drm_driver *driver,
>  				 struct device *parent);
>  int drm_dev_register(struct drm_device *dev, unsigned long flags);

-- 
Regards,

Laurent Pinchart

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