[PATCH 1/1] drm/xe/pcode: Fix pcode_try_request polling delays

[PATCH 1/1] drm/xe/pcode: Fix pcode_try_request polling delays

[Alan Previn]

Honor OOB max for udelay. Also add a backoff for 'wait'
as it exponentially increases so that doesn't overflow
and also caps to a sane limit so we dont suffer overtly
extended wait times as we progress.

Fixes: 7dc9b92dcfef ("drm/xe: Remove i915_utils dependency from xe_pcode.")
Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
 drivers/gpu/drm/xe/xe_pcode.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xe/xe_pcode.c b/drivers/gpu/drm/xe/xe_pcode.c
index 266deecbb100..25d1a354ee36 100644
--- a/drivers/gpu/drm/xe/xe_pcode.c
+++ b/drivers/gpu/drm/xe/xe_pcode.c
@@ -146,6 +146,7 @@ int xe_pcode_read(struct xe_tile *tile, u32 mbox, u32 *val0, u32 *val1)
 	return err;
 }
 
+#define __XE_MMIO_WAIT_MAX_BACKOFF_100MS (100 * USEC_PER_MSEC)
 static int pcode_try_request(struct xe_tile *tile, u32 mbox,
 			     u32 request, u32 reply_mask, u32 reply,
 			     int *status, bool atomic, int timeout_us, bool locked)
@@ -165,10 +166,11 @@ static int pcode_try_request(struct xe_tile *tile, u32 mbox,
 			return 0;
 
 		if (atomic)
-			udelay(wait);
+			udelay(min_t(int, wait, (MAX_UDELAY_MS * 1000)));
 		else
 			usleep_range(wait, wait << 1);
 		wait <<= 1;
+		wait = min_t(int, wait << 1, __XE_MMIO_WAIT_MAX_BACKOFF_100MS);
 	}
 
 	return -ETIMEDOUT;

base-commit: d8a353e238652bec29fd452babdaae0044d30b9b
-- 
2.43.0

Re: [PATCH 1/1] drm/xe/pcode: Fix pcode_try_request polling delays

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/xe/pcode: duplicate left shift of wait in pcode_try_request()
- [High] drm/xe/pcode: uncapped wait added to slept in pcode_try_request()
--

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

drm/xe/pcode: Fix pcode_try_request polling delays

This commit clamps the atomic udelay() argument in pcode_try_request() to
MAX_UDELAY_MS * 1000 and adds a 100 ms upper bound to the exponential
backoff wait interval.

