Re: [Intel-gfx] [PATCH] drm/i915: Do not disable preemption for resets

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

On 26/09/2023 10:18, Andi Shyti wrote:
> Hi Tvrtko,
> 
>> Commit ade8a0f59844 ("drm/i915: Make all GPU resets atomic") added a
>> preempt disable section over the hardware reset callback to prepare the
>> driver for being able to reset from atomic contexts.
>>
>> In retrospect I can see that the work item at a time was about removing
>> the struct mutex from the reset path. Code base also briefly entertained
>> the idea of doing the reset under stop_machine in order to serialize
>> userspace mmap and temporary glitch in the fence registers (see
>> eb8d0f5af4ec ("drm/i915: Remove GPU reset dependence on struct_mutex"),
>> but that never materialized and was soon removed in 2caffbf11762
>> ("drm/i915: Revoke mmaps and prevent access to fence registers across
>> reset") and replaced with a SRCU based solution.
>>
>> As such, as far as I can see, today we still have a requirement that
>> resets must not sleep (invoked from submission tasklets), but no need to
>> support invoking them from a truly atomic context.
>>
>> Given that the preemption section is problematic on RT kernels, since the
>> uncore lock becomes a sleeping lock and so is invalid in such section,
>> lets try and remove it. Potential downside is that our short waits on GPU
>> to complete the reset may get extended if CPU scheduling interferes, but
>> in practice that probably isn't a deal breaker.
>>
>> In terms of mechanics, since the preemption disabled block is being
>> removed we just need to replace a few of the wait_for_atomic macros into
>> busy looping versions which will work (and not complain) when called from
>> non-atomic sections.
> 
> looks reasonable, few unrelated questions
> 
>> ---
>>   drivers/gpu/drm/i915/gt/intel_reset.c | 12 +++++-------
>>   1 file changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
>> index e2152f75ba2e..6916eba3bd33 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
>> @@ -167,13 +167,13 @@ static int i915_do_reset(struct intel_gt *gt,
>>   	/* Assert reset for at least 20 usec, and wait for acknowledgement. */
> 
> is this /20/50/ ?

Unrelated change but okay.

> 
>>   	pci_write_config_byte(pdev, I915_GDRST, GRDOM_RESET_ENABLE);
>>   	udelay(50);
>> -	err = wait_for_atomic(i915_in_reset(pdev), 50);
>> +	err = _wait_for_atomic(i915_in_reset(pdev), 50, 0);
> 
> wait_for_atomic() waits in milliseconds, while _wait_for_atomic()
> waits in microseconds, I think you need to update the timer.

Ah.. well spotted!

> Do you think we might need a wait_for_atomic_preempt() macro?
> 
> 	err = wait_for_atomic_preempt(i915_in_reset(pdev), 50);

I don't see what it would do? _wait_for_atomic when ATOMIC == 0 already 
enables preemption. To allow passing in milliseconds? I fear one more 
macro would create more confusion.

Regards,

Tvrtko