Re: [PATCH 14/20] drm/i915: enable SDEIER later

[Ben Widawsky <ben@bwidawsk.net>]

On Fri, Mar 07, 2014 at 08:10:30PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> On the preinstall stage we should just disable all the interrupts, but
> we currently enable all the south display interrupts due to the way we
> touch SDEIER at the IRQ handlers (note: they are still masked and our
> IRQ handler is disabled).

I think this statement is false. The interrupt is enabled right after
preinstall(). For the nomodeset case, this actually seems to make some
difference. It still looks fine to me though.

> Instead of doing that, let's make the
> preinstall stage just disable all the south interrupts, and do the
> proper interrupt dance/ordering at the postinstall stage, including an
> assert to check if everything is behaving as expected.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 95f535b..4479e29 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2814,13 +2814,24 @@ static void ibx_irq_preinstall(struct drm_device *dev)
>  
>  	if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev))
>  		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.
> -	 */
> +/*
> + * 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_device *dev)

sde_irq_postinstall()?

> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (HAS_PCH_NOP(dev))
> +		return;
> +
> +	WARN_ON(I915_READ(SDEIER) != 0);
>  	I915_WRITE(SDEIER, 0xffffffff);
>  	POSTING_READ(SDEIER);
>  }
> @@ -3026,6 +3037,8 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
>  
>  	dev_priv->irq_mask = ~display_mask;
>  
> +	ibx_irq_pre_postinstall(dev);
> +
>  	GEN5_IRQ_INIT(DE, dev_priv->irq_mask, display_mask | extra_mask);
>  
>  	gen5_gt_irq_postinstall(dev);
> @@ -3217,6 +3230,8 @@ static int gen8_irq_postinstall(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> +	ibx_irq_pre_postinstall(dev);
> +
>  	gen8_gt_irq_postinstall(dev_priv);
>  	gen8_de_irq_postinstall(dev_priv);
>  
> -- 
> 1.8.5.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center