Quoting Ville Syrjala (2017-11-13 15:32:14)
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Fixes: b7792d8b54cc ("drm/i915: Wait for pipe to start before sampling vblank timestamps on gen2")
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxx>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
