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

[Lucas De Marchi]

On Wed, Apr 19, 2023 at 10:39:55PM +0300, Jani Nikula wrote:
>On Wed, 19 Apr 2023, Lucas De Marchi <lucas.demarchi at intel.com> wrote:
>> 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.
>
>If you hide the underlying type with a typedef, don't look inside. If
>you need to look inside, don't hide the type. See coding-style.rst.

I only typedef'ed to follow what was already there. To be honest, I
think we can lose the typedef.

>
>> *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.
>
>To me, the main benefit is readability and self-documenting code:
>
>	if (reg.mcr)
>
>vs.
>
>	if (is_mcr_reg(reg))

which is also against our coding style, so it should be
xe_reg_is_mcr(reg).  My personal taste here: I don't think it
reads better than `if (reg.mcr)`.  If anything, one would have to
consult the meaning of "mcr" rather than what one vs the other is
doing.

Lucas De Marchi

>
>
>BR,
>Jani.
>
>-- 
>Jani Nikula, Intel Open Source Graphics Center