Re: [PATCH 3/5] drm/i915: add GEN2_ prefix to the I{E, I, M, S}R registers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Apr 10, 2019 at 04:53:42PM -0700, Paulo Zanoni wrote:
> This discussion started because we use token pasting in the
> GEN{2,3}_IRQ_INIT and GEN{2,3}_IRQ_RESET macros, so gen2-4 passes an
> empty argument to those macros, making the code a little weird. The
> original proposal was to just add a comment as the empty argument, but
> Ville suggested we just add a prefix to the registers, and that indeed
> sounds like a more elegant solution.
> 
> Now doing this is kinda against our rules for register naming since we
> only add gens or platform names as register prefixes when the given
> gen/platform changes a register that already existed before. On the
> other hand, we have so many instances of IIR/IMR in comments that
> adding a prefix would make the users of these register more easily
> findable, in addition to make our token pasting macros actually
> readable. So IMHO opening an exception here is worth it.
> 
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c     |  6 +--
>  drivers/gpu/drm/i915/i915_gpu_error.c   |  4 +-
>  drivers/gpu/drm/i915/i915_irq.c         | 52 ++++++++++++-------------
>  drivers/gpu/drm/i915/i915_reg.h         |  8 ++--
>  drivers/gpu/drm/i915/i915_reset.c       |  3 +-
>  drivers/gpu/drm/i915/intel_overlay.c    |  4 +-
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 10 ++---
>  7 files changed, 44 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 77b3252bdb2e..5823ffb17821 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -833,11 +833,11 @@ static int i915_interrupt_info(struct seq_file *m, void *data)
>  
>  	} else if (!HAS_PCH_SPLIT(dev_priv)) {
>  		seq_printf(m, "Interrupt enable:    %08x\n",
> -			   I915_READ(IER));
> +			   I915_READ(GEN2_IER));
>  		seq_printf(m, "Interrupt identity:  %08x\n",
> -			   I915_READ(IIR));
> +			   I915_READ(GEN2_IIR));
>  		seq_printf(m, "Interrupt mask:      %08x\n",
> -			   I915_READ(IMR));
> +			   I915_READ(GEN2_IMR));
>  		for_each_pipe(dev_priv, pipe)
>  			seq_printf(m, "Pipe %c stat:         %08x\n",
>  				   pipe_name(pipe),
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 43b68fdc8967..f51ff683dd2e 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -1635,9 +1635,9 @@ static void capture_reg_state(struct i915_gpu_state *error)
>  		error->gtier[0] = I915_READ(GTIER);
>  		error->ngtier = 1;
>  	} else if (IS_GEN(dev_priv, 2)) {
> -		error->ier = I915_READ16(IER);
> +		error->ier = I915_READ16(GEN2_IER);
>  	} else if (!IS_VALLEYVIEW(dev_priv)) {
> -		error->ier = I915_READ(IER);
> +		error->ier = I915_READ(GEN2_IER);
>  	}
>  	error->eir = I915_READ(EIR);
>  	error->pgtbl_er = I915_READ(PGTBL_ER);
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index b1f1db2bd879..2910b06913af 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -153,16 +153,16 @@ static void gen3_irq_reset(struct drm_i915_private *dev_priv, i915_reg_t imr,
>  
>  static void gen2_irq_reset(struct drm_i915_private *dev_priv)
>  {
> -	I915_WRITE16(IMR, 0xffff);
> -	POSTING_READ16(IMR);
> +	I915_WRITE16(GEN2_IMR, 0xffff);
> +	POSTING_READ16(GEN2_IMR);
>  
> -	I915_WRITE16(IER, 0);
> +	I915_WRITE16(GEN2_IER, 0);
>  
>  	/* IIR can theoretically queue up two events. Be paranoid. */
> -	I915_WRITE16(IIR, 0xffff);
> -	POSTING_READ16(IIR);
> -	I915_WRITE16(IIR, 0xffff);
> -	POSTING_READ16(IIR);
> +	I915_WRITE16(GEN2_IIR, 0xffff);
> +	POSTING_READ16(GEN2_IIR);
> +	I915_WRITE16(GEN2_IIR, 0xffff);
> +	POSTING_READ16(GEN2_IIR);
>  }
>  
>  #define GEN8_IRQ_RESET_NDX(type, which) \
> @@ -199,17 +199,17 @@ static void gen3_assert_iir_is_zero(struct drm_i915_private *dev_priv,
>  
>  static void gen2_assert_iir_is_zero(struct drm_i915_private *dev_priv)
>  {
> -	u16 val = I915_READ16(IIR);
> +	u16 val = I915_READ16(GEN2_IIR);
>  
>  	if (val == 0)
>  		return;
>  
>  	WARN(1, "Interrupt register 0x%x is not zero: 0x%08x\n",
> -	     i915_mmio_reg_offset(IIR), val);
> -	I915_WRITE16(IIR, 0xffff);
> -	POSTING_READ16(IIR);
> -	I915_WRITE16(IIR, 0xffff);
> -	POSTING_READ16(IIR);
> +	     i915_mmio_reg_offset(GEN2_IIR), val);
> +	I915_WRITE16(GEN2_IIR, 0xffff);
> +	POSTING_READ16(GEN2_IIR);
> +	I915_WRITE16(GEN2_IIR, 0xffff);
> +	POSTING_READ16(GEN2_IIR);
>  }
>  
>  static void gen3_irq_init(struct drm_i915_private *dev_priv,
> @@ -229,9 +229,9 @@ static void gen2_irq_init(struct drm_i915_private *dev_priv,
>  {
>  	gen2_assert_iir_is_zero(dev_priv);
>  
> -	I915_WRITE16(IER, ier_val);
> -	I915_WRITE16(IMR, imr_val);
> -	POSTING_READ16(IMR);
> +	I915_WRITE16(GEN2_IER, ier_val);
> +	I915_WRITE16(GEN2_IMR, imr_val);
> +	POSTING_READ16(GEN2_IMR);
>  }
>  
>  #define GEN8_IRQ_INIT_NDX(type, which, imr_val, ier_val) \
> @@ -4344,7 +4344,7 @@ static irqreturn_t i8xx_irq_handler(int irq, void *arg)
>  		u16 eir = 0, eir_stuck = 0;
>  		u16 iir;
>  
> -		iir = I915_READ16(IIR);
> +		iir = I915_READ16(GEN2_IIR);
>  		if (iir == 0)
>  			break;
>  
> @@ -4357,7 +4357,7 @@ static irqreturn_t i8xx_irq_handler(int irq, void *arg)
>  		if (iir & I915_MASTER_ERROR_INTERRUPT)
>  			i8xx_error_irq_ack(dev_priv, &eir, &eir_stuck);
>  
> -		I915_WRITE16(IIR, iir);
> +		I915_WRITE16(GEN2_IIR, iir);
>  
>  		if (iir & I915_USER_INTERRUPT)
>  			intel_engine_breadcrumbs_irq(dev_priv->engine[RCS0]);
> @@ -4384,7 +4384,7 @@ static void i915_irq_reset(struct drm_device *dev)
>  
>  	i9xx_pipestat_irq_reset(dev_priv);
>  
> -	GEN3_IRQ_RESET();
> +	GEN3_IRQ_RESET(GEN2_);

These do look a bit odd with the gen3+gen2 in the same sentence.
Hence not entitely convinced that GEN2_ is the best prefix. But
meh.

Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>

>  }
>  
>  static int i915_irq_postinstall(struct drm_device *dev)
> @@ -4416,7 +4416,7 @@ static int i915_irq_postinstall(struct drm_device *dev)
>  		dev_priv->irq_mask &= ~I915_DISPLAY_PORT_INTERRUPT;
>  	}
>  
> -	GEN3_IRQ_INIT(, dev_priv->irq_mask, enable_mask);
> +	GEN3_IRQ_INIT(GEN2_, dev_priv->irq_mask, enable_mask);
>  
>  	/* Interrupt setup is already guaranteed to be single-threaded, this is
>  	 * just to make the assert_spin_locked check happy. */
> @@ -4448,7 +4448,7 @@ static irqreturn_t i915_irq_handler(int irq, void *arg)
>  		u32 hotplug_status = 0;
>  		u32 iir;
>  
> -		iir = I915_READ(IIR);
> +		iir = I915_READ(GEN2_IIR);
>  		if (iir == 0)
>  			break;
>  
> @@ -4465,7 +4465,7 @@ static irqreturn_t i915_irq_handler(int irq, void *arg)
>  		if (iir & I915_MASTER_ERROR_INTERRUPT)
>  			i9xx_error_irq_ack(dev_priv, &eir, &eir_stuck);
>  
> -		I915_WRITE(IIR, iir);
> +		I915_WRITE(GEN2_IIR, iir);
>  
>  		if (iir & I915_USER_INTERRUPT)
>  			intel_engine_breadcrumbs_irq(dev_priv->engine[RCS0]);
> @@ -4493,7 +4493,7 @@ static void i965_irq_reset(struct drm_device *dev)
>  
>  	i9xx_pipestat_irq_reset(dev_priv);
>  
> -	GEN3_IRQ_RESET();
> +	GEN3_IRQ_RESET(GEN2_);
>  }
>  
>  static int i965_irq_postinstall(struct drm_device *dev)
> @@ -4536,7 +4536,7 @@ static int i965_irq_postinstall(struct drm_device *dev)
>  	if (IS_G4X(dev_priv))
>  		enable_mask |= I915_BSD_USER_INTERRUPT;
>  
> -	GEN3_IRQ_INIT(, dev_priv->irq_mask, enable_mask);
> +	GEN3_IRQ_INIT(GEN2_, dev_priv->irq_mask, enable_mask);
>  
>  	/* Interrupt setup is already guaranteed to be single-threaded, this is
>  	 * just to make the assert_spin_locked check happy. */
> @@ -4594,7 +4594,7 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)
>  		u32 hotplug_status = 0;
>  		u32 iir;
>  
> -		iir = I915_READ(IIR);
> +		iir = I915_READ(GEN2_IIR);
>  		if (iir == 0)
>  			break;
>  
> @@ -4610,7 +4610,7 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)
>  		if (iir & I915_MASTER_ERROR_INTERRUPT)
>  			i9xx_error_irq_ack(dev_priv, &eir, &eir_stuck);
>  
> -		I915_WRITE(IIR, iir);
> +		I915_WRITE(GEN2_IIR, iir);
>  
>  		if (iir & I915_USER_INTERRUPT)
>  			intel_engine_breadcrumbs_irq(dev_priv->engine[RCS0]);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 9c206e803ab3..6a150243cabb 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2713,10 +2713,10 @@ enum i915_power_well_id {
>  #define VLV_GU_CTL0	_MMIO(VLV_DISPLAY_BASE + 0x2030)
>  #define VLV_GU_CTL1	_MMIO(VLV_DISPLAY_BASE + 0x2034)
>  #define SCPD0		_MMIO(0x209c) /* 915+ only */
> -#define IER		_MMIO(0x20a0)
> -#define IIR		_MMIO(0x20a4)
> -#define IMR		_MMIO(0x20a8)
> -#define ISR		_MMIO(0x20ac)
> +#define GEN2_IER	_MMIO(0x20a0)
> +#define GEN2_IIR	_MMIO(0x20a4)
> +#define GEN2_IMR	_MMIO(0x20a8)
> +#define GEN2_ISR	_MMIO(0x20ac)
>  #define VLV_GUNIT_CLOCK_GATE	_MMIO(VLV_DISPLAY_BASE + 0x2060)
>  #define   GINT_DIS		(1 << 22)
>  #define   GCFG_DIS		(1 << 8)
> diff --git a/drivers/gpu/drm/i915/i915_reset.c b/drivers/gpu/drm/i915/i915_reset.c
> index 68875ba43b8d..b75ac660c3c2 100644
> --- a/drivers/gpu/drm/i915/i915_reset.c
> +++ b/drivers/gpu/drm/i915/i915_reset.c
> @@ -1223,7 +1223,8 @@ void i915_clear_error_registers(struct drm_i915_private *i915)
>  		 */
>  		DRM_DEBUG_DRIVER("EIR stuck: 0x%08x, masking\n", eir);
>  		rmw_set(uncore, EMR, eir);
> -		intel_uncore_write(uncore, IIR, I915_MASTER_ERROR_INTERRUPT);
> +		intel_uncore_write(uncore, GEN2_IIR,
> +				   I915_MASTER_ERROR_INTERRUPT);
>  	}
>  
>  	if (INTEL_GEN(i915) >= 8) {
> diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> index a882b8d42bd9..eb317759b5d3 100644
> --- a/drivers/gpu/drm/i915/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/intel_overlay.c
> @@ -446,7 +446,7 @@ static int intel_overlay_release_old_vid(struct intel_overlay *overlay)
>  	if (!overlay->old_vma)
>  		return 0;
>  
> -	if (I915_READ(ISR) & I915_OVERLAY_PLANE_FLIP_PENDING_INTERRUPT) {
> +	if (I915_READ(GEN2_ISR) & I915_OVERLAY_PLANE_FLIP_PENDING_INTERRUPT) {
>  		/* synchronous slowpath */
>  		struct i915_request *rq;
>  
> @@ -1430,7 +1430,7 @@ intel_overlay_capture_error_state(struct drm_i915_private *dev_priv)
>  		return NULL;
>  
>  	error->dovsta = I915_READ(DOVSTA);
> -	error->isr = I915_READ(ISR);
> +	error->isr = I915_READ(GEN2_ISR);
>  	error->base = overlay->flip_addr;
>  
>  	memcpy_fromio(&error->regs, overlay->regs, sizeof(error->regs));
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index af35f99c5940..029fd8ec1857 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -977,15 +977,15 @@ static void
>  i9xx_irq_enable(struct intel_engine_cs *engine)
>  {
>  	engine->i915->irq_mask &= ~engine->irq_enable_mask;
> -	intel_uncore_write(engine->uncore, IMR, engine->i915->irq_mask);
> -	intel_uncore_posting_read_fw(engine->uncore, IMR);
> +	intel_uncore_write(engine->uncore, GEN2_IMR, engine->i915->irq_mask);
> +	intel_uncore_posting_read_fw(engine->uncore, GEN2_IMR);
>  }
>  
>  static void
>  i9xx_irq_disable(struct intel_engine_cs *engine)
>  {
>  	engine->i915->irq_mask |= engine->irq_enable_mask;
> -	intel_uncore_write(engine->uncore, IMR, engine->i915->irq_mask);
> +	intel_uncore_write(engine->uncore, GEN2_IMR, engine->i915->irq_mask);
>  }
>  
>  static void
> @@ -994,7 +994,7 @@ i8xx_irq_enable(struct intel_engine_cs *engine)
>  	struct drm_i915_private *dev_priv = engine->i915;
>  
>  	dev_priv->irq_mask &= ~engine->irq_enable_mask;
> -	I915_WRITE16(IMR, dev_priv->irq_mask);
> +	I915_WRITE16(GEN2_IMR, dev_priv->irq_mask);
>  	POSTING_READ16(RING_IMR(engine->mmio_base));
>  }
>  
> @@ -1004,7 +1004,7 @@ i8xx_irq_disable(struct intel_engine_cs *engine)
>  	struct drm_i915_private *dev_priv = engine->i915;
>  
>  	dev_priv->irq_mask |= engine->irq_enable_mask;
> -	I915_WRITE16(IMR, dev_priv->irq_mask);
> +	I915_WRITE16(GEN2_IMR, dev_priv->irq_mask);
>  }
>  
>  static int
> -- 
> 2.20.1

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux