Re: [Intel-gfx] [PATCH 1/3] drm/i915: refactor the IRQ init/reset macros
Em ter, 2019-04-09 às 21:10 +0300, Ville Syrjälä escreveu:
> 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 :)
>
You're right. I'll fix the commit message.
> > 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_postinstall804 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_reset616 433-183
> > i965_irq_reset 374 180-194
> > cherryview_irq_reset 379 185-194
> > i915_irq_reset 407 209-198
> > ibx_irq_reset332 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_reset471 76-395
> > gen8_gt_irq_reset775 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
> > ---
> > 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), 0x); \
> > - POSTING_READ(GEN8_##type##_IMR(which)); \
> > - I915_WRITE(GEN8_##type##_IER(which), 0); \
> > - I915_WRITE(GEN8_##type##_IIR(which), 0x); \
> > - POSTING_READ(GEN8_##type##_IIR(which)); \
> > - I915_WRITE(GEN8_##type##_IIR(which), 0x); \
> > - POSTING_READ(GEN8_##type##_IIR(which)); \
> > -} while (0)
> > -
> > -#define GEN3_IRQ_RESET(type) do { \
> > - I915_WRITE(type##IMR, 0x); \
> > - POSTING_READ(type##IMR); \
> > - I915_WRITE(type##IER, 0); \
> > - I915_WRITE(type##IIR, 0x); \
> > - POSTING_READ(type##IIR); \
> > - I915_WRITE(type##IIR, 0x); \
> > - POSTING_READ(type##IIR); \
> > -} while (0)
> > -
> > -#define GEN2_IRQ_RESET(type) do { \
> > - I915_WRITE16(type##IMR, 0x); \
> > - POSTING_READ16(type##IMR); \
> > - I915_WRITE16(type##IER, 0); \
> > - I915_WRITE16(type##IIR, 0x); \
> > - POSTING_READ16(type##IIR); \
> > - I915_WRITE16(type##IIR, 0x); \
> > - 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, 0x);
> > + POSTING_READ(imr);
> > +
> > + I915_WRITE(ier, 0);
> > +
> > + /* IIR can theoretically queue up two events. Be paranoid. */
> > + I915_WRITE(iir, 0x);
> > + POSTING_READ(iir);
> > + I915_WRITE(iir, 0x);
> > + POSTING_READ(iir);
> > +}
> > +
> > +static void gen2_irq_reset(struct
Re: [Intel-gfx] [PATCH 1/3] drm/i915: refactor the IRQ init/reset macros
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_postinstall804 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_reset616 433-183
> i965_irq_reset 374 180-194
> cherryview_irq_reset 379 185-194
> i915_irq_reset 407 209-198
> ibx_irq_reset332 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_reset471 76-395
> gen8_gt_irq_reset775 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
> ---
> 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), 0x); \
> - POSTING_READ(GEN8_##type##_IMR(which)); \
> - I915_WRITE(GEN8_##type##_IER(which), 0); \
> - I915_WRITE(GEN8_##type##_IIR(which), 0x); \
> - POSTING_READ(GEN8_##type##_IIR(which)); \
> - I915_WRITE(GEN8_##type##_IIR(which), 0x); \
> - POSTING_READ(GEN8_##type##_IIR(which)); \
> -} while (0)
> -
> -#define GEN3_IRQ_RESET(type) do { \
> - I915_WRITE(type##IMR, 0x); \
> - POSTING_READ(type##IMR); \
> - I915_WRITE(type##IER, 0); \
> - I915_WRITE(type##IIR, 0x); \
> - POSTING_READ(type##IIR); \
> - I915_WRITE(type##IIR, 0x); \
> - POSTING_READ(type##IIR); \
> -} while (0)
> -
> -#define GEN2_IRQ_RESET(type) do { \
> - I915_WRITE16(type##IMR, 0x); \
> - POSTING_READ16(type##IMR); \
> - I915_WRITE16(type##IER, 0); \
> - I915_WRITE16(type##IIR, 0x); \
> - POSTING_READ16(type##IIR); \
> - I915_WRITE16(type##IIR, 0x); \
> - 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, 0x);
> + POSTING_READ(imr);
> +
> + I915_WRITE(ier, 0);
> +
> + /* IIR can theoretically queue up two events. Be paranoid. */
> + I915_WRITE(iir, 0x);
> + POSTING_READ(iir);
> + I915_WRITE(iir, 0x);
> + 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, 0x);
> + POSTING_READ16(imr);
> +
> + I915_WRITE16(ier, 0);
> +
> + /* IIR can theoretically queue up two
[Intel-gfx] [PATCH 1/3] drm/i915: refactor the IRQ init/reset macros
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).
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_postinstall804 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_reset616 433-183
i965_irq_reset 374 180-194
cherryview_irq_reset 379 185-194
i915_irq_reset 407 209-198
ibx_irq_reset332 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_reset471 76-395
gen8_gt_irq_reset775 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
---
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), 0x); \
- POSTING_READ(GEN8_##type##_IMR(which)); \
- I915_WRITE(GEN8_##type##_IER(which), 0); \
- I915_WRITE(GEN8_##type##_IIR(which), 0x); \
- POSTING_READ(GEN8_##type##_IIR(which)); \
- I915_WRITE(GEN8_##type##_IIR(which), 0x); \
- POSTING_READ(GEN8_##type##_IIR(which)); \
-} while (0)
-
-#define GEN3_IRQ_RESET(type) do { \
- I915_WRITE(type##IMR, 0x); \
- POSTING_READ(type##IMR); \
- I915_WRITE(type##IER, 0); \
- I915_WRITE(type##IIR, 0x); \
- POSTING_READ(type##IIR); \
- I915_WRITE(type##IIR, 0x); \
- POSTING_READ(type##IIR); \
-} while (0)
-
-#define GEN2_IRQ_RESET(type) do { \
- I915_WRITE16(type##IMR, 0x); \
- POSTING_READ16(type##IMR); \
- I915_WRITE16(type##IER, 0); \
- I915_WRITE16(type##IIR, 0x); \
- POSTING_READ16(type##IIR); \
- I915_WRITE16(type##IIR, 0x); \
- 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, 0x);
+ POSTING_READ(imr);
+
+ I915_WRITE(ier, 0);
+
+ /* IIR can theoretically queue up two events. Be paranoid. */
+ I915_WRITE(iir, 0x);
+ POSTING_READ(iir);
+ I915_WRITE(iir, 0x);
+ 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, 0x);
+ POSTING_READ16(imr);
+
+ I915_WRITE16(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);
+}
+
+#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) \
+
