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

[Lucas De Marchi]

On Wed, Apr 19, 2023 at 01:13:59PM -0700, Matt Roper wrote:
>On Wed, Apr 19, 2023 at 10:49:41PM +0300, Jani Nikula wrote:
>> On Wed, 19 Apr 2023, Matt Roper <matthew.d.roper at intel.com> 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,
>> > 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.
>>
>> The history of i915_reg_t is that there were subtle bugs where the
>> arguments to the register write call were accidentally swapped, they
>> went unnoticed, and needed lengthy debugging to finally figure out.
>>
>> This can happen with xe_mmio_write32().
>>
>> Strong typing for i915_reg_t has positive score 9 on Rusty's API Design
>> Manifesto [1].
>
>Exactly.  That's why I think we should stick with the strong typing for
>MCR vs non-MCR as well, rather than just adding a bunch of runtime
>assertions.  It sounds like the proposal here would only earn a score of
>5 by that website's metrics, whereas we have a 9 in i915.

I'd say it's a "false 9" for i915: I detailed in the other thread how we
trigger bugs with the false impression of safety.  For the bugs I fixed
regarding the misuse of MCR had nothing to to with the type: it was
either because register was declared wrongly or the runtime check showed
up. Examples (git log --grep MCR --author=lucas.demarchi at intel.com to
get some):

	abac0c5d9e0cbb7acad3affbf3a1ec00b4bb1a58
	2bb891af095ef287aee454bef930d75565361fea
	6a8b2e4984f73f8d00c8c16b87a8b115d34088e4

With the masked vs non-masked in i915 we get a straight 3(?) or 2(?). Not
sure documentation and spec stand for the same thing there.

Lucas De Marchi