Deadlock in intel_user_framebuffer_destroy()

Deadlock in intel_user_framebuffer_destroy()

[Lukas Wunner]

Hi,

a deadlock was introduced by commit 60a5ca015ffd2aacfe5674b5a401cd2a37159e07

Author: Ville Syrjälä <ville.syrjala@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").

This leads to the following call chain while &dev->struct_mutex is locked:

intel_fbdev.c:intelfb_create()
  -> drm_crtc.c:drm_framebuffer_unreference()
       -> drm_crtc.c:drm_framebuffer_free()
            -> intel_display.c:intel_user_framebuffer_destroy()

The last function in that call chain attempts to re-lock the mutex.
The functionality added by Chris Wilson is thus rendered broken.

Best regards,

Lukas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Deadlock in intel_user_framebuffer_destroy()

[Chris Wilson]

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@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.

t a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 6372cfc..a9b8c43 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -209,8 +209,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
        int size, ret;
        bool prealloc = false;
 
-       mutex_lock(&dev->struct_mutex);
-
        if (intel_fb &&
            (sizes->fb_width > intel_fb->base.width ||
             sizes->fb_height > intel_fb->base.height)) {
@@ -221,6 +219,9 @@ static int intelfb_create(struct drm_fb_helper *helper,
                drm_framebuffer_unreference(&intel_fb->base);
                intel_fb = ifbdev->fb = NULL;
        }
+
+       mutex_lock(&dev->struct_mutex);
+
        if (!intel_fb || WARN_ON(!intel_fb->obj)) {
                DRM_DEBUG_KMS("no BIOS fb, allocating a new one\n");
                ret = intelfb_alloc(helper, sizes);

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Deadlock in intel_user_framebuffer_destroy()

[Jani Nikula]

On Wed, 03 Jun 2015, Chris Wilson <chris@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@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?

BR,
Jani.


>
> t a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 6372cfc..a9b8c43 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -209,8 +209,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
>         int size, ret;
>         bool prealloc = false;
>  
> -       mutex_lock(&dev->struct_mutex);
> -
>         if (intel_fb &&
>             (sizes->fb_width > intel_fb->base.width ||
>              sizes->fb_height > intel_fb->base.height)) {
> @@ -221,6 +219,9 @@ static int intelfb_create(struct drm_fb_helper *helper,
>                 drm_framebuffer_unreference(&intel_fb->base);
>                 intel_fb = ifbdev->fb = NULL;
>         }
> +
> +       mutex_lock(&dev->struct_mutex);
> +
>         if (!intel_fb || WARN_ON(!intel_fb->obj)) {
>                 DRM_DEBUG_KMS("no BIOS fb, allocating a new one\n");
>                 ret = intelfb_alloc(helper, sizes);
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Deadlock in intel_user_framebuffer_destroy()

[Lukas Wunner]

[-- Attachment #1: Type: text/plain, Size: 412 bytes --]

Hi Jani,

On Mon, Jun 15, 2015 at 09:44:15AM +0300, Jani Nikula wrote:
> On Wed, 03 Jun 2015, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > Just move the mutex_lock down a step.
> Lucas, did you try this?

Yes, works for me. I was going to submit the attached patch as part of
the MacBook Pro GPU switching patch set (once it's finished) but feel
free to merge now.

Thanks for following up on this.

Lukas

[-- Attachment #2: v2-0018-drm-i915-Fix-deadlock-upon-discarding-BIOS-fb.patch --]
[-- Type: text/plain, Size: 3859 bytes --]

>From b502824816e20e65f7b0b17b18efec161f7560ed Mon Sep 17 00:00:00 2001
Message-Id: <b502824816e20e65f7b0b17b18efec161f7560ed.1434188640.git.lukas@wunner.de>
In-Reply-To: <77dc50305ee6fa2db076907496e373cc237e4901.1434188640.git.lukas@wunner.de>
References: <1dcf15ad548ee6602134a71e2e1af11609b4c32b.1434188640.git.lukas@wunner.de>
	<82e0890fdef2fcfa467f78a397641c39d7d9cfdd.1434188640.git.lukas@wunner.de>
	<84b1a363ca1860d523ce52b11aa83239ebf2e163.1434188640.git.lukas@wunner.de>
	<450b5fe4557644dbc5245a386e94c6ccc6654b92.1434188640.git.lukas@wunner.de>
	<4327e6c5baf23370cfb1cd09105693978f6981d3.1434188640.git.lukas@wunner.de>
	<1eb55048182574dd0443e7f83a44dbdab99bbe86.1434188640.git.lukas@wunner.de>
	<e395b70b5680ec8551848550f90f563250323fe8.1434188640.git.lukas@wunner.de>
	<226f1ae95e50070a6b17999d46c850d3abcb4496.1434188640.git.lukas@wunner.de>
	<a151a053833b7e6561479b8e24ab6a669a0f6673.1434188640.git.lukas@wunner.de>
	<afab3acad3d94c25b7f7582d37dd4ec3b35afa3b.1434188640.git.lukas@wunner.de>
	<293413a2703dda0c40967031531b20fbab6214a9.1434188640.git.lukas@wunner.de>
	<05704f90341a290bae3701b3ca9efc8b84729e20.1434188640.git.lukas@wunner.de>
	<5875c0ca739463fae31618bd6a9f07d3953b14cc.1434188640.git.lukas@wunner.de>
	<a5bac6cead2e8f6e3e481abdf5f3476883be64d0.1434188640.git.lukas@wunner.de>
	<6871930e1d0f3d8dcb702bbb4f9b121d48a334ca.1434188640.git.lukas@wunner.de>
	<a6d60e1981004ba1e8dc4ce37b78657b2edf61c8.1434188640.git.lukas@wunner.de>
	<77dc50305ee6fa2db076907496e373cc237e4901.1434188640.git.lukas@wunner.de>
From: Lukas Wunner <lukas@wunner.de>
Date: Wed, 3 Jun 2015 14:57:41 +0100
Subject: [PATCH v2 18/20] drm/i915: Fix deadlock upon discarding BIOS fb
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
To: dri-devel@lists.freedesktop.org

From: Chris Wilson <chris@chris-wilson.co.uk>

A deadlock was introduced by commit 60a5ca015ffd:

Author: Ville SyrjÀlÀ <ville.syrjala@linux.intel.com>
Date:   Fri Jun 13 11:10:53 2014 +0300

    drm/i915: Add locking around framebuffer_references--

The commit amended intel_user_framebuffer_destroy() with locking of
dev->struct_mutex.

A few weeks prior Chris Wilson had amended intelfb_create() with a call
to drm_framebuffer_unreference() while dev->struct_mutex is locked
(edd586fe705e, "drm/i915: Discard BIOS framebuffers too small to
accommodate chosen mode"). That function calls drm_framebuffer_free(),
which calls intel_user_framebuffer_destroy(), which now tries to acquire
the lock once more, rendering the functionality added by Chris Wilson
broken.

Leave the offending commit as is but adapt Chris Wilson's code by moving
mutex_lock() below the call to drm_framebuffer_unreference().

Fixes: 60a5ca015ffd2aacfe5674b5a401cd2a37159e07
[Lukas: Amend Chris' patch with commit message]
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 4e7e7da..bea3218 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -183,8 +183,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	int size, ret;
 	bool prealloc = false;
 
-	mutex_lock(&dev->struct_mutex);
-
 	if (intel_fb &&
 	    (sizes->fb_width > intel_fb->base.width ||
 	     sizes->fb_height > intel_fb->base.height)) {
@@ -195,6 +193,9 @@ static int intelfb_create(struct drm_fb_helper *helper,
 		drm_framebuffer_unreference(&intel_fb->base);
 		intel_fb = ifbdev->fb = NULL;
 	}
+
+	mutex_lock(&dev->struct_mutex);
+
 	if (!intel_fb || WARN_ON(!intel_fb->obj)) {
 		DRM_DEBUG_KMS("no BIOS fb, allocating a new one\n");
 		ret = intelfb_alloc(helper, sizes);
-- 
2.1.0


[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Deadlock in intel_user_framebuffer_destroy()

[Chris Wilson]

On Mon, Jun 15, 2015 at 09:44:15AM +0300, Jani Nikula wrote:
> On Wed, 03 Jun 2015, Chris Wilson <chris@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@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

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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Deadlock in intel_user_framebuffer_destroy()

[Tvrtko Ursulin]

Hi,

On 06/15/2015 08:53 AM, Chris Wilson wrote:
> On Mon, Jun 15, 2015 at 09:44:15AM +0300, Jani Nikula wrote:
>> On Wed, 03 Jun 2015, Chris Wilson <chris@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@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
>
> 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;
>   }
>

intelfb_alloc wants struct_mutex, both for __intel_framebuffer_create 
and pin_and_fence. And also there is that double obj unreference in the 
failure path from the former Jani spotted.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: 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@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@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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Deadlock in intel_user_framebuffer_destroy()

[Chris Wilson]

On Mon, Jun 15, 2015 at 02:02:23PM +0200, Daniel Vetter wrote:
> 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@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@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?

There wasn't, I just rewrote it incorrectly. There's also a
drm_framebuffer_remove() called by intelfb_alloc which needs to be moved
out of the mutex. A much larger disentangling of the functions involved
here is required. Tvrstko volunteered himself. Brave, very brave :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Deadlock in intel_user_framebuffer_destroy()

[Jani Nikula]

On Mon, 15 Jun 2015, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Mon, Jun 15, 2015 at 02:02:23PM +0200, Daniel Vetter wrote:
>> 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@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@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?
>
> There wasn't, I just rewrote it incorrectly. There's also a
> drm_framebuffer_remove() called by intelfb_alloc which needs to be moved
> out of the mutex. A much larger disentangling of the functions involved
> here is required. Tvrstko volunteered himself. Brave, very brave :)

-ETIMEDOUT

IIUC we still need the fix, but there's no update from Lucas addressing
review. Is that right?

BR,
Jani.


> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Deadlock in intel_user_framebuffer_destroy()

[Lukas Wunner]

Hi Jani,

On Mon, Jun 29, 2015 at 02:24:19PM +0300, Jani Nikula wrote:
> On Mon, 15 Jun 2015, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > There wasn't, I just rewrote it incorrectly. There's also a
> > drm_framebuffer_remove() called by intelfb_alloc which needs to be moved
> > out of the mutex. A much larger disentangling of the functions involved
> > here is required. Tvrstko volunteered himself. Brave, very brave :)
> -ETIMEDOUT
> IIUC we still need the fix, but there's no update from Lucas addressing
> review. Is that right?

Sorry, I was not copied on Tvrtko's e-mail of June 15 ("drm/i915:
Fix failure paths around initial fbdev allocation") and though
I'm subscribed to intel-gfx I didn't realize that particular
e-mail required a reaction from me.

I've just sent out my review and I'll also test the patch and
report back.

Thanks,

Lukas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Deadlock in intel_user_framebuffer_destroy()

[Jani Nikula]

On Mon, 29 Jun 2015, Lukas Wunner <lukas@wunner.de> wrote:
> Hi Jani,
>
> On Mon, Jun 29, 2015 at 02:24:19PM +0300, Jani Nikula wrote:
>> On Mon, 15 Jun 2015, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> > There wasn't, I just rewrote it incorrectly. There's also a
>> > drm_framebuffer_remove() called by intelfb_alloc which needs to be moved
>> > out of the mutex. A much larger disentangling of the functions involved
>> > here is required. Tvrstko volunteered himself. Brave, very brave :)
>> -ETIMEDOUT
>> IIUC we still need the fix, but there's no update from Lucas addressing
>> review. Is that right?
>
> Sorry, I was not copied on Tvrtko's e-mail of June 15 ("drm/i915:
> Fix failure paths around initial fbdev allocation") and though
> I'm subscribed to intel-gfx I didn't realize that particular
> e-mail required a reaction from me.

No worries, I didn't realize these two threads were connected!

BR,
Jani.

>
> I've just sent out my review and I'll also test the patch and
> report back.
>
> Thanks,
>
> Lukas

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx