Commit f2b50c11 authored by Daniel Vetter's avatar Daniel Vetter

drm: Fixup locking for universal cursor planes

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@gmail.com>
Cc: Pallavi G<pallavi.g@intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: default avatarMatt Roper <matthew.d.roper@intel.com>
Tested-by: default avatarMatt Roper <matthew.d.roper@intel.com>
Reviewed-by: default avatarDavid Herrmann <dh.herrmann@gmail.com>
Signed-off-by: default avatarDaniel Vetter <daniel.vetter@intel.com>
parent da8f4396
...@@ -2263,7 +2263,7 @@ int drm_mode_getplane(struct drm_device *dev, void *data, ...@@ -2263,7 +2263,7 @@ int drm_mode_getplane(struct drm_device *dev, void *data,
* *
* src_{x,y,w,h} are provided in 16.16 fixed point format * src_{x,y,w,h} are provided in 16.16 fixed point format
*/ */
static int setplane_internal(struct drm_plane *plane, static int __setplane_internal(struct drm_plane *plane,
struct drm_crtc *crtc, struct drm_crtc *crtc,
struct drm_framebuffer *fb, struct drm_framebuffer *fb,
int32_t crtc_x, int32_t crtc_y, int32_t crtc_x, int32_t crtc_y,
...@@ -2272,12 +2272,10 @@ static int setplane_internal(struct drm_plane *plane, ...@@ -2272,12 +2272,10 @@ static int setplane_internal(struct drm_plane *plane,
uint32_t src_x, uint32_t src_y, uint32_t src_x, uint32_t src_y,
uint32_t src_w, uint32_t src_h) uint32_t src_w, uint32_t src_h)
{ {
struct drm_device *dev = plane->dev;
int ret = 0; int ret = 0;
unsigned int fb_width, fb_height; unsigned int fb_width, fb_height;
int i; int i;
drm_modeset_lock_all(dev);
/* No fb means shut it down */ /* No fb means shut it down */
if (!fb) { if (!fb) {
plane->old_fb = plane->fb; plane->old_fb = plane->fb;
...@@ -2345,10 +2343,28 @@ static int setplane_internal(struct drm_plane *plane, ...@@ -2345,10 +2343,28 @@ static int setplane_internal(struct drm_plane *plane,
if (plane->old_fb) if (plane->old_fb)
drm_framebuffer_unreference(plane->old_fb); drm_framebuffer_unreference(plane->old_fb);
plane->old_fb = NULL; plane->old_fb = NULL;
drm_modeset_unlock_all(dev);
return ret; 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;
} }
/** /**
...@@ -2714,6 +2730,7 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, ...@@ -2714,6 +2730,7 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc,
int ret = 0; int ret = 0;
BUG_ON(!crtc->cursor); 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 * Obtain fb we'll be using (either new or existing) and take an extra
...@@ -2733,11 +2750,9 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, ...@@ -2733,11 +2750,9 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc,
fb = NULL; fb = NULL;
} }
} else { } else {
mutex_lock(&dev->mode_config.mutex);
fb = crtc->cursor->fb; fb = crtc->cursor->fb;
if (fb) if (fb)
drm_framebuffer_reference(fb); drm_framebuffer_reference(fb);
mutex_unlock(&dev->mode_config.mutex);
} }
if (req->flags & DRM_MODE_CURSOR_MOVE) { if (req->flags & DRM_MODE_CURSOR_MOVE) {
...@@ -2759,7 +2774,7 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, ...@@ -2759,7 +2774,7 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc,
* setplane_internal will take care of deref'ing either the old or new * setplane_internal will take care of deref'ing either the old or new
* framebuffer depending on success. * 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, crtc_x, crtc_y, crtc_w, crtc_h,
0, 0, src_w, src_h); 0, 0, src_w, src_h);
...@@ -2795,10 +2810,12 @@ static int drm_mode_cursor_common(struct drm_device *dev, ...@@ -2795,10 +2810,12 @@ static int drm_mode_cursor_common(struct drm_device *dev,
* If this crtc has a universal cursor plane, call that plane's update * If this crtc has a universal cursor plane, call that plane's update
* handler rather than using legacy cursor handlers. * handler rather than using legacy cursor handlers.
*/ */
if (crtc->cursor)
return drm_mode_cursor_universal(crtc, req, file_priv);
drm_modeset_lock_crtc(crtc); 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 (req->flags & DRM_MODE_CURSOR_BO) {
if (!crtc->funcs->cursor_set && !crtc->funcs->cursor_set2) { if (!crtc->funcs->cursor_set && !crtc->funcs->cursor_set2) {
ret = -ENXIO; ret = -ENXIO;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment