Re: [PATCH i-g-t] tests/amdgpu: insert pause mnemonic only on x86 / x86_64 platforms

[vitaly prosyak <vprosyak@amd.com>]

[-- Attachment #1: Type: text/plain, Size: 3022 bytes --]

Hi Kamil,

Thanks for catching the incorrect macro names.

As an alternative to correcting the guard, I've prepared a patch removing
the optional x86 pause instruction and its architecture guard from
wait_for_value64().

The sleep path, volatile polling reads, and timeout handling remain
unchanged. When no polling interval is requested, the function continues
busy-waiting without a CPU hint.

This simplifies the helper and removes its x86 assembly dependency, at the
cost of losing the spin-wait optimization on x86. I verified that amd_basic
builds on x86_64; ARM/AArch64 cross-compilation remains untested.

Would you accept this approach instead of correcting the macro names?

Regards,
Vitaly


On 2026-10-20 15:31, Kamil Konieczny wrote:
> Hi vitaly,
> On 2026-10-06 at 19:08:58 -0400, vitaly prosyak wrote:
>> Thanks Dmitry!
>>
>> Reviewed-by: Vitaly Prosyak <vitaly.prosyak@amd.com>
>>
>> On 2026-10-06 10:29, Kamil Konieczny wrote:
>>> Hi Dmitry,
>>> On 2026-10-02 at 22:14:01 +0300, Dmitry Baryshkov wrote:
>>>> The "pause" assembly mnemonic is only available on IA-32 platforms,
>>>> breaking e.g. debug IGT builds on ARM / AArch64. The issue manifests
>>>> only with -O1 or -O0, the default -O2 seems to optimize the condition
>>>> away.
>>>>
>>>> Guard this inline assembly with the preprocessor checks, making sure
>>>> that other platforms don't see the invalid assembly.
>>>>
>>>> FAILED: [code=1] tests/amdgpu/amd_basic.p/amd_basic.c.o
>>>> Assembler messages:
>>>> {standard input}:3702: Error: unknown mnemonic `pause' -- `pause'
>>>>
>>> Please add in Cc amdgpu devs, found by git log -- testname.c
>>> here
>>> Cc: Jesse Zhang <jesse.zhang@amd.com>
>>> Cc: Vitaly Prosyak <vitaly.prosyak@amd.com>
>>>
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
>>> LGTM
>>> Reviewed-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
>>>
>>>
>>> Regards,
>>> Kamil
>>>
>>>> ---
>>>>  tests/amdgpu/amd_basic.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/tests/amdgpu/amd_basic.c b/tests/amdgpu/amd_basic.c
>>>> index 418db286f81f..494934c1c321 100644
>>>> --- a/tests/amdgpu/amd_basic.c
>>>> +++ b/tests/amdgpu/amd_basic.c
>>>> @@ -705,8 +705,10 @@ static int wait_for_value64(volatile uint64_t *ptr, uint64_t expected,
>>>>  		/* Sleep if interval specified, otherwise tight loop */
>>>>  		if (check_interval_ns > 0)
>>>>  			nanosleep(&sleep_time, NULL);
>>>> +#if defined(x86_64) || defined(i386)
> Our AI review pointed out that there are no such defines used
> by gcc, they should be __x86_64__ and __i386__ respectivly,
> so this commit 125c0289617f ("tests/amdgpu: insert pause mnemonic only on x86 / x86_64 platforms")
> should be fixed. Sorry for this, I should cross check it with
> simple
>
> #else
> #error "Unexpected"
> #endif
>
> and see this error pop-up.
>
>
>
> Regards,
> Kamil
>
>
>>>>  		else
>>>>  			__asm__ __volatile__("pause" ::: "memory"); /* CPU hint for spin-wait */
>>>> +#endif
>>>>  	}
>>>>  }
>>>>  
>>>> -- 
>>>> 2.53.0
>>>>

[-- Attachment #2: 0001-tests-amdgpu-remove-the-x86-pause-hint-from-value-po.patch --]
[-- Type: text/x-patch, Size: 2664 bytes --]

From 745fedc1ac3febd13f3fa423fc4216da77684a05 Mon Sep 17 00:00:00 2001
From: Vitaly Prosyak <vitaly.prosyak@amd.com>
Date: Wed, 21 Oct 2026 10:50:38 -0400
Subject: [PATCH] tests/amdgpu: remove the x86 pause hint from value polling

wait_for_value64() uses nanosleep() when a polling interval is requested
and otherwise polls until the expected value is observed or the timeout
expires. The pause instruction is an optional x86 spin-wait hint, not a
requirement for detecting completion or enforcing the timeout.

The original unguarded assembly was rejected by ARM/AArch64 assemblers
in debug builds. Commit 125c0289617f guarded it, but used x86_64 and i386
instead of the compiler predefined __x86_64__ and __i386__ macros. That
silently excludes the hint on normal GCC x86 builds as well.

Rather than correct the guard and retain architecture-specific assembly
for this polling helper, remove the optional hint and its guard entirely.
Keep the nanosleep path, volatile value reads, and timeout handling
unchanged. With no interval requested, the helper continues tight polling
without an architecture-specific instruction.

This deliberately gives up the x86 pause optimization, which can reduce
spin-loop power consumption and contention with an SMT sibling. It is not
a power or performance improvement; it trades that optimization for a
simpler polling implementation with no x86 assembly dependency. Relative
to the existing incorrect GCC guard, the hint was already omitted.

The removed memory clobber is a compiler barrier, not a hardware memory
fence. This change does not add or claim cross-architecture memory-ordering
guarantees; it retains the existing volatile polling accesses.

This is an alternative to correcting the architecture macro names, not an
additional fix to apply on top of that alternative.

ARM/AArch64 cross-compilation and GPU runtime testing were not performed.

Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
Cc: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Signed-off-by: Vitaly Prosyak <vitaly.prosyak@amd.com>
---
 tests/amdgpu/amd_basic.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/tests/amdgpu/amd_basic.c b/tests/amdgpu/amd_basic.c
index 494934c1c..590163b7b 100644
--- a/tests/amdgpu/amd_basic.c
+++ b/tests/amdgpu/amd_basic.c
@@ -705,10 +705,6 @@ static int wait_for_value64(volatile uint64_t *ptr, uint64_t expected,
 		/* Sleep if interval specified, otherwise tight loop */
 		if (check_interval_ns > 0)
 			nanosleep(&sleep_time, NULL);
-#if defined(x86_64) || defined(i386)
-		else
-			__asm__ __volatile__("pause" ::: "memory"); /* CPU hint for spin-wait */
-#endif
 	}
 }
 
-- 
2.55.0