| Message ID | 20180524190406.2973-2-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
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 --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);