| Message ID | 1371037046-3732-12-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 :) > + } > } > > /** > 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
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
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); + } } /** 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 */
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). 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(-)