drm: only take the crtc lock for ->cursor_move

->cursor_move uses mostly the same facilities in drivers as
->cursor_set, so pretty much nothing to fix up:

- ast/gma500/i915: They all use per-crtc registers to update the
  cursor position. ast again touches the global cursor cache, but
  that's ok since there's only one crtc.

- nouveau: nv50+ is again special, updates happen through the per-crtc
  channel (without pushbufs), so it's not protected by the new evo
  lock introduced earlier. But since this channel is per-crtc, we
  should be fine anyway.

- radeon: A bit a mess: avivo asics need a workaround when both output
  pipes are enabled, which means it'll access the crtc list. Just
  reading that flag is ok though as long as radeon _always_ grabs all
  locks when changing the crtc configuration. Which means with the
  current scheme it cannot do an optimized modeset which only locks
  the relevant crtcs. This can be fixed though by introducing a bit of
  global state with separate locks and ensure in the modeset code that
  the cursor will be updated appropriately when enabling the 2nd pipe
  (on affected asics).

- vmwgfx: I still don't understand what it's doing exactly, so apply
  the same trick for now.

v2: Fixup unlocking for the error cases, spotted by Richard Wilbur.

v3: Another error-case fixup.

Reviewed-by: Rob Clark <rob@ti.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 58fa69e5ff4cffad89b782670f015673aeb963aa..4af6a3d5c9a1b4d55d0bfa2e1cb28baac197eaa2 100644 (file)
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2039,34 +2039,32 @@ int drm_mode_cursor_ioctl(struct drm_device *dev,
        obj = drm_mode_object_find(dev, req->crtc_id, DRM_MODE_OBJECT_CRTC);
        if (!obj) {
                DRM_DEBUG_KMS("Unknown CRTC ID %d\n", req->crtc_id);
-               ret = -EINVAL;
-               goto out;
+               return -EINVAL;
        }
        crtc = obj_to_crtc(obj);
 
+       mutex_lock(&crtc->mutex);
        if (req->flags & DRM_MODE_CURSOR_BO) {
                if (!crtc->funcs->cursor_set) {
                        ret = -ENXIO;
                        goto out;
                }
                /* Turns off the cursor if handle is 0 */
-               mutex_lock(&crtc->mutex);
                ret = crtc->funcs->cursor_set(crtc, file_priv, req->handle,
                                              req->width, req->height);
-               mutex_unlock(&crtc->mutex);
        }
 
        if (req->flags & DRM_MODE_CURSOR_MOVE) {
                if (crtc->funcs->cursor_move) {
-                       drm_modeset_lock_all(dev);
                        ret = crtc->funcs->cursor_move(crtc, req->x, req->y);
-                       drm_modeset_unlock_all(dev);
                } else {
                        ret = -EFAULT;
                        goto out;
                }
        }
 out:
+       mutex_unlock(&crtc->mutex);
+
        return ret;
 }
 

diff --git a/drivers/gpu/drm/radeon/radeon_cursor.c b/drivers/gpu/drm/radeon/radeon_cursor.c
index ad6df625e8b8a1cf869ac06bb898503022406a6b..c1680e6d76ad932211975e0cdc2d76da031ae901 100644 (file)
--- a/drivers/gpu/drm/radeon/radeon_cursor.c
+++ b/drivers/gpu/drm/radeon/radeon_cursor.c
@@ -245,8 +245,14 @@ int radeon_crtc_cursor_move(struct drm_crtc *crtc,
                int i = 0;
                struct drm_crtc *crtc_p;
 
-               /* avivo cursor image can't end on 128 pixel boundary or
+               /*
+                * avivo cursor image can't end on 128 pixel boundary or
                 * go past the end of the frame if both crtcs are enabled
+                *
+                * NOTE: It is safe to access crtc->enabled of other crtcs
+                * without holding either the mode_config lock or the other
+                * crtc's lock as long as write access to this flag _always_
+                * grabs all locks.
                 */
                list_for_each_entry(crtc_p, &crtc->dev->mode_config.crtc_list, head) {
                        if (crtc_p->enabled)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 8d82e631c305d732fafb7635b6f9bba871884883..3e3c7ab33ca252008713e1050b297592d784e354 100644 (file)
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -264,10 +264,23 @@ int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
        du->cursor_x = x + crtc->x;
        du->cursor_y = y + crtc->y;
 
+       /*
+        * FIXME: Unclear whether there's any global state touched by the
+        * cursor_set function, especially vmw_cursor_update_position looks
+        * suspicious. For now take the easy route and reacquire all locks. We
+        * can do this since the caller in the drm core doesn't check anything
+        * which is protected by any looks.
+        */
+       mutex_unlock(&crtc->mutex);
+       drm_modeset_lock_all(dev_priv->dev);
+
        vmw_cursor_update_position(dev_priv, shown,
                                   du->cursor_x + du->hotspot_x,
                                   du->cursor_y + du->hotspot_y);
 
+       drm_modeset_unlock_all(dev_priv->dev);
+       mutex_lock(&crtc->mutex);
+
        return 0;
 }