[Intel-gfx] Deadlock in intel_user_framebuffer_destroy()

[Daniel Vetter]

On Mon, Jun 15, 2015 at 08:53:02AM +0100, Chris Wilson wrote:
> On Mon, Jun 15, 2015 at 09:44:15AM +0300, Jani Nikula wrote:
> > On Wed, 03 Jun 2015, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> > > On Wed, Jun 03, 2015 at 03:43:32PM +0200, Lukas Wunner wrote:
> > >> Hi,
> > >> 
> > >> a deadlock was introduced by commit 60a5ca015ffd2aacfe5674b5a401cd2a37159e07
> > >> 
> > >> Author: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > >> Date:   Fri Jun 13 11:10:53 2014 +0300
> > >> 
> > >>     drm/i915: Add locking around framebuffer_references--
> > >> 
> > >> 
> > >> The commit amended intel_display.c:intel_user_framebuffer_destroy() with
> > >> mutex_lock(&dev->struct_mutex).
> > >> 
> > >> A few weeks prior Chris Wilson had amended intel_fbdev.c:intelfb_create()
> > >> with a call to drm_framebuffer_unreference() while &dev->struct_mutex is
> > >> locked (commit edd586fe705e819bc711b5ed7194a0b6f9f1a7e1, "drm/i915: Discard
> > >> BIOS framebuffers too small to accommodate chosen mode").
> > >> 
> > >
> > > Just move the mutex_lock down a step.
> > 
> > Lucas, did you try this?
> 
> There's a goto unlock that also needed to be disabled, such as

Your previous patch placed the mutex_lock before the goto out_unlock - I
fail to see what has been broken with that version? Can you resubmit that
as a proper patch with sob and Lucas' t-b?

Thanks, Daniel

> 
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index dda99c0d6be1..fc7ec5138fb7 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -213,8 +213,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
>         bool prealloc = false;
>         int ret;
>  
> -       mutex_lock(&dev->struct_mutex);
> -
>         if (intel_fb &&
>             (sizes->fb_width > intel_fb->base.width ||
>              sizes->fb_height > intel_fb->base.height)) {
> @@ -229,7 +227,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
>                 DRM_DEBUG_KMS("no BIOS fb, allocating a new one\n");
>                 ret = intelfb_alloc(helper, sizes);
>                 if (ret)
> -                       goto out_unlock;
> +                       return ret;
>                 intel_fb = ifbdev->fb;
>         } else {
>                 DRM_DEBUG_KMS("re-using BIOS fb\n");
> @@ -241,6 +239,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
>         obj = intel_fb->obj;
>         vma = i915_gem_obj_to_ggtt(obj, NULL);
>  
> +       mutex_lock(&dev->struct_mutex);
>         info = framebuffer_alloc(0, &dev->pdev->dev);
>         if (!info) {
>                 ret = -ENOMEM;
> @@ -311,7 +310,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  out_unpin:
>         drm_gem_object_unreference(&obj->base);
> -out_unlock:
>         mutex_unlock(&dev->struct_mutex);
>         return ret;
>  }
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch