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

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

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

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

Right.  Indeed, this patch changes that behavior.

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

Makes sense.  After reading this, I realized that we have some kind of
inconsistency in the current RTP code: "yes-or" would produce a match,
but "or-yes" would not.

I think we can add a preliminary patch to fix that: treat empty
conjunctions as false, meaning that, in the "or-yes" case, there would
be no short-circuit to false (the current behavior) and evaluation would
continue to catch the "yes" match.

With that in place, this patch would be only about making sure that we
fully parse the reulset without changing the matching behavior. I'll do
that in v2.

--
Gustavo Sousa

> 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