Re: [PATCH 3/6] drm/xe/rtp: Do not break parsing when missing context

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

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

> On Wed, Jan 14, 2026 at 07:49:53PM -0300, Gustavo Sousa wrote:
>> With the current implementation, the RTP framework will cause parsing of
>> the rule set to be interrupted if one rule requires a context item (gt
>> or hwe) that is missing (i.e. when the value is NULL).
>> 
>> This is arguably a semantic error instead of a syntactic one, meaning
>> that RTP should not interrupt parsing the rules.  With the current
>> behavior, we would miss detecting other errors that could appear in the
>> remaining rules and could also prevent valid rules joined by "OR" from
>> being evaluated.
>> 
>> Make sure that we do not stop parsing the rule set when detecting
>> missing context and let's add rtp_rules_test_cases to reflect that.
>> 
>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> ---
>>  drivers/gpu/drm/xe/tests/xe_rtp_test.c | 28 ++++++++++++++++
>>  drivers/gpu/drm/xe/xe_rtp.c            | 60 ++++++++++++++++++++++------------
>>  2 files changed, 68 insertions(+), 20 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/xe/tests/xe_rtp_test.c b/drivers/gpu/drm/xe/tests/xe_rtp_test.c
>> index b2286dd9d92a..19c7142b2fe4 100644
>> --- a/drivers/gpu/drm/xe/tests/xe_rtp_test.c
>> +++ b/drivers/gpu/drm/xe/tests/xe_rtp_test.c
>> @@ -177,6 +177,34 @@ static const struct rtp_rules_test_case rtp_rules_cases[] = {
>>  		.expected_err = -EINVAL,
>>  		XE_RTP_RULES(OR, FUNC(match_yes)),
>>  	},
>> +
>> +	/* No match because hwe is NULL. */
>> +	{
>> +		.name = "engine-class",
>
> Nitpick:  The names for these tests doesn't really make it clear what's
> being tested.  Something like "missing-context-engine-class" might be
> better?
>
> I'm wondering if an even more useful test would be one that scans our
> actual RTP tables and just looks for anything on gt_was[] or oob_was[]
> that has an engine class rule, or anything on device_oob_was[] that has
> either an IP version rule or an engine class rule.  If we catch those
> mistakes during early kunit testing, then we shouldn't really need to
> worry about what the behavior would be if one of them survived to be
> parsed on a live device.

That seems like a good idea.

>
> Anyway, the rest of the patch looks fine, so aside from improving the
> names,
>
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

Thanks!

--
Gustavo Sousa

>
>
> Matt
>
>> +		.expected_match = false,
>> +		XE_RTP_RULES(ENGINE_CLASS(RENDER)),
>> +	},
>> +
>> +	/*
>> +	 * Missing context (hwe==NULL) does not cause parsing to stop, hence we
>> +	 * expect a match.
>> +	 */
>> +	{
>> +		.name = "engine-class-or-yes",
>> +		.expected_match = true,
>> +		XE_RTP_RULES(ENGINE_CLASS(RENDER), OR, FUNC(match_yes)),
>> +	},
>> +
>> +	/*
>> +	 * Missing context (hwe==NULL) does not cause parsing to stop, hence we
>> +	 * expect a syntax error.
>> +	 */
>> +	{
>> +		.name = "engine-class-or-or-yes",
>> +		.expected_match = false,
>> +		.expected_err = -EINVAL,
>> +		XE_RTP_RULES(ENGINE_CLASS(RENDER), OR, OR, FUNC(match_yes)),
>> +	},
>>  };
>>  
>>  static void xe_rtp_rules_tests(struct kunit *test)
>> diff --git a/drivers/gpu/drm/xe/xe_rtp.c b/drivers/gpu/drm/xe/xe_rtp.c
>> index e955df6c22ca..e0bed5ac1369 100644
>> --- a/drivers/gpu/drm/xe/xe_rtp.c
>> +++ b/drivers/gpu/drm/xe/xe_rtp.c
>> @@ -61,60 +61,76 @@ static bool rule_matches_with_err(const struct xe_device *xe,
>>  				xe->info.subplatform == r->subplatform;
>>  			break;
>>  		case XE_RTP_MATCH_GRAPHICS_VERSION:
>> -			if (drm_WARN_ON(&xe->drm, !gt))
>> -				goto error;
>> +			if (drm_WARN_ON(&xe->drm, !gt)) {
>> +				match = false;
>> +				break;
>> +			}
>>  
>>  			match = xe->info.graphics_verx100 == r->ver_start &&
>>  				(!has_samedia(xe) || !xe_gt_is_media_type(gt));
>>  			break;
>>  		case XE_RTP_MATCH_GRAPHICS_VERSION_RANGE:
>> -			if (drm_WARN_ON(&xe->drm, !gt))
>> -				goto error;
>> +			if (drm_WARN_ON(&xe->drm, !gt)) {
>> +				match = false;
>> +				break;
>> +			}
>>  
>>  			match = xe->info.graphics_verx100 >= r->ver_start &&
>>  				xe->info.graphics_verx100 <= r->ver_end &&
>>  				(!has_samedia(xe) || !xe_gt_is_media_type(gt));
>>  			break;
>>  		case XE_RTP_MATCH_GRAPHICS_VERSION_ANY_GT:
>> -			if (drm_WARN_ON(&xe->drm, !gt))
>> -				goto error;
>> +			if (drm_WARN_ON(&xe->drm, !gt)) {
>> +				match = false;
>> +				break;
>> +			}
>>  
>>  			match = xe->info.graphics_verx100 == r->ver_start;
>>  			break;
>>  		case XE_RTP_MATCH_GRAPHICS_STEP:
>> -			if (drm_WARN_ON(&xe->drm, !gt))
>> -				goto error;
>> +			if (drm_WARN_ON(&xe->drm, !gt)) {
>> +				match = false;
>> +				break;
>> +			}
>>  
>>  			match = xe->info.step.graphics >= r->step_start &&
>>  				xe->info.step.graphics < r->step_end &&
>>  				(!has_samedia(xe) || !xe_gt_is_media_type(gt));
>>  			break;
>>  		case XE_RTP_MATCH_MEDIA_VERSION:
>> -			if (drm_WARN_ON(&xe->drm, !gt))
>> -				goto error;
>> +			if (drm_WARN_ON(&xe->drm, !gt)) {
>> +				match = false;
>> +				break;
>> +			}
>>  
>>  			match = xe->info.media_verx100 == r->ver_start &&
>>  				(!has_samedia(xe) || xe_gt_is_media_type(gt));
>>  			break;
>>  		case XE_RTP_MATCH_MEDIA_VERSION_RANGE:
>> -			if (drm_WARN_ON(&xe->drm, !gt))
>> -				goto error;
>> +			if (drm_WARN_ON(&xe->drm, !gt)) {
>> +				match = false;
>> +				break;
>> +			}
>>  
>>  			match = xe->info.media_verx100 >= r->ver_start &&
>>  				xe->info.media_verx100 <= r->ver_end &&
>>  				(!has_samedia(xe) || xe_gt_is_media_type(gt));
>>  			break;
>>  		case XE_RTP_MATCH_MEDIA_STEP:
>> -			if (drm_WARN_ON(&xe->drm, !gt))
>> -				goto error;
>> +			if (drm_WARN_ON(&xe->drm, !gt)) {
>> +				match = false;
>> +				break;
>> +			}
>>  
>>  			match = xe->info.step.media >= r->step_start &&
>>  				xe->info.step.media < r->step_end &&
>>  				(!has_samedia(xe) || xe_gt_is_media_type(gt));
>>  			break;
>>  		case XE_RTP_MATCH_MEDIA_VERSION_ANY_GT:
>> -			if (drm_WARN_ON(&xe->drm, !gt))
>> -				goto error;
>> +			if (drm_WARN_ON(&xe->drm, !gt)) {
>> +				match = false;
>> +				break;
>> +			}
>>  
>>  			match = xe->info.media_verx100 == r->ver_start;
>>  			break;
>> @@ -125,14 +141,18 @@ static bool rule_matches_with_err(const struct xe_device *xe,
>>  			match = xe->info.is_dgfx;
>>  			break;
>>  		case XE_RTP_MATCH_ENGINE_CLASS:
>> -			if (drm_WARN_ON(&xe->drm, !hwe))
>> -				goto error;
>> +			if (drm_WARN_ON(&xe->drm, !hwe)) {
>> +				match = false;
>> +				break;
>> +			}
>>  
>>  			match = hwe->class == r->engine_class;
>>  			break;
>>  		case XE_RTP_MATCH_NOT_ENGINE_CLASS:
>> -			if (drm_WARN_ON(&xe->drm, !hwe))
>> -				goto error;
>> +			if (drm_WARN_ON(&xe->drm, !hwe)) {
>> +				match = false;
>> +				break;
>> +			}
>>  
>>  			match = hwe->class != r->engine_class;
>>  			break;
>> 
>> -- 
>> 2.52.0
>> 
>
> -- 
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation