[Intel-gfx] [PATCH] drm: Fixup locking for universal cursor planes

[Daniel Vetter]

Bunch of things amiss:
- Updating crtc->cursor_x/y was done without any locking. Spotted by
  David Herrmann.
- Dereferencing crtc->cursor->fb was using the wrong lock, should take
  the crtc lock.
- Grabbing _all_ modeset locks torpedoes the reason why we added
  fine-grained locks originally: Cursor updates shouldn't stall on
  background stuff like probing outputs.

Best is to just grab the crtc lock around everything and drop all the
other locking. The only issue is that we can't switch planes between
crtcs with that, so make sure that never happens when someone uses
universal plane helpers. This shouldn't be a possible regression ever
since legacy ioctls also only grabbed the crtc lock, so switching
crtcs was never possible for the underlying plane object. And i915
(the only user of universal cursors thus far) has fixed cursor->crtc
links.

Cc: David Herrmann <dh.herrmann at gmail.com>
Cc: Pallavi G<pallavi.g at intel.com>
Cc: Matt Roper <matthew.d.roper at intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>

--
Totally untested, but should aim in the right direction. Roughly.

Cheers, Daniel
---
 drivers/gpu/drm/drm_crtc.c | 51 ++++++++++++++++++++++++++++++----------------
 1 file changed, 34 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index b1271a8d8ce7..8c1870114a07 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2262,21 +2262,19 @@ out:
  *
  * src_{x,y,w,h} are provided in 16.16 fixed point format
  */
-static int setplane_internal(struct drm_plane *plane,
-			     struct drm_crtc *crtc,
-			     struct drm_framebuffer *fb,
-			     int32_t crtc_x, int32_t crtc_y,
-			     uint32_t crtc_w, uint32_t crtc_h,
-			     /* src_{x,y,w,h} values are 16.16 fixed point */
-			     uint32_t src_x, uint32_t src_y,
-			     uint32_t src_w, uint32_t src_h)
+static int __setplane_internal(struct drm_plane *plane,
+			       struct drm_crtc *crtc,
+			       struct drm_framebuffer *fb,
+			       int32_t crtc_x, int32_t crtc_y,
+			       uint32_t crtc_w, uint32_t crtc_h,
+			       /* src_{x,y,w,h} values are 16.16 fixed point */
+			       uint32_t src_x, uint32_t src_y,
+			       uint32_t src_w, uint32_t src_h)
 {
-	struct drm_device *dev = plane->dev;
 	int ret = 0;
 	unsigned int fb_width, fb_height;
 	int i;
 
-	drm_modeset_lock_all(dev);
 	/* No fb means shut it down */
 	if (!fb) {
 		plane->old_fb = plane->fb;
@@ -2344,10 +2342,28 @@ out:
 	if (plane->old_fb)
 		drm_framebuffer_unreference(plane->old_fb);
 	plane->old_fb = NULL;
-	drm_modeset_unlock_all(dev);
 
 	return ret;
+}
 
+static int setplane_internal(struct drm_plane *plane,
+			     struct drm_crtc *crtc,
+			     struct drm_framebuffer *fb,
+			     int32_t crtc_x, int32_t crtc_y,
+			     uint32_t crtc_w, uint32_t crtc_h,
+			     /* src_{x,y,w,h} values are 16.16 fixed point */
+			     uint32_t src_x, uint32_t src_y,
+			     uint32_t src_w, uint32_t src_h)
+{
+	int ret;
+
+	drm_modeset_lock_all(plane->dev);
+	ret = __setplane_internal(plane, crtc, fb,
+				  crtc_x, crtc_y, crtc_w, crtc_h,
+				  src_x, src_y, src_w, src_h);
+	drm_modeset_unlock_all(plane->dev);
+
+	return ret;
 }
 
 /**
@@ -2713,6 +2729,7 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc,
 	int ret = 0;
 
 	BUG_ON(!crtc->cursor);
+	WARN_ON(crtc->cursor->crtc != crtc && crtc->cursor->crtc != NULL);
 
 	/*
 	 * Obtain fb we'll be using (either new or existing) and take an extra
@@ -2732,11 +2749,9 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc,
 			fb = NULL;
 		}
 	} else {
-		mutex_lock(&dev->mode_config.mutex);
 		fb = crtc->cursor->fb;
 		if (fb)
 			drm_framebuffer_reference(fb);
-		mutex_unlock(&dev->mode_config.mutex);
 	}
 
 	if (req->flags & DRM_MODE_CURSOR_MOVE) {
@@ -2758,7 +2773,7 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc,
 	 * setplane_internal will take care of deref'ing either the old or new
 	 * framebuffer depending on success.
 	 */
-	ret = setplane_internal(crtc->cursor, crtc, fb,
+	ret = __setplane_internal(crtc->cursor, crtc, fb,
 				crtc_x, crtc_y, crtc_w, crtc_h,
 				0, 0, src_w, src_h);
 
@@ -2794,10 +2809,12 @@ static int drm_mode_cursor_common(struct drm_device *dev,
 	 * If this crtc has a universal cursor plane, call that plane's update
 	 * handler rather than using legacy cursor handlers.
 	 */
-	if (crtc->cursor)
-		return drm_mode_cursor_universal(crtc, req, file_priv);
-
 	drm_modeset_lock_crtc(crtc);
+	if (crtc->cursor) {
+		ret = drm_mode_cursor_universal(crtc, req, file_priv);
+		goto out;
+	}
+
 	if (req->flags & DRM_MODE_CURSOR_BO) {
 		if (!crtc->funcs->cursor_set && !crtc->funcs->cursor_set2) {
 			ret = -ENXIO;
-- 
2.0.1