Re: [Intel-xe] [PATCH v2 08/11] drm/xe: Remove dependency on i915_reg.h

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

On Fri, 17 Feb 2023, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> On Fri, Feb 17, 2023 at 03:27:12PM -0500, Rodrigo Vivi wrote:
>>On Thu, Feb 16, 2023 at 04:52:23PM -0800, Lucas De Marchi wrote:
>>> +#define GGC					_MMIO(0x108040)
>>> +#define   GMS_MASK				REG_GENMASK(15, 8)
>>> +#define   GGMS_MASK				REG_GENMASK(7, 6)
>>
>>one of the things that I'm always confused is with these REG_{BIT,GENMASK,FIELD_PREP}.
>>Why can't we simply use the regular "non-REG_" versions like every other driver
>>(aside of i915 of course).
>
> it was some time ago, I didn't remember exactly and was guessing it was
> because GENMASK returns a long, while our registers are 32bits.
> Indeed, commit 09b434d4f6d2 ("drm/i915: introduce REG_BIT() and
> REG_GENMASK() to define register contents"):
>
> 	We define the above as wrappers to BIT() and GENMASK() respectively to
> 	force u32 type to go with our register size, and to add compile time
> 	checks on the bit numbers.

This, plus FIELD_PREP() doesn't always generate an integer constant
expression making it hard to use in some cases. Which I believe we see
over and over again in i915 gem/gt code that uses FIELD_PREP(). It's
even compiler dependent I think.

> we may revisit that and maybe generalize GENMASK32/BIT32 that guarantees a
> u32 type is returned.

Obviously not a bad idea, but additionally forcing integer constant
expression might not be something everyone wants in generic code.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center