Re: [PATCH] drm/xe: Promote make_u64 macro to .h file

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 13 Feb 2024, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> On Tue, Feb 13, 2024 at 10:05:43PM +0100, Michal Wajdeczko wrote:
>> We shouldn't hide generic macros inside .c files as this may lead
>> to unnecessary code duplication if similar functionality is needed
>> elsewhere.  Promote make_u64 to .h as soon we will want to reuse it.
>
> On the other hand, we shouldn't create an internal library of macros
> that then start to get used indiscriminately as if it was Linux core
> helpers.
>
> Changes like this was how the i915_utils.h started.
>
> If we really think that this macro is useful somewhere else, then
> we should be looking to make that as part of the linux/types.h

Agreed.

Well, maybe not specifically types.h, but maybe a separate header.

There's a need for something like this:

$ git grep "hi[ )]*<< *\(8\|16\|32\)[ )]*|.*lo"

Ditto for taking higher/lower half of a variable.


BR,
Jani.


>
>> 
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_gt_pagefault.c | 2 --
>>  drivers/gpu/drm/xe/xe_macros.h       | 8 ++++++++
>>  2 files changed, 8 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.c b/drivers/gpu/drm/xe/xe_gt_pagefault.c
>> index c26e4fcca01e..76a018ec8136 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_pagefault.c
>> +++ b/drivers/gpu/drm/xe/xe_gt_pagefault.c
>> @@ -557,8 +557,6 @@ static int handle_acc(struct xe_gt *gt, struct acc *acc)
>>  	return ret;
>>  }
>>  
>> -#define make_u64(hi__, low__)  ((u64)(hi__) << 32 | (u64)(low__))
>> -
>>  #define ACC_MSG_LEN_DW        4
>>  
>>  static bool get_acc(struct acc_queue *acc_queue, struct acc *acc)
>> diff --git a/drivers/gpu/drm/xe/xe_macros.h b/drivers/gpu/drm/xe/xe_macros.h
>> index daf56c846d03..b79d775c737e 100644
>> --- a/drivers/gpu/drm/xe/xe_macros.h
>> +++ b/drivers/gpu/drm/xe/xe_macros.h
>> @@ -7,6 +7,7 @@
>>  #define _XE_MACROS_H_
>>  
>>  #include <linux/bug.h>
>> +#include <linux/types.h>
>>  
>>  #define XE_WARN_ON WARN_ON
>>  
>> @@ -15,4 +16,11 @@
>>  			    "Ioctl argument check failed at %s:%d: %s", \
>>  			    __FILE__, __LINE__, #cond), 1))
>>  
>> +/**
>> + * make_u64 - make u64 value from two u32 values
>> + * @hi__: value representing upper 32 bits
>> + * @lo__: value representing lower 32 bits
>> + */
>> +#define make_u64(hi__, low__)  ((u64)(hi__) << 32 | (u64)(low__))
>> +
>>  #endif
>> -- 
>> 2.43.0
>> 

-- 
Jani Nikula, Intel