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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Nov 13, 2017 at 09:03:34PM +0000, Chris Wilson wrote:
> 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()?

In theory we could. However if we're being pedantic wait_for_pipe_off()
should be passed a crtc state since it wants to get at the
cpu_transcoder. It's not actually a problem on gen2 since we would
never take that codepath. However it feels a bit ugly.

What I could do is try to refactor these into a nicer form for the
case where we don't have the crtc state, and then handle the
cpu_transcoder case somewhere a bit higher up.

> 
> 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

-- 
Ville Syrjälä
Intel OTC