[Intel-gfx] [PATCH 1/3] drm/i915: refactor the IRQ init/reset macros

Paulo Zanoni paulo.r.zanoni at intel.com
Wed Apr 10 20:45:18 UTC 2019


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 at 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) ?

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 at linux.intel.com>

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 at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx



More information about the Intel-gfx mailing list