ACK/Cmnt: [PATCH 1/2] drm/i915: Fix ref->mutex deadlock in i915_active_wait()
andrea.righi at canonical.com
Fri Apr 10 06:45:27 UTC 2020
On Thu, Apr 09, 2020 at 09:45:02AM -0700, Sultan Alsawaf wrote:
> On Thu, Apr 09, 2020 at 06:19:23PM +0200, Andrea Righi wrote:
> > BTW, is 750bde2fd4ff really needed? Looking at those commits it seems
> > that only 274cbf20fd10 and 093b92287363 should be enough to prevent the
> > deadlock.
> I think it is needed. In the commit message for 750bde2fd4ff, Chris says that
> there is a synchronization issue when retirement occurs in a worker on another
> CPU. The very first commit in my list, 274cbf20fd10, does exactly that: it moves
> retirement into a worker for some cases, so it appears that 750bde2fd4ff is a
> necessary fix for that.
> > I've tried to backport these two and it seems a doable backport (I
> > have the patches against focal, but I haven't tested them, I just
> > build-tested).
> Doable, but I think it carries a lot more risk. Backporting i915 fixes is a
> dangerous game because its code changes so much in between releases. I've tried
> to backport other i915 stuff before, which wasn't difficult to backport at all,
> but then the backport caused panics because extra mutex locks were needed. Scary
> > However, if you confirm that your patch has been tested I think it can
> > be an easier fix and a reasonable compromise. Under this assumption:
> Yep, and you can test on your own too! Just boot with `mem=2G` or something, and
> then add a printk inside of i915_active_request_retire() to notice when it gets
> called. If you don't deadlock when it gets called then it's a success.
OK, it makes sense, thanks for the clarification! Under all these
assumptions my ACK is valid then. :)
More information about the kernel-team