Re: [PATCH 2/2] drm/xe: Add wa_14025941587 to xe2, xe3 and xe3p platforms

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Early return when maxcnt is 0 silently bypasses Wa_14025941587
- [Medium] Mutated idledly variable causes spurious WARN_ON and violates WA 14025941587 constraint
--

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

drm/xe: Add wa_14025941587 to xe2, xe3 and xe3p platforms

Avoid programming the IDLEDLY timer to less than 5 microseconds.
Apply wa_14025941587 to Graphics Versions 20.01 to 35.11
and Media Versions 13.01 to 35.03

> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> index 6ffb0ece05612..033740c34aa66 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
[ ... ]
> @@ -614,10 +618,38 @@ static void adjust_idledly(struct xe_hw_engine *hwe)
>  
>  		maxcnt *= maxcnt_units_ns;
>  
> -		if (xe_gt_WARN_ON(gt, idledly >= maxcnt || inhibit_switch)) {
> -			idledly = DIV_ROUND_CLOSEST_ULL((u64)(maxcnt - 1) * 1000,
> -							idledly_units_ps);
> -			xe_mmio_write32(&gt->mmio, RING_IDLEDLY(hwe->mmio_base), idledly);

[Severity: Medium]
In adjust_idledly(), there is an existing check just before this hunk:

	maxcnt = REG_FIELD_GET(IDLE_WAIT_TIME, maxcnt);

	if (!maxcnt)
		return;

If maxcnt evaluates to 0, the function executes an early return. Does this
unintentionally bypass the new Wa_14025941587 workaround added below,
skipping the 5 microsecond limit enforcement if the initial maxcnt from the
hardware happens to be 0?

> +		/* Wa_14025941587: applied prior to Wa_16023105232,
> +		 * as the latter has higher priority, although conflict
> +		 * is not expected.
> +		 */
> +		if (XE_GT_WA(gt, 14025941587) &&
> +		    idledly < mincnt_idledly_ns) {
> +			idledly = mincnt_idledly_ns;
> +			wa_applied = true;
> +		}
> +
> +		if (XE_GT_WA(gt, 16023105232) &&
> +		    xe_gt_WARN_ON(gt, idledly >= maxcnt || inhibit_switch)) {
> +			idledly = (maxcnt - 1);

[Severity: Medium]
Can this sequence trigger a spurious WARN_ON and violate the 5000ns minimum?

If the original idledly is less than 5000ns, WA 14025941587 mutates idledly
to 5000. Immediately after, if maxcnt happens to be 5000 or less (e.g. 4480),
the WARN_ON will trigger because it evaluates the mutated idledly
(5000 >= 4480), even if the original hardware value was safe (e.g.
2000 < 4480). 

Following the WARN_ON, idledly is then set to maxcnt - 1 (e.g. 4479), which
silently breaks the 5000ns minimum required by Wa_14025941587.

Should the second workaround evaluate the original hardware state rather than
the software-mutated value?

> +			wa_applied = true;
> +			/* inhibit_switch override to keep
> +			 * semantics of Wa_16023105232
> +			 */
> +			if (inhibit_switch)
> +				idledly_reg_val &= ~INHIBIT_SWITCH_UNTIL_PREEMPTED;
> +		}

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