Quoting Ville Syrjala (2017-11-13 15:32:14)
From: Ville Syrjälä ville.syrjala@linux.intel.com
Previously I was under the impression that the scanline counter reads 0 when the pipe is off. Turns out that's not correct, and instead the scanline counter simply stops when the pipe stops, and it retains it's last value until the pipe starts up again, at which point the scanline counter jumps to vblank start.
These jumps can cause the timestamp to jump backwards by one frame. Since we use the timestamps to guesstimage also the frame counter value on gen2, that would cause the frame counter to also jump backwards, which leads to a massice difference from the previous value. The end result is that flips/vblank events don't appear to complete as they're stuck waiting for the frame counter to catch up to that massive difference.
Fix the problem properly by actually making sure the scanline counter has started to move before we assume that it's safe to enable vblank processing.
Cc: stable@vger.kernel.org Cc: Daniel Vetter daniel.vetter@ffwll.ch Fixes: b7792d8b54cc ("drm/i915: Wait for pipe to start before sampling vblank timestamps on gen2") Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/i915/intel_display.c | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 0ebf3f283b87..810b1147a0ac 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -998,7 +998,8 @@ enum transcoder intel_pipe_to_cpu_transcoder(struct drm_i915_private *dev_priv, return crtc->config->cpu_transcoder; } -static bool pipe_dsl_stopped(struct drm_i915_private *dev_priv, enum pipe pipe) +static bool pipe_dsl_stopped(struct drm_i915_private *dev_priv,
enum pipe pipe, bool stopped){ i915_reg_t reg = PIPEDSL(pipe); u32 line1, line2; @@ -1013,7 +1014,7 @@ static bool pipe_dsl_stopped(struct drm_i915_private *dev_priv, enum pipe pipe) msleep(5); line2 = I915_READ(reg) & line_mask;
return line1 == line2;
return (line1 == line2) == stopped;} /* @@ -1048,11 +1049,21 @@ static void intel_wait_for_pipe_off(struct intel_crtc *crtc) WARN(1, "pipe_off wait timed out\n"); } else { /* Wait for the display line to settle */
if (wait_for(pipe_dsl_stopped(dev_priv, pipe), 100))
if (wait_for(pipe_dsl_stopped(dev_priv, pipe, true), 100)) WARN(1, "pipe_off wait timed out\n"); }} +static void intel_wait_for_pipe_on(struct intel_crtc *crtc) +{
struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);enum pipe pipe = crtc->pipe;/* Wait for the display line to start moving */if (wait_for(pipe_dsl_stopped(dev_priv, pipe, false), 100))WARN(1, "pipe_on wait timed out\n");
3 wait_for(pipe_dsl_stopped()), please make a function to only have one expansion of that macro :)
+}
/* Only for pre-ILK configs */ void assert_pll(struct drm_i915_private *dev_priv, enum pipe pipe, bool state) @@ -1935,15 +1946,14 @@ static void intel_enable_pipe(struct intel_crtc *crtc) POSTING_READ(reg); /*
* Until the pipe starts DSL will read as 0, which would cause* an apparent vblank timestamp jump, which messes up also the* frame count when it's derived from the timestamps. So let's* wait for the pipe to start properly before we call* drm_crtc_vblank_on()
* Until the pipe starts DSL can give a bogus value, which cause* an apparent vblank timestamp jump when the DSL resets to its* proper value, which messes up also the frame count when it's* derived from the timestamps. So let's wait for the pipe to* start properly before we call drm_crtc_vblank_on() */
if (dev->max_vblank_count == 0 &&wait_for(intel_get_crtc_scanline(crtc) != crtc->scanline_offset, 50))DRM_ERROR("pipe %c didn't start\n", pipe_name(pipe));
if (dev->max_vblank_count == 0)intel_wait_for_pipe_on(crtc);} /** @@ -14707,7 +14717,7 @@ void i830_disable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe) I915_WRITE(PIPECONF(pipe), 0); POSTING_READ(PIPECONF(pipe));
if (wait_for(pipe_dsl_stopped(dev_priv, pipe), 100))
if (wait_for(pipe_dsl_stopped(dev_priv, pipe, true), 100)) DRM_ERROR("pipe %c off wait timed out\n", pipe_name(pipe));
Is there a reason why we couldn't use intel_wait_for_pipe_off() here, it gives the clearer symmetry to intel_wait_for_pipe_on()?
Other than nitpicks, the code does what it says on the tin, and it's pretty convincing in its argument, Reviewed-by: Chris Wilson chris@chris-wilson.co.uk -Chris