Re: [RFC PATCH 3/3] drm/xe: Use MI_SEMAPHORE_WAIT_64 in ULLS postamble where supported

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/xe: 64-bit semaphore wait on 32-bit slots in emit_ulls_postamble()
--

commit 909e8abff72efe9fa35cc90369278500a84b3c16
Author: Tangudu Tilak Tirumalesh <tilak.tirumalesh.tangudu@intel.com>

drm/xe: Use MI_SEMAPHORE_WAIT_64 in ULLS postamble where supported

This commit updates emit_ulls_postamble() to emit MI_SEMAPHORE_WAIT_64 on
platforms where gt->info.has_sem64 is set, and increases ULLS_JOB_SIZE_DW from
48 to 50 to accommodate the 2 additional dwords.

> diff --git a/drivers/gpu/drm/xe/xe_ring_ops.c b/drivers/gpu/drm/xe/xe_ring_ops.c
> index 267bb1c26f997..e4073fce87664 100644
> --- a/drivers/gpu/drm/xe/xe_ring_ops.c
> +++ b/drivers/gpu/drm/xe/xe_ring_ops.c
> @@ -562,14 +562,30 @@ static int emit_ulls_ring_tail(struct xe_gt *gt, struct xe_lrc *lrc, u32 *dw,
>  static int emit_ulls_postamble(struct xe_gt *gt, struct xe_lrc *lrc, u32 *dw,
>  			       int i, u32 seqno, u32 head)
>  {
> -	dw[i++] = MI_SEMAPHORE_WAIT |
> -		MI_SEMW_GGTT |
> -		MI_SEMW_POLL |
> -		MI_SEMW_COMPARE(SAD_EQ_SDD);
> -	dw[i++] = LRC_MIGRATION_ULLS_SEMAPHORE_SIGNAL;
> -	dw[i++] = xe_lrc_ulls_semaphore_ggtt_addr(lrc, seqno + 1);
> -	dw[i++] = 0;
> -	dw[i++] = 0;
> +	u32 addr = xe_lrc_ulls_semaphore_ggtt_addr(lrc, seqno + 1);
> +
> +	/* Wa_16029281913: legacy semaphore has an issue on some Xe3p platforms, prefer sem64 */
> +	if (gt->info.has_sem64) {
> +		dw[i++] = MI_SEMAPHORE_WAIT_64 |
> +			MI_SEMW_GGTT |
> +			MI_SEMW64_INHIBIT_CTX_SWITCH |
> +			MI_SEMW_COMPARE(SAD_EQ_SDD);
> +		dw[i++] = LRC_MIGRATION_ULLS_SEMAPHORE_SIGNAL;
> +		dw[i++] = 0;
> +		dw[i++] = addr;

[Severity: High]
Does using MI_SEMAPHORE_WAIT_64 here cause a GPU hang because the ULLS
semaphore slots in PPHWSP are still spaced at 32-bit intervals?

In drivers/gpu/drm/xe/xe_lrc.c, xe_lrc_ulls_semaphore_ggtt_addr() and
xe_lrc_set_ulls_semaphore() index the semaphore array with a 4-byte stride:

#define semaphore_offset(seqno) \
	(sizeof(u32) * ((seqno) % LRC_MIGRATION_ULLS_SEMAPHORE_COUNT))

u32 xe_lrc_ulls_semaphore_ggtt_addr(struct xe_lrc *lrc, u32 seqno)
{
	...
	return __xe_lrc_ulls_ggtt_addr(lrc) + semaphore_offset(seqno);
}

void xe_lrc_set_ulls_semaphore(struct xe_lrc *lrc, u32 seqno)
{
	...
	iosys_map_incr(&map, semaphore_offset(seqno));
	xe_map_write32(xe, &map, LRC_MIGRATION_ULLS_SEMAPHORE_SIGNAL);
}

MI_SEMAPHORE_WAIT_64 performs a 64-bit comparison (against 0x0000000000000001
with SAD_EQ_SDD) and requires an 8-byte aligned GGTT address (bits 2:0 are
MBZ).

When seqno + 1 is odd, addr is only 4-byte aligned (addr & 7 == 4). When the
hardware masks bits 2:0, the 64-bit read accesses the qword at addr & ~7,
which holds semaphore[seqno] (cleared to 0 by emit_ulls_preamble()) in the
lower 32 bits and semaphore[seqno + 1] (set to 1 by
xe_lrc_set_ulls_semaphore()) in the upper 32 bits, reading 0x0000000100000000
instead of 0x0000000000000001.

Even when seqno + 1 is even and 8-byte aligned, the 64-bit read at
semaphore[seqno + 1] spans both semaphore[seqno + 1] in the low 32 bits and
semaphore[seqno + 2] in the high 32 bits. If both jobs seqno + 1 and
seqno + 2 are submitted before job seqno reaches its postamble,
xe_lrc_set_ulls_semaphore() writes 1 to both slots, making the 64-bit value
at addr 0x0000000100000001. Because semaphore[seqno + 2] is only cleared when
job seqno + 2 executes its preamble on the engine, the SAD_EQ_SDD check on
job seqno will never match and the engine will hang.

Should semaphore_offset() in drivers/gpu/drm/xe/xe_lrc.c use sizeof(u64) so
that each ULLS semaphore slot is 8-byte aligned with its upper 32 bits zeroed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102100018.2209034-1-tilak.tirumalesh.tangudu@intel.com?part=3