Re: [PATCH v2 5/7] drm/xe/nvlp: Implement Wa_14026539277

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

Matt Roper <matthew.d.roper@intel.com> writes:

> On Fri, Mar 06, 2026 at 04:01:05PM -0300, Gustavo Sousa wrote:
>> Matt Roper <matthew.d.roper@intel.com> writes:
>> 
>> > On Fri, Mar 06, 2026 at 02:28:25PM -0300, Gustavo Sousa wrote:
>> >> Implement the KMD part of Wa_14026539277, which applies to NVL-P A0.
>> >> The KMD implementation is just one component of the workaround, which
>> >> also depends on Pcode to implement its part in order to be complete.
>> >> 
>> >> v2:
>> >>   - Add FUNC(xe_rtp_match_not_sriov_vf) to skip applying the workaround
>> >>     to SRIOV VFs. (Matt)
>> >> 
>> >> Cc: Matt Roper <matthew.d.roper@intel.com>
>> >> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> >> ---
>> >>  drivers/gpu/drm/xe/regs/xe_gt_regs.h |  4 ++++
>> >>  drivers/gpu/drm/xe/xe_gt.c           | 27 +++++++++++++++++++++++++++
>> >>  drivers/gpu/drm/xe/xe_wa_oob.rules   |  2 ++
>> >>  3 files changed, 33 insertions(+)
>> >> 
>> >> diff --git a/drivers/gpu/drm/xe/regs/xe_gt_regs.h b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
>> >> index 66ddad767ad4..a83cafbe03fd 100644
>> >> --- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
>> >> +++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
>> >> @@ -452,6 +452,10 @@
>> >>  
>> >>  #define XEHPC_L3CLOS_MASK(i)			XE_REG_MCR(0xb194 + (i) * 8)
>> >>  
>> >> +#define L2COMPUTESIDECTRL			XE_REG_MCR(0xb1c0)
>> >> +#define   CECTRL				REG_GENMASK(2, 1)
>> >> +#define   CECTRL_CENODATA_ALWAYS		REG_FIELD_PREP(CECTRL, 0x0)
>> >> +
>> >>  #define XE2_GLOBAL_INVAL			XE_REG(0xb404)
>> >>  
>> >>  #define XE2LPM_L3SQCREG2			XE_REG_MCR(0xb604)
>> >> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
>> >> index b455af1e6072..3c8692f9b8cf 100644
>> >> --- a/drivers/gpu/drm/xe/xe_gt.c
>> >> +++ b/drivers/gpu/drm/xe/xe_gt.c
>> >> @@ -450,6 +450,25 @@ int xe_gt_record_default_lrcs(struct xe_gt *gt)
>> >>  	return err;
>> >>  }
>> >>  
>> >> +static void xe_gt_wa_14026539277(struct xe_gt *gt)
>> >> +{
>> >> +	u32 val;
>> >> +
>> >> +	if (!XE_GT_WA(gt, 14026539277))
>> >> +		return;
>> >> +
>> >> +	/*
>> >> +	 * L2COMPUTESIDECTRL has a specific offset for media and the GSI offset
>> >> +	 * does not apply.
>> >> +	 */
>> >> +	xe_gt_assert(gt, xe_gt_is_main_type(gt));
>> >> +
>> >> +	val = xe_gt_mcr_unicast_read_any(gt, L2COMPUTESIDECTRL);
>> >> +	val &= ~CECTRL;
>> >> +	val |= CECTRL_CENODATA_ALWAYS;
>> >> +	xe_gt_mcr_multicast_write(gt, L2COMPUTESIDECTRL, val);
>> >> +}
>> >> +
>> >>  int xe_gt_init_early(struct xe_gt *gt)
>> >>  {
>> >>  	int err;
>> >> @@ -575,6 +594,14 @@ static int gt_init_with_gt_forcewake(struct xe_gt *gt)
>> >>  	 */
>> >>  	gt->info.gmdid = xe_mmio_read32(&gt->mmio, GMD_ID);
>> >>  
>> >> +	/*
>> >> +	 * Wa_14026539277 can't be implemented as a regular GT workaround (i.e.
>> >> +	 * as an entry in gt_was[]) because we would get the hardware already in
>> >> +	 * a bad state by the time it would be applied.  Hence, we implement it
>> >> +	 * as an OOB workaround and apply it early to prevent that.
>> >> +	 */
>> >> +	xe_gt_wa_14026539277(gt);
>> >> +
>> >>  	return 0;
>> >>  }
>> >>  
>> >> diff --git a/drivers/gpu/drm/xe/xe_wa_oob.rules b/drivers/gpu/drm/xe/xe_wa_oob.rules
>> >> index 80b54b195f20..03a0bf0aeb6e 100644
>> >> --- a/drivers/gpu/drm/xe/xe_wa_oob.rules
>> >> +++ b/drivers/gpu/drm/xe/xe_wa_oob.rules
>> >> @@ -58,3 +58,5 @@
>> >>  
>> >>  14025883347	MEDIA_VERSION_RANGE(1301, 3503)
>> >>  		GRAPHICS_VERSION_RANGE(2004, 3005)
>> >> +
>> >> +14026539277	PLATFORM(NOVALAKE_P), PLATFORM_STEP(A0, B0), GRAPHICS_VERSION(3510), FUNC(xe_rtp_match_not_sriov_vf)
>> >
>> > I don't think it's right that we have both platform matches and IP
>> > matches here; that's not something that should usually happen because
>> > the workaround is either tied to the platform (NVL) or tied to the IP
>> > (Xe3p_LPG).  For device workarounds, the handling in our graphics
>> > workaround database can be a bit confusing since what we're looking at
>> > is really just a proxy/placeholder ticket for something that was filed
>> > in a different database originally.  Due to how the databases work, they
>> > have to slap some IP release on the proxy ticket, but in this case we
>> > don't need to add a match for those to our driver rules; just the
>> > platform information is sufficient.
>> >
>> > That would also mean that this should probably be an XE_DEVICE_WA()
>> > rather than an XE_GT_WA() and the workaround function we're adding here
>> > should be renamed.
>> 
>> Hm... But are we meant to apply the programming to the media GT as well?
>> I thought the issue for this workaround was observed only on the primary
>> GT.
>> 
>
> Device workarounds operate on the xe_device rather than on any xe_gt.
> So if a device workaround asks you to do something with with GTs, you
> need to extract the relevant GTs from the device.  Novalake is
> single-tile, so in this case we'd do something like
>
>      struct xe_gt *gt = xe_device_get_root_tile(xe)->primary_gt;
>
> to grab the primary GT if that's what we're supposed to be working
> with.

