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

[Lucas De Marchi]

On Mon, Apr 24, 2023 at 04:10:19PM -0700, Matt Roper wrote:
>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.

in this version we don't have the typedef anymore (and I forgot to
update the subject in this patch).

But I agree... since it's 90% done, there isn't much reason to stop here
instead of doing the final conversion.

Lucas De Marchi

>
>
>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