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@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()?
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@xxxxxxxxxxxxxxxxxx>
> -Chris
--
Ville Syrjälä
Intel OTC