[Intel-gfx] [PATCH 3/5] drm/i915: add GEN2_ prefix to the I{E, I, M, S}R registers
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ä
Signed-off-by: Paulo Zanoni
---
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, 0x);
- POSTING_READ16(IMR);
+ I915_WRITE16(GEN2_IMR, 0x);
+ 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, 0x);
- POSTING_READ16(IIR);
- I915_WRITE16(IIR, 0x);
- POSTING_READ16(IIR);
+ I915_WRITE16(GEN2_IIR, 0x);
+ POSTING_READ16(GEN2_IIR);
+ I915_WRITE16(GEN2_IIR, 0x);
+ 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, 0x);
- POSTING_READ16(IIR);
- I915_WRITE16(IIR, 0x);
- POSTING_READ16(IIR);
+i915_mmio_reg_offset(GEN2_IIR), val);
+ I915_WRITE16(GEN2_IIR, 0x);
+ POSTING_READ16(GEN2_IIR);
+ I915_WRITE16(GEN2_IIR, 0x);
+ 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);
-
Re: [Intel-gfx] [PATCH 3/5] drm/i915: add GEN2_ prefix to the I{E, I, M, S}R registers
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ä
> Signed-off-by: Paulo Zanoni
> ---
> 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, 0x);
> - POSTING_READ16(IMR);
> + I915_WRITE16(GEN2_IMR, 0x);
> + 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, 0x);
> - POSTING_READ16(IIR);
> - I915_WRITE16(IIR, 0x);
> - POSTING_READ16(IIR);
> + I915_WRITE16(GEN2_IIR, 0x);
> + POSTING_READ16(GEN2_IIR);
> + I915_WRITE16(GEN2_IIR, 0x);
> + 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, 0x);
> - POSTING_READ16(IIR);
> - I915_WRITE16(IIR, 0x);
> - POSTING_READ16(IIR);
> + i915_mmio_reg_offset(GEN2_IIR), val);
> + I915_WRITE16(GEN2_IIR, 0x);
> + POSTING_READ16(GEN2_IIR);
> + I915_WRITE16(GEN2_IIR, 0x);
> + POSTING_READ16(GEN2_IIR);
> }
>
> static void gen3_irq_init(struct drm_i
