| Message ID | 20150615075302.GR28462@nuc-i3427.alporthouse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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
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; }