Re: [Intel-gfx] [PATCH v2 1/5] drm/i915: Fix request locking during error capture & debugfs dump

[John Harrison <john.c.harrison@intel.com>]

On 1/18/2023 09:54, Andy Shevchenko wrote:
> On Wed, Jan 18, 2023 at 09:34:47AM -0800, John Harrison wrote:
>> On 1/18/2023 00:29, Andy Shevchenko wrote:
>>> On Tue, Jan 17, 2023 at 01:36:26PM -0800, John.C.Harrison@Intel.com wrote:
>>>> From: John Harrison <John.C.Harrison@Intel.com>
>>>>
>>>> When GuC support was added to error capture, the locking around the
>>>> request object was broken. Fix it up.
>>>>
>>>> The context based search manages the spinlocking around the search
>>>> internally. So it needs to grab the reference count internally as
>>>> well. The execlist only request based search relies on external
>>>> locking, so it needs an external reference count. So no change to that
>>>> code itself but the context version does change.
>>>>
>>>> The only other caller is the code for dumping engine state to debugfs.
>>>> That code wasn't previously getting an explicit reference at all as it
>>>> does everything while holding the execlist specific spinlock. So that
>>>> needs updaing as well as that spinlock doesn't help when using GuC
>>>> submission. Rather than trying to conditionally get/put depending on
>>>> submission model, just change it to always do the get/put.
>>>>
>>>> In addition, intel_guc_find_hung_context() was not acquiring the
>>>> correct spinlock before searching the request list. So fix that up too.
>>>> Fixes: dc0dad365c5e ("drm/i915/guc: Fix for error capture after full GPU reset
>>>> with GuC")
>>> Must be one line.
>> In my tree it is one line. git itself does the line wrap when creating the
>> email.
> Can you elaborate? I never have had such issue with git send-email (starting
> from v1.6.x of Git for sure).
Hmm. Confused. I think it must have been something accidental in a text 
editor when reviewing the patch. Re-creating the emails now isn't 
wrapping it.

>> I missed that I need to manually unwrap it again before actually
>> sending the email. Although the CI checkpatch also pointed this out in it's
>> own obscure manner.
> ...
>
>>>> Cc: Matthew Brost <matthew.brost@intel.com>
>>>> Cc: John Harrison <John.C.Harrison@Intel.com>
>>>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>>> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>> Cc: Matthew Auld <matthew.auld@intel.com>
>>>> Cc: Matt Roper <matthew.d.roper@intel.com>
>>>> Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>>>> Cc: Michael Cheng <michael.cheng@intel.com>
>>>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>>>> Cc: Tejas Upadhyay <tejaskumarx.surendrakumar.upadhyay@intel.com>
>>>> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>>> Cc: Aravind Iddamsetty <aravind.iddamsetty@intel.com>
>>>> Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
>>>> Cc: Bruce Chang <yu.bruce.chang@intel.com>
>>>> Cc: intel-gfx@lists.freedesktop.org
>>> Is it possible to utilize --to --cc parameters to git send-email instead of
>>> noisy Cc list?
>> This is the list auto-generated by the 'dim fixes' tool. I am told this is
>> the officially correct way to create a fixes patch - copy the output from
>> 'dim fixes' as is into the patch headers.
> Okay, so it may be question to the `dim` tool then...
>
> ...
>
>>> Stray change.
>> Intentional change to improve the readability of a function that is being
>> modified by other changes in this patch.
> But not described in the commit message. That's why "stray".
Didn't seem worth mentioning. I can add a comment about it.

John.

>