Re: [PATCH 11/24] drm/i915: improve SERR_INT clearing for fifo underrun reporting

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Jun 27, 2013 at 05:19:06PM -0300, Paulo Zanoni wrote:
> 2013/6/12 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > The current code won't report any fifo underruns on cpt if just one
> > pipe has fifo underrun reporting disabled. We can't enable the
> > interrupts, but we can still check the per-transcoder bits and so
> > report the underrun delayed if:
> > - We always clear the transcoder's bit (and none of the other bits)
> >   when enabling.
> > - We check the transcoder's bit after disabling (to avoid racing with
> >   the interrupt handler).
> 
> I guess you'll have to update this patch due to my bikeshed on IRC for
> patch 9. Anyway, comments below:
> 
> 
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 15 +++++++++++----
> >  drivers/gpu/drm/i915/i915_reg.h |  1 +
> >  2 files changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 685ad84..8627043 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -210,16 +210,23 @@ static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> >
> >         if (enable) {
> > +               I915_WRITE(SERR_INT,
> > +                          SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder));
> > +
> >                 if (!cpt_can_enable_serr_int(dev))
> >                         return;
> > -
> > -               I915_WRITE(SERR_INT, SERR_INT_TRANS_A_FIFO_UNDERRUN |
> > -                                    SERR_INT_TRANS_B_FIFO_UNDERRUN |
> > -                                    SERR_INT_TRANS_C_FIFO_UNDERRUN);
> >         }
> >
> >         ibx_display_interrupt_update(dev_priv, SDE_ERROR_CPT,
> >                                      enable ? SDE_ERROR_CPT : 0);
> > +
> > +       if (!enable) {
> > +               uint32_t tmp = I915_READ(SERR_INT);
> > +
> > +               if (tmp & SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder))
> > +                       DRM_DEBUG_KMS("uncleared pch fifo underrun on pipe %i\n",
> > +                                     pch_transcoder);
> 
> Use %c and transcoder_name(pch_transcoder), otherwise you trigger
> people's OCDs again :)
> 
> Also, I think the logic in this patch is inverted.
> 
> Imagine everything is running correctly and the bits are all in the
> ideal state. Then we get an underrun report on transcoder A. We'll
> detect this inside cpt_serr_int_handler, call
> intel_set_pch_fifo_underrun_reporting(dev, TRANSCODER_A, false), which
> will call cpt_set_fifo_underrun_reporting(dev, TRANSCODER_A, false).
> Then, inside your cpt_set_fifo_underrun_reporting, we'll disable
> SDE_ERROR_CPT, then read SERR_INT, see that the bit is 1 and print the
> DRM_DEBUG_KMS message you added. We'll return from
> cpt_set_fifo_underrun_reporting, then return from
> intel_set_pch_fifo_underrun_reporting, and then we'll print an error
> message again inside cpt_serr_int_handler. So we'll print 2 messages
> for a single underrun.
> 
> Instead of checking if the bit is 1 before disabling it, you should
> check if the bit is 1 before re-enabling it. The problem that you're
> trying to fix is that we don't report underruns while SDE_ERROR_CPT is
> disabled, so the solution is to ask "did we get any underruns while
> SDE_ERROR_CPT was disabled?", so inside the "if (enable)" case we need
> to check if the bit was 1, and on the "if (!enable)" case we need to
> clear the bit. Also, we should probably only print this "undetected
> pch fifo underrun" message if crtc->pch_fifo_underrun_disabled is
> false (otherwise we may report underruns on the same pipe multiple
> times).
> 
> I hope I'm not confused :)

Nope, the bug you've spotted is real afaics. But your proposed solution
isn't the best, since we want to report underruns latest when we disable
the pipe (even if underrun interrups are already disabled). If we only
check for the bit before re-enabling we could report false positives (in
case the disabling was indeed important and cause an expected underrun on
this pipe) and it's also already with a new configuration (if the change
is due to a modeset).

I have an idea how to fix this, I'll let you poke new holes into the
updated patch ;-)
-Daniel

> 
> 
> > +       }
> >  }
> >
> >  /**
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index b1fdca9..86e3987 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -3880,6 +3880,7 @@
> >  #define  SERR_INT_TRANS_C_FIFO_UNDERRUN        (1<<6)
> >  #define  SERR_INT_TRANS_B_FIFO_UNDERRUN        (1<<3)
> >  #define  SERR_INT_TRANS_A_FIFO_UNDERRUN        (1<<0)
> > +#define  SERR_INT_TRANS_FIFO_UNDERRUN(pipe)    (1<< (pipe*3))
> >
> >  /* digital port hotplug */
> >  #define PCH_PORT_HOTPLUG        0xc4030                /* SHOTPLUG_CTL */
> > --
> > 1.8.1.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch