All of lore.kernel.org
 help / color / mirror / Atom feed
From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
To: Paulo Zanoni <paulo.r.zanoni@intel.com>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 2/5] drm/i915: don't specify the IRQ register in the gen2 macros
Date: Thu, 11 Apr 2019 16:31:07 -0700	[thread overview]
Message-ID: <47de9feb-7755-1e5b-ca79-85bb0cf12585@intel.com> (raw)
In-Reply-To: <20190410235344.31199-3-paulo.r.zanoni@intel.com>



On 4/10/19 4:53 PM, Paulo Zanoni wrote:
> Like the gen3+ macros, the gen2 versions of the IRQ initialization
> macros take the register name in the 'type' argument. But gen2 only
> has one set of registers, so there's really no need to specify the
> type. This commit removes the type argument and uses the registers
> directly instead of passing them through variables.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_irq.c | 57 +++++++++++++++------------------
>   1 file changed, 25 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 60a3f4203ac3..b1f1db2bd879 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -151,19 +151,18 @@ static void gen3_irq_reset(struct drm_i915_private *dev_priv, i915_reg_t imr,
>   	POSTING_READ(iir);
>   }
>   
> -static void gen2_irq_reset(struct drm_i915_private *dev_priv, i915_reg_t imr,
> -			   i915_reg_t iir, i915_reg_t ier)
> +static void gen2_irq_reset(struct drm_i915_private *dev_priv)
>   {
> -	I915_WRITE16(imr, 0xffff);
> -	POSTING_READ16(imr);
> +	I915_WRITE16(IMR, 0xffff);
> +	POSTING_READ16(IMR);
>   
> -	I915_WRITE16(ier, 0);
> +	I915_WRITE16(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(IIR, 0xffff);
> +	POSTING_READ16(IIR);
> +	I915_WRITE16(IIR, 0xffff);
> +	POSTING_READ16(IIR);
>   }
>   
>   #define GEN8_IRQ_RESET_NDX(type, which) \
> @@ -176,8 +175,8 @@ static void gen2_irq_reset(struct drm_i915_private *dev_priv, i915_reg_t imr,
>   #define GEN3_IRQ_RESET(type) \
>   	gen3_irq_reset(dev_priv, type##IMR, type##IIR, type##IER)
>   
> -#define GEN2_IRQ_RESET(type) \
> -	gen2_irq_reset(dev_priv, type##IMR, type##IIR, type##IER)
> +#define GEN2_IRQ_RESET() \
> +	gen2_irq_reset(dev_priv)

We could potentially drop the macro entirely now since it doesn't really 
add any functional value. The same applies for GEN2_IRQ_INIT.

However, I see the argument for keeping things consistent and use the 
same "look" across gens, so with or without the change:

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Daniele

>   
>   /*
>    * We should clear IMR at preinstall/uninstall, and just check at postinstall.
> @@ -198,20 +197,19 @@ static void gen3_assert_iir_is_zero(struct drm_i915_private *dev_priv,
>   	POSTING_READ(reg);
>   }
>   
> -static void gen2_assert_iir_is_zero(struct drm_i915_private *dev_priv,
> -				    i915_reg_t reg)
> +static void gen2_assert_iir_is_zero(struct drm_i915_private *dev_priv)
>   {
> -	u16 val = I915_READ16(reg);
> +	u16 val = I915_READ16(IIR);
>   
>   	if (val == 0)
>   		return;
>   
>   	WARN(1, "Interrupt register 0x%x is not zero: 0x%08x\n",
> -	     i915_mmio_reg_offset(reg), val);
> -	I915_WRITE16(reg, 0xffff);
> -	POSTING_READ16(reg);
> -	I915_WRITE16(reg, 0xffff);
> -	POSTING_READ16(reg);
> +	     i915_mmio_reg_offset(IIR), val);
> +	I915_WRITE16(IIR, 0xffff);
> +	POSTING_READ16(IIR);
> +	I915_WRITE16(IIR, 0xffff);
> +	POSTING_READ16(IIR);
>   }
>   
>   static void gen3_irq_init(struct drm_i915_private *dev_priv,
> @@ -227,15 +225,13 @@ static void gen3_irq_init(struct drm_i915_private *dev_priv,
>   }
>   
>   static void gen2_irq_init(struct drm_i915_private *dev_priv,
> -			  i915_reg_t imr, u32 imr_val,
> -			  i915_reg_t ier, u32 ier_val,
> -			  i915_reg_t iir)
> +			  u32 imr_val, u32 ier_val)
>   {
> -	gen2_assert_iir_is_zero(dev_priv, iir);
> +	gen2_assert_iir_is_zero(dev_priv);
>   
> -	I915_WRITE16(ier, ier_val);
> -	I915_WRITE16(imr, imr_val);
> -	POSTING_READ16(imr);
> +	I915_WRITE16(IER, ier_val);
> +	I915_WRITE16(IMR, imr_val);
> +	POSTING_READ16(IMR);
>   }
>   
>   #define GEN8_IRQ_INIT_NDX(type, which, imr_val, ier_val) \
> @@ -253,11 +249,8 @@ static void gen2_irq_init(struct drm_i915_private *dev_priv,
>   		      type##IER, ier_val, \
>   		      type##IIR)
>   
> -#define GEN2_IRQ_INIT(type, imr_val, ier_val) \
> -	gen2_irq_init(dev_priv, \
> -		      type##IMR, imr_val, \
> -		      type##IER, ier_val, \
> -		      type##IIR)
> +#define GEN2_IRQ_INIT(imr_val, ier_val) \
> +	gen2_irq_init(dev_priv, imr_val, ier_val)
>   
>   static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir);
>   static void gen9_guc_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir);
> @@ -4247,7 +4240,7 @@ static int i8xx_irq_postinstall(struct drm_device *dev)
>   		I915_MASTER_ERROR_INTERRUPT |
>   		I915_USER_INTERRUPT;
>   
> -	GEN2_IRQ_INIT(, dev_priv->irq_mask, enable_mask);
> +	GEN2_IRQ_INIT(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. */
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

  reply	other threads:[~2019-04-11 23:31 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 [this message]
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ä
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=47de9feb-7755-1e5b-ca79-85bb0cf12585@intel.com \
    --to=daniele.ceraolospurio@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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.