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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 19 Apr 2023, Lucas De Marchi <lucas.demarchi@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@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@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.

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


BR,
Jani.

-- 
Jani Nikula, Intel Open Source Graphics Center