[Intel-xe] [PATCH v2 13/17] drm/xe: Introduce xe_reg/xe_reg_mcr

[Lucas De Marchi]

On Mon, Apr 24, 2023 at 01:29:12PM -0700, Matt Roper wrote:
>On Fri, Apr 21, 2023 at 03:32:54PM -0700, Lucas De Marchi wrote:
>> Stop using i915 types for registers. Use our own types. 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"
>
>This sentence got a bit garbled.

will fix

>
>> are mcr and masked, although only the former is being used.
>>
>> Additionally MCR registers have their own type. The only place that
>> should really look inside a xe_mcr_reg_t is that code dealing with the
>> steering and using other APIs when the register is MCR has been a source
>> of problem in the past.
>>
>> Most of the driver is agnostic to the register differences since they
>> either use the definition from the header or already call the correct
>> MCR_REG()/_MMIO() macros. By embeding the struct xe_reg inside the
>> struct it's also possible to guarantee the compiler will break if
>> using RANDOM_MCR_REG.reg is attempted, since now the u32 is inside the
>> inner struct.
>>
>> v2:
>>   - Deep a dedicated type for MCR registers to avoid misuse
>>     (Matt Roper, Jani)
>>   - Drop the typedef and just use a struct since it's not an opaque type
>>     (Jani)
>>   - Add more kernel-doc
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> ---
>>  drivers/gpu/drm/xe/regs/xe_reg_defs.h | 93 +++++++++++++++++++++++++++
>>  drivers/gpu/drm/xe/xe_gt_mcr.c        | 36 +++++++----
>>  drivers/gpu/drm/xe/xe_gt_mcr.h        | 11 ++--
>>  drivers/gpu/drm/xe/xe_irq.c           |  2 +-
>>  drivers/gpu/drm/xe/xe_mmio.c          |  2 +-
>>  5 files changed, 125 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/regs/xe_reg_defs.h b/drivers/gpu/drm/xe/regs/xe_reg_defs.h
>> index b5c25e31b889..c0a091206f27 100644
>> --- a/drivers/gpu/drm/xe/regs/xe_reg_defs.h
>> +++ b/drivers/gpu/drm/xe/regs/xe_reg_defs.h
>> @@ -8,4 +8,97 @@
>>
>>  #include "compat-i915-headers/i915_reg_defs.h"
>>
>> +/**
>> + * struct xe_reg - Register definition
>> + *
>> + * Register defintion to be used by the individual register. Although the same
>> + * definition is used for xe_reg and xe_reg_mcr, they use different internal
>> + * APIs for accesses.
>> + */
>> +struct xe_reg {
>> +	union {
>> +		struct {
>> +			/** @reg: address */
>> +			u32 reg:30;
>
>I wonder if we should just make this reg:22 for now?  All platforms
>supported today only have a 4MB MMIO area per tile, so 22 bits covers
>all valid reg offsets.  If someone typos an extra digit into a register
>definition, I think the compiler would help warn that they tried to
>define something outside the possible range.  It would also make it more
>clear exactly how many flag bits we have left here to work with for
>future expansion, assuming we want to stay within 32-bits total.
>
>If a future platform doubles the MMIO space for a tile, we can easily
>bump the number of bits up.  But I don't see any downside to just making
>it match the true register address space for now.

looks like a compelling reason indeed.  I will change it.


>
>Aside from that,
>
>Reviewed-by: Matt Roper <matthew.d.roper at intel.com>

thanks
Lucas De Marchi