[Intel-gfx] [PATCH 1/2] drm/i915: Fix vblank timestamp/frame counter jumps on gen2

[Chris Wilson]

Quoting Ville Syrjala (2017-11-13 15:32:14)
> From: Ville Syrjälä <ville.syrjala at 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 at vger.kernel.org
> Cc: Daniel Vetter <daniel.vetter at 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 at 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 at chris-wilson.co.uk>
-Chris