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_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) ?
Sure. I'll also try to add a few extra line breaks to the calls to try
to make the arguments a little more readable.
>
> 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>
Thanks a lot!
>
> > +{
> > + 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
