dri-devel Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/xe/mmio: Fix xe_mmio_wait32() to honor delay/sleep maximums
@ 2026-09-07 23:55 Alan Previn
  2026-09-08  0:02 ` sashiko-bot
  0 siblings, 1 reply; 7+ messages in thread
From: Alan Previn @ 2026-09-07 23:55 UTC (permalink / raw)
  To: intel-xe; +Cc: Alan Previn, dri-devel, Matt Roper

Check for overflow in udelay/usleep_range use in __xe_mmio_wait32
and pick the correct helper accordingly. Fix sign-overflow and avoid
growing delays becoming intollerably large by capping the in-loop
wait time. Simplify sleep_min assignment by removing redundant cast.

Fixes: 5c09bd6ccd41 ("drm/xe/mmio: Move xe_mmio_wait32() to xe_mmio.c")
Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
Assisted-by: Github-Copilot:Claude-Sonnet-4-6
---
 drivers/gpu/drm/xe/xe_mmio.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
index 7fa18dfcb5a2..cf2e05aaa4b2 100644
--- a/drivers/gpu/drm/xe/xe_mmio.c
+++ b/drivers/gpu/drm/xe/xe_mmio.c
@@ -349,11 +349,23 @@ static int __xe_mmio_wait32(struct xe_mmio *mmio, struct xe_reg reg, u32 mask, u
 		if (ktime_after(ktime_add_us(cur, wait), end))
 			wait = ktime_us_delta(end, cur);
 
-		if (atomic)
-			udelay(wait);
-		else
-			usleep_range(wait, wait << 1);
-		wait <<= 1;
+#define __XE_MMIO_WAIT_MAX_INLOOP_TIME (100 * USEC_PER_MSEC)
+		if (atomic) {
+			if (wait <= MAX_UDELAY_MS * USEC_PER_MSEC)
+				udelay(wait);
+			else
+				mdelay(DIV_ROUND_UP(wait, USEC_PER_MSEC));
+		} else {
+			usleep_range(wait, __XE_MMIO_WAIT_MAX_INLOOP_TIME);
+		}
+		/*
+		 * As we keep doubling the wait time for every check that fails, cap the
+		 * in-loop delay-or-sleep to less than 2x 100 miliseconds to prevent from
+		 * expanding 'wait' into exponentially longer wait times per loop that
+		 * end up delaying the next completion check way later than tolerable.
+		 */
+		wait = wait < __XE_MMIO_WAIT_MAX_INLOOP_TIME >> 1 ?
+		       wait << 1 : __XE_MMIO_WAIT_MAX_INLOOP_TIME;
 	}
 
 	if (ret != 0) {

base-commit: 71bc3b7cc55631a9b3807da98e2b2880838b9623
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/xe/mmio: Fix xe_mmio_wait32() to honor delay/sleep maximums
  2026-09-07 23:55 [PATCH] drm/xe/mmio: Fix xe_mmio_wait32() to honor delay/sleep maximums Alan Previn
@ 2026-09-08  0:02 ` sashiko-bot
  0 siblings, 0 replies; 7+ messages in thread
From: sashiko-bot @ 2026-09-08  0:02 UTC (permalink / raw)
  To: Alan Previn; +Cc: intel-xe, dri-devel

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Incorrect maximum delay in usleep_range() introduces massive timer slack, defeating exponential backoff and causing extreme latency and spurious timeouts.
- [Medium] The commit message claims to simplify a `sleep_min` assignment, but no such change exists in the code.
- [Low] 64-bit division on 32-bit architectures causes link errors.
--

commit 76cc600b7d97fbc1edcd9a133df8168f9ad41977
Author: Alan Previn <alan.previn.teres.alexis@intel.com>

drm/xe/mmio: Fix xe_mmio_wait32() to honor delay/sleep maximums

This patch adds overflow checks for delays in __xe_mmio_wait32() and
introduces caps to the maximum in-loop wait time. It also aims to pick
the correct sleep or delay helper based on the requested wait duration.

> drm/xe/mmio: Fix xe_mmio_wait32() to honor delay/sleep maximums
> 
> Check for overflow in udelay/usleep_range use in __xe_mmio_wait32
> and pick the correct helper accordingly. Fix sign-overflow and avoid
> growing delays becoming intollerably large by capping the in-loop
> wait time. Simplify sleep_min assignment by removing redundant cast.

[Severity: Medium]
The commit message claims to simplify the sleep_min assignment by removing
a redundant cast, but the variable sleep_min does not exist in the modified
code. Could the commit message be updated to reflect the actual changes?

> diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
> index 7fa18dfcb5a28..cf2e05aaa4b2f 100644
> --- a/drivers/gpu/drm/xe/xe_mmio.c
> +++ b/drivers/gpu/drm/xe/xe_mmio.c

[ ... ]

> @@ -349,11 +349,23 @@ static int __xe_mmio_wait32(struct xe_mmio *mmio, struct xe_reg reg, u32 mask, u
>  		if (ktime_after(ktime_add_us(cur, wait), end))
>  			wait = ktime_us_delta(end, cur);
>  
> -		if (atomic)
> -			udelay(wait);
> -		else
> -			usleep_range(wait, wait << 1);
> -		wait <<= 1;
> +#define __XE_MMIO_WAIT_MAX_INLOOP_TIME (100 * USEC_PER_MSEC)
> +		if (atomic) {
> +			if (wait <= MAX_UDELAY_MS * USEC_PER_MSEC)
> +				udelay(wait);
> +			else
> +				mdelay(DIV_ROUND_UP(wait, USEC_PER_MSEC));

[Severity: Low]
Wait is defined as an s64 type. Does using DIV_ROUND_UP(wait, USEC_PER_MSEC)
here cause a 64-bit division? On 32-bit architectures, this could lead to an
undefined reference to __divdi3 and break the build.

> +		} else {
> +			usleep_range(wait, __XE_MMIO_WAIT_MAX_INLOOP_TIME);

[Severity: High]
Does hardcoding __XE_MMIO_WAIT_MAX_INLOOP_TIME (100ms) as the maximum bound
for usleep_range() introduce massive timer slack? For example, on the first
iteration where wait is 10us, this allows the scheduler to delay the wakeup
for up to 100ms, completely defeating the intended exponential backoff.
This could cause massive latency spikes for fast MMIO operations in non-atomic
calls to __xe_mmio_wait32().

> +		}
> +		/*
> +		 * As we keep doubling the wait time for every check that fails, cap the
> +		 * in-loop delay-or-sleep to less than 2x 100 miliseconds to prevent from
> +		 * expanding 'wait' into exponentially longer wait times per loop that
> +		 * end up delaying the next completion check way later than tolerable.
> +		 */
> +		wait = wait < __XE_MMIO_WAIT_MAX_INLOOP_TIME >> 1 ?
> +		       wait << 1 : __XE_MMIO_WAIT_MAX_INLOOP_TIME;
>  	}

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH] drm/xe/mmio: Fix xe_mmio_wait32() to honor delay/sleep maximums
@ 2026-09-08 18:56 Alan Previn
  2026-09-08 19:03 ` sashiko-bot
  2026-09-09  8:00 ` Jani Nikula
  0 siblings, 2 replies; 7+ messages in thread
From: Alan Previn @ 2026-09-08 18:56 UTC (permalink / raw)
  To: intel-xe; +Cc: Alan Previn, dri-devel, Matt Roper

Check for overflow in udelay/usleep_range use in __xe_mmio_wait32
and pick the correct helper according to the wait time and atomic.
Implement a similar helper to replace DIV_ROUND_UP for 32-bit CPUs.
Avoid growing delays becoming intollerably large by capping the
in-loop wait time.

v2: - Fixed checkpatch failure.
    - Added helper for 64-bit DIV_ROUND_UP on 32-bit CPU (Shasiko
      review). Fixed bug max range in usleep_range(Shasiko review)

