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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

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.