Or simply loop over GTs and only apply to primary GTs? I prefer this
because it abstracts away the assumption of a single tile/primary GT
(even though we know this applies only to NVL-P and is unlikely to be
needed in other platforms).

>
>> So, should we make this a device workaround and add then make sure that
>> we apply it only on the primary GT in the function that implements it?
>> 
>> We will probably need to rework device OOB workarounds in
>> order to make this change, because stepping information is not ready by
>> the time device OOB workarounds are matched (i.e. when
>> xe_wa_process_device_oob() is called before we read the stepping
>> information into the device info).
>
> I guess we should just move the device stepping determination earlier.
> Figuring out the platform stepping only requires information from the
> PCI config space so it should be possible to do before anything else in
> the driver (i.e., we don't even need the MMIO BARs mapped yet to be able
> to determine the platform stepping --- and in theory there could be
> device workarounds that need to be taken into account during the very
> earliest parts of driver initialization).

Yep, sounds good.

I'm also going to need to make sure that xe_wa_process_device_oob() is
called after xe_sriov_probe_early(), because of
FUNC(xe_rtp_match_not_sriov_vf).

I'm wondering if it would be a good idea to have OOB workarounds being
checked lazily because of these ordering issues.  Not sure though, since
it would require more storage if we want to continue caching the results
in the bitmask (it would need an extra bitmask to check if it was alreay
evaluated); and also it adds some level of unpredictability, since there
would not be specific point where we would know workarounds are all
checked.

Another option is making sure we raise warnings for anything that is not
yet "ready" to be checked by the time a workaround check is done.  It
appears xe_device_sriov_mode() already contains an assert that would
cover that (although we could also have one specific for
xe_rtp_match_not_sriov_vf, to be safe), but I don't think the other
match functions contain that yet.

I think I like this last one better.

--
Gustavo Sousa

>
>
> Matt
>
>> 
>> --
>> Gustavo Sousa
>> 
>> >
>> >
>> > Matt
>> >
>> >> 
>> >> -- 
>> >> 2.52.0
>> >> 
>> >
>> > -- 
>> > Matt Roper
>> > Graphics Software Engineer
>> > Linux GPU Platform Enablement
>> > Intel Corporation
>
> -- 
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation