Skip to contentSkip to conversation

drm/vmwgfx: fix lock breakage

PATCH In review Rob Clark 4 participants 5 messages Oct 30, 2014 – Oct 31, 2014

Subsystems (3)
DRM drivers DRM misc DRM driver for VMware virtual GPU
lore
Share & tools
lore

Rob Clark

author
#0 lore raw

PATCH 1/1 drm/vmwgfx: fix lock breakage

After:

commit d059f652e73c35678d28d4cd09ab2cec89696af9

Author:     Daniel Vetter <[email protected]>
AuthorDate: Fri Jul 25 18:07:40 2014 +0200

drm: Handle legacy per-crtc locking with full acquire ctx

drm_mode_cursor_common() was switched to use drm_modeset_(un)lock_crtc() which uses full aquire ctx. So dropping/reaquiring the lock via drm_modeset_(un)lock() directly isn't the right thing to do, as lockdep kindly points out.

The 'FIXME's about sorting out whether vmwgfx *really* needs to lock-all for cursor updates still apply.

Signed-off-by Rob Clark <[email protected]>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +4 -4
index d2bc2b0..8fc1e38 100644
@@ -187,7 +187,7 @@ int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
 	 * can do this since the caller in the drm core doesn't check anything
 	 * which is protected by any looks.
 	 */
-	drm_modeset_unlock(&crtc->mutex);
+	drm_modeset_unlock_crtc(crtc);
 	drm_modeset_lock_all(dev_priv->dev);
 
 	/* A lot of the code assumes this */

@@ -252,7 +252,7 @@ int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
 	ret = 0;
 out:
 	drm_modeset_unlock_all(dev_priv->dev);
-	drm_modeset_lock(&crtc->mutex, NULL);
+	drm_modeset_lock_crtc(crtc);
 
 	return ret;
 }

@@ -273,7 +273,7 @@ int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
 	 * can do this since the caller in the drm core doesn't check anything
 	 * which is protected by any looks.
 	 */
-	drm_modeset_unlock(&crtc->mutex);
+	drm_modeset_unlock_crtc(crtc);
 	drm_modeset_lock_all(dev_priv->dev);
 
 	vmw_cursor_update_position(dev_priv, shown,

@@ -281,7 +281,7 @@ int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
 				   du->cursor_y + du->hotspot_y);
 
 	drm_modeset_unlock_all(dev_priv->dev);
-	drm_modeset_lock(&crtc->mutex, NULL);
+	drm_modeset_lock_crtc(crtc);
 
 	return 0;
 }
Signature · 6 lines
1.9.3

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
author
#1 lore raw
Quoted 8 lines · Rob Clark

After:

commit d059f652e73c35678d28d4cd09ab2cec89696af9

Author:     Daniel Vetter <[email protected]>
AuthorDate: Fri Jul 25 18:07:40 2014 +0200

drm: Handle legacy per-crtc locking with full acquire ctx

Quoted 8 lines

drm_mode_cursor_common() was switched to use drm_modeset_(un)lock_crtc() which uses full aquire ctx. So dropping/reaquiring the lock via drm_modeset_(un)lock() directly isn't the right thing to do, as lockdep kindly points out.

The 'FIXME's about sorting out whether vmwgfx *really* needs to lock-all for cursor updates still apply.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1155825

Quoted 46 lines
Signed-off-by Rob Clark <[email protected]>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +4 -5
index d2bc2b0..8fc1e38 100644
@@ -187,7 +187,7 @@ int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
         * can do this since the caller in the drm core doesn't check anything
         * which is protected by any looks.
         */
-       drm_modeset_unlock(&crtc->mutex);
+       drm_modeset_unlock_crtc(crtc);
        drm_modeset_lock_all(dev_priv->dev);
 
        /* A lot of the code assumes this */

@@ -252,7 +252,7 @@ int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
        ret = 0;
 out:
        drm_modeset_unlock_all(dev_priv->dev);
-       drm_modeset_lock(&crtc->mutex, NULL);
+       drm_modeset_lock_crtc(crtc);
 
        return ret;
 }

@@ -273,7 +273,7 @@ int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
         * can do this since the caller in the drm core doesn't check anything
         * which is protected by any looks.
         */
