On Thu, Mar 27, 2014 at 11:39:36AM -0300, Paulo Zanoni wrote:
> 2014-03-18 17:29 GMT-03:00 Ben Widawsky <b...@bwidawsk.net>:
> > On Fri, Mar 07, 2014 at 08:10:30PM -0300, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zan...@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.zan...@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
