Re: [PATCH 4/7] drm/xe/rtp: Add support for matching platform-level stepping

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

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

> On Thu, Mar 05, 2026 at 02:22:56PM -0800, Matt Roper wrote:
>> On Thu, Mar 05, 2026 at 09:02:31AM -0300, Gustavo Sousa wrote:
>> > Add support for matching platform-level stepping, which will be used for
>> > an upcoming NVL-P workaround.
>> > 
>> > As support for reading platform-level stepping information is added only
>> > as needed in the driver, add a warning when the rule finds a STEP_NONE
>> > value, which is an indication that the driver is missing such a support.
>> > 
>> > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> > ---
>> >  drivers/gpu/drm/xe/xe_rtp.c       |  7 +++++++
>> >  drivers/gpu/drm/xe/xe_rtp.h       | 20 ++++++++++++++++++++
>> >  drivers/gpu/drm/xe/xe_rtp_types.h |  1 +
>> >  3 files changed, 28 insertions(+)
>> > 
>> > diff --git a/drivers/gpu/drm/xe/xe_rtp.c b/drivers/gpu/drm/xe/xe_rtp.c
>> > index 7bfdc6795ce6..991f218f1cc3 100644
>> > --- a/drivers/gpu/drm/xe/xe_rtp.c
>> > +++ b/drivers/gpu/drm/xe/xe_rtp.c
>> > @@ -55,6 +55,13 @@ static bool rule_matches(const struct xe_device *xe,
>> >  			match = xe->info.platform == r->platform &&
>> >  				xe->info.subplatform == r->subplatform;
>> >  			break;
>> > +		case XE_RTP_MATCH_PLATFORM_STEP:
>> > +			if (drm_WARN_ON(&xe->drm, xe->info.step.platform == STEP_NONE))
>> > +				return false;
>> > +
>> > +			match = xe->info.step.platform >= r->step_start &&
>> > +				xe->info.step.platform < r->step_end;
>> > +			break;
>> >  		case XE_RTP_MATCH_GRAPHICS_VERSION:
>> >  			if (drm_WARN_ON(&xe->drm, !gt))
>> >  				return false;
>> > diff --git a/drivers/gpu/drm/xe/xe_rtp.h b/drivers/gpu/drm/xe/xe_rtp.h
>> > index be4195264286..7d6daa7eb1e4 100644
>> > --- a/drivers/gpu/drm/xe/xe_rtp.h
>> > +++ b/drivers/gpu/drm/xe/xe_rtp.h
>> > @@ -35,6 +35,10 @@ struct xe_reg_sr;
>> >  	{ .match_type = XE_RTP_MATCH_SUBPLATFORM,				\
>> >  	  .platform = plat__, .subplatform = sub__ }
>> >  
>> > +#define _XE_RTP_RULE_PLATFORM_STEP(start__, end__)				\
>> > +	{ .match_type = XE_RTP_MATCH_PLATFORM_STEP,				\
>> > +	  .step_start = start__, .step_end = end__ }
>> > +
>> >  #define _XE_RTP_RULE_GRAPHICS_STEP(start__, end__)				\
>> >  	{ .match_type = XE_RTP_MATCH_GRAPHICS_STEP,				\
>> >  	  .step_start = start__, .step_end = end__ }
>> > @@ -66,6 +70,22 @@ struct xe_reg_sr;
>> >  #define XE_RTP_RULE_SUBPLATFORM(plat_, sub_)					\
>> >  	_XE_RTP_RULE_SUBPLATFORM(XE_##plat_, XE_SUBPLATFORM_##plat_##_##sub_)
>> >  
>> > +/**
>> > + * XE_RTP_RULE_PLATFORM_STEP - Create rule matching platform-level stepping
>> > + * @start_: First stepping matching the rule
>> > + * @end_: First stepping that does not match the rule
>> > + *
>> > + * Note that the range matching this rule is [ @start_, @end_ ), i.e. inclusive
>> > + * on the left, exclusive on the right.
>> > + *
>> > + * You need to make sure that proper support for reading platform-level stepping
>> > + * information is present for the target platform before using this rule.
>> > + *
>> > + * Refer to XE_RTP_RULES() for expected usage.
>> > + */
>> > +#define XE_RTP_RULE_PLATFORM_STEP(start_, end_)					\
>> > +	_XE_RTP_RULE_PLATFORM_STEP(STEP_##start_, STEP_##end_)
>> 
>> To use this effectively someone will need to pair this with another rule
>> like
>> 
>>         PLATFORM(NOVALAKE_P), PLATFORM_STEP(A2, C1)
>> 
>> and using PLATFORM_STEP() by itself without a corresponding PLATFORM()
>> or SUBPLATFORM() would be a bug.  Maybe we should just make
>> PLATFORM_STEP a rule that takes the platform as well as the stepping
>> range so that it's impossible for anyone to ever use PLATFORM_STEP
>> without tying it to a specific platform?  And also make a corresponding
>> SUBPLATFORM_STEP() rule for cases where that's relevant.  E.g.,
>> 
>>         PLATFORM_STEP(NOVALAKE_P, A2, C1)
>> 
>> or
>> 
>>         SUBPLATFORM_STEP(BATTLEMAGE, BATTLEMAGE_G21, A0, B0)
>> 
>> We'd need to update the union in struct xe_rtp_rule, but the size of the
>> union shouldn't increase.
>> 
>> What do you think?
>
> Anyway, consider this feedback optional.  I think it's fine to land
> as-is if you want.
>
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

Thanks!

Yep, your suggestion makes sense.  I would say that the same argument
applies to GRAPHICS_STEP and MEDIA_STEP, right?  In other words, e.g. it
doesn't make much sense to use GRAPHICS_STEP without also using
GRAPHICS_VERSION.

So we can probably go ahead with merging this as-is and then have a
follow-up series handle stepping-matching macros in a way to always
require the relevant higher-level info (i.e. platform, subplatform,
graphics version or media version)?

--
Gustavo Sousa

>
>> 
>> 
>> Matt
>> 
>> > +
>> >  /**
>> >   * XE_RTP_RULE_GRAPHICS_STEP - Create rule matching graphics stepping
>> >   * @start_: First stepping matching the rule
>> > diff --git a/drivers/gpu/drm/xe/xe_rtp_types.h b/drivers/gpu/drm/xe/xe_rtp_types.h
>> > index 6ba7f226c227..166251615be1 100644
>> > --- a/drivers/gpu/drm/xe/xe_rtp_types.h
>> > +++ b/drivers/gpu/drm/xe/xe_rtp_types.h
>> > @@ -41,6 +41,7 @@ struct xe_rtp_action {
>> >  enum {
>> >  	XE_RTP_MATCH_PLATFORM,
>> >  	XE_RTP_MATCH_SUBPLATFORM,
>> > +	XE_RTP_MATCH_PLATFORM_STEP,
>> >  	XE_RTP_MATCH_GRAPHICS_VERSION,
>> >  	XE_RTP_MATCH_GRAPHICS_VERSION_RANGE,
>> >  	XE_RTP_MATCH_GRAPHICS_VERSION_ANY_GT,
>> > 
>> > -- 
>> > 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