-       drm_modeset_unlock(&crtc->mutex);
+       drm_modeset_unlock_crtc(crtc);
        drm_modeset_lock_all(dev_priv->dev);
 
        vmw_cursor_update_position(dev_priv, shown,

@@ -281,7 +281,7 @@ int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
                                   du->cursor_y + du->hotspot_y);
 
        drm_modeset_unlock_all(dev_priv->dev);
-       drm_modeset_lock(&crtc->mutex, NULL);
+       drm_modeset_lock_crtc(crtc);
 
        return 0;
 }
--
Quoted 2 lines

1.9.3

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Reviewed-by
#2 lore raw
Quoted 14 lines · Rob Clark
Quoted 13 lines · Rob Clark

After:

commit d059f652e73c35678d28d4cd09ab2cec89696af9

Author:     Daniel Vetter <[email protected]>
AuthorDate: Fri Jul 25 18:07:40 2014 +0200

drm: Handle legacy per-crtc locking with full acquire ctx

drm_mode_cursor_common() was switched to use drm_modeset_(un)lock_crtc() which uses full aquire ctx. So dropping/reaquiring the lock via drm_modeset_(un)lock() directly isn't the right thing to do, as lockdep kindly points out.

Quoted 10 lines
Quoted 3 lines

The 'FIXME's about sorting out whether vmwgfx *really* needs to lock-all for cursor updates still apply.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1155825

Quoted 4 lines
Signed-off-by Rob Clark <[email protected]>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Reviewed-by Jakob Bornecrantz <[email protected]>
Quoted 39 lines
Quoted 39 lines
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +4 -4
index d2bc2b0..8fc1e38 100644
@@ -187,7 +187,7 @@ int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
         * can do this since the caller in the drm core doesn't check anything
         * which is protected by any looks.
         */
-       drm_modeset_unlock(&crtc->mutex);
+       drm_modeset_unlock_crtc(crtc);
        drm_modeset_lock_all(dev_priv->dev);
 
        /* A lot of the code assumes this */

@@ -252,7 +252,7 @@ int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
        ret = 0;
 out:
        drm_modeset_unlock_all(dev_priv->dev);
-       drm_modeset_lock(&crtc->mutex, NULL);
+       drm_modeset_lock_crtc(crtc);
 
        return ret;
 }

@@ -273,7 +273,7 @@ int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
         * can do this since the caller in the drm core doesn't check anything
         * which is protected by any looks.
         */
-       drm_modeset_unlock(&crtc->mutex);
+       drm_modeset_unlock_crtc(crtc);
        drm_modeset_lock_all(dev_priv->dev);
 
        vmw_cursor_update_position(dev_priv, shown,

@@ -281,7 +281,7 @@ int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
                                   du->cursor_y + du->hotspot_y);
 
        drm_modeset_unlock_all(dev_priv->dev);
-       drm_modeset_lock(&crtc->mutex, NULL);
+       drm_modeset_lock_crtc(crtc);
Quoted 9 lines
Quoted 5 lines
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +0 -1
        return 0;
 }
--

1.9.3

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Tested-by on v1 1/1
#3 lore raw
In reply to Rob Clark
Quoted 13 lines · Rob Clark

After:

commit d059f652e73c35678d28d4cd09ab2cec89696af9

Author:     Daniel Vetter <[email protected]>
AuthorDate: Fri Jul 25 18:07:40 2014 +0200

drm: Handle legacy per-crtc locking with full acquire ctx

drm_mode_cursor_common() was switched to use drm_modeset_(un)lock_crtc() which uses full aquire ctx. So dropping/reaquiring the lock via drm_modeset_(un)lock() directly isn't the right thing to do, as lockdep kindly points out.

Quoted 4 lines

The 'FIXME's about sorting out whether vmwgfx *really* needs to lock-all for cursor updates still apply.

Signed-off-by Rob Clark <[email protected]>
Tested-by Thomas Hellstrom <[email protected]>

Thanks, Rob.

/Thomas

