| Message ID | 20201028213323.5423-20-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Futher cleanup around hpd pins and port identfiers | expand |
On Wed, Oct 28, 2020 at 11:33:23PM +0200, Ville Syrjälä wrote: >From: Ville Syrjälä <ville.syrjala@linux.intel.com> > >ibx_irq_pre_postinstall() looks totally pointless. We can just >init both SDEIMR and SDEIER at the same time before enabling the >master intererupt. It's equally racy as the other order due master interrupt >to doing all of this from the postinstall stage with the interrupt >handler already in place. That is, safe with MSI but racy with >shared legacy interrupts. Fortunately we should have MSI on all ilk+. > >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com> Lucas De Marchi >--- > drivers/gpu/drm/i915/i915_irq.c | 46 ++++++++++++--------------------- > 1 file changed, 17 insertions(+), 29 deletions(-) > >diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c >index 95268fca2fbc..fdd132e2ec76 100644 >--- a/drivers/gpu/drm/i915/i915_irq.c >+++ b/drivers/gpu/drm/i915/i915_irq.c >@@ -2910,24 +2910,6 @@ static void ibx_irq_reset(struct drm_i915_private *dev_priv) > I915_WRITE(SERR_INT, 0xffffffff); > } > >-/* >- * SDEIER is also touched by the interrupt handler to work around missed PCH >- * interrupts. Hence we can't update it after the interrupt handler is enabled - >- * instead we unconditionally enable all PCH interrupt sources here, but then >- * only unmask them as needed with SDEIMR. >- * >- * This function needs to be called before interrupts are enabled. >- */ >-static void ibx_irq_pre_postinstall(struct drm_i915_private *dev_priv) >-{ >- if (HAS_PCH_NOP(dev_priv)) >- return; >- >- drm_WARN_ON(&dev_priv->drm, I915_READ(SDEIER) != 0); >- I915_WRITE(SDEIER, 0xffffffff); >- POSTING_READ(SDEIER); >-} >- > static void vlv_display_irq_reset(struct drm_i915_private *dev_priv) > { > struct intel_uncore *uncore = &dev_priv->uncore; >@@ -3545,8 +3527,20 @@ static void bxt_hpd_irq_setup(struct drm_i915_private *dev_priv) > bxt_hpd_detection_setup(dev_priv); > } > >+/* >+ * SDEIER is also touched by the interrupt handler to work around missed PCH >+ * interrupts. Hence we can't update it after the interrupt handler is enabled - >+ * instead we unconditionally enable all PCH interrupt sources here, but then >+ * only unmask them as needed with SDEIMR. >+ * >+ * Note that we currently do this after installing the interrupt handler, >+ * but before we enable the master interrupt. That should be sufficient >+ * to avoid races with the irq handler, assuming we have MSI. Shared legacy >+ * interrupts could still race. >+ */ > static void ibx_irq_postinstall(struct drm_i915_private *dev_priv) > { >+ struct intel_uncore *uncore = &dev_priv->uncore; > u32 mask; > > if (HAS_PCH_NOP(dev_priv)) >@@ -3559,8 +3553,7 @@ static void ibx_irq_postinstall(struct drm_i915_private *dev_priv) > else > mask = SDE_GMBUS_CPT; > >- gen3_assert_iir_is_zero(&dev_priv->uncore, SDEIIR); >- I915_WRITE(SDEIMR, ~mask); >+ GEN3_IRQ_INIT(uncore, SDE, ~mask, 0xffffffff); > } > > static void ilk_irq_postinstall(struct drm_i915_private *dev_priv) >@@ -3593,14 +3586,12 @@ static void ilk_irq_postinstall(struct drm_i915_private *dev_priv) > > dev_priv->irq_mask = ~display_mask; > >- ibx_irq_pre_postinstall(dev_priv); >+ ibx_irq_postinstall(dev_priv); > > gen5_gt_irq_postinstall(&dev_priv->gt); > > GEN3_IRQ_INIT(uncore, DE, dev_priv->irq_mask, > display_mask | extra_mask); >- >- ibx_irq_postinstall(dev_priv); > } > > void valleyview_enable_display_irqs(struct drm_i915_private *dev_priv) >@@ -3725,15 +3716,12 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv) > > static void gen8_irq_postinstall(struct drm_i915_private *dev_priv) > { >- if (HAS_PCH_SPLIT(dev_priv)) >- ibx_irq_pre_postinstall(dev_priv); >- >- gen8_gt_irq_postinstall(&dev_priv->gt); >- gen8_de_irq_postinstall(dev_priv); >- > if (HAS_PCH_SPLIT(dev_priv)) > ibx_irq_postinstall(dev_priv); > >+ gen8_gt_irq_postinstall(&dev_priv->gt); >+ gen8_de_irq_postinstall(dev_priv); >+ > gen8_master_intr_enable(dev_priv->uncore.regs); > } > >-- >2.26.2 > >_______________________________________________ >Intel-gfx mailing list >Intel-gfx@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 95268fca2fbc..fdd132e2ec76 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2910,24 +2910,6 @@ static void ibx_irq_reset(struct drm_i915_private *dev_priv) I915_WRITE(SERR_INT, 0xffffffff); } -/* - * SDEIER is also touched by the interrupt handler to work around missed PCH - * interrupts. Hence we can't update it after the interrupt handler is enabled - - * instead we unconditionally enable all PCH interrupt sources here, but then - * only unmask them as needed with SDEIMR. - * - * This function needs to be called before interrupts are enabled. - */ -static void ibx_irq_pre_postinstall(struct drm_i915_private *dev_priv) -{ - if (HAS_PCH_NOP(dev_priv)) - return; - - drm_WARN_ON(&dev_priv->drm, I915_READ(SDEIER) != 0); - I915_WRITE(SDEIER, 0xffffffff); - POSTING_READ(SDEIER); -} - static void vlv_display_irq_reset(struct drm_i915_private *dev_priv) { struct intel_uncore *uncore = &dev_priv->uncore; @@ -3545,8 +3527,20 @@ static void bxt_hpd_irq_setup(struct drm_i915_private *dev_priv) bxt_hpd_detection_setup(dev_priv); } +/* + * SDEIER is also touched by the interrupt handler to work around missed PCH + * interrupts. Hence we can't update it after the interrupt handler is enabled - + * instead we unconditionally enable all PCH interrupt sources here, but then + * only unmask them as needed with SDEIMR. + * + * Note that we currently do this after installing the interrupt handler, + * but before we enable the master interrupt. That should be sufficient + * to avoid races with the irq handler, assuming we have MSI. Shared legacy + * interrupts could still race. + */ static void ibx_irq_postinstall(struct drm_i915_private *dev_priv) { + struct intel_uncore *uncore = &dev_priv->uncore; u32 mask; if (HAS_PCH_NOP(dev_priv)) @@ -3559,8 +3553,7 @@ static void ibx_irq_postinstall(struct drm_i915_private *dev_priv) else mask = SDE_GMBUS_CPT; - gen3_assert_iir_is_zero(&dev_priv->uncore, SDEIIR); - I915_WRITE(SDEIMR, ~mask); + GEN3_IRQ_INIT(uncore, SDE, ~mask, 0xffffffff); } static void ilk_irq_postinstall(struct drm_i915_private *dev_priv) @@ -3593,14 +3586,12 @@ static void ilk_irq_postinstall(struct drm_i915_private *dev_priv) dev_priv->irq_mask = ~display_mask; - ibx_irq_pre_postinstall(dev_priv); + ibx_irq_postinstall(dev_priv); gen5_gt_irq_postinstall(&dev_priv->gt); GEN3_IRQ_INIT(uncore, DE, dev_priv->irq_mask, display_mask | extra_mask); - - ibx_irq_postinstall(dev_priv); } void valleyview_enable_display_irqs(struct drm_i915_private *dev_priv) @@ -3725,15 +3716,12 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv) static void gen8_irq_postinstall(struct drm_i915_private *dev_priv) { - if (HAS_PCH_SPLIT(dev_priv)) - ibx_irq_pre_postinstall(dev_priv); - - gen8_gt_irq_postinstall(&dev_priv->gt); - gen8_de_irq_postinstall(dev_priv); - if (HAS_PCH_SPLIT(dev_priv)) ibx_irq_postinstall(dev_priv); + gen8_gt_irq_postinstall(&dev_priv->gt); + gen8_de_irq_postinstall(dev_priv); + gen8_master_intr_enable(dev_priv->uncore.regs); }