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

[Gustavo Sousa <gustavo.sousa@intel.com>]

"Tangudu, Tilak Tirumalesh" <tilak.tirumalesh.tangudu@intel.com> writes:

>> -----Original Message-----
>> From: Sousa, Gustavo <gustavo.sousa@intel.com>
>> Sent: 22 October 2026 00:05
>> To: Tangudu, Tilak Tirumalesh <tilak.tirumalesh.tangudu@intel.com>;
>> Tangudu, Tilak Tirumalesh <tilak.tirumalesh.tangudu@intel.com>;
>> Belgaumkar, Vinay <vinay.belgaumkar@intel.com>; Lin, Shuicheng
>> <shuicheng.lin@intel.com>; intel-xe@lists.freedesktop.org
>> Cc: Roper, Matthew D <matthew.d.roper@intel.com>
>> Subject: Re: [PATCH 2/2] drm/xe: Add wa_14025941587 to xe2, xe3 and xe3p
>> platforms
>> 
>> tilak.tirumalesh.tangudu@intel.com writes:
>> 
>> > From: Tangudu Tilak Tirumalesh <tilak.tirumalesh.tangudu@intel.com>
>> >
>> > 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
>> >
>> > v2: Use xe_rtp_match_not_sriov_vf, move to local variable
>> >     Remove warn and other knits            - Matt R
>> >
>> > v3: Add verbose comment - Tejas
>> >
>> > v4: Restore IDLE_DLY register on engine reset.
>> >     Add it to GUC save-restore list. -Vivek
>> >
>> > v5: Extend WA to Media Versions 13.01 to 35.03 - Vinay
>> >
>> > v6: Avoid clearing inhibit switch - Bala
>> >     Refactor code accordingly by adding idle_reg_val.
>> >
>> > v7: Rebased with the divide-by-zero/overflow guards living in
>> >     a separate hardening patch.
>> >
>> > v8: Preserve the Wa_16023105232 floor (DIV_ROUND_DOWN_ULL) and the
>> >     maxcnt == 0 guard from the hardening patch. Round up
>> >     (DIV_ROUND_UP_ULL) the Wa_14025941587 minimum conversion
>> instead,
>> >     so the tick-quantized delay cannot round back below 5 us.
>> >
>> > v9: Evaluate the Wa_16023105232 xe_gt_WARN_ON() against the value
>> >     read from hardware instead of the Wa_14025941587-bumped value,
>> >     so it no longer fires on the driver's own floor. Re-check the
>> >     rounded-up tick value against maxcnt and floor it if tick
>> >     quantization pushed it back to/above maxcnt, logging via
>> >     xe_gt_dbg since this is the driver's own value, not a hardware
>> >     anomaly.
>> >
>> > Assisted-by: GitHub_Copilot:claude-opus-4.8
>> > Signed-off-by: Tangudu Tilak Tirumalesh
>> > <tilak.tirumalesh.tangudu@intel.com>
>> > ---
>> >  drivers/gpu/drm/xe/xe_guc_ads.c    |  2 +-
>> >  drivers/gpu/drm/xe/xe_hw_engine.c  | 93
>> > ++++++++++++++++++++++++------  drivers/gpu/drm/xe/xe_wa_oob.rules |
>> > 2 +
>> >  3 files changed, 79 insertions(+), 18 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/xe/xe_guc_ads.c
>> > b/drivers/gpu/drm/xe/xe_guc_ads.c index ff8eee3831aa..58a3f6293ce4
>> > 100644
>> > --- a/drivers/gpu/drm/xe/xe_guc_ads.c
>> > +++ b/drivers/gpu/drm/xe/xe_guc_ads.c
>> > @@ -864,7 +864,7 @@ static unsigned int guc_mmio_regset_write(struct
>> xe_guc_ads *ads,
>> >  		}
>> >  	}
>> >
>> > -	if (XE_GT_WA(hwe->gt, 16023105232))
>> > +	if (XE_GT_WA(hwe->gt, 16023105232) || XE_GT_WA(hwe->gt,
>> > +14025941587))
>> >  		guc_mmio_regset_write_one(ads, regset_map,
>> >  					  RING_IDLEDLY(hwe->mmio_base),
>> >  					  count++);
>> > diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c
>> > b/drivers/gpu/drm/xe/xe_hw_engine.c
>> > index d868af8e3301..9680d2a5adec 100644
>> > --- a/drivers/gpu/drm/xe/xe_hw_engine.c
>> > +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
>> > @@ -585,44 +585,103 @@ static void hw_engine_init_early(struct xe_gt
>> *gt, struct xe_hw_engine *hwe,
>> >  	xe_reg_whitelist_process_engine(hwe);
>> >  }
>> >
>> > +static u32 idledly_floor_ticks(u32 idledly_ns, u32 idledly_units_ps)
>> > +{
>> > +	return DIV_ROUND_DOWN_ULL((u64)idledly_ns * 1000,
>> idledly_units_ps);
>> > +}
>> > +
>> >  static void adjust_idledly(struct xe_hw_engine *hwe)  {
>> >  	struct xe_gt *gt = hwe->gt;
>> > -	u32 idledly, maxcnt;
>> > +	u32 idledly, idledly_hw, 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;
>> > +	bool clamped_below_maxcnt = false;
>> > +
>> > +	if ((!IS_SRIOV_VF(gt_to_xe(gt)) && XE_GT_WA(gt, 16023105232)) ||
>> > +	    XE_GT_WA(gt, 14025941587)) {
>> 
>> Hey, eveyone.
>> 
>> I came across this recently I was wondering why Wa_14025941587 doesn't
>> check for SRIOV as done for Wa_16023105232.
>> 
>> I think VF doesn't have access to the IDLEDLY register.
>> 
>> I'll be changing this section of the driver soon.  I could send a patch making
>> sure that we bail early if on a VF.
>> 
>> --
>> Gustavo Sousa
> The patch has rtp func guard below in oob rules, this should be sufficient right ?

Yep, right. I realized that after I prematurely sent this email :'-)

That said, I decided to write patch that drops
FUNC(xe_rtp_match_not_sriov_vf) and just adds an early return in the
beginning of adjust_idledly() if VF mode is detected. The justification
is that with that we simplify the conditions in the OOB rules file and
we have consistency between all workarounds related to IDLEDLY (there is
another coming soon); and that adjust_idledly() should always bail if
the driver is in VF mode irrespective of which workarounds are active.

--
Gustavo Sousa

>
> 8< -----------------------
>> > +14025941587	GRAPHICS_VERSION_RANGE(2001, 3511),
>> FUNC(xe_rtp_match_not_sriov_vf)
>> > +		MEDIA_VERSION_RANGE(1301, 3503),
>> FUNC(xe_rtp_match_not_sriov_vf)
>
> ------------------------- >8
> -Tilak
>> 
>> > +		u32 mincnt_idledly_ns = 5000;
>> >
>> > -	if (!IS_SRIOV_VF(gt_to_xe(gt)) && XE_GT_WA(gt, 16023105232)) {
>> >  		/* xe_gt_clock_init() warns and zeroes timestamp_base on
>> unknown crystal clock. */
>> >  		if (!idledly_units_ps)
>> >  			return;
>> >
>> > -		idledly = xe_mmio_read32(&gt->mmio, RING_IDLEDLY(hwe-
>> >mmio_base));
>> > +		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_ULL((u64)idledly *
>> idledly_units_ps,
>> > 1000);
>> > +		idledly_hw = idledly;
>> >  		maxcnt = REG_FIELD_GET(IDLE_WAIT_TIME, maxcnt);
>> >  		maxcnt *= maxcnt_units_ns;
>> >
>> > -		/* Clear the inhibit switch without disturbing a valid delay. */
>> > -		if (inhibit_switch)
>> > +		/*
>> > +		 * Wa_14025941587 is applied before Wa_16023105232,
>> which takes
>> > +		 * priority if the two ever conflict (not expected in practice).
>> > +		 */
>> > +		if (XE_GT_WA(gt, 14025941587) &&
>> > +		    idledly < mincnt_idledly_ns) {
>> > +			idledly = mincnt_idledly_ns;
>> >  			wa_applied = true;
>> > +		}
>> >
>> > -		if (xe_gt_WARN_ON(gt, idledly >= maxcnt)) {
>> > -			/* Floor below maxcnt; write 0 to still clear the inhibit
>> bit. */
>> > -			idledly = maxcnt ?
>> > -				DIV_ROUND_DOWN_ULL((u64)(maxcnt - 1) *
>> 1000,
>> > -						   idledly_units_ps) : 0;
>> > -			wa_applied = true;
>> > +		if (XE_GT_WA(gt, 16023105232)) {
>> > +			/* Clear the inhibit switch without disturbing a valid
>> delay. */
>> > +			if (inhibit_switch) {
>> > +				idledly_reg_val &=
>> ~INHIBIT_SWITCH_UNTIL_PREEMPTED;
>> > +				wa_applied = true;
>> > +			}
>> > +
>> > +			/* Warn only on the value read from hardware. */
>> > +			xe_gt_WARN_ON(gt, idledly_hw >= maxcnt);
>> > +
>> > +			if (idledly >= maxcnt) {
>> > +				/* maxcnt may be 0 if IDLE_WAIT_TIME is
>> unprogrammed. */
>> > +				idledly = maxcnt ? maxcnt - 1 : 0;
>> > +				clamped_below_maxcnt = true;
>> > +				wa_applied = true;
>> > +			}
>> >  		}
>> >
>> > -		if (wa_applied)
>> > -			xe_mmio_write32(&gt->mmio, RING_IDLEDLY(hwe-
>> >mmio_base),
>> > -					REG_FIELD_PREP(IDLE_DELAY,
>> idledly));
>> > +		if (wa_applied) {
>> > +			u32 idledly_ticks;
>> > +
>> > +			/*
>> > +			 * Wa_16023105232 requires idledly < maxcnt, so
>> floor
>> > +			 * that clamp; otherwise round up to guarantee the
>> > +			 * Wa_14025941587 minimum survives tick
>> quantization.
>> > +			 */
>> > +			if (clamped_below_maxcnt)
>> > +				idledly_ticks = idledly_floor_ticks(idledly,
>> idledly_units_ps);
>> > +			else
>> > +				idledly_ticks =
>> DIV_ROUND_UP_ULL((u64)idledly * 1000,
>> > +
>> idledly_units_ps);
>> > +
>> > +			/*
>> > +			 * Tick quantization can still push the rounded-up
>> value
>> > +			 * to/above maxcnt; re-floor here so
>> Wa_16023105232 keeps
>> > +			 * priority even in that case.
>> > +			 */
>> > +			if (!clamped_below_maxcnt && XE_GT_WA(gt,
>> 16023105232) &&
>> > +			    (u64)idledly_ticks * idledly_units_ps >=
>> (u64)maxcnt * 1000) {
>> > +				xe_gt_dbg(gt, "idledly %s: %u ticks would
>> exceed maxcnt=%u, so flooring\n",
>> > +					  hwe->name, idledly_ticks, maxcnt);
>> > +				idledly = maxcnt ? maxcnt - 1 : 0;
>> > +				idledly_ticks = idledly_floor_ticks(idledly,
>> idledly_units_ps);
>> > +			}
>> > +
>> > +			idledly_reg_val &= ~IDLE_DELAY;
>> > +			idledly_reg_val |= REG_FIELD_PREP(IDLE_DELAY,
>> idledly_ticks);
>> > +			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);
>> > +		}
>> >  	}
>> >  }
>> >
>> > diff --git a/drivers/gpu/drm/xe/xe_wa_oob.rules
>> > b/drivers/gpu/drm/xe/xe_wa_oob.rules
>> > index dd69ad07f7a9..3001155f8d09 100644
>> > --- a/drivers/gpu/drm/xe/xe_wa_oob.rules
>> > +++ b/drivers/gpu/drm/xe/xe_wa_oob.rules
>> > @@ -72,3 +72,5 @@
>> >  16029897822	MEDIA_VERSION(3500)
>> >  		GRAPHICS_VERSION(3510)
>> >  14027054324	GRAPHICS_VERSION(3511)
>
>> > +14025941587	GRAPHICS_VERSION_RANGE(2001, 3511),
>> FUNC(xe_rtp_match_not_sriov_vf)
>> > +		MEDIA_VERSION_RANGE(1301, 3503),
>> FUNC(xe_rtp_match_not_sriov_vf)
>
>
>> > --
>> > 2.46.0