[PATCH 0/2] Define generic helpers for manipulating macro arguments

[Lucas De Marchi]

On Thu, May 02, 2024 at 11:24:36AM GMT, Michal Wajdeczko wrote:
>
>
>On 02.05.2024 09:24, Andy Shevchenko wrote:
>> On Thu, May 02, 2024 at 12:32:19AM +0200, Michal Wajdeczko wrote:
>>> Michal Wajdeczko (2):
>>>   args.h: add more helpers for manipulating macro arguments
>>>   drm/xe/rtp: Prefer helper macros from args.h
>>>
>>>  drivers/gpu/drm/xe/xe_rtp.h         |   4 +-
>>>  drivers/gpu/drm/xe/xe_rtp_helpers.h |  26 +++----
>>>  include/linux/args.h                | 103 ++++++++++++++++++++++++++++
>>>  3 files changed, 115 insertions(+), 18 deletions(-)
>>
>> It's good in general to have the code being deduplicated, but the above
>> statistics is a bit scary.
>
>The statistics here might be little blurred due to added documentation.
>Without documentation this would look a little better:
>
> drivers/gpu/drm/xe/xe_rtp.h         |  4 ++--
> drivers/gpu/drm/xe/xe_rtp_helpers.h | 26 ++++++++++----------------
> include/linux/args.h                | 26 ++++++++++++++++++++++++++
> 3 files changed, 38 insertions(+), 18 deletions(-)
>
>> Do we have more users where we indeed make code much
>> nicer?
>
>A new user (still inside Xe driver) for these macro is on it's way, but
>before that I wanted to start with a preparation step and promote any
>generic helpers to the outside of the Xe driver, as it was pointed in
>similar case with other helper macro [1]
>
>[1] https://patchwork.freedesktop.org/patch/578134/?series=129854&rev=1
>
>> If not, let's keep these where it belongs to for now.
>>
>
>If this proposal didn't get any traction, which won't surprise me based
>on outcome of [2], then at least I would have some ground for creating a
>local xe_args.h or use xe_macros.h as a location for common args macros.
>
>[2]
>https://lore.kernel.org/lkml/20240214214654.1700-1-michal.wajdeczko@intel.com/

I share the same concern as Andy. When I saw this series I was a bit
more concerned because there are plans to change the rtp part in xe,
moving it partially to code generation rather than using CPP.

However looking at the changes you are moving here, they are more about
the simpler macros. And it's nice to have more documentation and make
them less magical.

We need to be careful that some of the macros are not generic and were
purposely built. Example:  _XE_COUNT_ARGS() and COUNT_ARGS() are not the
same thing and return a different value for 0 args for example. I think
we don't rely on that anymore, but we should really pass this through
CI. It should generate 100% the same code, so checking the .o matches
after this change would be good.

As per xe_args.h vs args.h: I think we can take the intermediate step
doing a cleanup + documentation by moving to xe_args.h and later try
to promote xe_args.h to args.h.

thanks
Lucas De Marchi