> diff --git a/drivers/gpu/drm/xe/xe_pcode.c b/drivers/gpu/drm/xe/xe_pcode.c
> index 266deecbb100a..25d1a354ee369 100644
> --- a/drivers/gpu/drm/xe/xe_pcode.c
> +++ b/drivers/gpu/drm/xe/xe_pcode.c
> @@ -146,6 +146,7 @@ int xe_pcode_read(struct xe_tile *tile, u32 mbox, u32 *val0, u32 *val1)
>  	return err;
>  }
>  
> +#define __XE_MMIO_WAIT_MAX_BACKOFF_100MS (100 * USEC_PER_MSEC)
>  static int pcode_try_request(struct xe_tile *tile, u32 mbox,
>  			     u32 request, u32 reply_mask, u32 reply,
>  			     int *status, bool atomic, int timeout_us, bool locked)
> @@ -165,10 +166,11 @@ static int pcode_try_request(struct xe_tile *tile, u32 mbox,
>  			return 0;
>  
>  		if (atomic)
> -			udelay(wait);
> +			udelay(min_t(int, wait, (MAX_UDELAY_MS * 1000)));
>  		else
>  			usleep_range(wait, wait << 1);
>  		wait <<= 1;
> +		wait = min_t(int, wait << 1, __XE_MMIO_WAIT_MAX_BACKOFF_100MS);

[Severity: High]
Should the existing wait <<= 1 statement above be removed? Leaving both
shifts in pcode_try_request() shifts wait left twice on every iteration,
quadrupling wait at each step (10 -> 40 -> 160 -> 640 -> 2560 us) instead
of doubling it.

Because the loop header accumulates elapsed time via slept += wait after
the loop body finishes:

drivers/gpu/drm/xe/xe_pcode.c:pcode_try_request() {
	for (slept = 0; slept < timeout_us; slept += wait) {

slept also advances by 4x the delay just performed, causing
xe_pcode_ready() and xe_pcode_request() to time out with -ETIMEDOUT before
timeout_us has actually elapsed.

[Severity: High]
When atomic is true, udelay() is clamped to MAX_UDELAY_MS * 1000 (for
example, 5000 us on x86 or 2000 us on arm64), but wait continues to grow
up to __XE_MMIO_WAIT_MAX_BACKOFF_100MS (100000 us) and is added directly
to slept in the loop header:

drivers/gpu/drm/xe/xe_pcode.c:pcode_try_request() {
	for (slept = 0; slept < timeout_us; slept += wait) {

When xe_pcode_request() retries with preemption disabled:

drivers/gpu/drm/xe/xe_pcode.c:xe_pcode_request() {
	preempt_disable();
	ret = pcode_try_request(tile, mbox, request, reply_mask, reply, &status,
				true, 50 * 1000, true);
	preempt_enable();

won't slept accumulate the unclamped wait value once wait exceeds
MAX_UDELAY_MS * 1000, causing the 50 ms atomic retry to return -ETIMEDOUT
after only a fraction of the intended 50 ms delay?

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

Re: [PATCH 1/1] drm/xe/pcode: Fix pcode_try_request polling delays

[Teres Alexis, Alan Previn]

Both of shashiko's comments are valid - will fix it now.
Will also rename the backoff definition from __XE_MMIO_WAIT_MAX_BACKOFF_100MS 
to __XE_PCODE_WAIT_MAX_BACKOFF_100MS (copy and paste mistake)

...alan

[PATCH 1/1] drm/xe/pcode: Fix pcode_try_request polling delays

[Alan Previn]

Honor OOB max for udelay. Also add a backoff for 'wait'
as it exponentially increases so that doesn't overflow
and also caps to a sane limit so we dont suffer overtly
extended wait times as we progress.

v2: Fix accumnulation of 'wait' so 'slept' is accurate and
    accounts for atomic-udelay clamping. (Sashiko)

Fixes: 7dc9b92dcfef ("drm/xe: Remove i915_utils dependency from xe_pcode.")
Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
 drivers/gpu/drm/xe/xe_pcode.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_pcode.c b/drivers/gpu/drm/xe/xe_pcode.c
index 266deecbb100..d2ed5edf003e 100644
--- a/drivers/gpu/drm/xe/xe_pcode.c
+++ b/drivers/gpu/drm/xe/xe_pcode.c
@@ -146,6 +146,7 @@ int xe_pcode_read(struct xe_tile *tile, u32 mbox, u32 *val0, u32 *val1)
 	return err;
 }
 
+#define __XE_PCODE_WAIT_MAX_BACKOFF_100MS (100 * USEC_PER_MSEC)
 static int pcode_try_request(struct xe_tile *tile, u32 mbox,
 			     u32 request, u32 reply_mask, u32 reply,
 			     int *status, bool atomic, int timeout_us, bool locked)
@@ -164,11 +165,14 @@ static int pcode_try_request(struct xe_tile *tile, u32 mbox,
 		if ((*status == 0) && ((request & reply_mask) == reply))
 			return 0;
 
-		if (atomic)
-			udelay(wait);
-		else
+		wait = min_t(int, wait << 1, __XE_PCODE_WAIT_MAX_BACKOFF_100MS);
+		if (atomic) {
+			/* don't overflow udelay */
+			wait = min_t(int, wait, (MAX_UDELAY_MS * 1000);
+			udelay(wait));
+		} else {
 			usleep_range(wait, wait << 1);
-		wait <<= 1;
+		}
 	}
 
 	return -ETIMEDOUT;

base-commit: 2cd78713d917a02987f21b437561a5e1e13ddda2
-- 
2.43.0

✗ CI.KUnit: failure for series starting with [1/1] drm/xe/pcode: Fix pcode_try_request polling delays

[Patchwork]

== Series Details ==

Series: series starting with [1/1] drm/xe/pcode: Fix pcode_try_request polling delays
URL   : https://patchwork.freedesktop.org/series/177492/
State : failure

== Summary ==

+ trap cleanup EXIT
+ kunitconfigs=('/kernel/drivers/gpu/tests/.kunitconfig' '/kernel/drivers/gpu/drm/xe/.kunitconfig' '/kernel/drivers/gpu/drm/tests/.kunitconfig' '/kernel/drivers/gpu/drm/ttm/tests/.kunitconfig' '/kernel/drivers/dma-buf/.kunitconfig')
+ for kcfg in "${kunitconfigs[@]}"
+ [[ ! -f /kernel/drivers/gpu/tests/.kunitconfig ]]
+ /kernel/tools/testing/kunit/kunit.py run --kunitconfig /kernel/drivers/gpu/tests/.kunitconfig
[18:19:01] Configuring KUnit Kernel ...
Generating .config ...
Populating config with:
$ make ARCH=um O=.kunit olddefconfig
[18:19:07] Building KUnit Kernel ...
Populating config with:
$ make ARCH=um O=.kunit olddefconfig
Building with:
$ make all compile_commands.json scripts_gdb ARCH=um O=.kunit --jobs=48
[18:19:27] Starting KUnit Kernel (1/1)...
[18:19:27] ============================================================
Running tests with:
$ .kunit/linux kunit.enable=1 mem=1G console=tty kunit_shutdown=halt
[18:19:27] ================= gpu_buddy (14 subtests) ==================
[18:19:27] [PASSED] gpu_test_buddy_alloc_limit
[18:19:27] [PASSED] gpu_test_buddy_alloc_optimistic
[18:19:27] [PASSED] gpu_test_buddy_alloc_pessimistic
[18:19:27] [PASSED] gpu_test_buddy_alloc_pathological
[18:19:27] [PASSED] gpu_test_buddy_alloc_contiguous
[18:19:27] [PASSED] gpu_test_buddy_alloc_clear
[18:19:27] [PASSED] gpu_test_buddy_alloc_range
[18:19:28] [PASSED] gpu_test_buddy_alloc_range_bias
[18:19:29] [PASSED] gpu_test_buddy_fragmentation_performance
[18:19:29] [PASSED] gpu_test_buddy_dirty_tracker_performance
[18:19:29] [PASSED] gpu_test_buddy_alloc_exceeds_max_order
[18:19:29] [PASSED] gpu_test_buddy_offset_aligned_allocation
[18:19:29] [PASSED] gpu_test_buddy_subtree_offset_alignment_stress
[18:19:29] [PASSED] gpu_test_buddy_addr_to_block
[18:19:29] ==================== [PASSED] gpu_buddy ====================
[18:19:29] ============================================================
[18:19:29] Testing complete. Ran 14 tests: passed: 14
[18:19:29] Elapsed time: 28.058s total, 5.172s configuring, 20.818s building, 2.034s running

+ for kcfg in "${kunitconfigs[@]}"
+ [[ ! -f /kernel/drivers/gpu/drm/xe/.kunitconfig ]]
+ /kernel/tools/testing/kunit/kunit.py run --kunitconfig /kernel/drivers/gpu/drm/xe/.kunitconfig
ERROR:root:In file included from ../include/linux/nodemask.h:92,
                 from ../include/linux/numa.h:6,
                 from ../include/linux/cpumask.h:15,
                 from ../include/linux/smp.h:13,
                 from ../include/linux/interrupt_rc.h:18,
                 from ../include/linux/spinlock.h:60,
                 from ../include/linux/sched.h:38,
                 from ../include/linux/delay.h:13,
                 from ../drivers/gpu/drm/xe/xe_pcode.c:8:
../drivers/gpu/drm/xe/xe_pcode.c: In function ‘pcode_try_request’:
../drivers/gpu/drm/xe/xe_pcode.c:171:71: error: expected ‘)’ before ‘;’ token
  171 |                         wait = min_t(int, wait, (MAX_UDELAY_MS * 1000);
      |                                                                       ^
../include/linux/minmax.h:86:38: note: in definition of macro ‘__cmp_once_unique’
   86 |         ({ type ux = (x); type uy = (y); __cmp(op, ux, uy); })
      |                                      ^
../include/linux/minmax.h:161:27: note: in expansion of macro ‘__cmp_once’
  161 | #define min_t(type, x, y) __cmp_once(min, type, x, y)
      |                           ^~~~~~~~~~
../drivers/gpu/drm/xe/xe_pcode.c:171:32: note: in expansion of macro ‘min_t’
  171 |                         wait = min_t(int, wait, (MAX_UDELAY_MS * 1000);
      |                                ^~~~~
../include/linux/minmax.h:86:37: note: to match this ‘(’
   86 |         ({ type ux = (x); type uy = (y); __cmp(op, ux, uy); })
      |                                     ^
../include/linux/minmax.h:89:9: note: in expansion of macro ‘__cmp_once_unique’
   89 |         __cmp_once_unique(op, type, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_))
      |         ^~~~~~~~~~~~~~~~~
../include/linux/minmax.h:161:27: note: in expansion of macro ‘__cmp_once’
  161 | #define min_t(type, x, y) __cmp_once(min, type, x, y)
      |                           ^~~~~~~~~~
../drivers/gpu/drm/xe/xe_pcode.c:171:32: note: in expansion of macro ‘min_t’
  171 |                         wait = min_t(int, wait, (MAX_UDELAY_MS * 1000);
      |                                ^~~~~
make[7]: *** [../scripts/Makefile.build:290: drivers/gpu/drm/xe/xe_pcode.o] Error 1
make[7]: *** Waiting for unfinished jobs....
make[6]: *** [../scripts/Makefile.build:572: drivers/gpu/drm/xe] Error 2
make[5]: *** [../scripts/Makefile.build:572: drivers/gpu/drm] Error 2
make[4]: *** [../scripts/Makefile.build:572: drivers/gpu] Error 2
make[3]: *** [../scripts/Makefile.build:572: drivers] Error 2
make[3]: *** Waiting for unfinished jobs....
make[2]: *** [/kernel/Makefile:2244: .] Error 2
make[1]: *** [/kernel/Makefile:248: __sub-make] Error 2
make: *** [Makefile:248: __sub-make] Error 2

[18:19:30] Configuring KUnit Kernel ...
Regenerating .config ...
Populating config with:
$ make ARCH=um O=.kunit olddefconfig
[18:19:32] Building KUnit Kernel ...
Populating config with:
$ make ARCH=um O=.kunit olddefconfig
Building with:
$ make all compile_commands.json scripts_gdb ARCH=um O=.kunit --jobs=48
+ cleanup
++ stat -c %u:%g /kernel
+ chown -R 1003:1003 /kernel

Re: [PATCH 1/1] drm/xe/pcode: Fix pcode_try_request polling delays

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/xe/pcode: syntax error in pcode_try_request() due to misplaced parenthesis
--

commit 3c7a166635d4e14258beaf50fb0fab91cf40aa3f
Author: Alan Previn <alan.previn.teres.alexis@intel.com>

drm/xe/pcode: Fix pcode_try_request polling delays

This commit caps the exponential backoff delay in pcode_try_request() to 100ms
and clamps the wait duration to the maximum allowed udelay value when polling
in atomic context.

> diff --git a/drivers/gpu/drm/xe/xe_pcode.c b/drivers/gpu/drm/xe/xe_pcode.c
> index 266deecbb100a..d2ed5edf003e3 100644
> --- a/drivers/gpu/drm/xe/xe_pcode.c
> +++ b/drivers/gpu/drm/xe/xe_pcode.c
> @@ -146,6 +146,7 @@ int xe_pcode_read(struct xe_tile *tile, u32 mbox, u32 *val0, u32 *val1)
>  	return err;
>  }
>  
> +#define __XE_PCODE_WAIT_MAX_BACKOFF_100MS (100 * USEC_PER_MSEC)
>  static int pcode_try_request(struct xe_tile *tile, u32 mbox,
>  			     u32 request, u32 reply_mask, u32 reply,
>  			     int *status, bool atomic, int timeout_us, bool locked)
> @@ -164,11 +165,14 @@ static int pcode_try_request(struct xe_tile *tile, u32 mbox,
>  		if ((*status == 0) && ((request & reply_mask) == reply))
>  			return 0;
>  
> -		if (atomic)
> -			udelay(wait);
> -		else
> +		wait = min_t(int, wait << 1, __XE_PCODE_WAIT_MAX_BACKOFF_100MS);
> +		if (atomic) {
> +			/* don't overflow udelay */
> +			wait = min_t(int, wait, (MAX_UDELAY_MS * 1000);
> +			udelay(wait));

[Severity: High]
Does this break compilation of drivers/gpu/drm/xe/xe_pcode.c? In the atomic
branch of pcode_try_request(), the closing parenthesis for min_t() is missing
before the semicolon and appears after udelay(wait) instead:

			wait = min_t(int, wait, (MAX_UDELAY_MS * 1000));
			udelay(wait);

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