[Intel-xe] [PATCH v2 12/17] drm/xe/rtp: Improve magic macros for RTP tables
Lucas De Marchi
lucas.demarchi at intel.com
Tue Apr 25 13:17:05 UTC 2023
On Tue, Apr 25, 2023 at 01:03:54PM +0200, Michal Wajdeczko wrote:
>
>
>On 22.04.2023 00:32, Lucas De Marchi wrote:
>> The macros for writing the rtp tables have some downsides:
>>
>> 1) They are fragile, and when making a typo it's hard to know
>> where it's failing for someone not used to them
>> 2) They polute the global scope. The CONCAT/COUNT_ARGS/etc only
>> work because they are defined exactly the same as the ones in
>> kernel.h.
>
>hmm, I wonder why they were defined locally in first place ?
>Can't you always use definitions from kernel.h ?
I didn't want to include kernel.h for the 2 macros it would use
when people are actually trying to reduce the size of kernel.h due to
being kind of what our i915_reg.h became. Also the count args to 12 is
not exactly what we want.
>
>> 3) They are not compatible with rules/actions with registers
>> passing additional options
>>
>> This new implementation fixes (2) and (3).
>>
>> For (2), prefix everything with "_XE", so there is less chance of
>> clashes. Also, the maximum number of rules and actions is 4, so there is
>> no need to make the helper macros go until 12: reduce that to 5, so if
>> more arguments is passed, the compiler gives an error that there is not
>> definition with _5 suffix.
>
>I'm not convinced that adding local definitions with XE_ prefix that
>mimics functionality already defined in kernel.h is the right way to go
see above
>
>>
>> For (3), change the implementation to concentrate on "pasting a prefix
>> to each argument" rather than the more general "call any macro for each
>> argument". Hopefully this will avoid trying to extending this infra and
>> making it more complex. The new changes to how the arguments are
>> constructed using tuples will allow extending the interface where it's
>> needed: passing additional register fields to allow creating our own
>> xe_reg_t.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> ---
>> drivers/gpu/drm/xe/xe_rtp.h | 68 ++++++++++++++++++-------------------
>> 1 file changed, 33 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_rtp.h b/drivers/gpu/drm/xe/xe_rtp.h
>> index a3be7c77753a..53650f09efe9 100644
>> --- a/drivers/gpu/drm/xe/xe_rtp.h
>> +++ b/drivers/gpu/drm/xe/xe_rtp.h
>> @@ -22,43 +22,42 @@ struct xe_reg_sr;
>> /*
>> * Helper macros - not to be used outside this header.
>> */
>> -/* This counts to 12. Any more, it will return 13th argument. */
>> -#define __COUNT_ARGS(_0, _1, _2, _3, _4, _5, _6, _7, _8, _9, _10, _11, _12, _n, X...) _n
>> -#define COUNT_ARGS(X...) __COUNT_ARGS(, ##X, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0)
>> +#define _XE_ESC(...) __VA_ARGS__
>> +#define _XE_COUNT_ARGS(...) _XE_ESC(__XE_COUNT_ARGS(__VA_ARGS__,5,4,3,2,1,))
>> +#define __XE_COUNT_ARGS(_,_5,_4,_3,_2,X_,...) X_
>>
>> -#define __CONCAT(a, b) a ## b
>> -#define CONCATENATE(a, b) __CONCAT(a, b)
>> +#define _XE_CONCAT(a, b) __XE_CONCAT(a, b)
>> +#define __XE_CONCAT(a, b) a ## b
>>
>> -#define __CALL_FOR_EACH_1(MACRO_, x, ...) MACRO_(x)
>> -#define __CALL_FOR_EACH_2(MACRO_, x, ...) \
>> - MACRO_(x) __CALL_FOR_EACH_1(MACRO_, ##__VA_ARGS__)
>> -#define __CALL_FOR_EACH_3(MACRO_, x, ...) \
>> - MACRO_(x) __CALL_FOR_EACH_2(MACRO_, ##__VA_ARGS__)
>> -#define __CALL_FOR_EACH_4(MACRO_, x, ...) \
>> - MACRO_(x) __CALL_FOR_EACH_3(MACRO_, ##__VA_ARGS__)
>> +#define _XE_FIRST(...) _XE_ESC(__XE_FIRST(__VA_ARGS__,))
>> +#define __XE_FIRST(x_,...) x_
>> +#define _XE_TUPLE_TAIL(...) _XE_ESC(__XE_TUPLE_TAIL(__VA_ARGS__))
>> +#define __XE_TUPLE_TAIL(x_,...) (__VA_ARGS__)
>>
>> -#define _CALL_FOR_EACH(NARGS_, MACRO_, x, ...) \
>> - CONCATENATE(__CALL_FOR_EACH_, NARGS_)(MACRO_, x, ##__VA_ARGS__)
>> -#define CALL_FOR_EACH(MACRO_, x, ...) \
>> - _CALL_FOR_EACH(COUNT_ARGS(x, ##__VA_ARGS__), MACRO_, x, ##__VA_ARGS__)
>> +#define __XE_PASTE_SEP_COMMA ,
>> +#define __XE_PASTE_SEP_BITWISE_OR |
>> +#define __XE_PASTE(prefix_, sep_, args_) _XE_ESC(_XE_CONCAT(__XE_PASTE_,_XE_COUNT_ARGS args_)(prefix_, sep_, args_))
>> +#define __XE_PASTE_1(prefix_, sep_, args_) prefix_ args_
>> +#define __XE_PASTE_2(prefix_, sep_, args_) prefix_(_XE_FIRST args_) __XE_PASTE_SEP_ ## sep_ __XE_PASTE_1(prefix_, sep_, _XE_TUPLE_TAIL args_)
>> +#define __XE_PASTE_3(prefix_, sep_, args_) prefix_(_XE_FIRST args_) __XE_PASTE_SEP_ ## sep_ __XE_PASTE_2(prefix_, sep_, _XE_TUPLE_TAIL args_)
>> +#define __XE_PASTE_4(prefix_, sep_, args_) prefix_(_XE_FIRST args_) __XE_PASTE_SEP_ ## sep_ __XE_PASTE_3(prefix_, sep_, _XE_TUPLE_TAIL args_)
>>
>> -#define _XE_RTP_REG(x_) (x_), XE_RTP_REG_REGULAR
>> -#define _XE_RTP_MCR_REG(x_) (x_), XE_RTP_REG_MCR
>> +#define _XE_RTP_REG(x_) (x_), XE_RTP_REG_REGULAR
>> +#define _XE_RTP_MCR_REG(x_) (x_), XE_RTP_REG_MCR
>>
>> /*
>> * Helper macros for concatenating prefix - do not use them directly outside
>> * this header
>> */
>> -#define __ADD_XE_RTP_ENTRY_FLAG_PREFIX(x) CONCATENATE(XE_RTP_ENTRY_FLAG_, x) |
>> -#define __ADD_XE_RTP_ACTION_FLAG_PREFIX(x) CONCATENATE(XE_RTP_ACTION_FLAG_, x) |
>> -#define __ADD_XE_RTP_RULE_PREFIX(x) CONCATENATE(XE_RTP_RULE_, x) ,
>> -#define __ADD_XE_RTP_ACTION_PREFIX(x) CONCATENATE(XE_RTP_ACTION_, x) ,
>> +#define __XE_PASTE_XE_RTP_ENTRY_FLAG_(x_) _XE_CONCAT(XE_RTP_ENTRY_FLAG_, x_)
>> +#define __XE_PASTE_XE_RTP_ACTION_FLAG_(x_) _XE_CONCAT(XE_RTP_ACTION_FLAG_, x_)
>> +#define __XE_PASTE_XE_RTP_ACTION_(x_) _XE_CONCAT(XE_RTP_ACTION_, x_)
>> +#define __XE_PASTE_XE_RTP_RULE_(x_) _XE_CONCAT(XE_RTP_RULE_, x_)
>
>maybe local macros should be defined with full XE_RTP prefix and be
>tailored for your specific RTP usecases:
>
>#define _XE_RTP_CONCAT(a, b) __XE_RTP_CONCAT(a, b)
>#define __XE_RTP_CONCAT(a, b) XE_RTP_ ## a ## _ ## b
these are not to be used throughout the source code. The only difference
I see from this and what I did is the prefix being *XE_RTP_ rather than
*XE_. Since this is not to be used outside the header, I'm not seeing
much value in make it even longer.
>
>#define __XE_PASTE_XE_RTP_ACTION_(x_) _XE_RTP_CONCAT(ACTION, x_)
>#define __XE_PASTE_XE_RTP_RULE_(x_) _XE_RTP_CONCAT(RULE, x_)
this contradicts what you said and following that would make it very
ugly:
#define __XE_RTP_PASTE_XE_RTP_ACTION_(x_) _XE_RTP_CONCAT(ACTION, x_)
#define __XE_RTP_PASTE_XE_RTP_RULE_(x_) _XE_RTP_CONCAT(RULE, x_)
Lucas De Marchi
>
>then there will be no complains about overlapping functionality with
>macros from kernel.h
>
>>
>> /*
>> * Macros to encode rules to match against platform, IP version, stepping, etc.
>> * Shouldn't be used directly - see XE_RTP_RULES()
>> */
>> -
>> #define _XE_RTP_RULE_PLATFORM(plat__) \
>> { .match_type = XE_RTP_MATCH_PLATFORM, .platform = plat__ }
>>
>> @@ -315,8 +314,8 @@ struct xe_reg_sr;
>> * ...
>> * };
>> */
>> -#define XE_RTP_ENTRY_FLAG(f1_, ...) \
>> - .flags = (CALL_FOR_EACH(__ADD_XE_RTP_ENTRY_FLAG_PREFIX, f1_, ##__VA_ARGS__) 0)
>> +#define XE_RTP_ENTRY_FLAG(...) \
>> + .flags = (__XE_PASTE(__XE_PASTE_XE_RTP_ENTRY_FLAG_, BITWISE_OR, (__VA_ARGS__)))
>>
>> /**
>> * XE_RTP_ACTION_FLAG - Helper to add multiple flags to a struct xe_rtp_action
>> @@ -338,8 +337,8 @@ struct xe_reg_sr;
>> * ...
>> * };
>> */
>> -#define XE_RTP_ACTION_FLAG(f1_, ...) \
>> - .flags = (CALL_FOR_EACH(__ADD_XE_RTP_ACTION_FLAG_PREFIX, f1_, ##__VA_ARGS__) 0)
>> +#define XE_RTP_ACTION_FLAG(...) \
>> + .flags = (__XE_PASTE(__XE_PASTE_XE_RTP_ACTION_FLAG_, BITWISE_OR, (__VA_ARGS__)))
>>
>> /**
>> * XE_RTP_RULES - Helper to set multiple rules to a struct xe_rtp_entry entry
>> @@ -361,16 +360,15 @@ struct xe_reg_sr;
>> * ...
>> * };
>> */
>> -#define XE_RTP_RULES(r1, ...) \
>> - .n_rules = COUNT_ARGS(r1, ##__VA_ARGS__), \
>> +#define XE_RTP_RULES(...) \
>> + .n_rules = _XE_COUNT_ARGS(__VA_ARGS__), \
>> .rules = (const struct xe_rtp_rule[]) { \
>> - CALL_FOR_EACH(__ADD_XE_RTP_RULE_PREFIX, r1, ##__VA_ARGS__) \
>> + __XE_PASTE(__XE_PASTE_XE_RTP_RULE_, COMMA, (__VA_ARGS__)) \
>> }
>>
>> /**
>> * XE_RTP_ACTIONS - Helper to set multiple actions to a struct xe_rtp_entry
>> - * @a1: Action to take. Last part of XE_RTP_ACTION_*
>> - * @...: Additional rules, defined like @r1
>> + * @...: Actions to be taken
>> *
>> * At least one rule is needed and up to 4 are supported. Multiple rules are
>> * AND'ed together, i.e. all the rules must evaluate to true for the entry to
>> @@ -388,10 +386,10 @@ struct xe_reg_sr;
>> * ...
>> * };
>> */
>> -#define XE_RTP_ACTIONS(a1, ...) \
>> - .n_actions = COUNT_ARGS(a1, ##__VA_ARGS__), \
>> +#define XE_RTP_ACTIONS(...) \
>> + .n_actions = _XE_COUNT_ARGS(__VA_ARGS__), \
>> .actions = (const struct xe_rtp_action[]) { \
>> - CALL_FOR_EACH(__ADD_XE_RTP_ACTION_PREFIX, a1, ##__VA_ARGS__) \
>> + __XE_PASTE(__XE_PASTE_XE_RTP_ACTION_, COMMA, (__VA_ARGS__)) \
>> }
>>
>> void xe_rtp_process(const struct xe_rtp_entry *entries, struct xe_reg_sr *sr,
More information about the Intel-xe
mailing list