Re: [PATCH 1/3] drm/i915: refactor the IRQ init/reset macros

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Apr 08, 2019 at 05:37:27PM -0700, Paulo Zanoni wrote:
> The whole point of having macros here is for the token pasting
> necessary to automatically have IMR, IIR and IER selected. We don't
> really need or want all the inlining that happens as a consequence.
> The good thing about the current code is that it works regardless of
> the relative offsets between these registers (they change after gen3).

Did you mean "after gen4"? The DE/GT split happened at ilk, and I
guess that's when the four registers also got reshuffled for some
reason. Ignoring the funkyness of vlv/chv or course :)

> 
> One thing which we can do is to split the logic of what we do with
> imr/ier/iir to functions separate from the macros that pick them.
> That's what we do in this commit. This allows us to get rid of the
> gen8 duplicates and also all the inlining:
> 
> add/remove: 2/0 grow/shrink: 0/21 up/down: 383/-6006 (-5623)
> Function                                     old     new   delta
> gen3_irq_reset                                 -     233    +233
> gen3_irq_init                                  -     150    +150
> i8xx_irq_postinstall                         459     442     -17
> gen11_irq_postinstall                        804     744     -60
> ironlake_irq_postinstall                     450     353     -97
> vlv_display_irq_postinstall                  348     245    -103
> i965_irq_postinstall                         378     275    -103
> i915_irq_postinstall                         333     228    -105
> gen8_irq_power_well_post_enable              374     210    -164
> ironlake_irq_reset                           397     218    -179
> vlv_display_irq_reset                        616     433    -183
> i965_irq_reset                               374     180    -194
> cherryview_irq_reset                         379     185    -194
> i915_irq_reset                               407     209    -198
> ibx_irq_reset                                332     133    -199
> gen5_gt_irq_postinstall                      533     332    -201
> gen8_irq_power_well_pre_disable              434     204    -230
> gen8_gt_irq_postinstall                      469     196    -273
> gen8_de_irq_postinstall                     1200     805    -395
> gen5_gt_irq_reset                            471      76    -395
> gen8_gt_irq_reset                            775      99    -676
> gen8_irq_reset                              1100     333    -767
> gen11_irq_reset                             1959     686   -1273
> Total: Before=2262051, After=2256428, chg -0.25%
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 123 +++++++++++++++++++-------------
>  1 file changed, 73 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 6454ddc37f8b..a1e7944fb5d0 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -136,36 +136,45 @@ static const u32 hpd_icp[HPD_NUM_PINS] = {
>  	[HPD_PORT_F] = SDE_TC4_HOTPLUG_ICP
>  };
>  
> -/* IIR can theoretically queue up two events. Be paranoid. */
> -#define GEN8_IRQ_RESET_NDX(type, which) do { \
> -	I915_WRITE(GEN8_##type##_IMR(which), 0xffffffff); \
> -	POSTING_READ(GEN8_##type##_IMR(which)); \
> -	I915_WRITE(GEN8_##type##_IER(which), 0); \
> -	I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff); \
> -	POSTING_READ(GEN8_##type##_IIR(which)); \
> -	I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff); \
> -	POSTING_READ(GEN8_##type##_IIR(which)); \
> -} while (0)
> -
> -#define GEN3_IRQ_RESET(type) do { \
> -	I915_WRITE(type##IMR, 0xffffffff); \
> -	POSTING_READ(type##IMR); \
> -	I915_WRITE(type##IER, 0); \
> -	I915_WRITE(type##IIR, 0xffffffff); \
> -	POSTING_READ(type##IIR); \
> -	I915_WRITE(type##IIR, 0xffffffff); \
> -	POSTING_READ(type##IIR); \
> -} while (0)
> -
> -#define GEN2_IRQ_RESET(type) do { \
> -	I915_WRITE16(type##IMR, 0xffff); \
> -	POSTING_READ16(type##IMR); \
> -	I915_WRITE16(type##IER, 0); \
> -	I915_WRITE16(type##IIR, 0xffff); \
> -	POSTING_READ16(type##IIR); \
> -	I915_WRITE16(type##IIR, 0xffff); \
> -	POSTING_READ16(type##IIR); \
> -} while (0)
> +static void gen3_irq_reset(struct drm_i915_private *dev_priv, i915_reg_t imr,
> +			   i915_reg_t iir, i915_reg_t ier)
> +{
> +	I915_WRITE(imr, 0xffffffff);
> +	POSTING_READ(imr);
> +
> +	I915_WRITE(ier, 0);
> +
> +	/* IIR can theoretically queue up two events. Be paranoid. */
> +	I915_WRITE(iir, 0xffffffff);
> +	POSTING_READ(iir);
> +	I915_WRITE(iir, 0xffffffff);
> +	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)
> +{
> +	I915_WRITE16(imr, 0xffff);
> +	POSTING_READ16(imr);
> +
> +	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);
> +}
> +
> +#define GEN8_IRQ_RESET_NDX(type, which) \
> +	gen3_irq_reset(dev_priv, GEN8_##type##_IMR(which), \
> +		       GEN8_##type##_IIR(which), GEN8_##type##_IER(which))
> +
> +#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)
>  
>  /*
>   * We should clear IMR at preinstall/uninstall, and just check at postinstall.
> @@ -202,26 +211,40 @@ static void gen2_assert_iir_is_zero(struct drm_i915_private *dev_priv,
>  	POSTING_READ16(reg);
>  }
>  
> -#define GEN8_IRQ_INIT_NDX(type, which, imr_val, ier_val) do { \
> -	gen3_assert_iir_is_zero(dev_priv, GEN8_##type##_IIR(which)); \
> -	I915_WRITE(GEN8_##type##_IER(which), (ier_val)); \
> -	I915_WRITE(GEN8_##type##_IMR(which), (imr_val)); \
> -	POSTING_READ(GEN8_##type##_IMR(which)); \
> -} while (0)
> -
> -#define GEN3_IRQ_INIT(type, imr_val, ier_val) do { \
> -	gen3_assert_iir_is_zero(dev_priv, type##IIR); \
> -	I915_WRITE(type##IER, (ier_val)); \
> -	I915_WRITE(type##IMR, (imr_val)); \
> -	POSTING_READ(type##IMR); \
> -} while (0)
> -
> -#define GEN2_IRQ_INIT(type, imr_val, ier_val) do { \
> -	gen2_assert_iir_is_zero(dev_priv, type##IIR); \
> -	I915_WRITE16(type##IER, (ier_val)); \
> -	I915_WRITE16(type##IMR, (imr_val)); \
> -	POSTING_READ16(type##IMR); \
> -} while (0)
> +static void gen3_irq_init(struct drm_i915_private *dev_priv, i915_reg_t imr,
> +			  i915_reg_t iir, i915_reg_t ier, u32 imr_val,
> +			  u32 ier_val)

Maybe we should order these more like this
foo(imr, imr_value, ier, ier_value, iir) ?

Could make a bit more obvious which values goes to which register. But
I suppose as long as they're always wrapped in those macros it doesn't
really matter.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +{
> +	gen3_assert_iir_is_zero(dev_priv, iir);
> +
> +	I915_WRITE(ier, ier_val);
> +	I915_WRITE(imr, imr_val);
> +	POSTING_READ(imr);
> +}
> +
> +static void gen2_irq_init(struct drm_i915_private *dev_priv, i915_reg_t imr,
> +			  i915_reg_t iir, i915_reg_t ier, u32 imr_val,
> +			  u32 ier_val)
> +{
> +	gen2_assert_iir_is_zero(dev_priv, iir);
> +
> +	I915_WRITE16(ier, ier_val);
> +	I915_WRITE16(imr, imr_val);
> +	POSTING_READ16(imr);
> +}
> +
> +#define GEN8_IRQ_INIT_NDX(type, which, imr_val, ier_val) \
> +	gen3_irq_init(dev_priv, GEN8_##type##_IMR(which), \
> +		      GEN8_##type##_IIR(which), GEN8_##type##_IER(which), \
> +		      imr_val, ier_val)
> +
> +#define GEN3_IRQ_INIT(type, imr_val, ier_val) \
> +	gen3_irq_init(dev_priv, type##IMR, type##IIR, type##IER, imr_val, \
> +		      ier_val)
> +
> +#define GEN2_IRQ_INIT(type, imr_val, ier_val) \
> +	gen2_irq_init(dev_priv, type##IMR, type##IIR, type##IER, 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);
> -- 
> 2.20.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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