Fixes: 5c09bd6ccd41 ("drm/xe/mmio: Move xe_mmio_wait32() to xe_mmio.c")
Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
Assisted-by: Github-Copilot:Claude-Sonnet-5-0
---
 drivers/gpu/drm/xe/xe_mmio.c | 50 ++++++++++++++++++++++++++++++++----
 1 file changed, 45 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
index 7fa18dfcb5a2..1ae382f009de 100644
--- a/drivers/gpu/drm/xe/xe_mmio.c
+++ b/drivers/gpu/drm/xe/xe_mmio.c
@@ -7,6 +7,8 @@
 
 #include <linux/delay.h>
 #include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/math.h>
+#include <linux/math64.h>
 #include <linux/minmax.h>
 #include <linux/pci.h>
 
@@ -320,6 +322,30 @@ u64 xe_mmio_read64_2x32(struct xe_mmio *mmio, struct xe_reg reg)
 	return (u64)udw << 32 | ldw;
 }
 
+/**
+ * __div_round_up64() - alternative to DIV_ROUND_UP for use by __xe_mmio_wait32
+ * @dividend: 64 bit positive number to divide
+ * @divisor: 64 bit positive divisor
+ *
+ * DIV_ROUND_UP() relies on plain '/' and '%' operators, which for 64-bit
+ * operands on a 32-bit CPU get turned into calls to libgcc's __divdi3()/
+ * __moddi3(), routines the kernel does not link against. Provide a
+ * do_div()-based equivalent that works for signed 64-bit inputs on any
+ * architecture.
+ *
+ * Returns: rounded up division result
+ */
+static inline s64 __div_round_up64(s64 dividend, s64 divisor)
+{
+	u64 abs_dividend = abs(dividend);
+	u64 abs_divisor = abs(divisor);
+	u64 result = abs_dividend + abs_divisor - 1;
+
+	do_div(result, abs_divisor);
+	/* dont check for negative values as local caller only uses positive numbers */
+	return (s64)result;
+}
+
 static int __xe_mmio_wait32(struct xe_mmio *mmio, struct xe_reg reg, u32 mask, u32 val,
 			    u32 timeout_us, u32 *out_val, bool atomic, bool expect_match)
 {
@@ -349,11 +375,25 @@ static int __xe_mmio_wait32(struct xe_mmio *mmio, struct xe_reg reg, u32 mask, u
 		if (ktime_after(ktime_add_us(cur, wait), end))
 			wait = ktime_us_delta(end, cur);
 
-		if (atomic)
-			udelay(wait);
-		else
-			usleep_range(wait, wait << 1);
-		wait <<= 1;
+#define __XE_MMIO_WAIT_MAX_INLOOP_100MS (100 * USEC_PER_MSEC)
+		if (atomic) {
+			if (wait <= MAX_UDELAY_MS * USEC_PER_MSEC)
+				udelay(wait);
+			else if (BITS_PER_LONG == 32)
+				mdelay(DIV_ROUND_UP(wait, USEC_PER_MSEC));
+			else
+				mdelay(__div_round_up64(wait, USEC_PER_MSEC));
+		} else {
+			usleep_range(wait, wait + (wait >> 2)); /* range till wait + 25% */
+		}
+		/*
+		 * As we keep doubling the wait time for every check that fails, cap the
+		 * in-loop delay-or-sleep to less than 2x 100 milliseconds to prevent from
+		 * expanding 'wait' into exponentially longer wait times per loop that
+		 * end up delaying the next completion check way later than tolerable.
+		 */
+		wait = wait < __XE_MMIO_WAIT_MAX_INLOOP_100MS >> 1 ?
+		       wait << 1 : __XE_MMIO_WAIT_MAX_INLOOP_100MS;
 	}
 
 	if (ret != 0) {
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/xe/mmio: Fix xe_mmio_wait32() to honor delay/sleep maximums
  2026-09-08 18:56 Alan Previn
@ 2026-09-08 19:03 ` sashiko-bot
  2026-09-09  8:00 ` Jani Nikula
  1 sibling, 0 replies; 7+ messages in thread
From: sashiko-bot @ 2026-09-08 19:03 UTC (permalink / raw)
  To: Alan Previn; +Cc: dri-devel, intel-xe

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The custom helper `__div_round_up64` silently truncates 64-bit divisors to 32 bits, creating a mathematically unsound helper.
- [Low] Inverted architecture check for 64-bit division causes a `__divdi3` link error on 32-bit builds, and needlessly uses a custom fallback on 64-bit builds.
--

commit 2d4751e6d3870228fa27b4fdbbb10070a48ae9ce
Author: Alan Previn <alan.previn.teres.alexis@intel.com>

drm/xe/mmio: Fix xe_mmio_wait32() to honor delay/sleep maximums

This commit fixes xe_mmio_wait32() to honor delay and sleep maximums by
checking for overflow in udelay and usleep_range. It also adds a custom
64-bit division helper and caps the in-loop wait time to prevent
intolerably large delays.

> diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
> index 7fa18dfcb5a28..1ae382f009de5 100644
> --- a/drivers/gpu/drm/xe/xe_mmio.c
> +++ b/drivers/gpu/drm/xe/xe_mmio.c
[ ... ]
> +static inline s64 __div_round_up64(s64 dividend, s64 divisor)
> +{
> +	u64 abs_dividend = abs(dividend);
> +	u64 abs_divisor = abs(divisor);
> +	u64 result = abs_dividend + abs_divisor - 1;
> +
> +	do_div(result, abs_divisor);

[Severity: Medium]
Does passing a 64-bit divisor to do_div() cause silent truncation here?

The do_div() macro is strictly designed for 32-bit divisors and will
silently truncate the 64-bit abs_divisor parameter. While the current caller
in __xe_mmio_wait32() only passes USEC_PER_MSEC (which fits in 32 bits),
creating a generic helper that accepts an s64 divisor is structurally
unsafe.

Could this use DIV64_U64_ROUND_UP() from linux/math64.h instead?

> +	/* dont check for negative values as local caller only uses positive numbers */
> +	return (s64)result;
> +}
[ ... ]
> @@ -349,11 +375,25 @@ static int __xe_mmio_wait32(struct xe_mmio *mmio, struct xe_reg reg, u32 mask, u
[ ... ]
> -		if (atomic)
> -			udelay(wait);
> -		else
> -			usleep_range(wait, wait << 1);
> -		wait <<= 1;
> +#define __XE_MMIO_WAIT_MAX_INLOOP_100MS (100 * USEC_PER_MSEC)
> +		if (atomic) {
> +			if (wait <= MAX_UDELAY_MS * USEC_PER_MSEC)
> +				udelay(wait);
> +			else if (BITS_PER_LONG == 32)
> +				mdelay(DIV_ROUND_UP(wait, USEC_PER_MSEC));

[Severity: Low]
Does this architecture check invert the intended 64-bit division logic?

When BITS_PER_LONG == 32, this branch executes the native 64-bit division
DIV_ROUND_UP(wait, USEC_PER_MSEC). This causes a __divdi3 unresolved symbol
link error on 32-bit builds, which appears to be the exact build break this
helper was intended to avoid.

> +			else
> +				mdelay(__div_round_up64(wait, USEC_PER_MSEC));

Consequently, 64-bit builds will take this else branch and needlessly use
the custom division helper instead of the native division macro.

> +		} else {
> +			usleep_range(wait, wait + (wait >> 2)); /* range till wait + 25% */
> +		}
[ ... ]

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH] drm/xe/mmio: Fix xe_mmio_wait32() to honor delay/sleep maximums
@ 2026-09-08 20:33 Alan Previn
  0 siblings, 0 replies; 7+ messages in thread
From: Alan Previn @ 2026-09-08 20:33 UTC (permalink / raw)
  To: intel-xe; +Cc: Alan Previn, dri-devel, Matt Roper

Check for overflow in udelay/usleep_range use in __xe_mmio_wait32
and pick the correct helper according to the wait time and atomic.
Implement a similar helper to replace DIV_ROUND_UP for 32-bit CPUs.
Avoid growing delays becoming intollerably large by capping the
in-loop wait time.

v3: - Use div64_s64 kernel API for 32-bit vs 64-bit division.
v2: - Fixed checkpatch failure.
    - Added helper for 64-bit DIV_ROUND_UP on 32-bit CPU (Shasiko
      review). Fixed bug max range in usleep_range(Shasiko review)

Fixes: 5c09bd6ccd41 ("drm/xe/mmio: Move xe_mmio_wait32() to xe_mmio.c")
Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
Assisted-by: Github-Copilot:Claude-Sonnet-5-0
---
 drivers/gpu/drm/xe/xe_mmio.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
index 7fa18dfcb5a2..507095f33cec 100644
--- a/drivers/gpu/drm/xe/xe_mmio.c
+++ b/drivers/gpu/drm/xe/xe_mmio.c
@@ -7,6 +7,7 @@
 
 #include <linux/delay.h>
 #include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/math64.h>
 #include <linux/minmax.h>
 #include <linux/pci.h>
 
@@ -349,11 +350,23 @@ static int __xe_mmio_wait32(struct xe_mmio *mmio, struct xe_reg reg, u32 mask, u
 		if (ktime_after(ktime_add_us(cur, wait), end))
 			wait = ktime_us_delta(end, cur);
 
-		if (atomic)
-			udelay(wait);
-		else
-			usleep_range(wait, wait << 1);
-		wait <<= 1;
+#define __XE_MMIO_WAIT_MAX_INLOOP_100MS (100 * USEC_PER_MSEC)
+		if (atomic) {
+			if (wait <= MAX_UDELAY_MS * USEC_PER_MSEC)
+				udelay(wait);
+			else
+				mdelay(div64_s64(wait, USEC_PER_MSEC));
+		} else {
+			usleep_range(wait, wait + (wait >> 2)); /* range till wait + 25% */
+		}
+		/*
+		 * As we keep doubling the wait time for every check that fails, cap the
+		 * in-loop delay-or-sleep to less than 2x 100 milliseconds to prevent from
+		 * expanding 'wait' into exponentially longer wait times per loop that
+		 * end up delaying the next completion check way later than tolerable.
+		 */
+		wait = wait < __XE_MMIO_WAIT_MAX_INLOOP_100MS >> 1 ?
+		       wait << 1 : __XE_MMIO_WAIT_MAX_INLOOP_100MS;
 	}
 
 	if (ret != 0) {
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/xe/mmio: Fix xe_mmio_wait32() to honor delay/sleep maximums
  2026-09-08 18:56 Alan Previn
  2026-09-08 19:03 ` sashiko-bot
@ 2026-09-09  8:00 ` Jani Nikula
  2026-09-11  0:05   ` Teres Alexis, Alan Previn
  1 sibling, 1 reply; 7+ messages in thread
From: Jani Nikula @ 2026-09-09  8:00 UTC (permalink / raw)
  To: Alan Previn, intel-xe; +Cc: Alan Previn, dri-devel, Matt Roper

On Tue, 08 Sep 2026, Alan Previn <alan.previn.teres.alexis@intel.com> wrote:
> Check for overflow in udelay/usleep_range use in __xe_mmio_wait32
> and pick the correct helper according to the wait time and atomic.
> Implement a similar helper to replace DIV_ROUND_UP for 32-bit CPUs.
> Avoid growing delays becoming intollerably large by capping the
> in-loop wait time.

I think it might be a better idea to remove the exponentially growing
wait time instead. It might have seemed good on paper, but you should do
the math in typical use cases and see how it actually behaves. (Spoiler:
Lots of useless back-to-back reads in the beginning, and then quickly
increasing to waits beyond the timeout and delay maximums.)

This is code that should be possible to understand. I'd suggest
simplifying the whole thing instead of making it more complex.

I think it would be much better to migrate xe mmio to use
poll_timeout_us() and poll_timeout_us_atomic() instead. For the rare
cases that actually need the quick hammering followed by slower waits,
you can trivially do two calls with delays and timeouts considered for
*that* particular use case.

I also think it's a mistake (copy-pasted from i915) to specify "atomic"
as a parameter. They should be separate functions all the way from the
caller.


BR,
Jani.


>
> v2: - Fixed checkpatch failure.
>     - Added helper for 64-bit DIV_ROUND_UP on 32-bit CPU (Shasiko
>       review). Fixed bug max range in usleep_range(Shasiko review)
>
> Fixes: 5c09bd6ccd41 ("drm/xe/mmio: Move xe_mmio_wait32() to xe_mmio.c")
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> Assisted-by: Github-Copilot:Claude-Sonnet-5-0
> ---
>  drivers/gpu/drm/xe/xe_mmio.c | 50 ++++++++++++++++++++++++++++++++----
>  1 file changed, 45 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
> index 7fa18dfcb5a2..1ae382f009de 100644
> --- a/drivers/gpu/drm/xe/xe_mmio.c
> +++ b/drivers/gpu/drm/xe/xe_mmio.c
> @@ -7,6 +7,8 @@
>  
>  #include <linux/delay.h>
>  #include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/math.h>
> +#include <linux/math64.h>
>  #include <linux/minmax.h>
>  #include <linux/pci.h>
>  
> @@ -320,6 +322,30 @@ u64 xe_mmio_read64_2x32(struct xe_mmio *mmio, struct xe_reg reg)
>  	return (u64)udw << 32 | ldw;
>  }
>  
> +/**
> + * __div_round_up64() - alternative to DIV_ROUND_UP for use by __xe_mmio_wait32
> + * @dividend: 64 bit positive number to divide
> + * @divisor: 64 bit positive divisor
> + *
> + * DIV_ROUND_UP() relies on plain '/' and '%' operators, which for 64-bit
> + * operands on a 32-bit CPU get turned into calls to libgcc's __divdi3()/
> + * __moddi3(), routines the kernel does not link against. Provide a
> + * do_div()-based equivalent that works for signed 64-bit inputs on any
> + * architecture.
> + *
> + * Returns: rounded up division result
> + */
> +static inline s64 __div_round_up64(s64 dividend, s64 divisor)
> +{
> +	u64 abs_dividend = abs(dividend);
> +	u64 abs_divisor = abs(divisor);
> +	u64 result = abs_dividend + abs_divisor - 1;
> +
> +	do_div(result, abs_divisor);
> +	/* dont check for negative values as local caller only uses positive numbers */
> +	return (s64)result;
> +}
> +
>  static int __xe_mmio_wait32(struct xe_mmio *mmio, struct xe_reg reg, u32 mask, u32 val,
>  			    u32 timeout_us, u32 *out_val, bool atomic, bool expect_match)
>  {
> @@ -349,11 +375,25 @@ static int __xe_mmio_wait32(struct xe_mmio *mmio, struct xe_reg reg, u32 mask, u
>  		if (ktime_after(ktime_add_us(cur, wait), end))
>  			wait = ktime_us_delta(end, cur);
>  
> -		if (atomic)
> -			udelay(wait);
> -		else
> -			usleep_range(wait, wait << 1);
> -		wait <<= 1;
> +#define __XE_MMIO_WAIT_MAX_INLOOP_100MS (100 * USEC_PER_MSEC)
> +		if (atomic) {
> +			if (wait <= MAX_UDELAY_MS * USEC_PER_MSEC)
> +				udelay(wait);
> +			else if (BITS_PER_LONG == 32)
> +				mdelay(DIV_ROUND_UP(wait, USEC_PER_MSEC));
> +			else
> +				mdelay(__div_round_up64(wait, USEC_PER_MSEC));
> +		} else {
> +			usleep_range(wait, wait + (wait >> 2)); /* range till wait + 25% */
> +		}
> +		/*
> +		 * As we keep doubling the wait time for every check that fails, cap the
> +		 * in-loop delay-or-sleep to less than 2x 100 milliseconds to prevent from
> +		 * expanding 'wait' into exponentially longer wait times per loop that
> +		 * end up delaying the next completion check way later than tolerable.
> +		 */
> +		wait = wait < __XE_MMIO_WAIT_MAX_INLOOP_100MS >> 1 ?
> +		       wait << 1 : __XE_MMIO_WAIT_MAX_INLOOP_100MS;
>  	}
>  
>  	if (ret != 0) {

-- 
Jani Nikula, Intel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/xe/mmio: Fix xe_mmio_wait32() to honor delay/sleep maximums
  2026-09-09  8:00 ` Jani Nikula
@ 2026-09-11  0:05   ` Teres Alexis, Alan Previn
  0 siblings, 0 replies; 7+ messages in thread
From: Teres Alexis, Alan Previn @ 2026-09-11  0:05 UTC (permalink / raw)
  To: intel-xe@lists.freedesktop.org, jani.nikula@linux.intel.com
  Cc: dri-devel@lists.freedesktop.org, Roper, Matthew D

On Wed, 2026-09-09 at 11:00 +0300, Jani Nikula wrote:
> On Tue, 08 Sep 2026, Alan Previn <alan.previn.teres.alexis@intel.com> wrote:
> > Check for overflow in udelay/usleep_range use in __xe_mmio_wait32
> > and pick the correct helper according to the wait time and atomic.
> > Implement a similar helper to replace DIV_ROUND_UP for 32-bit CPUs.
> > Avoid growing delays becoming intollerably large by capping the
> > in-loop wait time.
> 
> I think it might be a better idea to remove the exponentially growing
> wait time instead. It might have seemed good on paper, but you should do
> the math in typical use cases and see how it actually behaves. (Spoiler:
> Lots of useless back-to-back reads in the beginning, and then quickly
> increasing to waits beyond the timeout and delay maximums.)
> 
> This is code that should be possible to understand. I'd suggest
> simplifying the whole thing instead of making it more complex.
> 
> I think it would be much better to migrate xe mmio to use
> poll_timeout_us() and poll_timeout_us_atomic() instead. For the rare
> cases that actually need the quick hammering followed by slower waits,
> you can trivially do two calls with delays and timeouts considered for
> *that* particular use case.

alan: right - will do.
> 
> I also think it's a mistake (copy-pasted from i915) to specify "atomic"
> as a parameter. They should be separate functions all the way from the
> caller.
> 
alan: not sure i understand why this is a mistake - the caller needs to
ensure they call with the correct parameter - so its either they call with
the correct parameter or the correct suffix version of the xe_mmio_wait
(if we decide to go with separate functions). Its basically the same no?
In any case, sure - will do that too.

> 
> BR,
> Jani.
> 
> 
> > 
> > v2: - Fixed checkpatch failure.
> >     - Added helper for 64-bit DIV_ROUND_UP on 32-bit CPU (Shasiko
> >       review). Fixed bug max range in usleep_range(Shasiko review)
> > 
> > Fixes: 5c09bd6ccd41 ("drm/xe/mmio: Move xe_mmio_wait32() to xe_mmio.c")
> > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> > Assisted-by: Github-Copilot:Claude-Sonnet-5-0
> > ---
> >  drivers/gpu/drm/xe/xe_mmio.c | 50 ++++++++++++++++++++++++++++++++----
> >  1 file changed, 45 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
> > index 7fa18dfcb5a2..1ae382f009de 100644
> > --- a/drivers/gpu/drm/xe/xe_mmio.c
> > +++ b/drivers/gpu/drm/xe/xe_mmio.c
> > @@ -7,6 +7,8 @@
> >  
> >  #include <linux/delay.h>
> >  #include <linux/io-64-nonatomic-lo-hi.h>
> > +#include <linux/math.h>
> > +#include <linux/math64.h>
> >  #include <linux/minmax.h>
> >  #include <linux/pci.h>
> >  
> > @@ -320,6 +322,30 @@ u64 xe_mmio_read64_2x32(struct xe_mmio *mmio, struct xe_reg reg)
> >  	return (u64)udw << 32 | ldw;
> >  }
> >  
> > +/**
> > + * __div_round_up64() - alternative to DIV_ROUND_UP for use by __xe_mmio_wait32
> > + * @dividend: 64 bit positive number to divide
> > + * @divisor: 64 bit positive divisor
> > + *
> > + * DIV_ROUND_UP() relies on plain '/' and '%' operators, which for 64-bit
> > + * operands on a 32-bit CPU get turned into calls to libgcc's __divdi3()/
> > + * __moddi3(), routines the kernel does not link against. Provide a
> > + * do_div()-based equivalent that works for signed 64-bit inputs on any
> > + * architecture.
> > + *
> > + * Returns: rounded up division result
> > + */
> > +static inline s64 __div_round_up64(s64 dividend, s64 divisor)
> > +{
> > +	u64 abs_dividend = abs(dividend);
> > +	u64 abs_divisor = abs(divisor);
> > +	u64 result = abs_dividend + abs_divisor - 1;
> > +
> > +	do_div(result, abs_divisor);
> > +	/* dont check for negative values as local caller only uses positive numbers */
> > +	return (s64)result;
> > +}
> > +
> >  static int __xe_mmio_wait32(struct xe_mmio *mmio, struct xe_reg reg, u32 mask, u32 val,
> >  			    u32 timeout_us, u32 *out_val, bool atomic, bool expect_match)
> >  {
> > @@ -349,11 +375,25 @@ static int __xe_mmio_wait32(struct xe_mmio *mmio, struct xe_reg reg, u32 mask, u
> >  		if (ktime_after(ktime_add_us(cur, wait), end))
> >  			wait = ktime_us_delta(end, cur);
> >  
> > -		if (atomic)
> > -			udelay(wait);
> > -		else
> > -			usleep_range(wait, wait << 1);
> > -		wait <<= 1;
> > +#define __XE_MMIO_WAIT_MAX_INLOOP_100MS (100 * USEC_PER_MSEC)
> > +		if (atomic) {
> > +			if (wait <= MAX_UDELAY_MS * USEC_PER_MSEC)
> > +				udelay(wait);
> > +			else if (BITS_PER_LONG == 32)
> > +				mdelay(DIV_ROUND_UP(wait, USEC_PER_MSEC));
> > +			else
> > +				mdelay(__div_round_up64(wait, USEC_PER_MSEC));
> > +		} else {
> > +			usleep_range(wait, wait + (wait >> 2)); /* range till wait + 25% */
> > +		}
> > +		/*
> > +		 * As we keep doubling the wait time for every check that fails, cap the
> > +		 * in-loop delay-or-sleep to less than 2x 100 milliseconds to prevent from
> > +		 * expanding 'wait' into exponentially longer wait times per loop that
> > +		 * end up delaying the next completion check way later than tolerable.
> > +		 */
> > +		wait = wait < __XE_MMIO_WAIT_MAX_INLOOP_100MS >> 1 ?
> > +		       wait << 1 : __XE_MMIO_WAIT_MAX_INLOOP_100MS;
> >  	}
> >  
> >  	if (ret != 0) {
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2026-09-11  0:05 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-07 23:55 [PATCH] drm/xe/mmio: Fix xe_mmio_wait32() to honor delay/sleep maximums Alan Previn
2026-09-08  0:02 ` sashiko-bot
  -- strict thread matches above, loose matches on Subject: below --
2026-09-08 18:56 Alan Previn
2026-09-08 19:03 ` sashiko-bot
2026-09-09  8:00 ` Jani Nikula
2026-09-11  0:05   ` Teres Alexis, Alan Previn
2026-09-08 20:33 Alan Previn

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox