Re: [PATCH] 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:

New issues:
- [High] 32-bit integer overflow in `adjust_idledly` time conversions corrupts valid hardware configurations.

Pre-existing issues:
- [High] Division by zero in `adjust_idledly` causes kernel panic on unrecognized crystal clock frequencies.
--

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

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

This patch applies wa_14025941587 to Graphic and Media versions to avoid
programming the IDLEDLY timer to less than 5 microseconds. It refactors
adjust_idledly() to support both the new and existing workarounds.

> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> index 010499766fce2..5a87f26d7dd4a 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
[ ... ]
> @@ -588,25 +588,57 @@ static void hw_engine_init_early(struct xe_gt *gt, struct xe_hw_engine *hwe,
>  static void adjust_idledly(struct xe_hw_engine *hwe)
>  {
>  	struct xe_gt *gt = hwe->gt;
> -	u32 idledly, maxcnt;
> +	u32 idledly, idledly_reg_val, maxcnt;
>  	u32 idledly_units_ps = 8 * gt->info.timestamp_base;
>  	u32 maxcnt_units_ns = 640;
> -	bool inhibit_switch = 0;
> +	bool inhibit_switch = false;
> +	bool wa_applied = false;
>  
> -	if (!IS_SRIOV_VF(gt_to_xe(hwe->gt)) && XE_GT_WA(gt, 16023105232)) {
> -		idledly = xe_mmio_read32(&gt->mmio, RING_IDLEDLY(hwe->mmio_base));
> +	if ((!IS_SRIOV_VF(gt_to_xe(hwe->gt)) && XE_GT_WA(gt, 16023105232)) ||
> +	    XE_GT_WA(gt, 14025941587)) {
> +		u32 mincnt_idledly_ns = 5000;
> +
> +		idledly_reg_val = xe_mmio_read32(&gt->mmio, RING_IDLEDLY(hwe->mmio_base));
>  		maxcnt = xe_mmio_read32(&gt->mmio, RING_PWRCTX_MAXCNT(hwe->mmio_base));
>  
> -		inhibit_switch = idledly & INHIBIT_SWITCH_UNTIL_PREEMPTED;
> -		idledly = REG_FIELD_GET(IDLE_DELAY, idledly);
> +		inhibit_switch = idledly_reg_val & INHIBIT_SWITCH_UNTIL_PREEMPTED;
> +		idledly = REG_FIELD_GET(IDLE_DELAY, idledly_reg_val);
>  		idledly = DIV_ROUND_CLOSEST(idledly * idledly_units_ps, 1000);

[Severity: High]
Does this multiplication overflow for delays > 4.29ms? Both idledly and
idledly_units_ps are 32-bit u32 variables, so the multiplication is done in
32-bit math. With idledly_units_ps typically around 666,664, any valid
register delay > 6442 (4.29ms) will overflow U32_MAX, resulting in a falsely
small value. Should this be cast to u64 and use DIV_ROUND_CLOSEST_ULL()
instead?

>  		maxcnt = REG_FIELD_GET(IDLE_WAIT_TIME, maxcnt);
>  		maxcnt *= maxcnt_units_ns;
>  
> -		if (xe_gt_WARN_ON(gt, idledly >= maxcnt || inhibit_switch)) {
> -			idledly = DIV_ROUND_CLOSEST(((maxcnt - 1) * 1000),
> -						    idledly_units_ps);
> -			xe_mmio_write32(&gt->mmio, RING_IDLEDLY(hwe->mmio_base), idledly);
> +		/* 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;
> +		}

[Severity: High]
If the earlier 32-bit multiplication overflows and produces a falsely small
wrapped value for idledly, will this workaround erroneously clamp a valid
large delay to a tiny 5us value, corrupting the hardware state?

[ ... ]
> +		if (wa_applied) {
> +			idledly_reg_val &= ~IDLE_DELAY;
> +			idledly_reg_val |= REG_FIELD_PREP(IDLE_DELAY,
> +						 DIV_ROUND_CLOSEST(idledly * 1000,
> +								   idledly_units_ps));

[Severity: High]
Can this write-back calculation also overflow 32-bit math if the intended
delay is large, writing corrupted garbage back to the register?

[Severity: High]
This is a pre-existing issue, but if idledly_units_ps is 0, will this cause
an unconditional kernel panic?

If read_crystal_clock() encounters an unrecognized clock frequency, it logs
a warning and sets *timestamp_base = 0, causing idledly_units_ps to be 0.
Because the idledly calculation above will also be 0, the check
idledly < mincnt_idledly_ns is now guaranteed to be true, setting
wa_applied = true.

When DIV_ROUND_CLOSEST performs division by the second argument
(idledly_units_ps), it seems it will trigger an immediate division by zero
exception. Should we check for zero before dividing?

> +			xe_gt_dbg(gt, "idledly %s: set %u max=%u inh=%u ts=%u\n",
> +				  hwe->name, idledly, maxcnt,
> +				  !!inhibit_switch, gt->info.timestamp_base);
> +			xe_mmio_write32(&gt->mmio,
> +					RING_IDLEDLY(hwe->mmio_base),
> +					idledly_reg_val);
>  		}
>  	}
>  }

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