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

[Andi Shyti <andi.shyti@linux.intel.com>]

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/ ?

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

Do you think we might need a wait_for_atomic_preempt() macro?

	err = wait_for_atomic_preempt(i915_in_reset(pdev), 50);

Thanks,
Andi