| Message ID | 1448050160-14124-1-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Nov 20, 2015 at 10:09:18PM +0200, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > We still get spurious pipe underruns on ILK/SNB/IVB under two > circumstances when dealing with PCH ports: > * When the pipe has been disabled, but FDI RX/TX is still enabled > * During FDI link training > > Both cases seem to happen at least when we do VGA+HDMI cloning > from the same pipe. I don't think I've seen them when not cloning, > but can't be 100% sure. > > Disable underrun reporting around those places to eliminate the > dmesg errors. > > Testcase: igt/kms_setmode/basic-clone-single-crtc > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Acked-by: Chris Wilson <chris@chris-wilson.co.uk> I wondered if logging the suppressed errors would be of any use? Does the check_cpu_fifo work if the reporting is disabled? Could we do a manual check and DRM_DEBUG_KMS() if the enable did generate a failure. If the check_cpu_fifo does work, won't we still get the error from the added check in atomic_commit()? -Chris
On Sat, Nov 21, 2015 at 10:51:50AM +0000, Chris Wilson wrote: > On Fri, Nov 20, 2015 at 10:09:18PM +0200, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > We still get spurious pipe underruns on ILK/SNB/IVB under two > > circumstances when dealing with PCH ports: > > * When the pipe has been disabled, but FDI RX/TX is still enabled > > * During FDI link training > > > > Both cases seem to happen at least when we do VGA+HDMI cloning > > from the same pipe. I don't think I've seen them when not cloning, > > but can't be 100% sure. > > > > Disable underrun reporting around those places to eliminate the > > dmesg errors. > > > > Testcase: igt/kms_setmode/basic-clone-single-crtc > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > Acked-by: Chris Wilson <chris@chris-wilson.co.uk> > > I wondered if logging the suppressed errors would be of any use? Hmm. Maybe just to confirm that they still happen, and thus suppressing is still neded. > Does > the check_cpu_fifo work if the reporting is disabled? Could we do a > manual check and DRM_DEBUG_KMS() if the enable did generate a failure. > If the check_cpu_fifo does work, won't we still get the error from the > added check in atomic_commit()? The check only looks at pipes that have underrun reporting enabled. I suppose it might be possible to have it check all the pipes. At the point where we call it no explicit suppression should be happening, so the only reason why underrun reporting would be disabled on any pipe is due to detecting a previous underrun via the interrupt.
On Mon, Nov 23, 2015 at 04:22:08PM +0200, Ville Syrjälä wrote: > On Sat, Nov 21, 2015 at 10:51:50AM +0000, Chris Wilson wrote: > > On Fri, Nov 20, 2015 at 10:09:18PM +0200, ville.syrjala@linux.intel.com wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > We still get spurious pipe underruns on ILK/SNB/IVB under two > > > circumstances when dealing with PCH ports: > > > * When the pipe has been disabled, but FDI RX/TX is still enabled > > > * During FDI link training > > > > > > Both cases seem to happen at least when we do VGA+HDMI cloning > > > from the same pipe. I don't think I've seen them when not cloning, > > > but can't be 100% sure. > > > > > > Disable underrun reporting around those places to eliminate the > > > dmesg errors. > > > > > > Testcase: igt/kms_setmode/basic-clone-single-crtc > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Acked-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > I wondered if logging the suppressed errors would be of any use? > > Hmm. Maybe just to confirm that they still happen, and thus suppressing > is still neded. > > > Does > > the check_cpu_fifo work if the reporting is disabled? Could we do a > > manual check and DRM_DEBUG_KMS() if the enable did generate a failure. > > If the check_cpu_fifo does work, won't we still get the error from the > > added check in atomic_commit()? > > The check only looks at pipes that have underrun reporting enabled. I > suppose it might be possible to have it check all the pipes. At the > point where we call it no explicit suppression should be happening, so > the only reason why underrun reporting would be disabled on any pipe is > due to detecting a previous underrun via the interrupt. Does the hw flag the underrun even if the interrupt is disabled? (Playing games with PIPESTAT_IER?) Could we then report (debug) that an underrun happened before we re-enable reporting? -Chris
On Mon, Nov 23, 2015 at 02:28:29PM +0000, Chris Wilson wrote: > On Mon, Nov 23, 2015 at 04:22:08PM +0200, Ville Syrjälä wrote: > > On Sat, Nov 21, 2015 at 10:51:50AM +0000, Chris Wilson wrote: > > > On Fri, Nov 20, 2015 at 10:09:18PM +0200, ville.syrjala@linux.intel.com wrote: > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > We still get spurious pipe underruns on ILK/SNB/IVB under two > > > > circumstances when dealing with PCH ports: > > > > * When the pipe has been disabled, but FDI RX/TX is still enabled > > > > * During FDI link training > > > > > > > > Both cases seem to happen at least when we do VGA+HDMI cloning > > > > from the same pipe. I don't think I've seen them when not cloning, > > > > but can't be 100% sure. > > > > > > > > Disable underrun reporting around those places to eliminate the > > > > dmesg errors. > > > > > > > > Testcase: igt/kms_setmode/basic-clone-single-crtc > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Acked-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > > > I wondered if logging the suppressed errors would be of any use? > > > > Hmm. Maybe just to confirm that they still happen, and thus suppressing > > is still neded. > > > > > Does > > > the check_cpu_fifo work if the reporting is disabled? Could we do a > > > manual check and DRM_DEBUG_KMS() if the enable did generate a failure. > > > If the check_cpu_fifo does work, won't we still get the error from the > > > added check in atomic_commit()? > > > > The check only looks at pipes that have underrun reporting enabled. I > > suppose it might be possible to have it check all the pipes. At the > > point where we call it no explicit suppression should be happening, so > > the only reason why underrun reporting would be disabled on any pipe is > > due to detecting a previous underrun via the interrupt. > > Does the hw flag the underrun even if the interrupt is disabled? > (Playing games with PIPESTAT_IER?) Could we then report (debug) that an > underrun happened before we re-enable reporting? Hmm. We use the IMR to disable this stuff on many platforms, so we do lose the IIR. So we'd need to frob IER instead to make this work universally. Currently it would only work for GMCH, IVB/HSW, CPT/PPT/LPT/WPT since PIPESTAT/ERR_INT/SERR_INT don't have IMR.
On Mon, Nov 23, 2015 at 04:55:17PM +0200, Ville Syrjälä wrote: > On Mon, Nov 23, 2015 at 02:28:29PM +0000, Chris Wilson wrote: > > On Mon, Nov 23, 2015 at 04:22:08PM +0200, Ville Syrjälä wrote: > > > On Sat, Nov 21, 2015 at 10:51:50AM +0000, Chris Wilson wrote: > > > > On Fri, Nov 20, 2015 at 10:09:18PM +0200, ville.syrjala@linux.intel.com wrote: > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > > > We still get spurious pipe underruns on ILK/SNB/IVB under two > > > > > circumstances when dealing with PCH ports: > > > > > * When the pipe has been disabled, but FDI RX/TX is still enabled > > > > > * During FDI link training > > > > > > > > > > Both cases seem to happen at least when we do VGA+HDMI cloning > > > > > from the same pipe. I don't think I've seen them when not cloning, > > > > > but can't be 100% sure. > > > > > > > > > > Disable underrun reporting around those places to eliminate the > > > > > dmesg errors. > > > > > > > > > > Testcase: igt/kms_setmode/basic-clone-single-crtc > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Acked-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > > > > > I wondered if logging the suppressed errors would be of any use? > > > > > > Hmm. Maybe just to confirm that they still happen, and thus suppressing > > > is still neded. > > > > > > > Does > > > > the check_cpu_fifo work if the reporting is disabled? Could we do a > > > > manual check and DRM_DEBUG_KMS() if the enable did generate a failure. > > > > If the check_cpu_fifo does work, won't we still get the error from the > > > > added check in atomic_commit()? > > > > > > The check only looks at pipes that have underrun reporting enabled. I > > > suppose it might be possible to have it check all the pipes. At the > > > point where we call it no explicit suppression should be happening, so > > > the only reason why underrun reporting would be disabled on any pipe is > > > due to detecting a previous underrun via the interrupt. > > > > Does the hw flag the underrun even if the interrupt is disabled? > > (Playing games with PIPESTAT_IER?) Could we then report (debug) that an > > underrun happened before we re-enable reporting? > > Hmm. We use the IMR to disable this stuff on many platforms, so we > do lose the IIR. So we'd need to frob IER instead to make this work > universally. Currently it would only work for GMCH, IVB/HSW, > CPT/PPT/LPT/WPT since PIPESTAT/ERR_INT/SERR_INT don't have IMR. Feels a bit too risky imo, changing all that code. Also, even in tests where underruns reproduce readily it's not 100%, so the occasional debug message won't be solid enough evidence. For me I think as long as we hide underruns only while doing modesets (where there's some expectation of them happening) we should be perfectly fine wrt test coverage. The real benefit is when enabling/disabling planes, fbc and all that stuff. -Daniel
On Fri, Nov 20, 2015 at 10:09:18PM +0200, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > We still get spurious pipe underruns on ILK/SNB/IVB under two > circumstances when dealing with PCH ports: > * When the pipe has been disabled, but FDI RX/TX is still enabled > * During FDI link training > > Both cases seem to happen at least when we do VGA+HDMI cloning > from the same pipe. I don't think I've seen them when not cloning, > but can't be 100% sure. > > Disable underrun reporting around those places to eliminate the > dmesg errors. > > Testcase: igt/kms_setmode/basic-clone-single-crtc > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> hsw seems to have a similar problem when both vga and hdmi are used, even when not cloned. Bad thing is that somehow the underrun reporting gets into a non-recoverable state and stuck there, so that no underrun and also nothing else (specifically crc irqs, that's why bat notices) work any more. Might be worth a shot to try the same trick there. Of course we still need to figure out why it can't recover from this, too. Anyway just an aside, ack on the entire series. -Daniel > --- > drivers/gpu/drm/i915/intel_display.c | 20 +++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 68fb449ded77..8a8104b7947d 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4137,6 +4137,12 @@ static void ironlake_pch_enable(struct drm_crtc *crtc) > I915_WRITE(FDI_RX_TUSIZE1(pipe), > I915_READ(PIPE_DATA_M1(pipe)) & TU_SIZE_MASK); > > + /* > + * Sometimes spurious CPU pipe underruns happen during FDI > + * training, at least with VGA+HDMI cloning. Suppress them. > + */ > + intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false); > + > /* For PCH output, training FDI link */ > dev_priv->display.fdi_link_train(crtc); > > @@ -4170,6 +4176,8 @@ static void ironlake_pch_enable(struct drm_crtc *crtc) > > intel_fdi_normal_train(crtc); > > + intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true); > + > /* For PCH DP, enable TRANS_DP_CTL */ > if (HAS_PCH_CPT(dev) && intel_crtc->config->has_dp_encoder) { > const struct drm_display_mode *adjusted_mode = > @@ -5062,12 +5070,22 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) > drm_crtc_vblank_off(crtc); > assert_vblank_disabled(crtc); > > + /* > + * Sometimes spurious CPU pipe underruns happen when the > + * pipe is already disabled, but FDI RX/TX is still enabled. > + * Happens at least with VGA+HDMI cloning. Suppress them. > + */ > + if (intel_crtc->config->has_pch_encoder) > + intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false); > + > intel_disable_pipe(intel_crtc); > > ironlake_pfit_disable(intel_crtc, false); > > - if (intel_crtc->config->has_pch_encoder) > + if (intel_crtc->config->has_pch_encoder) { > ironlake_fdi_disable(crtc); > + intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true); > + } > > for_each_encoder_on_crtc(dev, crtc, encoder) > if (encoder->post_disable) > -- > 2.4.10 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Nov 24, 2015 at 03:16:24PM +0100, Daniel Vetter wrote: > On Fri, Nov 20, 2015 at 10:09:18PM +0200, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > We still get spurious pipe underruns on ILK/SNB/IVB under two > > circumstances when dealing with PCH ports: > > * When the pipe has been disabled, but FDI RX/TX is still enabled > > * During FDI link training > > > > Both cases seem to happen at least when we do VGA+HDMI cloning > > from the same pipe. I don't think I've seen them when not cloning, > > but can't be 100% sure. > > > > Disable underrun reporting around those places to eliminate the > > dmesg errors. > > > > Testcase: igt/kms_setmode/basic-clone-single-crtc > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > hsw seems to have a similar problem when both vga and hdmi are used, even > when not cloned. Bad thing is that somehow the underrun reporting gets > into a non-recoverable state and stuck there, so that no underrun and also > nothing else (specifically crc irqs, that's why bat notices) work any > more. > > Might be worth a shot to try the same trick there. Of course we still need > to figure out why it can't recover from this, too. I don't have a HSW/BDW with VGA on me. I suppose I might need to go find one and see what's what. Series pushed to dinq. Thanks for the acks and reviews. > > Anyway just an aside, ack on the entire series. > -Daniel > > > --- > > drivers/gpu/drm/i915/intel_display.c | 20 +++++++++++++++++++- > > 1 file changed, 19 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 68fb449ded77..8a8104b7947d 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -4137,6 +4137,12 @@ static void ironlake_pch_enable(struct drm_crtc *crtc) > > I915_WRITE(FDI_RX_TUSIZE1(pipe), > > I915_READ(PIPE_DATA_M1(pipe)) & TU_SIZE_MASK); > > > > + /* > > + * Sometimes spurious CPU pipe underruns happen during FDI > > + * training, at least with VGA+HDMI cloning. Suppress them. > > + */ > > + intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false); > > + > > /* For PCH output, training FDI link */ > > dev_priv->display.fdi_link_train(crtc); > > > > @@ -4170,6 +4176,8 @@ static void ironlake_pch_enable(struct drm_crtc *crtc) > > > > intel_fdi_normal_train(crtc); > > > > + intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true); > > + > > /* For PCH DP, enable TRANS_DP_CTL */ > > if (HAS_PCH_CPT(dev) && intel_crtc->config->has_dp_encoder) { > > const struct drm_display_mode *adjusted_mode = > > @@ -5062,12 +5070,22 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) > > drm_crtc_vblank_off(crtc); > > assert_vblank_disabled(crtc); > > > > + /* > > + * Sometimes spurious CPU pipe underruns happen when the > > + * pipe is already disabled, but FDI RX/TX is still enabled. > > + * Happens at least with VGA+HDMI cloning. Suppress them. > > + */ > > + if (intel_crtc->config->has_pch_encoder) > > + intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false); > > + > > intel_disable_pipe(intel_crtc); > > > > ironlake_pfit_disable(intel_crtc, false); > > > > - if (intel_crtc->config->has_pch_encoder) > > + if (intel_crtc->config->has_pch_encoder) { > > ironlake_fdi_disable(crtc); > > + intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true); > > + } > > > > for_each_encoder_on_crtc(dev, crtc, encoder) > > if (encoder->post_disable) > > -- > > 2.4.10 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 68fb449ded77..8a8104b7947d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4137,6 +4137,12 @@ static void ironlake_pch_enable(struct drm_crtc *crtc) I915_WRITE(FDI_RX_TUSIZE1(pipe), I915_READ(PIPE_DATA_M1(pipe)) & TU_SIZE_MASK); + /* + * Sometimes spurious CPU pipe underruns happen during FDI + * training, at least with VGA+HDMI cloning. Suppress them. + */ + intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false); + /* For PCH output, training FDI link */ dev_priv->display.fdi_link_train(crtc); @@ -4170,6 +4176,8 @@ static void ironlake_pch_enable(struct drm_crtc *crtc) intel_fdi_normal_train(crtc); + intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true); + /* For PCH DP, enable TRANS_DP_CTL */ if (HAS_PCH_CPT(dev) && intel_crtc->config->has_dp_encoder) { const struct drm_display_mode *adjusted_mode = @@ -5062,12 +5070,22 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) drm_crtc_vblank_off(crtc); assert_vblank_disabled(crtc); + /* + * Sometimes spurious CPU pipe underruns happen when the + * pipe is already disabled, but FDI RX/TX is still enabled. + * Happens at least with VGA+HDMI cloning. Suppress them. + */ + if (intel_crtc->config->has_pch_encoder) + intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false); + intel_disable_pipe(intel_crtc); ironlake_pfit_disable(intel_crtc, false); - if (intel_crtc->config->has_pch_encoder) + if (intel_crtc->config->has_pch_encoder) { ironlake_fdi_disable(crtc); + intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true); + } for_each_encoder_on_crtc(dev, crtc, encoder) if (encoder->post_disable)