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

[Ben Widawsky <ben@bwidawsk.net>]

On Thu, Mar 27, 2014 at 11:39:36AM -0300, Paulo Zanoni wrote:
> 2014-03-18 17:29 GMT-03:00 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.
> 
> Could you please clarify this paragraph?

The point was, "IRQ handler is disabled" is false. At least when I last
checked. We've registered the interrupt by then, which means we can
potentially get interrupts from the hardware.

I think we just might disagree on what "enabled" means. Perhaps this is
due to me working for too long with buggy hardware. In any event, as I
said, it does look fine to me as is.

> 
> We currently enable SDEIER at ibx_irq_preinstall, but the reason why
> we do this is because we change SDEIER at the IRQ handler. So if we
> move that code from preinstall to postinstall, but at a point where no
> interrupts are enabled yet, we should be safe, and this is what the
> patch does.
> 
> >
> >> 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()?
> 
> I agree the current name is bad, but we use the IBX naming for
> everything on the PCH at i915_irq.c, and ironlake_irq_postinstall()
> already calls a function named ibx_irq_postinstall(), so I needed a
> name that means "call this just before the postinstall() steps". If we
> rename it to sde_irq_postinstall we may confuse users due to the fact
> that we also have ibx_irq_postinstall. So with the current patch, we
> have this:
> 
> void ironlake_irq_postinstall()
> {
>     /* We have to call this before enabling the interrupts */
>     ibx_irq_pre_postinstall();
>     /* Enable the interrupts */
>     GEN5_IRQ_INIT(DE, dev_priv->irq_mask, display_mask | extra_mask);
>     /* Now do the necessary postinstall adjustments to the PCH stuff */
>     ibx_irq_postinstall();
> }
> 
> But I'm still open to new naming suggestions.
> 

I gave my suggestion. If you and or the maintainer think it's not a
better one due to the existing scheme, then by all means, feel free to
move on. There's nothing functionally incorrect that I can spot.

> >
> >> +{
> >> +     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
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ben Widawsky, Intel Open Source Technology Center