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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx>
> +{
> + 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
