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

[Lucas De Marchi]

On Wed, Apr 19, 2023 at 07:06:51PM +0300, Jani Nikula wrote:
>On Wed, 19 Apr 2023, Lucas De Marchi <lucas.demarchi at intel.com> 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.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> ---
>>  drivers/gpu/drm/xe/regs/xe_reg_defs.h | 15 +++++++++++++++
>>  drivers/gpu/drm/xe/xe_gt_mcr.c        | 22 ++++++++++++++++------
>>  drivers/gpu/drm/xe/xe_gt_mcr.h        |  8 ++++----
>>  drivers/gpu/drm/xe/xe_irq.c           |  2 +-
>>  drivers/gpu/drm/xe/xe_mmio.c          |  2 +-
>>  5 files changed, 37 insertions(+), 12 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..1e78508c737b 100644
>> --- a/drivers/gpu/drm/xe/regs/xe_reg_defs.h
>> +++ b/drivers/gpu/drm/xe/regs/xe_reg_defs.h
>> @@ -8,4 +8,19 @@
>>
>>  #include "compat-i915-headers/i915_reg_defs.h"
>>
>> +typedef union {
>> +	struct {
>> +		u32 reg:30;
>> +		u32 mcr:1;
>> +		u32 masked:1;
>> +	};
>> +	u32 raw;
>> +} xe_reg_t;
>> +
>> +/* TODO: remove these once the register declarations are not using them anymore */
>> +#undef _MMIO
>> +#undef MCR_REG
>> +#define _MMIO(r)	((const xe_reg_t){ .reg = (r) })
>> +#define MCR_REG(r)	((const xe_reg_t){ .reg = (r), .mcr = 1 })
>> +
>>  #endif
>> diff --git a/drivers/gpu/drm/xe/xe_gt_mcr.c b/drivers/gpu/drm/xe/xe_gt_mcr.c
>> index aa04ba5a6dbe..b9631cfd5b81 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_mcr.c
>> +++ b/drivers/gpu/drm/xe/xe_gt_mcr.c
>> @@ -360,11 +360,13 @@ void xe_gt_mcr_set_implicit_defaults(struct xe_gt *gt)
>>   * returned.  Returns false if the caller need not perform any steering
>>   */
>>  static bool xe_gt_mcr_get_nonterminated_steering(struct xe_gt *gt,
>> -						 i915_mcr_reg_t reg,
>> +						 xe_reg_t reg,
>>  						 u8 *group, u8 *instance)
>>  {
>>  	const struct xe_mmio_range *implicit_ranges;
>>
>> +	drm_WARN_ON(&gt_to_xe(gt)->drm, !reg.mcr);
>
>I'd add some is_mcr_reg() style macro and use it throughout instead of
>poking directly at xe_reg_t guts. The idea should be that xe_reg_t is
>opaque.

humn... in xe the tendency is not to hide too much as it creates a
unneeded level of indirection. I don't see us needing to change
xe_reg_t much in future or make it depend on platform, etc.  I think a
helper like that could be added if we end up with such need.

*changing*  the values underneath the struct is probably something that
we should avoid doing (there are a few places we do though to account
for base offset), but I don't see a problem *reading* it. We should
probably sprinkle some const around.

The extra verbosity imposed by the wrapper function call doesn't bring
much benefit IMO.

Lucas De Marchi