drm/plane: Make framebuffer refcounting the responsibility of setplane_internal callers
diff mbox

Message ID 20171208095436.63626-1-maarten.lankhorst@linux.intel.com
State New
Headers show

Commit Message

Maarten Lankhorst Dec. 8, 2017, 9:54 a.m. UTC
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>
---
 drivers/gpu/drm/drm_plane.c | 42 ++++++++++++++++++++----------------------
 1 file changed, 20 insertions(+), 22 deletions(-)

Comments

Daniel Vetter Dec. 8, 2017, 10:09 a.m. UTC | #1
On Fri, Dec 08, 2017 at 10:54:36AM +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>

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

Please push to drm-misc-fixes as soon as CI approves it fully (which is
probably next Monday, no rush).
-Daniel

> ---
>  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
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patch
diff mbox

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) {