Re: [PATCH] drm/xe/mmio: Fix xe_mmio_wait32() to honor delay/sleep maximums

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mmio: fixed 10us delay in long waits causes hrtimer/context switch storms
--

commit c92012bfe0677557677a0dcb08c9a338f44452fd
Author: Alan Previn <alan.previn.teres.alexis@intel.com>
Subject: drm/xe/mmio: Fix xe_mmio_wait32() to honor delay/sleep maximums

This commit replaces the custom wait implementation in the driver with the
standard poll_timeout_us and poll_timeout_us_atomic macros. It drops the
atomic flag from xe_mmio_wait32 parameters, updating callers and adding
explicit _atomic variants where needed.

> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
> --- a/drivers/gpu/drm/xe/xe_guc.c
> +++ b/drivers/gpu/drm/xe/xe_guc.c
[ ... ]
> @@ -1574,7 +1574,7 @@ int xe_guc_mmio_send_recv(struct xe_guc *guc, const u32 *request,
>  		BUILD_BUG_ON((GUC_HXG_TYPE_RESPONSE_SUCCESS ^ GUC_HXG_TYPE_RESPONSE_FAILURE) != 1);
>  
>  		ret = xe_mmio_wait32(mmio, reply_reg, resp_mask, resp_mask,
> -				     2000000, &header, false);
> +					     2000000, &header);

[Severity: High]
Does this change cause an excessive number of hrtimers or context switches
for long waits?

For example, in xe_guc_mmio_send_recv(), the 2,000,000us timeout here could
result in ~200,000 sleep/wake cycles if the hardware is slow or hits the
timeout. The previous custom wait implementation used an exponential backoff to
handle these long wait periods without flooding the system.

[ ... ]
> diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
> --- a/drivers/gpu/drm/xe/xe_mmio.c
> +++ b/drivers/gpu/drm/xe/xe_mmio.c
[ ... ]
>  int xe_mmio_wait32(struct xe_mmio *mmio, struct xe_reg reg, u32 mask, u32 val, u32 timeout_us,
> -		   u32 *out_val, bool atomic)
> +		   u32 *out_val)
>  {
> -	return __xe_mmio_wait32(mmio, reg, mask, val, timeout_us, out_val, atomic, true);
> +	u32 read;
> +	int ret;
> +
> +	ret = poll_timeout_us(read = xe_mmio_read32(mmio, reg), (read & mask) == val,
> +			      10, timeout_us, false);

[Severity: High]
Will hardcoding a 10us sleep interval here cause context switch storms and
PCIe bus flooding during long operations?

Certain callers like __xe_driver_flr() wait up to 3 seconds, and
xe_gsc_wait_for_proxy_init_done() waits up to 1 second. Using a fixed 10us
interval means those paths might invoke usleep_range() tens of thousands of
times if the full timeout is reached.

Could this degrade system performance, and does it warrant re-introducing
an exponential backoff scaling for longer wait periods instead of a fixed
interval?

> +	if (out_val)
> +		*out_val = read;
> +
> +	return ret;
>  }
[ ... ]
>  int xe_mmio_wait32_not(struct xe_mmio *mmio, struct xe_reg reg, u32 mask, u32 val, u32 timeout_us,
> -		       u32 *out_val, bool atomic)
> +		       u32 *out_val)
>  {
> -	return __xe_mmio_wait32(mmio, reg, mask, val, timeout_us, out_val, atomic, false);
> +	u32 read;
> +	int ret;
> +
> +	ret = poll_timeout_us(read = xe_mmio_read32(mmio, reg), (read & mask) != val,
> +			      10, timeout_us, false);

[Severity: High]
Similar to xe_mmio_wait32() above, does applying a fixed 10us sleep interval
in xe_mmio_wait32_not() risk hrtimer storms for callers with large timeouts?

> +	if (out_val)
> +		*out_val = read;
> +
> +	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914215746.272721-2-alan.previn.teres.alexis@intel.com?part=1