diff mbox

[3/3] drm/i915: Check for underruns after crtc disable

Message ID 1448050160-14124-3-git-send-email-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ville Syrjala Nov. 20, 2015, 8:09 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

To get a better idea if underruns occurred during crtc disabling,
let's check for them explicitly. This helps in cases where the
error interrupt isn't active, or there is no underrun interrupt
support at all.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Chris Wilson Nov. 21, 2015, 10:49 a.m. UTC | #1 
On Fri, Nov 20, 2015 at 10:09:20PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> To get a better idea if underruns occurred during crtc disabling,
> let's check for them explicitly. This helps in cases where the
> error interrupt isn't active, or there is no underrun interrupt
> support at all.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Would this be better the vblank after enabling?
-Chris
Ville Syrjala Nov. 23, 2015, 2:10 p.m. UTC | #2 
On Sat, Nov 21, 2015 at 10:49:04AM +0000, Chris Wilson wrote:
> On Fri, Nov 20, 2015 at 10:09:20PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > To get a better idea if underruns occurred during crtc disabling,
> > let's check for them explicitly. This helps in cases where the
> > error interrupt isn't active, or there is no underrun interrupt
> > support at all.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Would this be better the vblank after enabling?

We do that too.
Chris Wilson Nov. 23, 2015, 2:42 p.m. UTC | #3 
On Mon, Nov 23, 2015 at 04:10:32PM +0200, Ville Syrjälä wrote:
> On Sat, Nov 21, 2015 at 10:49:04AM +0000, Chris Wilson wrote:
> > On Fri, Nov 20, 2015 at 10:09:20PM +0200, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > To get a better idea if underruns occurred during crtc disabling,
> > > let's check for them explicitly. This helps in cases where the
> > > error interrupt isn't active, or there is no underrun interrupt
> > > support at all.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Would this be better the vblank after enabling?
> 
> We do that too.

Oh, crtc disabling. I can't read.
-Chris
Daniel Vetter Nov. 24, 2015, 2:14 p.m. UTC | #4 
On Mon, Nov 23, 2015 at 02:42:21PM +0000, Chris Wilson wrote:
> On Mon, Nov 23, 2015 at 04:10:32PM +0200, Ville Syrjälä wrote:
> > On Sat, Nov 21, 2015 at 10:49:04AM +0000, Chris Wilson wrote:
> > > On Fri, Nov 20, 2015 at 10:09:20PM +0200, ville.syrjala@linux.intel.com wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > To get a better idea if underruns occurred during crtc disabling,
> > > > let's check for them explicitly. This helps in cases where the
> > > > error interrupt isn't active, or there is no underrun interrupt
> > > > support at all.
> > > > 
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Would this be better the vblank after enabling?
> > 
> > We do that too.
> 
> Oh, crtc disabling. I can't read.

Hm, should we also double-check before disabling, to catch anything that
happened right before the modeset and avoid confusing it with underruns
happening during the modeset?

But this is a good idea already.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Ville Syrjala Nov. 24, 2015, 2:21 p.m. UTC | #5 
On Tue, Nov 24, 2015 at 03:14:23PM +0100, Daniel Vetter wrote:
> On Mon, Nov 23, 2015 at 02:42:21PM +0000, Chris Wilson wrote:
> > On Mon, Nov 23, 2015 at 04:10:32PM +0200, Ville Syrjälä wrote:
> > > On Sat, Nov 21, 2015 at 10:49:04AM +0000, Chris Wilson wrote:
> > > > On Fri, Nov 20, 2015 at 10:09:20PM +0200, ville.syrjala@linux.intel.com wrote:
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > 
> > > > > To get a better idea if underruns occurred during crtc disabling,
> > > > > let's check for them explicitly. This helps in cases where the
> > > > > error interrupt isn't active, or there is no underrun interrupt
> > > > > support at all.
> > > > > 
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > Would this be better the vblank after enabling?
> > > 
> > > We do that too.
> > 
> > Oh, crtc disabling. I can't read.
> 
> Hm, should we also double-check before disabling, to catch anything that
> happened right before the modeset and avoid confusing it with underruns
> happening during the modeset?

Hmm. Yeah that might be a decent idea. Although we don't wait for the
planes to be disabled before we disable the pipe so in theory some weird
underrun stemming from the plane disable might still get mixed in there.

> 
> But this is a good idea already.
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8a8104b7947d..21f703397f09 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13402,6 +13402,13 @@  static int intel_atomic_commit(struct drm_device *dev,
 			dev_priv->display.crtc_disable(crtc);
 			intel_crtc->active = false;
 			intel_disable_shared_dpll(intel_crtc);
+
+			/*
+			 * Underruns don't always raise
+			 * interrupts, so check manually.
+			 */
+			intel_check_cpu_fifo_underruns(dev_priv);
+			intel_check_pch_fifo_underruns(dev_priv);
 		}
 	}