diff mbox series

[04/36] drm/i915: Trim the ironlake+ irq handler

Message ID 20200601072446.19548-4-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show
Series [01/36] drm/i915: Handle very early engine initialisation failure | expand

Commit Message

Chris Wilson June 1, 2020, 7:24 a.m. UTC 
Ever noticed that our interrupt handlers are where we spend most of our
time on a busy system? In part this is unavoidable as each interrupt
requires to poll and reset several registers, but we can try and so as
efficiently as possible.

Function                                     old     new   delta
ilk_irq_handler                             2317    2156    -161

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_irq.c | 59 ++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 31 deletions(-)

Comments

Mika Kuoppala June 1, 2020, 11:49 a.m. UTC | #1 
Chris Wilson <chris@chris-wilson.co.uk> writes:

> Ever noticed that our interrupt handlers are where we spend most of our
> time on a busy system? In part this is unavoidable as each interrupt
> requires to poll and reset several registers, but we can try and so as
> efficiently as possible.
>
> Function                                     old     new   delta
> ilk_irq_handler                             2317    2156    -161
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 59 ++++++++++++++++-----------------
>  1 file changed, 28 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 63579ab71cf6..07c0c7ea3795 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2097,69 +2097,66 @@ static void ivb_display_irq_handler(struct drm_i915_private *dev_priv,
>   */
>  static irqreturn_t ilk_irq_handler(int irq, void *arg)
>  {
> -	struct drm_i915_private *dev_priv = arg;
> +	struct drm_i915_private *i915 = arg;
> +	void __iomem * const regs = i915->uncore.regs;
>  	u32 de_iir, gt_iir, de_ier, sde_ier = 0;
> -	irqreturn_t ret = IRQ_NONE;
>  
> -	if (!intel_irqs_enabled(dev_priv))
> +	if (!unlikely(intel_irqs_enabled(i915)))

Put ! inside the unlikely for readability please.

>  		return IRQ_NONE;
>  
>  	/* IRQs are synced during runtime_suspend, we don't require a wakeref */
> -	disable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
> +	disable_rpm_wakeref_asserts(&i915->runtime_pm);
>  
>  	/* disable master interrupt before clearing iir  */
> -	de_ier = I915_READ(DEIER);
> -	I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
> +	de_ier = raw_reg_read(regs, DEIER);
> +	raw_reg_write(regs, DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
>  
>  	/* Disable south interrupts. We'll only write to SDEIIR once, so further
>  	 * interrupts will will be stored on its back queue, and then we'll be
>  	 * able to process them after we restore SDEIER (as soon as we restore
>  	 * it, we'll get an interrupt if SDEIIR still has something to process
>  	 * due to its back queue). */
> -	if (!HAS_PCH_NOP(dev_priv)) {
> -		sde_ier = I915_READ(SDEIER);
> -		I915_WRITE(SDEIER, 0);
> +	if (!HAS_PCH_NOP(i915)) {
> +		sde_ier = raw_reg_read(regs, SDEIER);
> +		raw_reg_write(regs, SDEIER, 0);
>  	}
>  
>  	/* Find, clear, then process each source of interrupt */
>  
> -	gt_iir = I915_READ(GTIIR);
> +	gt_iir = raw_reg_read(regs, GTIIR);
>  	if (gt_iir) {
> -		I915_WRITE(GTIIR, gt_iir);
> -		ret = IRQ_HANDLED;
> -		if (INTEL_GEN(dev_priv) >= 6)
> -			gen6_gt_irq_handler(&dev_priv->gt, gt_iir);
> +		raw_reg_write(regs, GTIIR, gt_iir);
> +		if (INTEL_GEN(i915) >= 6)
> +			gen6_gt_irq_handler(&i915->gt, gt_iir);
>  		else
> -			gen5_gt_irq_handler(&dev_priv->gt, gt_iir);
> +			gen5_gt_irq_handler(&i915->gt, gt_iir);
>  	}
>  
> -	de_iir = I915_READ(DEIIR);
> +	de_iir = raw_reg_read(regs, DEIIR);
>  	if (de_iir) {
> -		I915_WRITE(DEIIR, de_iir);
> -		ret = IRQ_HANDLED;
> -		if (INTEL_GEN(dev_priv) >= 7)
> -			ivb_display_irq_handler(dev_priv, de_iir);
> +		raw_reg_write(regs, DEIIR, de_iir);
> +		if (INTEL_GEN(i915) >= 7)
> +			ivb_display_irq_handler(i915, de_iir);
>  		else
> -			ilk_display_irq_handler(dev_priv, de_iir);
> +			ilk_display_irq_handler(i915, de_iir);
>  	}
>  
> -	if (INTEL_GEN(dev_priv) >= 6) {
> -		u32 pm_iir = I915_READ(GEN6_PMIIR);
> +	if (INTEL_GEN(i915) >= 6) {
> +		u32 pm_iir = raw_reg_read(regs, GEN6_PMIIR);
>  		if (pm_iir) {
> -			I915_WRITE(GEN6_PMIIR, pm_iir);
> -			ret = IRQ_HANDLED;
> -			gen6_rps_irq_handler(&dev_priv->gt.rps, pm_iir);
> +			raw_reg_write(regs, GEN6_PMIIR, pm_iir);
> +			gen6_rps_irq_handler(&i915->gt.rps, pm_iir);
>  		}
>  	}
>  
> -	I915_WRITE(DEIER, de_ier);
> -	if (!HAS_PCH_NOP(dev_priv))
> -		I915_WRITE(SDEIER, sde_ier);
> +	raw_reg_write(regs, DEIER, de_ier);
> +	if (sde_ier)
> +		raw_reg_write(regs, SDEIER, sde_ier);
>  
>  	/* IRQs are synced during runtime_suspend, we don't require a wakeref */
> -	enable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
> +	enable_rpm_wakeref_asserts(&i915->runtime_pm);
>  
> -	return ret;
> +	return IRQ_HANDLED;

Change in here is that if we catch a intr without any work, we now
return handled instead of none. 

And as we have not actually done anything to prevent the next
one to kicking off, this is potentially dangerous.

But we explicitly clear the enables tho, but is it enough?

-Mika

>  }
>  
>  static void bxt_hpd_irq_handler(struct drm_i915_private *dev_priv,
> -- 
> 2.20.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Chris Wilson June 1, 2020, noon UTC | #2 
Quoting Mika Kuoppala (2020-06-01 12:49:49)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Ever noticed that our interrupt handlers are where we spend most of our
> > time on a busy system? In part this is unavoidable as each interrupt
> > requires to poll and reset several registers, but we can try and so as
> > efficiently as possible.
> >
> > Function                                     old     new   delta
> > ilk_irq_handler                             2317    2156    -161
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 59 ++++++++++++++++-----------------
> >  1 file changed, 28 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 63579ab71cf6..07c0c7ea3795 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -2097,69 +2097,66 @@ static void ivb_display_irq_handler(struct drm_i915_private *dev_priv,
> >   */
> >  static irqreturn_t ilk_irq_handler(int irq, void *arg)
> >  {
> > -     struct drm_i915_private *dev_priv = arg;
> > +     struct drm_i915_private *i915 = arg;
> > +     void __iomem * const regs = i915->uncore.regs;
> >       u32 de_iir, gt_iir, de_ier, sde_ier = 0;
> > -     irqreturn_t ret = IRQ_NONE;
> >  
> > -     if (!intel_irqs_enabled(dev_priv))
> > +     if (!unlikely(intel_irqs_enabled(i915)))
> 
> Put ! inside the unlikely for readability please.
> 
> >               return IRQ_NONE;
> >  
> >       /* IRQs are synced during runtime_suspend, we don't require a wakeref */
> > -     disable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
> > +     disable_rpm_wakeref_asserts(&i915->runtime_pm);
> >  
> >       /* disable master interrupt before clearing iir  */
> > -     de_ier = I915_READ(DEIER);
> > -     I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
> > +     de_ier = raw_reg_read(regs, DEIER);
> > +     raw_reg_write(regs, DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
> >  
> >       /* Disable south interrupts. We'll only write to SDEIIR once, so further
> >        * interrupts will will be stored on its back queue, and then we'll be
> >        * able to process them after we restore SDEIER (as soon as we restore
> >        * it, we'll get an interrupt if SDEIIR still has something to process
> >        * due to its back queue). */
> > -     if (!HAS_PCH_NOP(dev_priv)) {
> > -             sde_ier = I915_READ(SDEIER);
> > -             I915_WRITE(SDEIER, 0);
> > +     if (!HAS_PCH_NOP(i915)) {
> > +             sde_ier = raw_reg_read(regs, SDEIER);
> > +             raw_reg_write(regs, SDEIER, 0);
> >       }
> >  
> >       /* Find, clear, then process each source of interrupt */
> >  
> > -     gt_iir = I915_READ(GTIIR);
> > +     gt_iir = raw_reg_read(regs, GTIIR);
> >       if (gt_iir) {
> > -             I915_WRITE(GTIIR, gt_iir);
> > -             ret = IRQ_HANDLED;
> > -             if (INTEL_GEN(dev_priv) >= 6)
> > -                     gen6_gt_irq_handler(&dev_priv->gt, gt_iir);
> > +             raw_reg_write(regs, GTIIR, gt_iir);
> > +             if (INTEL_GEN(i915) >= 6)
> > +                     gen6_gt_irq_handler(&i915->gt, gt_iir);
> >               else
> > -                     gen5_gt_irq_handler(&dev_priv->gt, gt_iir);
> > +                     gen5_gt_irq_handler(&i915->gt, gt_iir);
> >       }
> >  
> > -     de_iir = I915_READ(DEIIR);
> > +     de_iir = raw_reg_read(regs, DEIIR);
> >       if (de_iir) {
> > -             I915_WRITE(DEIIR, de_iir);
> > -             ret = IRQ_HANDLED;
> > -             if (INTEL_GEN(dev_priv) >= 7)
> > -                     ivb_display_irq_handler(dev_priv, de_iir);
> > +             raw_reg_write(regs, DEIIR, de_iir);
> > +             if (INTEL_GEN(i915) >= 7)
> > +                     ivb_display_irq_handler(i915, de_iir);
> >               else
> > -                     ilk_display_irq_handler(dev_priv, de_iir);
> > +                     ilk_display_irq_handler(i915, de_iir);
> >       }
> >  
> > -     if (INTEL_GEN(dev_priv) >= 6) {
> > -             u32 pm_iir = I915_READ(GEN6_PMIIR);
> > +     if (INTEL_GEN(i915) >= 6) {
> > +             u32 pm_iir = raw_reg_read(regs, GEN6_PMIIR);
> >               if (pm_iir) {
> > -                     I915_WRITE(GEN6_PMIIR, pm_iir);
> > -                     ret = IRQ_HANDLED;
> > -                     gen6_rps_irq_handler(&dev_priv->gt.rps, pm_iir);
> > +                     raw_reg_write(regs, GEN6_PMIIR, pm_iir);
> > +                     gen6_rps_irq_handler(&i915->gt.rps, pm_iir);
> >               }
> >       }
> >  
> > -     I915_WRITE(DEIER, de_ier);
> > -     if (!HAS_PCH_NOP(dev_priv))
> > -             I915_WRITE(SDEIER, sde_ier);
> > +     raw_reg_write(regs, DEIER, de_ier);
> > +     if (sde_ier)
> > +             raw_reg_write(regs, SDEIER, sde_ier);
> >  
> >       /* IRQs are synced during runtime_suspend, we don't require a wakeref */
> > -     enable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
> > +     enable_rpm_wakeref_asserts(&i915->runtime_pm);
> >  
> > -     return ret;
> > +     return IRQ_HANDLED;
> 
> Change in here is that if we catch a intr without any work, we now
> return handled instead of none. 
> 
> And as we have not actually done anything to prevent the next
> one to kicking off, this is potentially dangerous.
> 
> But we explicitly clear the enables tho, but is it enough?

It's MSI-X, to get here means there was an interrupt. Let's check how
much it adds to track IRQ_HANDLED.
-Chris
Chris Wilson June 1, 2020, 12:08 p.m. UTC | #3 
Quoting Chris Wilson (2020-06-01 13:00:43)
> Quoting Mika Kuoppala (2020-06-01 12:49:49)
> > Chris Wilson <chris@chris-wilson.co.uk> writes:
> > 
> > > Ever noticed that our interrupt handlers are where we spend most of our
> > > time on a busy system? In part this is unavoidable as each interrupt
> > > requires to poll and reset several registers, but we can try and so as
> > > efficiently as possible.
> > >
> > > Function                                     old     new   delta
> > > ilk_irq_handler                             2317    2156    -161

With irqreturn_t ret,

ilk_irq_handler.cold                          63      72      +9
ilk_irq_handler                             2221    2083    -138

I love how it can change so much simply by recompiling the baseline.
-Chris
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 63579ab71cf6..07c0c7ea3795 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2097,69 +2097,66 @@  static void ivb_display_irq_handler(struct drm_i915_private *dev_priv,
  */
 static irqreturn_t ilk_irq_handler(int irq, void *arg)
 {
-	struct drm_i915_private *dev_priv = arg;
+	struct drm_i915_private *i915 = arg;
+	void __iomem * const regs = i915->uncore.regs;
 	u32 de_iir, gt_iir, de_ier, sde_ier = 0;
-	irqreturn_t ret = IRQ_NONE;
 
-	if (!intel_irqs_enabled(dev_priv))
+	if (!unlikely(intel_irqs_enabled(i915)))
 		return IRQ_NONE;
 
 	/* IRQs are synced during runtime_suspend, we don't require a wakeref */
-	disable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
+	disable_rpm_wakeref_asserts(&i915->runtime_pm);
 
 	/* disable master interrupt before clearing iir  */
-	de_ier = I915_READ(DEIER);
-	I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
+	de_ier = raw_reg_read(regs, DEIER);
+	raw_reg_write(regs, DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
 
 	/* Disable south interrupts. We'll only write to SDEIIR once, so further
 	 * interrupts will will be stored on its back queue, and then we'll be
 	 * able to process them after we restore SDEIER (as soon as we restore
 	 * it, we'll get an interrupt if SDEIIR still has something to process
 	 * due to its back queue). */
-	if (!HAS_PCH_NOP(dev_priv)) {
-		sde_ier = I915_READ(SDEIER);
-		I915_WRITE(SDEIER, 0);
+	if (!HAS_PCH_NOP(i915)) {
+		sde_ier = raw_reg_read(regs, SDEIER);
+		raw_reg_write(regs, SDEIER, 0);
 	}
 
 	/* Find, clear, then process each source of interrupt */
 
-	gt_iir = I915_READ(GTIIR);
+	gt_iir = raw_reg_read(regs, GTIIR);
 	if (gt_iir) {
-		I915_WRITE(GTIIR, gt_iir);
-		ret = IRQ_HANDLED;
-		if (INTEL_GEN(dev_priv) >= 6)
-			gen6_gt_irq_handler(&dev_priv->gt, gt_iir);
+		raw_reg_write(regs, GTIIR, gt_iir);
+		if (INTEL_GEN(i915) >= 6)
+			gen6_gt_irq_handler(&i915->gt, gt_iir);
 		else
-			gen5_gt_irq_handler(&dev_priv->gt, gt_iir);
+			gen5_gt_irq_handler(&i915->gt, gt_iir);
 	}
 
-	de_iir = I915_READ(DEIIR);
+	de_iir = raw_reg_read(regs, DEIIR);
 	if (de_iir) {
-		I915_WRITE(DEIIR, de_iir);
-		ret = IRQ_HANDLED;
-		if (INTEL_GEN(dev_priv) >= 7)
-			ivb_display_irq_handler(dev_priv, de_iir);
+		raw_reg_write(regs, DEIIR, de_iir);
+		if (INTEL_GEN(i915) >= 7)
+			ivb_display_irq_handler(i915, de_iir);
 		else
-			ilk_display_irq_handler(dev_priv, de_iir);
+			ilk_display_irq_handler(i915, de_iir);
 	}
 
-	if (INTEL_GEN(dev_priv) >= 6) {
-		u32 pm_iir = I915_READ(GEN6_PMIIR);
+	if (INTEL_GEN(i915) >= 6) {
+		u32 pm_iir = raw_reg_read(regs, GEN6_PMIIR);
 		if (pm_iir) {
-			I915_WRITE(GEN6_PMIIR, pm_iir);
-			ret = IRQ_HANDLED;
-			gen6_rps_irq_handler(&dev_priv->gt.rps, pm_iir);
+			raw_reg_write(regs, GEN6_PMIIR, pm_iir);
+			gen6_rps_irq_handler(&i915->gt.rps, pm_iir);
 		}
 	}
 
-	I915_WRITE(DEIER, de_ier);
-	if (!HAS_PCH_NOP(dev_priv))
-		I915_WRITE(SDEIER, sde_ier);
+	raw_reg_write(regs, DEIER, de_ier);
+	if (sde_ier)
+		raw_reg_write(regs, SDEIER, sde_ier);
 
 	/* IRQs are synced during runtime_suspend, we don't require a wakeref */
-	enable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
+	enable_rpm_wakeref_asserts(&i915->runtime_pm);
 
-	return ret;
+	return IRQ_HANDLED;
 }
 
 static void bxt_hpd_irq_handler(struct drm_i915_private *dev_priv,