Quoted 36 lines
---
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +3 -3
index d2bc2b0..8fc1e38 100644
@@ -187,7 +187,7 @@ int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
 	 * can do this since the caller in the drm core doesn't check anything
 	 * which is protected by any looks.
 	 */
-	drm_modeset_unlock(&crtc->mutex);
+	drm_modeset_unlock_crtc(crtc);
 	drm_modeset_lock_all(dev_priv->dev);
 
 	/* A lot of the code assumes this */

@@ -252,7 +252,7 @@ int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
 	ret = 0;
 out:
 	drm_modeset_unlock_all(dev_priv->dev);
-	drm_modeset_lock(&crtc->mutex, NULL);
+	drm_modeset_lock_crtc(crtc);
 
 	return ret;
 }

@@ -273,7 +273,7 @@ int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
 	 * can do this since the caller in the drm core doesn't check anything
 	 * which is protected by any looks.
 	 */
-	drm_modeset_unlock(&crtc->mutex);
+	drm_modeset_unlock_crtc(crtc);
 	drm_modeset_lock_all(dev_priv->dev);
 
 	vmw_cursor_update_position(dev_priv, shown,

drivers/gpu/drm/vmwgfx/vmwgfx_kms.c int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y) · line 287

Quoted 8 lines
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +1 -1
@@ -281,7 +281,7 @@ int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
 				   du->cursor_y + du->hotspot_y);
 
 	drm_modeset_unlock_all(dev_priv->dev);
-	drm_modeset_lock(&crtc->mutex, NULL);
+	drm_modeset_lock_crtc(crtc);
 
 	return 0;
 }

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Reviewed-by on v1 1/1
#4 lore raw
In reply to Rob Clark
Quoted 26 lines · Rob Clark

After:

commit d059f652e73c35678d28d4cd09ab2cec89696af9

Author:     Daniel Vetter <[email protected]>
AuthorDate: Fri Jul 25 18:07:40 2014 +0200

drm: Handle legacy per-crtc locking with full acquire ctx

drm_mode_cursor_common() was switched to use drm_modeset_(un)lock_crtc() which uses full aquire ctx. So dropping/reaquiring the lock via drm_modeset_(un)lock() directly isn't the right thing to do, as lockdep kindly points out.

The 'FIXME's about sorting out whether vmwgfx *really* needs to lock-all for cursor updates still apply.

Signed-off-by Rob Clark <[email protected]>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +0 -0
index d2bc2b0..8fc1e38 100644

drivers/gpu/drm/vmwgfx/vmwgfx_kms.c int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv, · line 191

Quoted 6 lines
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +1 -1
@@ -187,7 +187,7 @@ int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
 	 * can do this since the caller in the drm core doesn't check anything
 	 * which is protected by any looks.
 	 */
-	drm_modeset_unlock(&crtc->mutex);
+	drm_modeset_unlock_crtc(crtc);
 	drm_modeset_lock_all(dev_priv->dev);

Oh right this is simpler, but it also grabs a new acquire context. So not too terribly fair really, but otoh neither was the old one.

Reviewed-by Daniel Vetter <[email protected]>
Quoted 32 lines
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +3 -3
 	/* A lot of the code assumes this */

@@ -252,7 +252,7 @@ int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
 	ret = 0;
 out:
 	drm_modeset_unlock_all(dev_priv->dev);
-	drm_modeset_lock(&crtc->mutex, NULL);
+	drm_modeset_lock_crtc(crtc);
 
 	return ret;
 }

@@ -273,7 +273,7 @@ int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
 	 * can do this since the caller in the drm core doesn't check anything
 	 * which is protected by any looks.
 	 */
-	drm_modeset_unlock(&crtc->mutex);
+	drm_modeset_unlock_crtc(crtc);
 	drm_modeset_lock_all(dev_priv->dev);
 
 	vmw_cursor_update_position(dev_priv, shown,

@@ -281,7 +281,7 @@ int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
 				   du->cursor_y + du->hotspot_y);
 
 	drm_modeset_unlock_all(dev_priv->dev);
-	drm_modeset_lock(&crtc->mutex, NULL);
+	drm_modeset_lock_crtc(crtc);
 
 	return 0;
 }
Signature · 1 line
1.9.3
Signature · 7 lines
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel