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

[David Herrmann]

Hi

On Fri, Sep 12, 2014 at 5:07 PM, Daniel Vetter <daniel.vetter at ffwll.ch> 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 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);

Might be confusing to call into drivers with different locks held
depending on the plane. But on the other hand, I think drivers should
just assume only the involved CRTCs are locked, so it sounds fine.
With atomic we get the same behavior, presumably.

Reviewed-by: David Herrmann <dh.herrmann at gmail.com>

Thanks
David

> +
> +       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
>