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

BR,
Jani.


> +
>  	for (int type = 0; type < IMPLICIT_STEERING; type++) {
>  		if (!gt->steering[type].ranges)
>  			continue;
> @@ -436,11 +438,13 @@ static void mcr_unlock(struct xe_gt *gt) {
>   *
>   * Caller needs to make sure the relevant forcewake wells are up.
>   */
> -static u32 rw_with_mcr_steering(struct xe_gt *gt, i915_mcr_reg_t reg, u8 rw_flag,
> +static u32 rw_with_mcr_steering(struct xe_gt *gt, xe_reg_t reg, u8 rw_flag,
>  				int group, int instance, u32 value)
>  {
>  	u32 steer_reg, steer_val, val = 0;
>  
> +	drm_WARN_ON(&gt_to_xe(gt)->drm, !reg.mcr);
> +
>  	lockdep_assert_held(&gt->mcr_lock);
>  
>  	if (GRAPHICS_VERx100(gt_to_xe(gt)) >= 1270) {
> @@ -494,12 +498,14 @@ static u32 rw_with_mcr_steering(struct xe_gt *gt, i915_mcr_reg_t reg, u8 rw_flag
>   *
>   * Returns the value from a non-terminated instance of @reg.
>   */
> -u32 xe_gt_mcr_unicast_read_any(struct xe_gt *gt, i915_mcr_reg_t reg)
> +u32 xe_gt_mcr_unicast_read_any(struct xe_gt *gt, xe_reg_t reg)
>  {
>  	u8 group, instance;
>  	u32 val;
>  	bool steer;
>  
> +	drm_WARN_ON(&gt_to_xe(gt)->drm, !reg.mcr);
> +
>  	steer = xe_gt_mcr_get_nonterminated_steering(gt, reg, &group, &instance);
>  
>  	if (steer) {
> @@ -525,11 +531,13 @@ u32 xe_gt_mcr_unicast_read_any(struct xe_gt *gt, i915_mcr_reg_t reg)
>   * group/instance.
>   */
>  u32 xe_gt_mcr_unicast_read(struct xe_gt *gt,
> -			   i915_mcr_reg_t reg,
> +			   xe_reg_t reg,
>  			   int group, int instance)
>  {
>  	u32 val;
>  
> +	drm_WARN_ON(&gt_to_xe(gt)->drm, !reg.mcr);
> +
>  	mcr_lock(gt);
>  	val = rw_with_mcr_steering(gt, reg, MCR_OP_READ, group, instance, 0);
>  	mcr_unlock(gt);
> @@ -548,9 +556,11 @@ u32 xe_gt_mcr_unicast_read(struct xe_gt *gt,
>   * Write an MCR register in unicast mode after steering toward a specific
>   * group/instance.
>   */
> -void xe_gt_mcr_unicast_write(struct xe_gt *gt, i915_mcr_reg_t reg, u32 value,
> +void xe_gt_mcr_unicast_write(struct xe_gt *gt, xe_reg_t reg, u32 value,
>  			     int group, int instance)
>  {
> +	drm_WARN_ON(&gt_to_xe(gt)->drm, !reg.mcr);
> +
>  	mcr_lock(gt);
>  	rw_with_mcr_steering(gt, reg, MCR_OP_WRITE, group, instance, value);
>  	mcr_unlock(gt);
> @@ -564,7 +574,7 @@ void xe_gt_mcr_unicast_write(struct xe_gt *gt, i915_mcr_reg_t reg, u32 value,
>   *
>   * Write an MCR register in multicast mode to update all instances.
>   */
> -void xe_gt_mcr_multicast_write(struct xe_gt *gt, i915_mcr_reg_t reg, u32 value)
> +void xe_gt_mcr_multicast_write(struct xe_gt *gt, xe_reg_t reg, u32 value)
>  {
>  	/*
>  	 * Synchronize with any unicast operations.  Once we have exclusive
> diff --git a/drivers/gpu/drm/xe/xe_gt_mcr.h b/drivers/gpu/drm/xe/xe_gt_mcr.h
> index 2a6cd38c8cb7..492d9519784a 100644
> --- a/drivers/gpu/drm/xe/xe_gt_mcr.h
> +++ b/drivers/gpu/drm/xe/xe_gt_mcr.h
> @@ -15,13 +15,13 @@ void xe_gt_mcr_init(struct xe_gt *gt);
>  
>  void xe_gt_mcr_set_implicit_defaults(struct xe_gt *gt);
>  
> -u32 xe_gt_mcr_unicast_read(struct xe_gt *gt, i915_mcr_reg_t reg,
> +u32 xe_gt_mcr_unicast_read(struct xe_gt *gt, xe_reg_t reg,
>  			   int group, int instance);
> -u32 xe_gt_mcr_unicast_read_any(struct xe_gt *gt, i915_mcr_reg_t reg);
> +u32 xe_gt_mcr_unicast_read_any(struct xe_gt *gt, xe_reg_t reg);
>  
> -void xe_gt_mcr_unicast_write(struct xe_gt *gt, i915_mcr_reg_t reg, u32 value,
> +void xe_gt_mcr_unicast_write(struct xe_gt *gt, xe_reg_t reg, u32 value,
>  			     int group, int instance);
> -void xe_gt_mcr_multicast_write(struct xe_gt *gt, i915_mcr_reg_t reg, u32 value);
> +void xe_gt_mcr_multicast_write(struct xe_gt *gt, xe_reg_t reg, u32 value);
>  
>  void xe_gt_mcr_steering_dump(struct xe_gt *gt, struct drm_printer *p);
>  
> diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c
> index 13f9f220bca0..8e5f8e7c16c8 100644
> --- a/drivers/gpu/drm/xe/xe_irq.c
> +++ b/drivers/gpu/drm/xe/xe_irq.c
> @@ -27,7 +27,7 @@
>  #define IIR(offset)				_MMIO(offset + 0x8)
>  #define IER(offset)				_MMIO(offset + 0xc)
>  
> -static void assert_iir_is_zero(struct xe_gt *gt, i915_reg_t reg)
> +static void assert_iir_is_zero(struct xe_gt *gt, xe_reg_t reg)
>  {
>  	u32 val = xe_mmio_read32(gt, reg.reg);
>  
> diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
> index a93838e23b7b..1029b9f27988 100644
> --- a/drivers/gpu/drm/xe/xe_mmio.c
> +++ b/drivers/gpu/drm/xe/xe_mmio.c
> @@ -397,7 +397,7 @@ int xe_mmio_init(struct xe_device *xe)
>  	DRM_XE_MMIO_READ |\
>  	DRM_XE_MMIO_WRITE)
>  
> -static const i915_reg_t mmio_read_whitelist[] = {
> +static const xe_reg_t mmio_read_whitelist[] = {
>  	RING_TIMESTAMP(RENDER_RING_BASE),
>  };

-- 
Jani Nikula, Intel Open Source Graphics Center