Re: [PATCH 24/52] drm: Manage drm_gem_init with drmm_

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

Hi Daniel,

Thank you for the patch.

On Wed, Feb 19, 2020 at 11:20:54AM +0100, Daniel Vetter wrote:
> We might want to look into pushing this down into drm_mm_init, but
> that would mean rolling out return codes to a pile of functions
> unfortunately. So let's leave that for now.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_drv.c      |  8 +-------
>  drivers/gpu/drm/drm_gem.c      | 21 ++++++++++-----------
>  drivers/gpu/drm/drm_internal.h |  1 -
>  3 files changed, 11 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 03a1fb377830..7b3df1188da9 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -688,13 +688,10 @@ int drm_dev_init(struct drm_device *dev,
>  
>  	ret = drm_dev_set_unique(dev, dev_name(parent));
>  	if (ret)
> -		goto err_setunique;
> +		goto err;
>  
>  	return 0;
>  
> -err_setunique:
> -	if (drm_core_check_feature(dev, DRIVER_GEM))
> -		drm_gem_destroy(dev);
>  err:
>  	drm_managed_release(dev);
>  
> @@ -756,9 +753,6 @@ EXPORT_SYMBOL(devm_drm_dev_init);
>  void drm_dev_fini(struct drm_device *dev)
>  {
>  	drm_vblank_cleanup(dev);
> -
> -	if (drm_core_check_feature(dev, DRIVER_GEM))
> -		drm_gem_destroy(dev);
>  }
>  EXPORT_SYMBOL(drm_dev_fini);
>  
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 0b6e6623735e..31095e0f6b9f 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -44,6 +44,7 @@
>  #include <drm/drm_drv.h>
>  #include <drm/drm_file.h>
>  #include <drm/drm_gem.h>
> +#include <drm/drm_managed.h>
>  #include <drm/drm_print.h>
>  #include <drm/drm_vma_manager.h>
>  
> @@ -77,6 +78,12 @@
>   * up at a later date, and as our interface with shmfs for memory allocation.
>   */
>  
> +static void
> +drm_gem_init_release(struct drm_device *dev, void *ptr)
> +{
> +	drm_vma_offset_manager_destroy(dev->vma_offset_manager);
> +}
> +
>  /**
>   * drm_gem_init - Initialize the GEM device fields
>   * @dev: drm_devic structure to initialize
> @@ -89,7 +96,8 @@ drm_gem_init(struct drm_device *dev)
>  	mutex_init(&dev->object_name_lock);
>  	idr_init_base(&dev->object_name_idr, 1);
>  
> -	vma_offset_manager = kzalloc(sizeof(*vma_offset_manager), GFP_KERNEL);
> +	vma_offset_manager = drmm_kzalloc(dev, sizeof(*vma_offset_manager),
> +					  GFP_KERNEL);
>  	if (!vma_offset_manager) {
>  		DRM_ERROR("out of memory\n");
>  		return -ENOMEM;
> @@ -100,16 +108,7 @@ drm_gem_init(struct drm_device *dev)
>  				    DRM_FILE_PAGE_OFFSET_START,
>  				    DRM_FILE_PAGE_OFFSET_SIZE);
>  
> -	return 0;
> -}
> -
> -void
> -drm_gem_destroy(struct drm_device *dev)
> -{
> -
> -	drm_vma_offset_manager_destroy(dev->vma_offset_manager);
> -	kfree(dev->vma_offset_manager);
> -	dev->vma_offset_manager = NULL;
> +	return drmm_add_action(dev, drm_gem_init_release, NULL);

This looks fine as such (although I'm not sure if the managed API
overhead is really worth it for core code), but it leads to a potential
issue: if we handle more of the cleanup through the managed API, how do
we ensure that the cleanup functions are called in the right order (when
order matters) ?

>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index 8c2628dfc6c7..cb09e95a795e 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -144,7 +144,6 @@ void drm_sysfs_lease_event(struct drm_device *dev);
>  /* drm_gem.c */
>  struct drm_gem_object;
>  int drm_gem_init(struct drm_device *dev);
> -void drm_gem_destroy(struct drm_device *dev);
>  int drm_gem_handle_create_tail(struct drm_file *file_priv,
>  			       struct drm_gem_object *obj,
>  			       u32 *handlep);

-- 
Regards,

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