[Intel-xe] [PATCH v2 16/17] drm/xe: Plumb xe_reg_t into WAs, rtp, etc

[Matt Roper]

On Mon, Apr 24, 2023 at 03:50:34PM -0700, Lucas De Marchi wrote:
> On Mon, Apr 24, 2023 at 02:20:52PM -0700, Matt Roper wrote:
> > On Fri, Apr 21, 2023 at 03:32:57PM -0700, Lucas De Marchi wrote:
> > > Now that xe_reg_t is a type that can be used by xe, convert the rest of
> > > the driver to use it:
> > 
> > s/the rest of the driver/more of the driver/
> > 
> > We still need to fix xe_mmio_*() to take this type instead of a u32
> > before we consider the conversion "done."  I assume that's coming in
> > another series after this one?
> 
> My plan was to convert xe_mmio_* on a follow up, but then when I
> finished this series I noticed the one thing noted in the commit that introduced
> xe_reg/xe_reg_mcr: now the latter doesn't have a .reg field anymore, so passing
> a mcr reg to xe_mmio_* already breaks the build

There's still the issue that led to the creation of i915_reg_t all those
years ago:  xe_mmio_write32() just takes two u32's right now, so it's
possible to misorder those and wind up trying to write your register
offset into a location specified by the value.  Jani mentioned that that
happened a lot back in the early i915 days and caused hard-to-debug
issues.  Plus passing foo.reg in a bunch of places is kind of ugly
(especially since kernel coding style requires that typedefs only be
used for opaque types).

If we really need to keep a bunch of direct accesses into xe_reg_t's
members scattered throughout the driver (rather than making dedicated
accessors like xe_reg_offset()), then I think we'd at least need to kill
the typedef and move to a 'struct xe_reg' instead to not violate kernel
coding style so much.


Matt

> 
> That left me wondering if we just go ahead and do the conversion to
> avoid the uses of THIS_REG.reg or just leave it as is. More from IRC:
> 
> <demarchi> mattrope: rodrigovivi jani next version of my register cleanup
> series will retain the strong type for mcr. I also found a way to make calls to
> xe_mmio_*(gt, FOO.reg) error out, even if xe_mmio* only receives a u32, not a
> xe_reg
> 
> <demarchi> so... the incentive to make xe_mmio receive xe_reg rather than u32
> is actually smaller. I want to do a s/.reg/.addr/  in the xe_reg struct... but
> if we are going to end up converting xe_mmio_* to xe_reg (even with the smaller
> incentive now), then maybe I should wait for that?
> 
> <demarchi> thoughts?
> 
> <demarchi> to see what I'm talking about:
> `git grep -e "\\.reg[^_s]" -- drivers/gpu/drm/xe`  (with a few false positives)
> 
> <rodrigovivi> I honestly don't have strong preferences there... as long as we
> get rid of the 'i915_reg_t' there I'm good... and a bonus if we combine the
> mcr...
> 
> Lucas De Marchi

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