Re: [PATCH 24/51] drm: Manage drm_vblank_cleanup with drmm_

[Sam Ravnborg <sam@ravnborg.org>]

On Mon, Mar 02, 2020 at 11:26:04PM +0100, Daniel Vetter wrote:
> Nothing special here, except that this is the first time that we
> automatically clean up something that's initialized with an explicit
> driver call. But the cleanup was done at the very of the release
the very what?

> sequence for all drivers, and that's still the case. At least without
> more uses of drmm_ through explicit driver calls.

This patch does not simplify things in itself.
The motivation here is to remove the drm_dev_fini()
dependencies from the drivers.

So drm_dev_fini() can be dropped in a follow-up patch.
Maybe extend the changelog a little?

> Also for this one we need drmm_kcalloc, so lets add those
> 
> v2: Sort includes (Laurent)
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Acked-by: Sam Ravnborg <sam@ravnborg.org>

> ---
>  drivers/gpu/drm/drm_drv.c      |  1 -
>  drivers/gpu/drm/drm_internal.h |  1 -
>  drivers/gpu/drm/drm_vblank.c   | 31 ++++++++++++-------------------
>  include/drm/drm_managed.h      | 16 ++++++++++++++++
>  4 files changed, 28 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 9a646155dfc6..90b6ae81d431 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -752,7 +752,6 @@ EXPORT_SYMBOL(devm_drm_dev_init);
>   */
>  void drm_dev_fini(struct drm_device *dev)
>  {
> -	drm_vblank_cleanup(dev);
>  }
>  EXPORT_SYMBOL(drm_dev_fini);
>  
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index cb09e95a795e..e67015d07c4c 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -94,7 +94,6 @@ void drm_managed_release(struct drm_device *dev);
>  
>  /* drm_vblank.c */
>  void drm_vblank_disable_and_save(struct drm_device *dev, unsigned int pipe);
> -void drm_vblank_cleanup(struct drm_device *dev);
>  
>  /* IOCTLS */
>  int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 47fc4339ec7f..5a6ec8aa0873 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -30,6 +30,7 @@
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_drv.h>
>  #include <drm/drm_framebuffer.h>
> +#include <drm/drm_managed.h>
>  #include <drm/drm_modeset_helper_vtables.h>
>  #include <drm/drm_print.h>
>  #include <drm/drm_vblank.h>
> @@ -425,14 +426,10 @@ static void vblank_disable_fn(struct timer_list *t)
>  	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
>  }
>  
> -void drm_vblank_cleanup(struct drm_device *dev)
> +static void drm_vblank_init_release(struct drm_device *dev, void *ptr)
>  {
>  	unsigned int pipe;
>  
> -	/* Bail if the driver didn't call drm_vblank_init() */
> -	if (dev->num_crtcs == 0)
> -		return;
> -
>  	for (pipe = 0; pipe < dev->num_crtcs; pipe++) {
>  		struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
>  
> @@ -441,10 +438,6 @@ void drm_vblank_cleanup(struct drm_device *dev)
>  
>  		del_timer_sync(&vblank->disable_timer);
>  	}
> -
> -	kfree(dev->vblank);
> -
> -	dev->num_crtcs = 0;
>  }
>  
>  /**
> @@ -453,25 +446,29 @@ void drm_vblank_cleanup(struct drm_device *dev)
>   * @num_crtcs: number of CRTCs supported by @dev
>   *
>   * This function initializes vblank support for @num_crtcs display pipelines.
> - * Cleanup is handled by the DRM core, or through calling drm_dev_fini() for
> - * drivers with a &drm_driver.release callback.
> + * Cleanup is handled automatically through a cleanup function added with
> + * drmm_add_action().
>   *
>   * Returns:
>   * Zero on success or a negative error code on failure.
>   */
>  int drm_vblank_init(struct drm_device *dev, unsigned int num_crtcs)
>  {
> -	int ret = -ENOMEM;
> +	int ret;
>  	unsigned int i;
>  
>  	spin_lock_init(&dev->vbl_lock);
>  	spin_lock_init(&dev->vblank_time_lock);
>  
> +	dev->vblank = drmm_kcalloc(dev, num_crtcs, sizeof(*dev->vblank), GFP_KERNEL);
> +	if (!dev->vblank)
> +		return -ENOMEM;
> +
>  	dev->num_crtcs = num_crtcs;
>  
> -	dev->vblank = kcalloc(num_crtcs, sizeof(*dev->vblank), GFP_KERNEL);
> -	if (!dev->vblank)
> -		goto err;
> +	ret = drmm_add_action(dev, drm_vblank_init_release, NULL);
> +	if (ret)
> +		return ret;
>  
>  	for (i = 0; i < num_crtcs; i++) {
>  		struct drm_vblank_crtc *vblank = &dev->vblank[i];
> @@ -486,10 +483,6 @@ int drm_vblank_init(struct drm_device *dev, unsigned int num_crtcs)
>  	DRM_INFO("Supports vblank timestamp caching Rev 2 (21.10.2013).\n");
>  
>  	return 0;
> -
> -err:
> -	dev->num_crtcs = 0;
> -	return ret;
>  }
>  EXPORT_SYMBOL(drm_vblank_init);
>  
> diff --git a/include/drm/drm_managed.h b/include/drm/drm_managed.h
> index 5280209dff92..2b1ba2ad5582 100644
> --- a/include/drm/drm_managed.h
> +++ b/include/drm/drm_managed.h
> @@ -4,6 +4,7 @@
>  #define _DRM_MANAGED_H_
>  
>  #include <linux/gfp.h>
> +#include <linux/overflow.h>
>  #include <linux/types.h>
>  
>  struct drm_device;
> @@ -28,6 +29,21 @@ static inline void *drmm_kzalloc(struct drm_device *dev, size_t size, gfp_t gfp)
>  {
>  	return drmm_kmalloc(dev, size, gfp | __GFP_ZERO);
>  }
> +static inline void *drmm_kmalloc_array(struct drm_device *dev,
> +				       size_t n, size_t size, gfp_t flags)
> +{
> +	size_t bytes;
> +
> +	if (unlikely(check_mul_overflow(n, size, &bytes)))
> +		return NULL;
> +
> +	return drmm_kmalloc(dev, bytes, flags);
> +}
> +static inline void *drmm_kcalloc(struct drm_device *dev,
> +				 size_t n, size_t size, gfp_t flags)
> +{
> +	return drmm_kmalloc_array(dev, n, size, flags | __GFP_ZERO);
> +}
>  char *drmm_kstrdup(struct drm_device *dev, const char *s, gfp_t gfp);
>  
>  void drmm_kfree(struct drm_device *dev, void *data);
> -- 
> 2.24.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel