[Intel-gfx] [PATCH 4/5] drm/i915/icl: Deal with GT INT DW correctly
Michel Thierry
michel.thierry at intel.com
Thu Apr 5 18:27:27 UTC 2018
On 4/5/2018 7:00 AM, Mika Kuoppala wrote:
> From: Oscar Mateo <oscar.mateo at intel.com>
>
> BSpec says:
>
> "Second level interrupt events are stored in the GT INT DW. GT INT DW is
> a double buffered structure. A snapshot of events is taken when SW reads
> GT INT DW. From the time of read to the time of SW completely clearing
> GT INT DW (to indicate end of service), all incoming interrupts are logged
> in a secondary storage structure. this guarantees that the record of
> interrupts SW is servicing will not change while under service".
>
> We read GT INT DW in several places now:
>
> - The IRQ handler (banks 0 and 1) where, hopefully, it is completely
> cleared (operation now covered with the irq lock).
> - The 'reset' interrupts functions for RPS and GuC logs, where we clear
> the bit we are interested in and leave the others for the normal
> interrupt handler.
> - The 'enable' interrupts functions for RPS and GuC logs, as a measure
> of precaution. Here we could relax a bit and don't check GT INT DW
> at all or, if we do, at least we should clear the offending bit
> (which is what this patch does).
>
> Note that, if every bit is cleared on reading GT INT DW, the register
> won't be locked. Also note that, according to the BSpec, GT INT DW
> cannot be cleared without first servicing the Selector & Shared IIR
> registers.
>
> v2:
> - Remove some code duplication (Tvrtko)
> - Make sure GT_INTR_DW are protected by the irq spinlock, since it's a
> global resource (Tvrtko)
>
> v3: Optimize the spinlock (Tvrtko)
>
> v4: Rebase.
> v5:
> - take spinlock on outer scope to please sparse (Mika)
> - use raw_reg accessors (Mika)
>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> Signed-off-by: Oscar Mateo <oscar.mateo at intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com> (v4)
> Signed-off-by: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_irq.c | 112 +++++++++++++++++++++++++++-------------
> 1 file changed, 75 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 0b471775ce38..653bab682d5e 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -243,6 +243,41 @@ void i915_hotplug_interrupt_update(struct drm_i915_private *dev_priv,
> spin_unlock_irq(&dev_priv->irq_lock);
> }
>
> +static u32
> +gen11_gt_engine_identity(struct drm_i915_private * const i915,
> + const unsigned int bank, const unsigned int bit);
> +
> +static bool gen11_reset_one_iir(struct drm_i915_private * const i915,
> + const unsigned int bank,
> + const unsigned int bit)
> +{
> + void __iomem * const regs = i915->regs;
> + u32 dw;
> +
> + lockdep_assert_held(&i915->irq_lock);
> +
> + dw = raw_reg_read(regs, GEN11_GT_INTR_DW(bank));
> + if (dw & BIT(bit)) {
> + /*
> + * According to the BSpec, DW_IIR bits cannot be cleared without
> + * first servicing the Selector & Shared IIR registers.
> + */
> + gen11_gt_engine_identity(i915, bank, bit);
> +
> + /*
> + * We locked GT INT DW by reading it. If we want to (try
> + * to) recover from this succesfully, we need to clear
> + * our bit, otherwise we are locking the register for
> + * everybody.
> + */
> + raw_reg_write(regs, GEN11_GT_INTR_DW(bank), BIT(bit));
> +
> + return true;
> + }
> +
> + return false;
> +}
> +
> /**
> * ilk_update_display_irq - update DEIMR
> * @dev_priv: driver private
> @@ -412,26 +447,12 @@ static void gen6_disable_pm_irq(struct drm_i915_private *dev_priv, u32 disable_m
> /* though a barrier is missing here, but don't really need a one */
> }
>
> -static u32
> -gen11_gt_engine_identity(struct drm_i915_private * const i915,
> - const unsigned int bank, const unsigned int bit);
> -
> void gen11_reset_rps_interrupts(struct drm_i915_private *dev_priv)
> {
> - u32 dw;
> -
> spin_lock_irq(&dev_priv->irq_lock);
>
> - /*
> - * According to the BSpec, DW_IIR bits cannot be cleared without
> - * first servicing the Selector & Shared IIR registers.
> - */
> - dw = I915_READ_FW(GEN11_GT_INTR_DW0);
> - while (dw & BIT(GEN11_GTPM)) {
> - gen11_gt_engine_identity(dev_priv, 0, GEN11_GTPM);
> - I915_WRITE_FW(GEN11_GT_INTR_DW0, BIT(GEN11_GTPM));
> - dw = I915_READ_FW(GEN11_GT_INTR_DW0);
> - }
> + while (gen11_reset_one_iir(dev_priv, 0, GEN11_GTPM))
> + ;
>
> dev_priv->gt_pm.rps.pm_iir = 0;
>
> @@ -455,10 +476,12 @@ void gen6_enable_rps_interrupts(struct drm_i915_private *dev_priv)
>
> spin_lock_irq(&dev_priv->irq_lock);
> WARN_ON_ONCE(rps->pm_iir);
> +
> if (INTEL_GEN(dev_priv) >= 11)
> - WARN_ON_ONCE(I915_READ_FW(GEN11_GT_INTR_DW0) & BIT(GEN11_GTPM));
> + WARN_ON_ONCE(gen11_reset_one_iir(dev_priv, 0, GEN11_GTPM));
> else
> WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) & dev_priv->pm_rps_events);
> +
> rps->interrupts_enabled = true;
> gen6_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
>
> @@ -2778,6 +2801,8 @@ gen11_gt_engine_identity(struct drm_i915_private * const i915,
> u32 timeout_ts;
> u32 ident;
>
> + lockdep_assert_held(&i915->irq_lock);
> +
> raw_reg_write(regs, GEN11_IIR_REG_SELECTOR(bank), BIT(bit));
>
> /*
> @@ -2856,36 +2881,49 @@ gen11_gt_identity_handler(struct drm_i915_private * const i915,
> }
>
> static void
> -gen11_gt_irq_handler(struct drm_i915_private * const i915,
> - const u32 master_ctl)
> +gen11_gt_bank_handler(struct drm_i915_private * const i915,
> + const unsigned int bank)
> {
> void __iomem * const regs = i915->regs;
> - unsigned int bank;
> + unsigned long intr_dw;
> + unsigned int bit;
>
> - for (bank = 0; bank < 2; bank++) {
> - unsigned long intr_dw;
> - unsigned int bit;
> + lockdep_assert_held(&i915->irq_lock);
>
> - if (!(master_ctl & GEN11_GT_DW_IRQ(bank)))
> - continue;
> + intr_dw = raw_reg_read(regs, GEN11_GT_INTR_DW(bank));
>
> - intr_dw = raw_reg_read(regs, GEN11_GT_INTR_DW(bank));
> + if (unlikely(!intr_dw)) {
> + DRM_ERROR("GT_INTR_DW%u blank!\n", bank);
> + return;
> + }
>
> - if (unlikely(!intr_dw)) {
> - DRM_ERROR("GT_INTR_DW%u blank!\n", bank);
> - continue;
> - }
> + for_each_set_bit(bit, &intr_dw, 32) {
> + const u32 ident = gen11_gt_engine_identity(i915,
> + bank, bit);
> +
> + gen11_gt_identity_handler(i915, ident);
> + }
>
> - for_each_set_bit(bit, &intr_dw, 32) {
> - const u32 ident = gen11_gt_engine_identity(i915,
> - bank, bit);
> + /* Clear must be after shared has been served for engine */
> + raw_reg_write(regs, GEN11_GT_INTR_DW(bank), intr_dw);
> +}
>
> - gen11_gt_identity_handler(i915, ident);
> - }
> +static void
> +gen11_gt_irq_handler(struct drm_i915_private * const i915,
> + const u32 master_ctl)
> +{
> + unsigned int bank;
> +
> + spin_lock(&i915->irq_lock);
>
> - /* Clear must be after shared has been served for engine */
> - raw_reg_write(regs, GEN11_GT_INTR_DW(bank), intr_dw);
> + for (bank = 0; bank < 2; bank++) {
> + if (!(master_ctl & GEN11_GT_DW_IRQ(bank)))
> + continue;
> +
> + gen11_gt_bank_handler(i915, bank);
Since you refactored this to please sparse (thank you for that), I would
also have changed the if() logic to call gen11_gt_bank_handler and omit
continue, i.e.:
+ for (bank = 0; bank < 2; bank++) {
+ if (master_ctl & GEN11_GT_DW_IRQ(bank))
+ gen11_gt_bank_handler(i915, bank);
+ }
> +
> + spin_unlock(&i915->irq_lock);
> }
>
> static irqreturn_t gen11_irq_handler(int irq, void *arg)
>
But it does what is supposed to do so,
Reviewed-by: Michel Thierry <michel.thierry at intel.com>
BTW, now that we have gen11_reset_one_iir, we can 'correctly clear lost
ctx-switch interrupts across reset for Gen11' and get rid of the TODO in
clear_gtiir() ;)
-Michel
More information about the Intel-gfx
mailing list