Re: [PATCH 5/6] drm/xe/rtp: Fully parse the ruleset

[Matt Roper <matthew.d.roper@intel.com>]

On Wed, Jan 14, 2026 at 07:49:55PM -0300, Gustavo Sousa wrote:
> The function rule_matches() short-circuits evaluation of the implicit
> conjunctions (each substring of rules not containing OR) and the
> explicit disjunctions (implicit conjunctions joined by OR). In other
> words:
> 
>   - in a conjunction, once a rule evaluate to false, we skip to the next
>     OR (if any) to evaluate the next conjunction;
>   - in a disjunction, once a conjunction evaluates to true, we return
>     true and skip evaluating all the remaining rules.
> 
> While this behavior results in a correct logical value, it has the
> side-effect that rule set does not get fully "parsed", allowing
> incomplete constructs like (rule1, OR) to evaluate to true when rule1 is
> true.  We should treat such constructs as invalid and treat them the
> same way we do for stuff like (OR, rule1).
> 
> As such, update rule_matches() to "parse" the whole rule set, and that
> while keeping the short-circuit aspect of evaluation.

I took "keeping the short-circuit aspect of evaluation" to mean that a
"yes-or" rule would still be treated as a match functionally, but
flagged as an error for kunit checks.  But it seems we're also changing
the behavior to reject the whole record in that case, which seems like
it would just make the mistake worse in non-kunit settings.

I think it would be better to treat "or-yes" and "yes-or" type mistakes
as a match, but flag them as errors in kunit.  I'd expect the most
common case we'd have such a mistake would be if someone is deleting
something that no longer applies (e.g., a pre-production workaround set
of rules) and they just forget to also remove the adjacent OR.  The
remaining parts of the rule should still be valid with that kind of
mistake, so treating the whole rule as not a match seems wrong.


Matt

> 
> Let's also update the kunit test to include instances of those
> incomplete constructs to reflect this change.
> 
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> ---
>  drivers/gpu/drm/xe/tests/xe_rtp_test.c | 12 +++++++
>  drivers/gpu/drm/xe/xe_rtp.c            | 61 +++++++++++++++++++---------------
>  2 files changed, 46 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/tests/xe_rtp_test.c b/drivers/gpu/drm/xe/tests/xe_rtp_test.c
> index 19c7142b2fe4..f0122553644e 100644
> --- a/drivers/gpu/drm/xe/tests/xe_rtp_test.c
> +++ b/drivers/gpu/drm/xe/tests/xe_rtp_test.c
> @@ -177,6 +177,18 @@ static const struct rtp_rules_test_case rtp_rules_cases[] = {
>  		.expected_err = -EINVAL,
>  		XE_RTP_RULES(OR, FUNC(match_yes)),
>  	},
> +	{
> +		.name = "anything-or",
> +		.expected_match = false,
> +		.expected_err = -EINVAL,
> +		XE_RTP_RULES(FUNC(match_yes), OR),
> +	},
> +	{
> +		.name = "anything-or-or-anything",
> +		.expected_match = false,
> +		.expected_err = -EINVAL,
> +		XE_RTP_RULES(FUNC(match_yes), OR, OR, FUNC(match_yes)),
> +	},
>  
>  	/* No match because hwe is NULL. */
>  	{
> diff --git a/drivers/gpu/drm/xe/xe_rtp.c b/drivers/gpu/drm/xe/xe_rtp.c
> index dabc2e74e2ce..55df9c16a3cc 100644
> --- a/drivers/gpu/drm/xe/xe_rtp.c
> +++ b/drivers/gpu/drm/xe/xe_rtp.c
> @@ -123,47 +123,54 @@ static bool rule_matches_with_err(const struct xe_device *xe,
>  {
>  	const struct xe_rtp_rule *r;
>  	unsigned int i, rcount = 0;
> +	bool parse_only = false;
> +	bool match = false;
>  
>  	if (err)
>  		*err = 0;
>  
>  	for (r = rules, i = 0; i < n_rules; r = &rules[++i]) {
> -		if (r->match_type == XE_RTP_MATCH_OR)
> -			/*
> -			 * This is only reached if a complete set of
> -			 * rules passed or none were evaluated. For both cases,
> -			 * shortcut the other rules and return the proper value.
> -			 */
> -			goto done;
> +		if (r->match_type == XE_RTP_MATCH_OR) {
> +			if (drm_WARN_ON(&xe->drm, !rcount)) {
> +				parse_only = true;
> +				match = false;
> +				if (err)
> +					*err = -EINVAL;
> +			} else if (match) {
> +				parse_only = true;
> +			}
> +
> +			rcount = 0;
> +
> +			continue;
> +		}
> +
> +		rcount++;
> +
> +		if (parse_only || !rule_match_item(xe, gt, hwe, r)) {
> +			if (!parse_only)
> +				match = false;
>  
> -		if (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;
> +			while (i + 1 < n_rules && rules[i + 1].match_type != XE_RTP_MATCH_OR) {
> +				i++;
> +				rcount++;
> +			}
> +		} else {
> +			match = true;
>  		}
>  	}
>  
> -done:
> -	if (drm_WARN_ON(&xe->drm, !rcount))
> -		goto error;
> -
> -	return true;
> -
> -error:
> -	if (err)
> -		*err = -EINVAL;
> +	if (drm_WARN_ON(&xe->drm, !rcount)) {
> +		match = false;
> +		if (err)
> +			*err = -EINVAL;
> +	}
>  
> -	return false;
> +	return match;
>  }
>  
>  static bool rule_matches(const struct xe_device *xe,
> 
> -- 
> 2.52.0
> 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation