Re: [Intel-gfx] [PATCH] drm/i915/gt: Move execlists_reset() out of line

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

On 21/01/2021 10:58, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2021-01-21 10:55:49)
>>
>> On 21/01/2021 00:32, Chris Wilson wrote:
>>> Reduce the bulk of execlists_submission_tasklet by moving the unlikely
>>> reset function out of line.
>>>
>>> add/remove: 1/0 grow/shrink: 0/1 up/down: 960/-935 (25)
>>> Function                                     old     new   delta
>>> execlists_reset                                -     960    +960
>>> execlists_submission_tasklet                6629    5694    -935
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> ---
>>>    .../drm/i915/gt/intel_execlists_submission.c  | 36 +++++++++----------
>>>    1 file changed, 17 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>>> index 740ff05fd692..43cc85241886 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>>> @@ -2299,10 +2299,13 @@ static void execlists_capture(struct intel_engine_cs *engine)
>>>        kfree(cap);
>>>    }
>>>    
>>> -static void execlists_reset(struct intel_engine_cs *engine, const char *msg)
>>> +static noinline void execlists_reset(struct intel_engine_cs *engine)
>>>    {
>>> +     struct intel_engine_execlists *el = &engine->execlists;
>>>        const unsigned int bit = I915_RESET_ENGINE + engine->id;
>>>        unsigned long *lock = &engine->gt->reset.flags;
>>> +     unsigned long eir = fetch_and_zero(&el->error_interrupt);
>>
>> We got the locking wrong for this one. Irq handler side is under
>> gt->irq_lock, but there are unlocked rmw cycles in the tasklet. Not
>> counting this fetch_and_zero which is also unlocked.
> 
> It doesn't require locking.

		/* Disable the error interrupt until after the reset */
		if (likely(eir)) {
			ENGINE_WRITE(engine, RING_EMR, ~0u);
			ENGINE_WRITE(engine, RING_EIR, eir);
			WRITE_ONCE(engine->execlists.error_interrupt, eir);
			tasklet = true;
		}

Okay I did not grep with enough context.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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