Re: [PATCH 1/3] drm/plane: Make framebuffer refcounting the responsibility of setplane_internal callers

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Dec 20, 2017 at 10:35:43AM +0100, Maarten Lankhorst wrote:
> lock_all_ctx in setplane_internal may return -EINTR, and
> __setplane_internal could return -EDEADLK. Making more
> special cases for fb would make the code even harder to
> read, so the easiest solution is not taking over the fb
> refcount, and making callers responsible for dropping
> the ref.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102707
> Fixes: 13736ba3b38b ("drm/legacy: Convert setplane ioctl locking to interruptible.")
> Testcase: kms_atomic_interruptible
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

I thought I reviewed this one before already, but probably just a paste
somewhere on irc.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_plane.c | 42 ++++++++++++++++++++----------------------
>  1 file changed, 20 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 37a93cdffb4a..2c90519576a3 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -558,11 +558,10 @@ int drm_plane_check_pixel_format(const struct drm_plane *plane, u32 format)
>  }
>  
>  /*
> - * setplane_internal - setplane handler for internal callers
> + * __setplane_internal - setplane handler for internal callers
>   *
> - * Note that we assume an extra reference has already been taken on fb.  If the
> - * update fails, this reference will be dropped before return; if it succeeds,
> - * the previous framebuffer (if any) will be unreferenced instead.
> + * This function will take a reference on the new fb for the plane
> + * on success.
>   *
>   * src_{x,y,w,h} are provided in 16.16 fixed point format
>   */
> @@ -630,14 +629,12 @@ static int __setplane_internal(struct drm_plane *plane,
>  	if (!ret) {
>  		plane->crtc = crtc;
>  		plane->fb = fb;
> -		fb = NULL;
> +		drm_framebuffer_get(plane->fb);
>  	} else {
>  		plane->old_fb = NULL;
>  	}
>  
>  out:
> -	if (fb)
> -		drm_framebuffer_put(fb);
>  	if (plane->old_fb)
>  		drm_framebuffer_put(plane->old_fb);
>  	plane->old_fb = NULL;
> @@ -685,6 +682,7 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
>  	struct drm_plane *plane;
>  	struct drm_crtc *crtc = NULL;
>  	struct drm_framebuffer *fb = NULL;
> +	int ret;
>  
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EINVAL;
> @@ -717,15 +715,16 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
>  		}
>  	}
>  
> -	/*
> -	 * setplane_internal will take care of deref'ing either the old or new
> -	 * framebuffer depending on success.
> -	 */
> -	return setplane_internal(plane, crtc, fb,
> -				 plane_req->crtc_x, plane_req->crtc_y,
> -				 plane_req->crtc_w, plane_req->crtc_h,
> -				 plane_req->src_x, plane_req->src_y,
> -				 plane_req->src_w, plane_req->src_h);
> +	ret = setplane_internal(plane, crtc, fb,
> +				plane_req->crtc_x, plane_req->crtc_y,
> +				plane_req->crtc_w, plane_req->crtc_h,
> +				plane_req->src_x, plane_req->src_y,
> +				plane_req->src_w, plane_req->src_h);
> +
> +	if (fb)
> +		drm_framebuffer_put(fb);
> +
> +	return ret;
>  }
>  
>  static int drm_mode_cursor_universal(struct drm_crtc *crtc,
> @@ -788,13 +787,12 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc,
>  		src_h = fb->height << 16;
>  	}
>  
> -	/*
> -	 * setplane_internal will take care of deref'ing either the old or new
> -	 * framebuffer depending on success.
> -	 */
>  	ret = __setplane_internal(crtc->cursor, crtc, fb,
> -				crtc_x, crtc_y, crtc_w, crtc_h,
> -				0, 0, src_w, src_h, ctx);
> +				  crtc_x, crtc_y, crtc_w, crtc_h,
> +				  0, 0, src_w, src_h, ctx);
> +
> +	if (fb)
> +		drm_framebuffer_put(fb);
>  
>  	/* Update successful; save new cursor position, if necessary */
>  	if (ret == 0 && req->flags & DRM_MODE_CURSOR_MOVE) {
> -- 
> 2.15.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx