Re: [PATCH 4/6] drm: Introduce drm_framebuffer_{get,put}()

[Sean Paul <seanpaul@chromium.org>]

On Wed, Feb 08, 2017 at 07:24:06PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> For consistency with other reference counting APIs in the kernel, add
> drm_framebuffer_get() and drm_framebuffer_put() to reference count DRM
> framebuffers.
> 
> Compatibility aliases are added to keep existing code working. To help
> speed up the transition, all the instances of the old functions in the
> DRM core are already replaced in this commit.
> 
> The existing semantic patch for the DRM subsystem-wide conversion is
> extended to account for these new helpers.
> 

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/drm/drm_atomic.c             |  6 ++--
>  drivers/gpu/drm/drm_atomic_helper.c      |  4 +--
>  drivers/gpu/drm/drm_crtc.c               |  8 +++---
>  drivers/gpu/drm/drm_framebuffer.c        | 34 +++++++++++-----------
>  drivers/gpu/drm/drm_plane.c              | 12 ++++----
>  include/drm/drm_framebuffer.h            | 49 ++++++++++++++++++++++++--------
>  scripts/coccinelle/api/drm-get-put.cocci | 10 +++++++
>  7 files changed, 79 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 82bad40b2f3e..740650b450c5 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -733,7 +733,7 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>  		struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val);
>  		drm_atomic_set_fb_for_plane(state, fb);
>  		if (fb)
> -			drm_framebuffer_unreference(fb);
> +			drm_framebuffer_put(fb);
>  	} else if (property == config->prop_in_fence_fd) {
>  		if (state->fence)
>  			return -EINVAL;
> @@ -1837,12 +1837,12 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
>  		if (ret == 0) {
>  			struct drm_framebuffer *new_fb = plane->state->fb;
>  			if (new_fb)
> -				drm_framebuffer_reference(new_fb);
> +				drm_framebuffer_get(new_fb);
>  			plane->fb = new_fb;
>  			plane->crtc = plane->state->crtc;
>  
>  			if (plane->old_fb)
> -				drm_framebuffer_unreference(plane->old_fb);
> +				drm_framebuffer_put(plane->old_fb);
>  		}
>  		plane->old_fb = NULL;
>  	}
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 9f2c28ddf948..4dcd64ca34c8 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3151,7 +3151,7 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
>  	memcpy(state, plane->state, sizeof(*state));
>  
>  	if (state->fb)
> -		drm_framebuffer_reference(state->fb);
> +		drm_framebuffer_get(state->fb);
>  
>  	state->fence = NULL;
>  }
> @@ -3191,7 +3191,7 @@ EXPORT_SYMBOL(drm_atomic_helper_plane_duplicate_state);
>  void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
>  {
>  	if (state->fb)
> -		drm_framebuffer_unreference(state->fb);
> +		drm_framebuffer_put(state->fb);
>  
>  	if (state->fence)
>  		dma_fence_put(state->fence);
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 9594c623799b..e2974d3c92e7 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -471,9 +471,9 @@ int drm_mode_set_config_internal(struct drm_mode_set *set)
>  
>  	drm_for_each_crtc(tmp, crtc->dev) {
>  		if (tmp->primary->fb)
> -			drm_framebuffer_reference(tmp->primary->fb);
> +			drm_framebuffer_get(tmp->primary->fb);
>  		if (tmp->primary->old_fb)
> -			drm_framebuffer_unreference(tmp->primary->old_fb);
> +			drm_framebuffer_put(tmp->primary->old_fb);
>  		tmp->primary->old_fb = NULL;
>  	}
>  
> @@ -567,7 +567,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>  			}
>  			fb = crtc->primary->fb;
>  			/* Make refcounting symmetric with the lookup path. */
> -			drm_framebuffer_reference(fb);
> +			drm_framebuffer_get(fb);
>  		} else {
>  			fb = drm_framebuffer_lookup(dev, crtc_req->fb_id);
>  			if (!fb) {
> @@ -680,7 +680,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>  
>  out:
>  	if (fb)
> -		drm_framebuffer_unreference(fb);
> +		drm_framebuffer_put(fb);
>  
>  	if (connector_set) {
>  		for (i = 0; i < crtc_req->count_connectors; i++) {
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index 11daa24692aa..5e8e1d06866a 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -52,13 +52,13 @@
>   * metadata fields.
>   *
>   * The lifetime of a drm framebuffer is controlled with a reference count,
> - * drivers can grab additional references with drm_framebuffer_reference() and
> - * drop them again with drm_framebuffer_unreference(). For driver-private
> - * framebuffers for which the last reference is never dropped (e.g. for the
> - * fbdev framebuffer when the struct &struct drm_framebuffer is embedded into
> - * the fbdev helper struct) drivers can manually clean up a framebuffer at
> - * module unload time with drm_framebuffer_unregister_private(). But doing this
> - * is not recommended, and it's better to have a normal free-standing &struct
> + * drivers can grab additional references with drm_framebuffer_get() and drop
> + * them again with drm_framebuffer_put(). For driver-private framebuffers for
> + * which the last reference is never dropped (e.g. for the fbdev framebuffer
> + * when the struct &struct drm_framebuffer is embedded into the fbdev helper
> + * struct) drivers can manually clean up a framebuffer at module unload time
> + * with drm_framebuffer_unregister_private(). But doing this is not
> + * recommended, and it's better to have a normal free-standing &struct
>   * drm_framebuffer.
>   */
>  
> @@ -374,7 +374,7 @@ int drm_mode_rmfb(struct drm_device *dev,
>  	mutex_unlock(&file_priv->fbs_lock);
>  
>  	/* drop the reference we picked up in framebuffer lookup */
> -	drm_framebuffer_unreference(fb);
> +	drm_framebuffer_put(fb);
>  
>  	/*
>  	 * we now own the reference that was stored in the fbs list
> @@ -394,12 +394,12 @@ int drm_mode_rmfb(struct drm_device *dev,
>  		flush_work(&arg.work);
>  		destroy_work_on_stack(&arg.work);
>  	} else
> -		drm_framebuffer_unreference(fb);
> +		drm_framebuffer_put(fb);
>  
>  	return 0;
>  
>  fail_unref:
> -	drm_framebuffer_unreference(fb);
> +	drm_framebuffer_put(fb);
>  	return -ENOENT;
>  }
>  
> @@ -453,7 +453,7 @@ int drm_mode_getfb(struct drm_device *dev,
>  		ret = -ENODEV;
>  	}
>  
> -	drm_framebuffer_unreference(fb);
> +	drm_framebuffer_put(fb);
>  
>  	return ret;
>  }
> @@ -540,7 +540,7 @@ int drm_mode_dirtyfb_ioctl(struct drm_device *dev,
>  out_err2:
>  	kfree(clips);
>  out_err1:
> -	drm_framebuffer_unreference(fb);
> +	drm_framebuffer_put(fb);
>  
>  	return ret;
>  }
> @@ -580,7 +580,7 @@ void drm_fb_release(struct drm_file *priv)
>  			list_del_init(&fb->filp_head);
>  
>  			/* This drops the fpriv->fbs reference. */
> -			drm_framebuffer_unreference(fb);
> +			drm_framebuffer_put(fb);
>  		}
>  	}
>  
> @@ -661,7 +661,7 @@ EXPORT_SYMBOL(drm_framebuffer_init);
>   *
>   * If successful, this grabs an additional reference to the framebuffer -
>   * callers need to make sure to eventually unreference the returned framebuffer
> - * again, using @drm_framebuffer_unreference.
> + * again, using drm_framebuffer_put().
>   */
>  struct drm_framebuffer *drm_framebuffer_lookup(struct drm_device *dev,
>  					       uint32_t id)
> @@ -687,8 +687,8 @@ EXPORT_SYMBOL(drm_framebuffer_lookup);
>   *
>   * NOTE: This function is deprecated. For driver-private framebuffers it is not
>   * recommended to embed a framebuffer struct info fbdev struct, instead, a
> - * framebuffer pointer is preferred and drm_framebuffer_unreference() should be
> - * called when the framebuffer is to be cleaned up.
> + * framebuffer pointer is preferred and drm_framebuffer_put() should be called
> + * when the framebuffer is to be cleaned up.
>   */
>  void drm_framebuffer_unregister_private(struct drm_framebuffer *fb)
>  {
> @@ -790,7 +790,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
>  		drm_modeset_unlock_all(dev);
>  	}
>  
> -	drm_framebuffer_unreference(fb);
> +	drm_framebuffer_put(fb);
>  }
>  EXPORT_SYMBOL(drm_framebuffer_remove);
>  
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index f42590049a3a..a22e76837065 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -293,7 +293,7 @@ void drm_plane_force_disable(struct drm_plane *plane)
>  		return;
>  	}
>  	/* disconnect the plane from the fb and crtc: */
> -	drm_framebuffer_unreference(plane->old_fb);
> +	drm_framebuffer_put(plane->old_fb);
>  	plane->old_fb = NULL;
>  	plane->fb = NULL;
>  	plane->crtc = NULL;
> @@ -520,9 +520,9 @@ static int __setplane_internal(struct drm_plane *plane,
>  
>  out:
>  	if (fb)
> -		drm_framebuffer_unreference(fb);
> +		drm_framebuffer_put(fb);
>  	if (plane->old_fb)
> -		drm_framebuffer_unreference(plane->old_fb);
> +		drm_framebuffer_put(plane->old_fb);
>  	plane->old_fb = NULL;
>  
>  	return ret;
> @@ -638,7 +638,7 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc,
>  	} else {
>  		fb = crtc->cursor->fb;
>  		if (fb)
> -			drm_framebuffer_reference(fb);
> +			drm_framebuffer_get(fb);
>  	}
>  
>  	if (req->flags & DRM_MODE_CURSOR_MOVE) {
> @@ -902,9 +902,9 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  	if (ret && crtc->funcs->page_flip_target)
>  		drm_crtc_vblank_put(crtc);
>  	if (fb)
> -		drm_framebuffer_unreference(fb);
> +		drm_framebuffer_put(fb);
>  	if (crtc->primary->old_fb)
> -		drm_framebuffer_unreference(crtc->primary->old_fb);
> +		drm_framebuffer_put(crtc->primary->old_fb);
>  	crtc->primary->old_fb = NULL;
>  	drm_modeset_unlock_crtc(crtc);
>  
> diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
> index dd1e3e99dcff..5244f059d23a 100644
> --- a/include/drm/drm_framebuffer.h
> +++ b/include/drm/drm_framebuffer.h
> @@ -101,8 +101,8 @@ struct drm_framebuffer_funcs {
>   * cleanup (like releasing the reference(s) on the backing GEM bo(s))
>   * should be deferred.  In cases like this, the driver would like to
>   * hold a ref to the fb even though it has already been removed from
> - * userspace perspective. See drm_framebuffer_reference() and
> - * drm_framebuffer_unreference().
> + * userspace perspective. See drm_framebuffer_get() and
> + * drm_framebuffer_put().
>   *
>   * The refcount is stored inside the mode object @base.
>   */
> @@ -204,25 +204,50 @@ void drm_framebuffer_cleanup(struct drm_framebuffer *fb);
>  void drm_framebuffer_unregister_private(struct drm_framebuffer *fb);
>  
>  /**
> - * drm_framebuffer_reference - incr the fb refcnt
> - * @fb: framebuffer
> + * drm_framebuffer_get - acquire a framebuffer reference
> + * @fb: DRM framebuffer
> + *
> + * This function increments the framebuffer's reference count.
> + */
> +static inline void drm_framebuffer_get(struct drm_framebuffer *fb)
> +{
> +	drm_mode_object_get(&fb->base);
> +}
> +
> +/**
> + * drm_framebuffer_put - release a framebuffer reference
> + * @fb: DRM framebuffer
> + *
> + * This function decrements the framebuffer's reference count and frees the
> + * framebuffer if the reference count drops to zero.
> + */
> +static inline void drm_framebuffer_put(struct drm_framebuffer *fb)
> +{
> +	drm_mode_object_put(&fb->base);
> +}
> +
> +/**
> + * drm_framebuffer_reference - acquire a framebuffer reference
> + * @fb: DRM framebuffer
>   *
> - * This functions increments the fb's refcount.
> + * This is a compatibility alias for drm_framebuffer_get() and should not be
> + * used by new code.
>   */
>  static inline void drm_framebuffer_reference(struct drm_framebuffer *fb)
>  {
> -	drm_mode_object_reference(&fb->base);
> +	drm_framebuffer_get(fb);
>  }
>  
>  /**
> - * drm_framebuffer_unreference - unref a framebuffer
> - * @fb: framebuffer to unref
> + * drm_framebuffer_unreference - release a framebuffer reference
> + * @fb: DRM framebuffer
>   *
> - * This functions decrements the fb's refcount and frees it if it drops to zero.
> + * This is a compatibility alias for drm_framebuffer_put() and should not be
> + * used by new code.
>   */
>  static inline void drm_framebuffer_unreference(struct drm_framebuffer *fb)
>  {
> -	drm_mode_object_unreference(&fb->base);
> +	drm_framebuffer_put(fb);
>  }
>  
>  /**
> @@ -248,9 +273,9 @@ static inline void drm_framebuffer_assign(struct drm_framebuffer **p,
>  					  struct drm_framebuffer *fb)
>  {
>  	if (fb)
> -		drm_framebuffer_reference(fb);
> +		drm_framebuffer_get(fb);
>  	if (*p)
> -		drm_framebuffer_unreference(*p);
> +		drm_framebuffer_put(*p);
>  	*p = fb;
>  }
>  
> diff --git a/scripts/coccinelle/api/drm-get-put.cocci b/scripts/coccinelle/api/drm-get-put.cocci
> index 8a4c2cb7889e..fd298c24a465 100644
> --- a/scripts/coccinelle/api/drm-get-put.cocci
> +++ b/scripts/coccinelle/api/drm-get-put.cocci
> @@ -26,6 +26,12 @@ expression object;
>  |
>  - drm_connector_unreference(object)
>  + drm_connector_put(object)
> +|
> +- drm_framebuffer_reference(object)
> ++ drm_framebuffer_get(object)
> +|
> +- drm_framebuffer_unreference(object)
> ++ drm_framebuffer_put(object)
>  )
>  
>  @r depends on report@
> @@ -41,6 +47,10 @@ drm_mode_object_reference@p(object)
>  drm_connector_unreference@p(object)
>  |
>  drm_connector_reference@p(object)
> +|
> +drm_framebuffer_unreference@p(object)
> +|
> +drm_framebuffer_reference@p(object)
>  )
>  
>  @script:python depends on report@
> -- 
> 2.11.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel