diff mbox

[2/2] drm/i915: Simplify ilk-ivb underrun suppression

Message ID 20180524190406.2973-2-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ville Syrjälä May 24, 2018, 7:04 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Let's suppress the underruns around every modeset sequence instead
of trying to avoid it. Planes are disabled at this point anyway so
we don't really gain anything from keeping the underrun reporting
enabled. Also for PCH ports we already suppress all underruns here
anyway so trying avoid it for the CPU eDP doesn't seem all that
important.

Maybe this gets rid of some lingering spurious underruns?

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

Comments

Chris Wilson May 25, 2018, 3:20 p.m. UTC | #1
Quoting Ville Syrjala (2018-05-24 20:04:06)
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Let's suppress the underruns around every modeset sequence instead
> of trying to avoid it. Planes are disabled at this point anyway so
> we don't really gain anything from keeping the underrun reporting
> enabled. Also for PCH ports we already suppress all underruns here
> anyway so trying avoid it for the CPU eDP doesn't seem all that
> important.
> 
> Maybe this gets rid of some lingering spurious underruns?

I'll bite. Isn't the reason for enabling underrung report for the
modeset itself to detect errors in our sequence?

How certain are we that these are hw limitations vs sw bugs?
-Chris
Ville Syrjälä May 25, 2018, 3:43 p.m. UTC | #2
On Fri, May 25, 2018 at 04:20:07PM +0100, Chris Wilson wrote:
> Quoting Ville Syrjala (2018-05-24 20:04:06)
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Let's suppress the underruns around every modeset sequence instead
> > of trying to avoid it. Planes are disabled at this point anyway so
> > we don't really gain anything from keeping the underrun reporting
> > enabled. Also for PCH ports we already suppress all underruns here
> > anyway so trying avoid it for the CPU eDP doesn't seem all that
> > important.
> > 
> > Maybe this gets rid of some lingering spurious underruns?
> 
> I'll bite. Isn't the reason for enabling underrung report for the
> modeset itself to detect errors in our sequence?

In theory CPU FIFO underruns shouldn't happen until we have some planes
enabled. Otherwise we have no data going through the FIFOs and thus
reporting an underrun isn't particularly sane. That doesn't stop gen2
from doing it though, but gen3+ seem to follow the more reasonable
interpretation of what a FIFO underrun means.

I suppose PCH FIFO underruns are a bit different as there is data flowing
as soon as the CPU pipe starts running, whether or not any planes have
been enabled. So those could certainly indicate some kind of programming
sequence error. Or it could just be totally expected that we start the
PCH side of the pipe first and there's a small bit of time when the CPU
pipe isn't yet pushing out pixels, and that's when the PCH side reports
the underrun.

> 
> How certain are we that these are hw limitations vs sw bugs?

To the best of my knowledge we are reasonably close to the sequence
listed in bspec. And while it's at least theoretically possible that
there's some change we could make to eliminate the underruns I don't
suppose anyone has the time or energy to try out all possible
variations.

And as long as the underrun (even if it's real) has vanished by the
time we enable the planes I think we are reasonably safe wrt. getting
the correct looking output to the user's display.
Chris Wilson May 25, 2018, 3:55 p.m. UTC | #3
Quoting Ville Syrjälä (2018-05-25 16:43:42)
> On Fri, May 25, 2018 at 04:20:07PM +0100, Chris Wilson wrote:
> > Quoting Ville Syrjala (2018-05-24 20:04:06)
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Let's suppress the underruns around every modeset sequence instead
> > > of trying to avoid it. Planes are disabled at this point anyway so
> > > we don't really gain anything from keeping the underrun reporting
> > > enabled. Also for PCH ports we already suppress all underruns here
> > > anyway so trying avoid it for the CPU eDP doesn't seem all that
> > > important.
> > > 
> > > Maybe this gets rid of some lingering spurious underruns?
> > 
> > I'll bite. Isn't the reason for enabling underrung report for the
> > modeset itself to detect errors in our sequence?
> 
> In theory CPU FIFO underruns shouldn't happen until we have some planes
> enabled. Otherwise we have no data going through the FIFOs and thus
> reporting an underrun isn't particularly sane. That doesn't stop gen2
> from doing it though, but gen3+ seem to follow the more reasonable
> interpretation of what a FIFO underrun means.

Makes sense.
 
> I suppose PCH FIFO underruns are a bit different as there is data flowing
> as soon as the CPU pipe starts running, whether or not any planes have
> been enabled. So those could certainly indicate some kind of programming
> sequence error. Or it could just be totally expected that we start the
> PCH side of the pipe first and there's a small bit of time when the CPU
> pipe isn't yet pushing out pixels, and that's when the PCH side reports
> the underrun.
> 
> > 
> > How certain are we that these are hw limitations vs sw bugs?
> 
> To the best of my knowledge we are reasonably close to the sequence
> listed in bspec. And while it's at least theoretically possible that
> there's some change we could make to eliminate the underruns I don't
> suppose anyone has the time or energy to try out all possible
> variations.
> 
> And as long as the underrun (even if it's real) has vanished by the
> time we enable the planes I think we are reasonably safe wrt. getting
> the correct looking output to the user's display.

Also makes sense. And if glitches during modesetting itself, we hope
nobody complains ;)

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
Ville Syrjälä May 25, 2018, 7:23 p.m. UTC | #4
On Fri, May 25, 2018 at 04:55:36PM +0100, Chris Wilson wrote:
> Quoting Ville Syrjälä (2018-05-25 16:43:42)
> > On Fri, May 25, 2018 at 04:20:07PM +0100, Chris Wilson wrote:
> > > Quoting Ville Syrjala (2018-05-24 20:04:06)
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > Let's suppress the underruns around every modeset sequence instead
> > > > of trying to avoid it. Planes are disabled at this point anyway so
> > > > we don't really gain anything from keeping the underrun reporting
> > > > enabled. Also for PCH ports we already suppress all underruns here
> > > > anyway so trying avoid it for the CPU eDP doesn't seem all that
> > > > important.
> > > > 
> > > > Maybe this gets rid of some lingering spurious underruns?
> > > 
> > > I'll bite. Isn't the reason for enabling underrung report for the
> > > modeset itself to detect errors in our sequence?
> > 
> > In theory CPU FIFO underruns shouldn't happen until we have some planes
> > enabled. Otherwise we have no data going through the FIFOs and thus
> > reporting an underrun isn't particularly sane. That doesn't stop gen2
> > from doing it though, but gen3+ seem to follow the more reasonable
> > interpretation of what a FIFO underrun means.
> 
> Makes sense.
>  
> > I suppose PCH FIFO underruns are a bit different as there is data flowing
> > as soon as the CPU pipe starts running, whether or not any planes have
> > been enabled. So those could certainly indicate some kind of programming
> > sequence error. Or it could just be totally expected that we start the
> > PCH side of the pipe first and there's a small bit of time when the CPU
> > pipe isn't yet pushing out pixels, and that's when the PCH side reports
> > the underrun.
> > 
> > > 
> > > How certain are we that these are hw limitations vs sw bugs?
> > 
> > To the best of my knowledge we are reasonably close to the sequence
> > listed in bspec. And while it's at least theoretically possible that
> > there's some change we could make to eliminate the underruns I don't
> > suppose anyone has the time or energy to try out all possible
> > variations.
> > 
> > And as long as the underrun (even if it's real) has vanished by the
> > time we enable the planes I think we are reasonably safe wrt. getting
> > the correct looking output to the user's display.
> 
> Also makes sense. And if glitches during modesetting itself, we hope
> nobody complains ;)
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Thanks. Pushed.
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b5fa4943372a..dc7204d27a24 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5470,10 +5470,8 @@  static void ironlake_crtc_enable(struct intel_crtc_state *pipe_config,
 	 *
 	 * Spurious PCH underruns also occur during PCH enabling.
 	 */
-	if (intel_crtc->config->has_pch_encoder || IS_GEN5(dev_priv))
-		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
-	if (intel_crtc->config->has_pch_encoder)
-		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
+	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
+	intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
 
 	if (intel_crtc->config->has_pch_encoder)
 		intel_prepare_shared_dpll(intel_crtc);
@@ -5717,10 +5715,8 @@  static void ironlake_crtc_disable(struct intel_crtc_state *old_crtc_state,
 	 * 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_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
-	}
+	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
+	intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
 
 	intel_encoders_disable(crtc, old_crtc_state, old_state);