Re: [PATCH v3 19/19] drm/i915: Get rid of ibx_irq_pre_postinstall()

[Lucas De Marchi <lucas.demarchi@xxxxxxxxx>]

On Wed, Oct 28, 2020 at 11:33:23PM +0200, Ville Syrjälä wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>

Reviewed-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>

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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx