Re: [PATCH] drm/i915: Fix failure paths around initial fbdev allocation

From: Lukas Wunner <lukas@wunner.de>
To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: Fix failure paths around initial fbdev allocation
Date: Mon, 29 Jun 2015 19:48:38 +0200	[thread overview]
Message-ID: <20150629174838.GA8131@wunner.de> (raw)
In-Reply-To: <559160FC.1070605@linux.intel.com>

Hi Tvrtko,

On Mon, Jun 29, 2015 at 04:15:08PM +0100, Tvrtko Ursulin wrote:
> On 06/29/2015 03:39 PM, Lukas Wunner wrote:
> >On Mon, Jun 15, 2015 at 11:49:52AM +0100, Tvrtko Ursulin wrote:
> >>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>
> >>We had two failure modes here:
> >>
> >>1.
> >>Deadlock in intelfb_alloc failure path where it calls drm_framebuffer_remove,
> >>which grabs the struct mutex and intelfb_create (caller of intelfb_alloc) was
> >>already holding it.
> >
> >s/intelfb_alloc/intelfb_create/
> >s/(caller of intelfb_alloc)//
> 
> I looked again and don't see that I made a mistake with regards to this?

Sorry, I was confused myself:

Apart from the two issues you mentioned in the commit message there's
a third one fixed by this patch which could briefly be described as:

"Deadlock in intelfb_create failure path where it calls
drm_framebuffer_unreference, which grabs the struct mutex
and intelfb_create was already holding it."

Only that single issue was reported by me, the others were subsequently
discovered by you or your colleagues.

A somewhat more elaborate description is in my original report:
http://lists.freedesktop.org/archives/intel-gfx/2015-June/067965.html

> >I will also test the patch and report back in a bit.
> 
> That will be very useful. I did not test it extensively myself, just fired
> it off quickly since I was briefly hitting this failure path myself.

Okay the patch works for me (in the specific scenario on the
MacBook Pro where the existing fb is discarded because it is
too small and a new one is allocated).

PRTS did report some issue on Baytrail though:
http://lists.freedesktop.org/archives/intel-gfx/2015-June/069978.html

One nitpick is that your commit message exceeds 72 characters per line.

Also, Ville had posted a suggestion which was not discussed further,
removing the drm_gem_object_unreference() from __intel_framebuffer_create()
in favor of the one you now removed from intelfb_alloc(). (Personally
I'm totally indifferent on that.)

Best regards,

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