[Intel-gfx] [PATCH] drm: Drop grab fpriv->fbs_lock in drm_fb_release

[Paulo Zanoni]

2014-09-24 16:55 GMT-03:00 Daniel Vetter <daniel.vetter at ffwll.ch>:
> Paulo Zanoni reported a lockdep splat with a locking inversion between
> fpriv->fbs_lock and the modeset locks. This issue was introduced in
>
> commit f2b50c1161590c3bcdbf3455fe4c575f1c1bd293
> Author: Daniel Vetter <daniel.vetter at ffwll.ch>
> Date:   Fri Sep 12 17:07:32 2014 +0200
>
>     drm: Fixup locking for universal cursor planes
>
> This here is actually one of the rare cases where lockdep hits a false
> positive: The deadlock only happens in drm_fb_release, which cleans up
> the file private structure when all the references are gone. So the
> locking is the very last one and no one else can deadlock. It also
> doesn't protect anything at all, since all ioctls are guaranteed to
> have returned at this point - otherwise they'd still hold a reference
> on the file.
>
> So let's just drop it and replace it with a big comment.
>
> Cc: David Herrmann <dh.herrmann at gmail.com>
> Cc: Matt Roper <matthew.d.roper at intel.com>
> Cc: Paulo Zanoni <przanoni at gmail.com>
> Reported-by: Paulo Zanoni <przanoni at gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>

Apparently, it fixes the problem I was seeing while running
igt/pm_rpm. It was not 100% reproducible, but it seems to be gone.

Smoke-tested-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
Testcase: igt/pm_rpm

> ---
>  drivers/gpu/drm/drm_crtc.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index b7021069b078..e79c8d3700d8 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -3400,7 +3400,16 @@ void drm_fb_release(struct drm_file *priv)
>         struct drm_device *dev = priv->minor->dev;
>         struct drm_framebuffer *fb, *tfb;
>
> -       mutex_lock(&priv->fbs_lock);
> +       /*
> +        * When the file gets released that means no one else can access the fb
> +        * list any more, so no need to grab fpriv->fbs_lock. And we need to to
> +        * avoid upsetting lockdep since the universal cursor code adds a
> +        * framebuffer while holding mutex locks.
> +        *
> +        * Note that a real deadlock between fpriv->fbs_lock and the modeset
> +        * locks is impossible here since no one else but this function can get
> +        * at it any more.
> +        */
>         list_for_each_entry_safe(fb, tfb, &priv->fbs, filp_head) {
>
>                 mutex_lock(&dev->mode_config.fb_lock);
> @@ -3413,7 +3422,6 @@ void drm_fb_release(struct drm_file *priv)
>                 /* This will also drop the fpriv->fbs reference. */
>                 drm_framebuffer_remove(fb);
>         }
> -       mutex_unlock(&priv->fbs_lock);
>  }
>
>  /**
> --
> 2.1.0
>



-- 
Paulo Zanoni