From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com ([134.134.136.100]:13342 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751234AbdKMVc2 (ORCPT ); Mon, 13 Nov 2017 16:32:28 -0500 Date: Mon, 13 Nov 2017 23:32:24 +0200 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Chris Wilson Cc: intel-gfx@lists.freedesktop.org, Daniel Vetter , stable@vger.kernel.org Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915: Fix vblank timestamp/frame counter jumps on gen2 Message-ID: <20171113213224.GB10981@intel.com> References: <20171113153215.2719-1-ville.syrjala@linux.intel.com> <151060701456.20436.3897854237384092405@mail.alporthouse.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <151060701456.20436.3897854237384092405@mail.alporthouse.com> Sender: stable-owner@vger.kernel.org List-ID: 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� > > > > 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 > > Fixes: b7792d8b54cc ("drm/i915: Wait for pipe to start before sampling vblank timestamps on gen2") > > Signed-off-by: Ville Syrj�l� > > --- > > 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 -- Ville Syrj�l� Intel OTC From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915: Fix vblank timestamp/frame counter jumps on gen2 Date: Mon, 13 Nov 2017 23:32:24 +0200 Message-ID: <20171113213224.GB10981@intel.com> References: <20171113153215.2719-1-ville.syrjala@linux.intel.com> <151060701456.20436.3897854237384092405@mail.alporthouse.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Return-path: Content-Disposition: inline In-Reply-To: <151060701456.20436.3897854237384092405@mail.alporthouse.com> Sender: stable-owner@vger.kernel.org To: Chris Wilson Cc: intel-gfx@lists.freedesktop.org, Daniel Vetter , stable@vger.kernel.org List-Id: intel-gfx@lists.freedesktop.org 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ä > > > > 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 > > Fixes: b7792d8b54cc ("drm/i915: Wait for pipe to start before sampling vblank timestamps on gen2") > > Signed-off-by: Ville Syrjälä > > --- > > 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 -- Ville Syrjälä Intel OTC