[PATCH 2/5] drm/xe/rtp: Allow to OR rules

[Lucas De Marchi]

On Mon, Jun 17, 2024 at 12:09:26PM GMT, Matt Roper wrote:
>On Mon, Jun 17, 2024 at 11:25:15AM -0700, Lucas De Marchi wrote:
>> Some workarounds started to depend in different set of conditions where
>> the action should be applied if any of them match. See e.g.
>> commit 24d0d98af1c3 ("drm/xe/xe2lpm: Fixup Wa_14020756599"). Add
>> XE_RTP_MATCH_OR that allows to implement a logical OR for the rules.
>> Normal precedence applies:
>>
>> 	r1, r2, OR, r3
>>
>> means
>>
>> 	(r1 AND r2) OR r3
>>
>> The check is shortcut as soon as a set of conditions match.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> ---
>>  drivers/gpu/drm/xe/tests/xe_rtp_test.c | 54 ++++++++++++++++++++++++++
>>  drivers/gpu/drm/xe/xe_rtp.c            | 20 +++++++++-
>>  drivers/gpu/drm/xe/xe_rtp.h            | 21 ++++++++++
>>  drivers/gpu/drm/xe/xe_rtp_types.h      |  1 +
>>  4 files changed, 94 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/tests/xe_rtp_test.c b/drivers/gpu/drm/xe/tests/xe_rtp_test.c
>> index 474a0b222ce1..bf7fee9b20cc 100644
>> --- a/drivers/gpu/drm/xe/tests/xe_rtp_test.c
>> +++ b/drivers/gpu/drm/xe/tests/xe_rtp_test.c
>> @@ -90,6 +90,60 @@ static const struct rtp_test_case cases[] = {
>>  			{}
>>  		},
>>  	},
>> +	{
>> +		.name = "match-or-first",
>> +		.expected_reg = REGULAR_REG1,
>> +		.expected_set_bits = REG_BIT(0),
>> +		.expected_clr_bits = REG_BIT(0),
>> +		.expected_count = 1,
>> +		.entries = (const struct xe_rtp_entry_sr[]) {
>> +			{ XE_RTP_NAME("first"),
>> +			  XE_RTP_RULES(FUNC(match_yes), OR, FUNC(match_no)),
>> +			  XE_RTP_ACTIONS(SET(REGULAR_REG1, REG_BIT(0)))
>> +			},
>> +			{}
>> +		},
>> +	},
>> +	{
>> +		.name = "match-or-last",
>> +		.expected_reg = REGULAR_REG1,
>> +		.expected_set_bits = REG_BIT(0),
>> +		.expected_clr_bits = REG_BIT(0),
>> +		.expected_count = 1,
>> +		.entries = (const struct xe_rtp_entry_sr[]) {
>> +			{ XE_RTP_NAME("last"),
>> +			  XE_RTP_RULES(FUNC(match_no), OR, FUNC(match_yes)),
>> +			  XE_RTP_ACTIONS(SET(REGULAR_REG1, REG_BIT(0)))
>> +			},
>> +			{}
>> +		},
>> +	},
>> +	{
>> +		.name = "match-or-empty",
>> +		.expected_reg = REGULAR_REG1,
>> +		.expected_set_bits = REG_BIT(0),
>> +		.expected_clr_bits = REG_BIT(0),
>> +		.expected_count = 1,
>> +		.entries = (const struct xe_rtp_entry_sr[]) {
>> +			{ XE_RTP_NAME("empty-matches-ok"),
>> +			  XE_RTP_RULES(OR, FUNC(match_no)),
>> +			  XE_RTP_ACTIONS(SET(REGULAR_REG1, REG_BIT(0)))
>> +			},
>> +			{}
>> +		},
>> +	},
>
>Is this a case we actually want to support?  See note farther down.

I just didn't want to add the check in the code as I thought it was not
worth it. So I decided to "document" the corner case as intended.

>
>> +	{
>> +		.name = "match-or-none",
>> +		.expected_reg = REGULAR_REG1,
>> +		.expected_count = 0,
>> +		.entries = (const struct xe_rtp_entry_sr[]) {
>> +			{ XE_RTP_NAME("none"),
>> +			  XE_RTP_RULES(FUNC(match_no), OR, FUNC(match_no)),
>> +			  XE_RTP_ACTIONS(SET(REGULAR_REG1, REG_BIT(0)))
>> +			},
>> +			{}
>> +		},
>> +	},
>>  	{
>>  		.name = "no-match-no-add-multiple-rules",
>>  		.expected_reg = REGULAR_REG1,
>> diff --git a/drivers/gpu/drm/xe/xe_rtp.c b/drivers/gpu/drm/xe/xe_rtp.c
>> index eff1c9c2f5cc..66ca58fa5739 100644
>> --- a/drivers/gpu/drm/xe/xe_rtp.c
>> +++ b/drivers/gpu/drm/xe/xe_rtp.c
>> @@ -40,6 +40,13 @@ static bool rule_matches(const struct xe_device *xe,
>>
>>  	for (r = rules, i = 0; i < n_rules; r = &rules[++i]) {
>>  		switch (r->match_type) {
>> +		case XE_RTP_MATCH_OR:
>> +			/*
>> +			 * This is only reached if a complete set of required
>> +			 * rules passed, so return success: the remaining rules
>> +			 * do not matter
>> +			 */
>> +			return true;
>
>Would it be worth adding an xe_assert here to make sure OR isn't the
>first/last rule in the list?  I can see someone deleting a workaround
>condition that's no longer relevant and accidentally leaving a dangling
>OR, leading to an unintentional "always pass" condition.

yeah... it doesn't seems good and I can't see why it would ever be
correct to have and empty side. There is at least 1 more corner case:

	FUNC(foo), OR, OR, FUNC(bar)

maybe instead of checking first/last, check a counter with the number of
rules processed != OR.

>
>Up to you whether this is something we want to deal with or not.  Either
>way,
>
>Reviewed-by: Matt Roper <matthew.d.roper at intel.com>


I will think about it and respin if needed. Thanks

Lucas De Marchi