| Message ID | 1394233836-3827-15-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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? 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. > >> +{ >> + 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
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
On Fri, Mar 28, 2014 at 7:20 AM, Ben Widawsky <ben@bwidawsk.net> wrote: > 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. I think a polished comment to explain that despite the irq handler being register we shouldn't yet get interrupts and hence won't race is justified here? -Daniel
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) +{ + 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);