[PATCH 09/15] drm/modes: move reference taking into object lookup.

[Daniel Vetter]

On Fri, Apr 15, 2016 at 03:10:40PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied at redhat.com>
> 
> When we lookup an ref counted object we now take a proper reference
> using kref_get_unless_zero.
> 
> Framebuffer lookup no longer needs do this itself.
> 
> Convert rmfb to using framebuffer lookup and deal with the fact
> it now gets an extra reference that we have to cleanup. This should
> mean we can avoid holding fb_lock across rmfb. (if I'm wrong let me
> know).
> 
> We also now only hold the fbs_lock around the list manipulation.
> 
> Signed-off-by: Dave Airlie <airlied at redhat.com>

Needs the same comment as patch 7 added to the commit message:

"Previously fb refcounting, and especially the weak reference
(kref_get_unless_zero) used in fb lookups have been protected by fb_lock.
But with the refactoring to share refcounting in the drm_mode_object base
class that switched to being protected by idr_mutex, which means fb_lock
critical sections can be reduced."

With that: Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>

> ---
>  drivers/gpu/drm/drm_crtc.c | 37 ++++++++++++++++++++-----------------
>  1 file changed, 20 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 21cb998..e47c4a2 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -364,6 +364,11 @@ static struct drm_mode_object *_object_find(struct drm_device *dev,
>  	if (obj &&
>  	    obj->type == DRM_MODE_OBJECT_BLOB)
>  		obj = NULL;
> +
> +	if (obj && obj->free_cb) {
> +		if (!kref_get_unless_zero(&obj->refcount))
> +			obj = NULL;
> +	}
>  	mutex_unlock(&dev->mode_config.idr_mutex);
>  
>  	return obj;
> @@ -495,11 +500,8 @@ struct drm_framebuffer *drm_framebuffer_lookup(struct drm_device *dev,
>  
>  	mutex_lock(&dev->mode_config.fb_lock);
>  	obj = _object_find(dev, id, DRM_MODE_OBJECT_FB);
> -	if (obj) {
> +	if (obj)
>  		fb = obj_to_fb(obj);
> -		if (!kref_get_unless_zero(&fb->base.refcount))
> -			fb = NULL;
> -	}
>  	mutex_unlock(&dev->mode_config.fb_lock);
>  
>  	return fb;
> @@ -3434,37 +3436,38 @@ int drm_mode_rmfb(struct drm_device *dev,
>  {
>  	struct drm_framebuffer *fb = NULL;
>  	struct drm_framebuffer *fbl = NULL;
> -	struct drm_mode_object *obj;
>  	uint32_t *id = data;
>  	int found = 0;
>  
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EINVAL;
>  
> +	fb = drm_framebuffer_lookup(dev, *id);
> +	if (!fb)
> +		return -ENOENT;
> +
>  	mutex_lock(&file_priv->fbs_lock);
> -	mutex_lock(&dev->mode_config.fb_lock);
> -	obj = _object_find(dev, *id, DRM_MODE_OBJECT_FB);
> -	if (!obj)
> -		goto fail_lookup;
> -	fb = obj_to_fb(obj);
>  	list_for_each_entry(fbl, &file_priv->fbs, filp_head)
>  		if (fb == fbl)
>  			found = 1;
> -	if (!found)
> -		goto fail_lookup;
> +	if (!found) {
> +		mutex_unlock(&file_priv->fbs_lock);
> +		goto fail_unref;
> +	}
>  
>  	list_del_init(&fb->filp_head);
> -	mutex_unlock(&dev->mode_config.fb_lock);
>  	mutex_unlock(&file_priv->fbs_lock);
>  
> +	/* we now own the reference that was stored in the fbs list */
>  	drm_framebuffer_unreference(fb);
>  
> -	return 0;
> +	/* drop the reference we picked up in framebuffer lookup */
> +	drm_framebuffer_unreference(fb);
>  
> -fail_lookup:
> -	mutex_unlock(&dev->mode_config.fb_lock);
> -	mutex_unlock(&file_priv->fbs_lock);
> +	return 0;
>  
> +fail_unref:
> +	drm_framebuffer_unreference(fb);
>  	return -ENOENT;
>  }
>  
> -- 
> 2.5.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch