On Fri, Sep 12, 2014 at 05:07:32PM +0200, Daniel Vetter wrote:
> 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@xxxxxxxxx>
> Cc: Pallavi G<pallavi.g@xxxxxxxxx>
> Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
>
> --
> Totally untested, but should aim in the right direction. Roughly.
>
> Cheers, Daniel
Looks good to me. I also ran this through the igt tests kms_plane,
kms_universal_plane, and kms_cursor_crc to make sure that we didn't
accidentally break something and they all ran fine.
So
Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
and
Tested-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> ---
> 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
>
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
