Re: [PATCH v2 7/7] drm/xe/rtp: Implement a structured parser for rule matching

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

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

> On Thu, Apr 30, 2026 at 05:20:08PM -0300, Gustavo Sousa wrote:
>> The current unwritten grammar for RTP rules is as follows:
>> 
>>           rules = disjunction;
>>     disjunction = conjunction { "OR" conjunction };
>>     conjunction = single_rule { single_rule }
>>                               /* AND operator is implicit */;
>>     single_rule = ? GRAPHICS_VERSION(...), MEDIA_VERSION(...),
>>                      FUNC(...), etc ?;
>
> We should probably add this grammar BNF to the DOC: section at the top
> of the file too.  Then it won't be "unwritten" anymore.  :-)

Cool. It could be on the kernel-doc for XE_RTP_RULES(). Ack on
incrementing this patch with the following then?

    diff --git a/drivers/gpu/drm/xe/xe_rtp.h b/drivers/gpu/drm/xe/xe_rtp.h
    index 562082b18d7b..2a8e536eaa5d 100644
    --- a/drivers/gpu/drm/xe/xe_rtp.h
    +++ b/drivers/gpu/drm/xe/xe_rtp.h
    @@ -394,18 +394,39 @@ struct xe_reg_sr;
      * XE_RTP_RULES - Helper to set multiple rules to a struct xe_rtp_entry_sr entry
      * @...: Rules
      *
    - * At least one rule is needed and up to 12 are supported. Multiple rules are
    - * AND'ed together, i.e. all the rules must evaluate to true for the entry to
    - * be processed. See XE_RTP_MATCH_* for the possible match rules. Example:
    + * When an RTP table is being processed, the rules of each entry are evaluated
    + * to check if they match the target entity (platform, gt or hwe, depending on
    + * the specific RTP table).
    + *
    + * The sequence of arguments of this macro must follow the following eBNF
    + * grammar::
    + *
    + *            rules = disjunction;
    + *      disjunction = conjunction, { "OR" conjunction };
    + *      conjunction = single_rule, { single_rule };
    + *				   (* the AND operator is implicit *)
    + *      single_rule = ? GRAPHICS_VERSION(...), MEDIA_VERSION(...),
    + *                      FUNC(...), etc ?
    + *
    + * Examples:
      *
      * .. code-block:: c
      *
      *	const struct xe_rtp_entry_sr wa_entries[] = {
      *		...
    - *		{ XE_RTP_NAME("test-entry"),
    + *		{ XE_RTP_NAME("entry-a"),
    + *		  // Match DG2-G10 with graphics steppings A0 up-to B0
    + *		  // (exclusive).
      *		  XE_RTP_RULES(SUBPLATFORM(DG2, G10), GRAPHICS_STEP(A0, B0)),
      *		  ...
      *		},
    + *		{ XE_RTP_NAME("entry-b"),
    + *		  // Match graphics version 20 (all steppings) or graphics
    + *		  // version 30 steppings A0 up-to B0 (exclusive).
    + *		  XE_RTP_RULES(GRAPHICS_VERSION(2000), OR,
    + *			       GRAPHICS_VERSION(3000), GRAPHICS_STEP(A0, B0))
    + *		  ...
    + *		},
      *		...
      *	};
      */

--
Gustavo Sousa

>
>> 
>> While rule_matches() currently works for the grammar above, it doesn't
>> easily resemble it.  Let's replace it with an implementation that is
>> structured in a way to resemble the grammar.
>> 
>> Such a new implementation, although a bit more verbose, is arguably
>> easier to reason about and to adapt to any extension we do to the
>> grammer in the future.
>> 
>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>
> Aside adding the kerneldoc, this looks good to me.  It will definitely
> make it easier to define and implement additions to the grammar in the
> future, if necessary.
>
>
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
>
>> ---
>>  drivers/gpu/drm/xe/xe_rtp.c | 138 ++++++++++++++++++++++++++++----------------
>>  1 file changed, 88 insertions(+), 50 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/xe/xe_rtp.c b/drivers/gpu/drm/xe/xe_rtp.c
>> index 976a2e1f5592..dec9d94e6fb0 100644
>> --- a/drivers/gpu/drm/xe/xe_rtp.c
>> +++ b/drivers/gpu/drm/xe/xe_rtp.c
>> @@ -30,11 +30,28 @@ static bool has_samedia(const struct xe_device *xe)
>>  	return xe->info.media_verx100 >= 1300;
>>  }
>>  
>> -static bool rule_match_item(const struct xe_device *xe,
>> -			    struct xe_gt *gt,
>> -			    struct xe_hw_engine *hwe,
>> -			    const struct xe_rtp_rule *r)
>> +struct rule_match_ctx {
>> +	const struct xe_device *xe;
>> +	struct xe_gt *gt;
>> +	struct xe_hw_engine *hwe;
>> +	const struct xe_rtp_rule *rules;
>> +	const unsigned int n_rules;
>> +	unsigned int head;
>> +	int err;
>> +};
>> +
>> +static bool rule_is_item(const struct xe_rtp_rule *r)
>> +{
>> +	return r->match_type != XE_RTP_MATCH_OR;
>> +}
>> +
>> +static bool rule_match_item(struct rule_match_ctx *match_ctx)
>>  {
>> +	const struct xe_device *xe = match_ctx->xe;
>> +	struct xe_gt *gt = match_ctx->gt;
>> +	struct xe_hw_engine *hwe = match_ctx->hwe;
>> +	const struct xe_rtp_rule *r = &match_ctx->rules[match_ctx->head];
>> +
>>  	switch (r->match_type) {
>>  	case XE_RTP_MATCH_PLATFORM:
>>  		return xe->info.platform == r->platform;
>> @@ -120,6 +137,63 @@ static bool rule_match_item(const struct xe_device *xe,
>>  	}
>>  }
>>  
>> +/*
>> + * Match a conjunctive set of rules (rules joined by an implicit "AND").
>> + *
>> + * Once one item evaluates to false, the remaining items are not evaluated
>> + * anymore.  Nevetheless, all rules are consumed to allow detecting syntax
>> + * errors.
>> + */
>> +static bool rule_match_and(struct rule_match_ctx *match_ctx, bool parse_only)
>> +{
>> +	bool match = true;
>> +	unsigned int count = 0;
>> +
>> +	while (match_ctx->head < match_ctx->n_rules &&
>> +	       rule_is_item(&match_ctx->rules[match_ctx->head])) {
>> +		if (!parse_only)
>> +			match = rule_match_item(match_ctx);
>> +
>> +		if (!match)
>> +			parse_only = true;
>> +
>> +		match_ctx->head++;
>> +		count++;
>> +	}
>> +
>> +	if (drm_WARN_ON(&match_ctx->xe->drm, !count)) {
>> +		match_ctx->err = -EINVAL;
>> +
>> +		if (!parse_only)
>> +			match = false;
>> +	}
>> +
>> +	return match;
>> +}
>> +
>> +/*
>> + * Match a disjunctive set of rules (subset of rules joined by
>> + * "XE_RTP_MATCH_OR").
>> + *
>> + * Once one subset evaluates to true, the remaining items are not evaluated
>> + * anymore. Nevetheless, all rules are consumed to allow detecting syntax
>> + * errors.
>> + */
>> +static bool rule_match_or(struct rule_match_ctx *match_ctx)
>> +{
>> +	bool match = rule_match_and(match_ctx, false);
>> +
>> +	while (match_ctx->head < match_ctx->n_rules &&
>> +	       match_ctx->rules[match_ctx->head].match_type == XE_RTP_MATCH_OR) {
>> +		/* Consume XE_RTP_MATCH_OR. */
>> +		match_ctx->head++;
>> +
>> +		match = rule_match_and(match_ctx, match);
>> +	}
>> +
>> +	return match;
>> +}
>> +
>>  static bool rule_matches_with_err(const struct xe_device *xe,
>>  				  struct xe_gt *gt,
>>  				  struct xe_hw_engine *hwe,
>> @@ -127,55 +201,19 @@ static bool rule_matches_with_err(const struct xe_device *xe,
>>  				  unsigned int n_rules,
>>  				  int *err)
>>  {
>> -	const struct xe_rtp_rule *r;
>> -	unsigned int i, rcount = 0;
>> -	bool short_circuit_or = false;
>> +	struct rule_match_ctx match_ctx = {
>> +		.xe = xe,
>> +		.gt = gt,
>> +		.hwe = hwe,
>> +		.rules = rules,
>> +		.n_rules = n_rules,
>> +	};
>> +	bool match = rule_match_or(&match_ctx);
>>  
>>  	if (err)
>> -		*err = 0;
>> -
>> -	for (r = rules, i = 0; i < n_rules; r = &rules[++i]) {
>> -		if (r->match_type == XE_RTP_MATCH_OR) {
>> -			if (drm_WARN_ON(&xe->drm, !rcount)) {
>> -				if (err)
>> -					*err = -EINVAL;
>> -				continue;
>> -			}
>> -
>> -			/*
>> -			 * This is only reached if a complete conjunction of
>> -			 * rules passed, in which case we short-circuit rule
>> -			 * evaluation, but still keep parsing to find any syntax
>> -			 * errors.
>> -			 */
>> -			short_circuit_or = true;
>> -			rcount = 0;
>> -			continue;
>> -		}
>> -
>> -		if (short_circuit_or || rule_match_item(xe, gt, hwe, r)) {
>> -			rcount++;
>> -		} else {
>> -			/*
>> -			 * Advance rules until we find XE_RTP_MATCH_OR to check
>> -			 * if there's another set of conditions to check
>> -			 */
>> -			while (++i < n_rules && rules[i].match_type != XE_RTP_MATCH_OR)
>> -				;
>> -
>> -			if (i >= n_rules)
>> -				return false;
>> -
>> -			rcount = 0;
>> -		}
>> -	}
>> -
>> -	if (drm_WARN_ON(&xe->drm, !rcount)) {
>> -		if (err)
>> -			*err = -EINVAL;
>> -	}
>> +		*err = match_ctx.err;
>>  
>> -	return short_circuit_or || rcount;
>> +	return match;
>>  }
>>  
>>  static bool rule_matches(const struct xe_device *xe,
>> 
>> -- 
>> 2.53.0
>> 
>
> -- 
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation