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

>
>> Fixes: 573ba126aef3 ("drm/i915/guc: Capture error state on context reset")
>> 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.

> ...
>
>> +	if (hung_rq)
>> +		i915_request_put(hung_rq);
> In Linux kernel the idiom is that freeing resources APIs should be NULL-aware
> (or ERR_PTR aware or both). Does i915 follows that? If so, the test should be
> inside i915_request_put() rather than in any of the callers.
That's as maybe. But this is how i915_request_put() currently works - it 
is simply a wrapper around 'dma_fence_put(&rq->fence);'. So passing in a 
null pointer will immediately cause a null pointer deref. If you want 
the put implementation to change and to re-work all its callers, that 
should be done in a separate patch and not piled on top of other changes.

>
> ...
>
>> @@ -4847,6 +4857,7 @@ void intel_guc_find_hung_context(struct intel_engine_cs *engine)
>>   			xa_lock(&guc->context_lookup);
>>   			goto done;
>>   		}
>> +
>>   next:
>>   		intel_context_put(ce);
>>   		xa_lock(&guc->context_lookup);
> Stray change.
Intentional change to improve the readability of a function that is 
being modified by other changes in this patch.

John.

>