[Intel-xe] [PATCH 11/17] drm/xe: Introduce xe_reg_t

[Lucas De Marchi]

On Wed, Apr 19, 2023 at 10:33:45AM -0700, Matt Roper wrote:
>On Wed, Apr 19, 2023 at 12:44:34AM -0700, Lucas De Marchi wrote:
>> Stop using i915 types for register our own xe_reg_t. Differently from
>> i915, this will keep under this will keep under the register definition
>> the knowledge for the different types of registers. For now, the "flags"
>> are mcr and masked, although only the former is being used.
>>
>> Most of the driver is agnostic to the register differences. Convert the
>> few places that care about that, namely xe_gt_mcr.c, to take the generic
>> type and warn if the wrong register is used.
>
>The disadvantage of this approach is that we don't get the nice
>type-checking that we have in i915 to catch register misuse at build
>time.  Instead we wind up with a bunch of run-time checks that only tell
>you that you used the wrong register semantics after the fact.  Wouldn't
>it be better to keep the strict types and let the compiler find mistakes
>for us at build time?
>
>The only real problem with using strict types is that there are a few
>places where the driver is just trying to refer to registers/offsets
>without actually accessing them (e.g., whitelists, OA register groups,

that is not true.

>etc.) and the strict typing makes it harder to provide a heterogeneous
>list of those registers.  But I still feel the benefits of compile-time
>checking outweighs the extra complexity we wind up with in a handful of
>places.

I disagree here because the strict type checking is only checking for
"regular vs mcr". Add "masked" in the mix (that has been a huge source
of mistakes in i915) and you already have 4 types(?) to deal with.

There are also places in the code where we want that "this is an
mcr/regular/masked register" info to be used later. See how the reg-sr is
handling that.

In the end I believe having this info encoded in the regs/*.h is
sufficient to avoid the mistakes we had in the past.

Any change in the code triggering these warnings has a very easy an
actionable fix.

If for some reason we want to only differentiate mcr/normal as strict
type checks,  then I think we need to provide a better struct for the
places that don't care about that. Something like below maybe

	typedef {
		xe_reg_t reg;
	} xe_reg_mcr_t;

but reg.mcr still being there. Or going the other way around:

	typedef union {
		xe_reg_normal_t normal;
		xe_reg_mcr_t	mcr;
	} xe_reg_t;

but xe_reg_normal_t and xe_reg_mcr_t actually having the same defintion.

Lucas De Marchi