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

[Matt Roper]

On Wed, Apr 19, 2023 at 01:19:49PM -0700, Lucas De Marchi wrote:
> 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.

The super critical checks (which don't exist yet in this series; they'd
presumably be added in a follow-up series) are the ones to make sure you
don't try to call xe_mmio_{read,write}32() on an MCR register.  Doing
that can lead to all kinds of subtle bugs (which become even more
problematic on the more recent platforms), and unfortunately that kind
of mistake is what people are going to write in the code 99% of the time
out of habit.


Matt

> 
> Besides, there is no such user in display/
> 
> 	$ git grep xe_gt_mcr -- drivers/gpu/drm/xe/display/
> 
> 
> Lucas De Marchi

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation