[Intel-xe] [PATCH v2 12/17] drm/xe/rtp: Improve magic macros for RTP tables
Michal Wajdeczko
michal.wajdeczko at intel.com
Tue Apr 25 17:00:31 UTC 2023
On 25.04.2023 15:17, Lucas De Marchi wrote:
> 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.
sorry, but I still don't follow
if a ready to use macro is defined, I see no reason to define own copy
just because we don't like to include header where it was added/defined
if size of the kernel.h is a real problem, then I would expect moving
CONCAT/COUNT macros to linux/concat.h/count.h rather then creating its
own macros in private header (unless they are really non-generic)
>
>>
>>> 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
also the actual outcome of the macro includes "XE_RTP" prefix being
added automatically (making this a non-generic concat macro)
> *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_)
or
#define __XE_RTP_PASTE_ACTION(a) _XE_RTP_CONCAT(ACTION, a)
#define __XE_RTP_PASTE_RULE(r) _XE_RTP_CONCAT(RULE, r)
>
>
> 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