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

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

Gustavo Sousa <gustavo.sousa@intel.com> writes:

> 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).

Hm... I'm thinking about simply keeping the same function (renamed) and
call site in xe_gt.c, replacing the check with XE_DEVICE_WA() and
applying only if it is the primary GT.  The reason for that is that we
need to apply this workaround early, and I am not sure we have a good
place to apply it on xe_device.c code.

We had issues when applying it as a regular gt_was[] entry, and we would
probably run into the same kind of issues if doing it afterxe_gt_init().

--
Gustavo Sousa

>
>>
>>> 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