From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 3/5] drm/i915: add GEN2_ prefix to the I{E, I, M, S}R registers
Date: Fri, 12 Apr 2019 10:33:22 +0300 [thread overview]
Message-ID: <20190412073322.GH3888@intel.com> (raw)
In-Reply-To: <20190410235344.31199-4-paulo.r.zanoni@intel.com>
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@linux.intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
> 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@linux.intel.com>
> }
>
> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2019-04-12 7:33 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-10 23:53 [PATCH 0/5] IRQ initialization debloat and conversion to uncore, v2 Paulo Zanoni
2019-04-10 23:53 ` [PATCH 1/5] drm/i915: refactor the IRQ init/reset macros Paulo Zanoni
2019-04-10 23:53 ` [PATCH 2/5] drm/i915: don't specify the IRQ register in the gen2 macros Paulo Zanoni
2019-04-11 23:31 ` Daniele Ceraolo Spurio
2019-04-10 23:53 ` [PATCH 3/5] drm/i915: add GEN2_ prefix to the I{E, I, M, S}R registers Paulo Zanoni
2019-04-12 7:33 ` Ville Syrjälä [this message]
2019-04-10 23:53 ` [PATCH 4/5] drm/i915: convert the IRQ initialization functions to intel_uncore Paulo Zanoni
2019-04-10 23:53 ` [PATCH 5/5] drm/i915: fully convert the IRQ initialization macros " Paulo Zanoni
2019-04-11 1:08 ` ✓ Fi.CI.BAT: success for IRQ initialization debloat and conversion to uncore (rev2) Patchwork
2019-04-12 20:57 ` Paulo Zanoni
2019-04-15 6:42 ` Tomi Sarvela
2019-04-15 23:04 ` Paulo Zanoni
2019-04-12 23:01 ` ✗ Fi.CI.SPARSE: warning for IRQ initialization debloat and conversion to uncore (rev3) Patchwork
2019-04-12 23:29 ` ✓ Fi.CI.BAT: success " Patchwork
2019-04-13 3:55 ` ✓ Fi.CI.IGT: " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190412073322.GH3888@intel.com \
--to=ville.syrjala@linux.intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=paulo.r.zanoni@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox