Re: [Intel-gfx] [PATCH] drm/i915/guc: Check for wedged before doing stuff

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 16/12/2021 20:30, John Harrison wrote:
> On 12/16/2021 00:47, Tvrtko Ursulin wrote:
>> On 15/12/2021 22:45, John.C.Harrison@Intel.com wrote:
>>> From: John Harrison <John.C.Harrison@Intel.com>
>>>
>>> A fault injection probe test hit a BUG_ON in a GuC error path. It
>>> showed that the GuC code could potentially attempt to do many things
>>> when the device is actually wedged. So, add a check in to prevent that.
>>>
>>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c 
>>> b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>>> index 97311119da6f..88f002c4d41b 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>>> @@ -1350,7 +1350,8 @@ submission_disabled(struct intel_guc *guc)
>>>       struct i915_sched_engine * const sched_engine = guc->sched_engine;
>>>         return unlikely(!sched_engine ||
>>> - !__tasklet_is_enabled(&sched_engine->tasklet));
>>> + !__tasklet_is_enabled(&sched_engine->tasklet) ||
>>> +            test_bit(I915_WEDGED, &guc_to_gt(guc)->reset.flags));
>>
>> Or intel_gt_is_wedged ?
> Hmm. I just copied the test from somewhere else. Is there any particular 
> reason why other bits of code would be doing the explicit test_bit 

Lets see:

$ grep intel_gt_is_wedged . -r | wc -l
55

$ grep test_bit.*I915_WEDGED, . -r
./gt/intel_gt.h:                   !test_bit(I915_WEDGED, &gt->reset.flags));
./gt/intel_gt.h:        return unlikely(test_bit(I915_WEDGED, &gt->reset.flags));
./gt/intel_reset.c:     if (test_bit(I915_WEDGED, &gt->reset.flags))
./gt/intel_reset.c:     if (test_bit(I915_WEDGED, &gt->reset.flags))
./gt/intel_reset.c:     if (!test_bit(I915_WEDGED, &gt->reset.flags))
./gt/intel_reset.c:     if (!test_bit(I915_WEDGED, &gt->reset.flags))
./gt/uc/intel_guc_submission.c:              test_bit(I915_WEDGED, &guc_to_gt(guc)->reset.flags))) {

So outside the components which own the flag only GuC goes direct therefore you might know better if there is a special reason for that.

The code there looks like this:

	/* Reset called during driver load or during wedge? */
	if (unlikely(!guc_submission_initialized(guc) ||
		     test_bit(I915_WEDGED, &guc_to_gt(guc)->reset.flags)))
		return;

Perhaps that check and then one you are adding could even be partly the same?

> rather than calling the helper? I see the helper has a BUG_ON. Can that 
> fire if called at the wrong time in the reset path?

The grep above suggests it should be safe. And looking at the assert it seems to check if someone set the fatal wedge bit without setting the "normal" wedge eg. setting it directly bypassing the helper. So should be fine.

Regards,

Tvrtko