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

[Lucas De Marchi]

On Wed, Apr 19, 2023 at 03:30:17PM -0400, Rodrigo Vivi wrote:
>On Wed, Apr 19, 2023 at 11:49:56AM -0700, Lucas De Marchi wrote:
>> 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
>
>I honestly liked the approach you took on this patch here. Getting rid of
>the i915_mcr is already a win by itself.
>
>One case that came to my mind though is some time sensitive mmio operations
>like display inside vblank areas. any runtime check could potentially
>increase latency, no?! If that's irrelevant time, than let's move with this
>but if it is really a problem, then explore your latest suggestion here
>with the compiler doing the checks for us seems a good alternative.

the additional check is inside xe_gt_mcr.c... I think the additional
check is negligible compared to the other operations there, with each
access to reg X requiring other 2 mmio accesses to take place.

Besides, there is no such user in display/

	$ git grep xe_gt_mcr -- drivers/gpu/drm/xe/display/


Lucas De Marchi