[PATCH 01/15] drm/vblank: Allow dynamic per-crtc max_vblank_count

[PATCH 01/15] drm/vblank: Allow dynamic per-crtc max_vblank_count

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

On i965gm we need to adjust max_vblank_count dynamically
depending on whether the TV encoder is used or not. To
that end add a per-crtc max_vblank_count that takes
precedence over its device wide counterpart. The driver
can now call drm_crtc_set_max_vblank_count() to configure
the per-crtc value before calling drm_vblank_on().

Also looks like there was some discussion about exynos needing
similar treatment.

Cc: stable@vger.kernel.org
Cc: Inki Dae <inki.dae@samsung.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_vblank.c | 39 ++++++++++++++++++++++++++++++++----
 include/drm/drm_vblank.h     |  8 ++++++++
 2 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 98e091175921..c3abbdca8aba 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -105,13 +105,20 @@ static void store_vblank(struct drm_device *dev, unsigned int pipe,
 	write_sequnlock(&vblank->seqlock);
 }
 
+static u32 drm_max_vblank_count(struct drm_device *dev, unsigned int pipe)
+{
+	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
+
+	return vblank->max_vblank_count ?: dev->max_vblank_count;
+}
+
 /*
  * "No hw counter" fallback implementation of .get_vblank_counter() hook,
  * if there is no useable hardware frame counter available.
  */
 static u32 drm_vblank_no_hw_counter(struct drm_device *dev, unsigned int pipe)
 {
-	WARN_ON_ONCE(dev->max_vblank_count != 0);
+	WARN_ON_ONCE(drm_max_vblank_count(dev, pipe) != 0);
 	return 0;
 }
 
@@ -198,6 +205,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
 	ktime_t t_vblank;
 	int count = DRM_TIMESTAMP_MAXRETRIES;
 	int framedur_ns = vblank->framedur_ns;
+	u32 max_vblank_count = drm_max_vblank_count(dev, pipe);
 
 	/*
 	 * Interrupts were disabled prior to this call, so deal with counter
@@ -216,9 +224,9 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
 		rc = drm_get_last_vbltimestamp(dev, pipe, &t_vblank, in_vblank_irq);
 	} while (cur_vblank != __get_vblank_counter(dev, pipe) && --count > 0);
 
-	if (dev->max_vblank_count != 0) {
+	if (max_vblank_count) {
 		/* trust the hw counter when it's around */
-		diff = (cur_vblank - vblank->last) & dev->max_vblank_count;
+		diff = (cur_vblank - vblank->last) & max_vblank_count;
 	} else if (rc && framedur_ns) {
 		u64 diff_ns = ktime_to_ns(ktime_sub(t_vblank, vblank->time));
 
@@ -258,7 +266,8 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
 		      pipe, vblank->count, diff, cur_vblank, vblank->last);
 
 	if (diff == 0) {
-		WARN_ON_ONCE(cur_vblank != vblank->last);
+		WARN_ON_ONCE(max_vblank_count &&
+			     cur_vblank != vblank->last);
 		return;
 	}
 
@@ -1204,6 +1213,28 @@ void drm_crtc_vblank_reset(struct drm_crtc *crtc)
 }
 EXPORT_SYMBOL(drm_crtc_vblank_reset);
 
+/**
+ * drm_crtc_set_max_vblank_count - configure the hw max vblank counter value
+ * @crtc: CRTC in question
+ * @max_vblank_count: max hardware vblank counter value
+ *
+ * Update the maximum hardware vblank counter value for @crtc. Useful
+ * for hardware where the operation of the hardware vblank counter
+ * depends on the active display configuration.
+ *
+ * If used, must be called before drm_vblank_on().
+ */
+void drm_crtc_set_max_vblank_count(struct drm_crtc *crtc,
+				   u32 max_vblank_count)
+{
+	struct drm_device *dev = crtc->dev;
+	unsigned int pipe = drm_crtc_index(crtc);
+	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
+
+	vblank->max_vblank_count = max_vblank_count;
+}
+EXPORT_SYMBOL(drm_crtc_set_max_vblank_count);
+
 /**
  * drm_crtc_vblank_on - enable vblank events on a CRTC
  * @crtc: CRTC in question
diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
index 6ad9630d4f48..ecb2cf9913e2 100644
--- a/include/drm/drm_vblank.h
+++ b/include/drm/drm_vblank.h
@@ -128,6 +128,12 @@ struct drm_vblank_crtc {
 	 * @last: Protected by &drm_device.vbl_lock, used for wraparound handling.
 	 */
 	u32 last;
+	/**
+	 * @max_vblank_count: Maximum value of the hardware vblank counter.
+	 * If non-zero this takes precedence over &drm_device.max_vblank_count
+	 * for this crtc. Otherwise &drm_device.max_vblank_count is used.
+	 */
+	u32 max_vblank_count;
 	/**
 	 * @inmodeset: Tracks whether the vblank is disabled due to a modeset.
 	 * For legacy driver bit 2 additionally tracks whether an additional
@@ -206,4 +212,6 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
 void drm_calc_timestamping_constants(struct drm_crtc *crtc,
 				     const struct drm_display_mode *mode);
 wait_queue_head_t *drm_crtc_vblank_waitqueue(struct drm_crtc *crtc);
+void drm_crtc_set_max_vblank_count(struct drm_crtc *crtc,
+				   u32 max_vblank_count);
 #endif
-- 
2.18.1

[PATCH 02/15] drm/i915: Don't try to use the hardware frame counter with i965gm TV output

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

On i965gm the hardware frame counter does not work when
the TV encoder is active. So let's not try to consult
the hardware frame counter in that case. Instead we'll
fall back to the timestamp based guesstimation method
used on gen2.

Note that the pipe timings generated by the TV encoder
are also rather peculiar. Apparently the pipe wants to
run at a much higher speed (related to the oversample
clock somehow it seems) but during the vertical active
period the TV encoder stalls the pipe every few lines
to keep its speed in check. But once the vertical
blanking period is reached the pipe gets to run at full
speed. This means our vblank timestamp estimates are
suspect. Fixing all that would require quite a bit
more work. This simple fix at least avoids the nasty
vblank timeouts that are happening currently.

Curiously the frame counter works just fine on i945gm
and gm45. I don't really understand what kind of mishap
occurred with the hardware design on i965gm. Sadly
I wasn't able to find any chicken bits etc. that would
fix the frame counter :(

Cc: stable@vger.kernel.org
Fixes: 51e31d49c890 ("drm/i915: Use generic vblank wait")
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93782
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c      | 10 ++++++-
 drivers/gpu/drm/i915/intel_display.c | 40 +++++++++++++++++++++++-----
 2 files changed, 43 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index d447d7d508f4..019cb685986c 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -4843,8 +4843,16 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
 		dev->max_vblank_count = 0xffffffff; /* full 32 bit counter */
 		dev->driver->get_vblank_counter = g4x_get_vblank_counter;
 	} else {
+		/*
+		 * On i965gm the hardware frame counter reads zero
+		 * when the TV encoder is used. Hence we configure
+		 * max_vblank_count dynamically for each crtc.
+		 */
+		if (IS_I965GM(dev_priv))
+			dev->max_vblank_count = 0;
+		else
+			dev->max_vblank_count = 0xffffff; /* only 24 bits of frame count */
 		dev->driver->get_vblank_counter = i915_get_vblank_counter;
-		dev->max_vblank_count = 0xffffff; /* only 24 bits of frame count */
 	}
 
 	/*
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 940577f8c041..2dccb3310ad2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5504,6 +5504,32 @@ static void intel_encoders_post_pll_disable(struct drm_crtc *crtc,
 	}
 }
 
+static u32 intel_crtc_max_vblank_count(const struct intel_crtc_state *crtc_state)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
+	u32 max_vblank_count = dev_priv->drm.max_vblank_count;
+
+	/*
+	 * On i965gm the hardware frame counter always
+	 * reads zero when the TV encoder is used :(
+	 * Otherwise we get the 24 bit frame counter.
+	 */
+	if (IS_I965GM(dev_priv) &&
+	    (crtc_state->output_types & BIT(INTEL_OUTPUT_TVOUT)) == 0)
+		max_vblank_count = 0xffffff; /* only 24 bits of frame count */
+
+	return max_vblank_count;
+}
+
+static void intel_crtc_vblank_on(const struct intel_crtc_state *crtc_state)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+
+	drm_crtc_set_max_vblank_count(&crtc->base,
+				      intel_crtc_max_vblank_count(crtc_state));
+	drm_crtc_vblank_on(&crtc->base);
+}
+
 static void ironlake_crtc_enable(struct intel_crtc_state *pipe_config,
 				 struct drm_atomic_state *old_state)
 {
@@ -5577,7 +5603,7 @@ static void ironlake_crtc_enable(struct intel_crtc_state *pipe_config,
 		ironlake_pch_enable(old_intel_state, pipe_config);
 
 	assert_vblank_disabled(crtc);
-	drm_crtc_vblank_on(crtc);
+	intel_crtc_vblank_on(pipe_config);
 
 	intel_encoders_enable(crtc, pipe_config, old_state);
 
@@ -5734,7 +5760,7 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
 		intel_ddi_set_vc_payload_alloc(pipe_config, true);
 
 	assert_vblank_disabled(crtc);
-	drm_crtc_vblank_on(crtc);
+	intel_crtc_vblank_on(pipe_config);
 
 	intel_encoders_enable(crtc, pipe_config, old_state);
 
@@ -6074,7 +6100,7 @@ static void valleyview_crtc_enable(struct intel_crtc_state *pipe_config,
 	intel_enable_pipe(pipe_config);
 
 	assert_vblank_disabled(crtc);
-	drm_crtc_vblank_on(crtc);
+	intel_crtc_vblank_on(pipe_config);
 
 	intel_encoders_enable(crtc, pipe_config, old_state);
 }
@@ -6133,7 +6159,7 @@ static void i9xx_crtc_enable(struct intel_crtc_state *pipe_config,
 	intel_enable_pipe(pipe_config);
 
 	assert_vblank_disabled(crtc);
-	drm_crtc_vblank_on(crtc);
+	intel_crtc_vblank_on(pipe_config);
 
 	intel_encoders_enable(crtc, pipe_config, old_state);
 }
@@ -15752,10 +15778,12 @@ intel_modeset_setup_hw_state(struct drm_device *dev,
 	 * waits, so we need vblank interrupts restored beforehand.
 	 */
 	for_each_intel_crtc(&dev_priv->drm, crtc) {
+		crtc_state = to_intel_crtc_state(crtc->base.state);
+
 		drm_crtc_vblank_reset(&crtc->base);
 
-		if (crtc->base.state->active)
-			drm_crtc_vblank_on(&crtc->base);
+		if (crtc_state->base.active)
+			intel_crtc_vblank_on(crtc_state);
 	}
 
 	intel_sanitize_plane_mapping(dev_priv);
-- 
2.18.1

Re: [PATCH 01/15] drm/vblank: Allow dynamic per-crtc max_vblank_count

[Daniel Vetter]

On Mon, Nov 12, 2018 at 06:59:45PM +0200, Ville Syrjala wrote:
> From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> 
> On i965gm we need to adjust max_vblank_count dynamically
> depending on whether the TV encoder is used or not. To
> that end add a per-crtc max_vblank_count that takes
> precedence over its device wide counterpart. The driver
> can now call drm_crtc_set_max_vblank_count() to configure
> the per-crtc value before calling drm_vblank_on().
> 
> Also looks like there was some discussion about exynos needing
> similar treatment.
> 
> Cc: stable@vger.kernel.org
> Cc: Inki Dae <inki.dae@samsung.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_vblank.c | 39 ++++++++++++++++++++++++++++++++----
>  include/drm/drm_vblank.h     |  8 ++++++++
>  2 files changed, 43 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 98e091175921..c3abbdca8aba 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -105,13 +105,20 @@ static void store_vblank(struct drm_device *dev, unsigned int pipe,
>  	write_sequnlock(&vblank->seqlock);
>  }
>  
> +static u32 drm_max_vblank_count(struct drm_device *dev, unsigned int pipe)
> +{
> +	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> +
> +	return vblank->max_vblank_count ?: dev->max_vblank_count;
> +}
> +
>  /*
>   * "No hw counter" fallback implementation of .get_vblank_counter() hook,
>   * if there is no useable hardware frame counter available.
>   */
>  static u32 drm_vblank_no_hw_counter(struct drm_device *dev, unsigned int pipe)
>  {
> -	WARN_ON_ONCE(dev->max_vblank_count != 0);
> +	WARN_ON_ONCE(drm_max_vblank_count(dev, pipe) != 0);
>  	return 0;
>  }
>  
> @@ -198,6 +205,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
>  	ktime_t t_vblank;
>  	int count = DRM_TIMESTAMP_MAXRETRIES;
>  	int framedur_ns = vblank->framedur_ns;
> +	u32 max_vblank_count = drm_max_vblank_count(dev, pipe);
>  
>  	/*
>  	 * Interrupts were disabled prior to this call, so deal with counter
> @@ -216,9 +224,9 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
>  		rc = drm_get_last_vbltimestamp(dev, pipe, &t_vblank, in_vblank_irq);
>  	} while (cur_vblank != __get_vblank_counter(dev, pipe) && --count > 0);
>  
> -	if (dev->max_vblank_count != 0) {
> +	if (max_vblank_count) {
>  		/* trust the hw counter when it's around */
> -		diff = (cur_vblank - vblank->last) & dev->max_vblank_count;
> +		diff = (cur_vblank - vblank->last) & max_vblank_count;
>  	} else if (rc && framedur_ns) {
>  		u64 diff_ns = ktime_to_ns(ktime_sub(t_vblank, vblank->time));
>  
> @@ -258,7 +266,8 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
>  		      pipe, vblank->count, diff, cur_vblank, vblank->last);
>  
>  	if (diff == 0) {
> -		WARN_ON_ONCE(cur_vblank != vblank->last);
> +		WARN_ON_ONCE(max_vblank_count &&
> +			     cur_vblank != vblank->last);

Unrelated bugfix for this warning? Should be a separate patch I think, or
I'm missing something.

>  		return;
>  	}
>  
> @@ -1204,6 +1213,28 @@ void drm_crtc_vblank_reset(struct drm_crtc *crtc)
>  }
>  EXPORT_SYMBOL(drm_crtc_vblank_reset);
>  
> +/**
> + * drm_crtc_set_max_vblank_count - configure the hw max vblank counter value
> + * @crtc: CRTC in question
> + * @max_vblank_count: max hardware vblank counter value
> + *
> + * Update the maximum hardware vblank counter value for @crtc. Useful
> + * for hardware where the operation of the hardware vblank counter
> + * depends on the active display configuration.
> + *
> + * If used, must be called before drm_vblank_on().

I think we should check this at runtime with a WARN_ON. Plus make the
comment here a bit clearer that this is indeed for runtime adjusting of
the max_vblank_count, in cases where that depends upon the connected
outputs.

> + */
> +void drm_crtc_set_max_vblank_count(struct drm_crtc *crtc,
> +				   u32 max_vblank_count)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	unsigned int pipe = drm_crtc_index(crtc);
> +	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> +
> +	vblank->max_vblank_count = max_vblank_count;
> +}
> +EXPORT_SYMBOL(drm_crtc_set_max_vblank_count);
> +
>  /**
>   * drm_crtc_vblank_on - enable vblank events on a CRTC
>   * @crtc: CRTC in question
> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> index 6ad9630d4f48..ecb2cf9913e2 100644
> --- a/include/drm/drm_vblank.h
> +++ b/include/drm/drm_vblank.h
> @@ -128,6 +128,12 @@ struct drm_vblank_crtc {
>  	 * @last: Protected by &drm_device.vbl_lock, used for wraparound handling.
>  	 */
>  	u32 last;
> +	/**
> +	 * @max_vblank_count: Maximum value of the hardware vblank counter.
> +	 * If non-zero this takes precedence over &drm_device.max_vblank_count
> +	 * for this crtc. Otherwise &drm_device.max_vblank_count is used.
> +	 */

I'd add "This should be set by calling drm_crtc_set_max_vblank_count()."

And please also add a note to the kerneldoc of drm_driver.max_vblank_count
pointing at &drm_vblank_crtc.max_vblank_count for per-crtc limits.

Aside from the nits lgtm. I think I'll skip looking at the TV out stuff
though ...

Cheers, Daniel

> +	u32 max_vblank_count;
>  	/**
>  	 * @inmodeset: Tracks whether the vblank is disabled due to a modeset.
>  	 * For legacy driver bit 2 additionally tracks whether an additional
> @@ -206,4 +212,6 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
>  void drm_calc_timestamping_constants(struct drm_crtc *crtc,
>  				     const struct drm_display_mode *mode);
>  wait_queue_head_t *drm_crtc_vblank_waitqueue(struct drm_crtc *crtc);
> +void drm_crtc_set_max_vblank_count(struct drm_crtc *crtc,
> +				   u32 max_vblank_count);
>  #endif
> -- 
> 2.18.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 01/15] drm/vblank: Allow dynamic per-crtc max_vblank_count

[Ville Syrjälä]

On Wed, Nov 21, 2018 at 10:27:27AM +0100, Daniel Vetter wrote:
> On Mon, Nov 12, 2018 at 06:59:45PM +0200, Ville Syrjala wrote:
> > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > 
> > On i965gm we need to adjust max_vblank_count dynamically
> > depending on whether the TV encoder is used or not. To
> > that end add a per-crtc max_vblank_count that takes
> > precedence over its device wide counterpart. The driver
> > can now call drm_crtc_set_max_vblank_count() to configure
> > the per-crtc value before calling drm_vblank_on().
> > 
> > Also looks like there was some discussion about exynos needing
> > similar treatment.
> > 
> > Cc: stable@vger.kernel.org
> > Cc: Inki Dae <inki.dae@samsung.com>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_vblank.c | 39 ++++++++++++++++++++++++++++++++----
> >  include/drm/drm_vblank.h     |  8 ++++++++
> >  2 files changed, 43 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > index 98e091175921..c3abbdca8aba 100644
> > --- a/drivers/gpu/drm/drm_vblank.c
> > +++ b/drivers/gpu/drm/drm_vblank.c
> > @@ -105,13 +105,20 @@ static void store_vblank(struct drm_device *dev, unsigned int pipe,
> >  	write_sequnlock(&vblank->seqlock);
> >  }
> >  
> > +static u32 drm_max_vblank_count(struct drm_device *dev, unsigned int pipe)
> > +{
> > +	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> > +
> > +	return vblank->max_vblank_count ?: dev->max_vblank_count;
> > +}
> > +
> >  /*
> >   * "No hw counter" fallback implementation of .get_vblank_counter() hook,
> >   * if there is no useable hardware frame counter available.
> >   */
> >  static u32 drm_vblank_no_hw_counter(struct drm_device *dev, unsigned int pipe)
> >  {
> > -	WARN_ON_ONCE(dev->max_vblank_count != 0);
> > +	WARN_ON_ONCE(drm_max_vblank_count(dev, pipe) != 0);
> >  	return 0;
> >  }
> >  
> > @@ -198,6 +205,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> >  	ktime_t t_vblank;
> >  	int count = DRM_TIMESTAMP_MAXRETRIES;
> >  	int framedur_ns = vblank->framedur_ns;
> > +	u32 max_vblank_count = drm_max_vblank_count(dev, pipe);
> >  
> >  	/*
> >  	 * Interrupts were disabled prior to this call, so deal with counter
> > @@ -216,9 +224,9 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> >  		rc = drm_get_last_vbltimestamp(dev, pipe, &t_vblank, in_vblank_irq);
> >  	} while (cur_vblank != __get_vblank_counter(dev, pipe) && --count > 0);
> >  
> > -	if (dev->max_vblank_count != 0) {
> > +	if (max_vblank_count) {
> >  		/* trust the hw counter when it's around */
> > -		diff = (cur_vblank - vblank->last) & dev->max_vblank_count;
> > +		diff = (cur_vblank - vblank->last) & max_vblank_count;
> >  	} else if (rc && framedur_ns) {
> >  		u64 diff_ns = ktime_to_ns(ktime_sub(t_vblank, vblank->time));
> >  
> > @@ -258,7 +266,8 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> >  		      pipe, vblank->count, diff, cur_vblank, vblank->last);
> >  
> >  	if (diff == 0) {
> > -		WARN_ON_ONCE(cur_vblank != vblank->last);
> > +		WARN_ON_ONCE(max_vblank_count &&
> > +			     cur_vblank != vblank->last);
> 
> Unrelated bugfix for this warning? Should be a separate patch I think, or
> I'm missing something.

Ah, yeah this was due to a quirk of i965gm hardware. The hw counter
does work until the exact point when we enable TV encoder. Thus we
will get non-zero values up to that point, and since the TV encoder
isn't yet throttling the pipe it presumably runs at the oversample
clock so our timestamp based estimates can give us a diff==0 even
though the pipe did indeed pass a vblank already. I forgot to
note this in the commit message.

I think we can handle this three ways:
1. do what I do here and just let the mismatch slip through
2. force i915_get_vblank_counter() to return 0 always when the
   TV encoder is going to be used
3. don't call drm_crtc_set_max_vblank_count() before drm_vblank_on()
   and instead delay it until just before we enable the TV encoder

I think option 3 is overly complicated to consider seriously. So
option 1 or option 2 is what I think we should do. For whatever
reason I went with option 1 here, but maybe option 2 is better
since it would be all contained within i915...

> 
> >  		return;
> >  	}
> >  
> > @@ -1204,6 +1213,28 @@ void drm_crtc_vblank_reset(struct drm_crtc *crtc)
> >  }
> >  EXPORT_SYMBOL(drm_crtc_vblank_reset);
> >  
> > +/**
> > + * drm_crtc_set_max_vblank_count - configure the hw max vblank counter value
> > + * @crtc: CRTC in question
> > + * @max_vblank_count: max hardware vblank counter value
> > + *
> > + * Update the maximum hardware vblank counter value for @crtc. Useful
> > + * for hardware where the operation of the hardware vblank counter
> > + * depends on the active display configuration.
> > + *
> > + * If used, must be called before drm_vblank_on().
> 
> I think we should check this at runtime with a WARN_ON. Plus make the
> comment here a bit clearer that this is indeed for runtime adjusting of
> the max_vblank_count, in cases where that depends upon the connected
> outputs.

Sure. I'll try to pimp up the docs a bit.

> 
> > + */
> > +void drm_crtc_set_max_vblank_count(struct drm_crtc *crtc,
> > +				   u32 max_vblank_count)
> > +{
> > +	struct drm_device *dev = crtc->dev;
> > +	unsigned int pipe = drm_crtc_index(crtc);
> > +	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> > +
> > +	vblank->max_vblank_count = max_vblank_count;
> > +}
> > +EXPORT_SYMBOL(drm_crtc_set_max_vblank_count);
> > +
> >  /**
> >   * drm_crtc_vblank_on - enable vblank events on a CRTC
> >   * @crtc: CRTC in question
> > diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> > index 6ad9630d4f48..ecb2cf9913e2 100644
> > --- a/include/drm/drm_vblank.h
> > +++ b/include/drm/drm_vblank.h
> > @@ -128,6 +128,12 @@ struct drm_vblank_crtc {
> >  	 * @last: Protected by &drm_device.vbl_lock, used for wraparound handling.
> >  	 */
> >  	u32 last;
> > +	/**
> > +	 * @max_vblank_count: Maximum value of the hardware vblank counter.
> > +	 * If non-zero this takes precedence over &drm_device.max_vblank_count
> > +	 * for this crtc. Otherwise &drm_device.max_vblank_count is used.
> > +	 */
> 
> I'd add "This should be set by calling drm_crtc_set_max_vblank_count()."
> 
> And please also add a note to the kerneldoc of drm_driver.max_vblank_count
> pointing at &drm_vblank_crtc.max_vblank_count for per-crtc limits.

Ack.

> 
> Aside from the nits lgtm. I think I'll skip looking at the TV out stuff
> though ...

:)

-- 
Ville Syrj�l�
Intel

Re: [PATCH 01/15] drm/vblank: Allow dynamic per-crtc max_vblank_count

[Daniel Vetter]

On Wed, Nov 21, 2018 at 01:37:51PM +0200, Ville Syrj�l� wrote:
> On Wed, Nov 21, 2018 at 10:27:27AM +0100, Daniel Vetter wrote:
> > On Mon, Nov 12, 2018 at 06:59:45PM +0200, Ville Syrjala wrote:
> > > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > > 
> > > On i965gm we need to adjust max_vblank_count dynamically
> > > depending on whether the TV encoder is used or not. To
> > > that end add a per-crtc max_vblank_count that takes
> > > precedence over its device wide counterpart. The driver
> > > can now call drm_crtc_set_max_vblank_count() to configure
> > > the per-crtc value before calling drm_vblank_on().
> > > 
> > > Also looks like there was some discussion about exynos needing
> > > similar treatment.
> > > 
> > > Cc: stable@vger.kernel.org
> > > Cc: Inki Dae <inki.dae@samsung.com>
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_vblank.c | 39 ++++++++++++++++++++++++++++++++----
> > >  include/drm/drm_vblank.h     |  8 ++++++++
> > >  2 files changed, 43 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > > index 98e091175921..c3abbdca8aba 100644
> > > --- a/drivers/gpu/drm/drm_vblank.c
> > > +++ b/drivers/gpu/drm/drm_vblank.c
> > > @@ -105,13 +105,20 @@ static void store_vblank(struct drm_device *dev, unsigned int pipe,
> > >  	write_sequnlock(&vblank->seqlock);
> > >  }
> > >  
> > > +static u32 drm_max_vblank_count(struct drm_device *dev, unsigned int pipe)
> > > +{
> > > +	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> > > +
> > > +	return vblank->max_vblank_count ?: dev->max_vblank_count;
> > > +}
> > > +
> > >  /*
> > >   * "No hw counter" fallback implementation of .get_vblank_counter() hook,
> > >   * if there is no useable hardware frame counter available.
> > >   */
> > >  static u32 drm_vblank_no_hw_counter(struct drm_device *dev, unsigned int pipe)
> > >  {
> > > -	WARN_ON_ONCE(dev->max_vblank_count != 0);
> > > +	WARN_ON_ONCE(drm_max_vblank_count(dev, pipe) != 0);
> > >  	return 0;
> > >  }
> > >  
> > > @@ -198,6 +205,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> > >  	ktime_t t_vblank;
> > >  	int count = DRM_TIMESTAMP_MAXRETRIES;
> > >  	int framedur_ns = vblank->framedur_ns;
> > > +	u32 max_vblank_count = drm_max_vblank_count(dev, pipe);
> > >  
> > >  	/*
> > >  	 * Interrupts were disabled prior to this call, so deal with counter
> > > @@ -216,9 +224,9 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> > >  		rc = drm_get_last_vbltimestamp(dev, pipe, &t_vblank, in_vblank_irq);
> > >  	} while (cur_vblank != __get_vblank_counter(dev, pipe) && --count > 0);
> > >  
> > > -	if (dev->max_vblank_count != 0) {
> > > +	if (max_vblank_count) {
> > >  		/* trust the hw counter when it's around */
> > > -		diff = (cur_vblank - vblank->last) & dev->max_vblank_count;
> > > +		diff = (cur_vblank - vblank->last) & max_vblank_count;
> > >  	} else if (rc && framedur_ns) {
> > >  		u64 diff_ns = ktime_to_ns(ktime_sub(t_vblank, vblank->time));
> > >  
> > > @@ -258,7 +266,8 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> > >  		      pipe, vblank->count, diff, cur_vblank, vblank->last);
> > >  
> > >  	if (diff == 0) {
> > > -		WARN_ON_ONCE(cur_vblank != vblank->last);
> > > +		WARN_ON_ONCE(max_vblank_count &&
> > > +			     cur_vblank != vblank->last);
> > 
> > Unrelated bugfix for this warning? Should be a separate patch I think, or
> > I'm missing something.
> 
> Ah, yeah this was due to a quirk of i965gm hardware. The hw counter
> does work until the exact point when we enable TV encoder. Thus we
> will get non-zero values up to that point, and since the TV encoder
> isn't yet throttling the pipe it presumably runs at the oversample
> clock so our timestamp based estimates can give us a diff==0 even
> though the pipe did indeed pass a vblank already. I forgot to
> note this in the commit message.
> 
> I think we can handle this three ways:
> 1. do what I do here and just let the mismatch slip through
> 2. force i915_get_vblank_counter() to return 0 always when the
>    TV encoder is going to be used
> 3. don't call drm_crtc_set_max_vblank_count() before drm_vblank_on()
>    and instead delay it until just before we enable the TV encoder
> 
> I think option 3 is overly complicated to consider seriously. So
> option 1 or option 2 is what I think we should do. For whatever
> reason I went with option 1 here, but maybe option 2 is better
> since it would be all contained within i915...

Delay drm_crtc_vblank_on until the vblank is stable? That seems like the
semantically clean solution to me, instead of hacking around in core code
when drivers leak garbage out ...

-Daniel


> > 
> > >  		return;
> > >  	}
> > >  
> > > @@ -1204,6 +1213,28 @@ void drm_crtc_vblank_reset(struct drm_crtc *crtc)
> > >  }
> > >  EXPORT_SYMBOL(drm_crtc_vblank_reset);
> > >  
> > > +/**
> > > + * drm_crtc_set_max_vblank_count - configure the hw max vblank counter value
> > > + * @crtc: CRTC in question
> > > + * @max_vblank_count: max hardware vblank counter value
> > > + *
> > > + * Update the maximum hardware vblank counter value for @crtc. Useful
> > > + * for hardware where the operation of the hardware vblank counter
> > > + * depends on the active display configuration.
> > > + *
> > > + * If used, must be called before drm_vblank_on().
> > 
> > I think we should check this at runtime with a WARN_ON. Plus make the
> > comment here a bit clearer that this is indeed for runtime adjusting of
> > the max_vblank_count, in cases where that depends upon the connected
> > outputs.
> 
> Sure. I'll try to pimp up the docs a bit.
> 
> > 
> > > + */
> > > +void drm_crtc_set_max_vblank_count(struct drm_crtc *crtc,
> > > +				   u32 max_vblank_count)
> > > +{
> > > +	struct drm_device *dev = crtc->dev;
> > > +	unsigned int pipe = drm_crtc_index(crtc);
> > > +	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> > > +
> > > +	vblank->max_vblank_count = max_vblank_count;
> > > +}
> > > +EXPORT_SYMBOL(drm_crtc_set_max_vblank_count);
> > > +
> > >  /**
> > >   * drm_crtc_vblank_on - enable vblank events on a CRTC
> > >   * @crtc: CRTC in question
> > > diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> > > index 6ad9630d4f48..ecb2cf9913e2 100644
> > > --- a/include/drm/drm_vblank.h
> > > +++ b/include/drm/drm_vblank.h
> > > @@ -128,6 +128,12 @@ struct drm_vblank_crtc {
> > >  	 * @last: Protected by &drm_device.vbl_lock, used for wraparound handling.
> > >  	 */
> > >  	u32 last;
> > > +	/**
> > > +	 * @max_vblank_count: Maximum value of the hardware vblank counter.
> > > +	 * If non-zero this takes precedence over &drm_device.max_vblank_count
> > > +	 * for this crtc. Otherwise &drm_device.max_vblank_count is used.
> > > +	 */
> > 
> > I'd add "This should be set by calling drm_crtc_set_max_vblank_count()."
> > 
> > And please also add a note to the kerneldoc of drm_driver.max_vblank_count
> > pointing at &drm_vblank_crtc.max_vblank_count for per-crtc limits.
> 
> Ack.
> 
> > 
> > Aside from the nits lgtm. I think I'll skip looking at the TV out stuff
> > though ...
> 
> :)
> 
> -- 
> Ville Syrj�l�
> Intel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 01/15] drm/vblank: Allow dynamic per-crtc max_vblank_count

[Ville Syrjälä]

On Wed, Nov 21, 2018 at 04:19:36PM +0100, Daniel Vetter wrote:
> On Wed, Nov 21, 2018 at 01:37:51PM +0200, Ville Syrj�l� wrote:
> > On Wed, Nov 21, 2018 at 10:27:27AM +0100, Daniel Vetter wrote:
> > > On Mon, Nov 12, 2018 at 06:59:45PM +0200, Ville Syrjala wrote:
> > > > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > > > 
> > > > On i965gm we need to adjust max_vblank_count dynamically
> > > > depending on whether the TV encoder is used or not. To
> > > > that end add a per-crtc max_vblank_count that takes
> > > > precedence over its device wide counterpart. The driver
> > > > can now call drm_crtc_set_max_vblank_count() to configure
> > > > the per-crtc value before calling drm_vblank_on().
> > > > 
> > > > Also looks like there was some discussion about exynos needing
> > > > similar treatment.
> > > > 
> > > > Cc: stable@vger.kernel.org
> > > > Cc: Inki Dae <inki.dae@samsung.com>
> > > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_vblank.c | 39 ++++++++++++++++++++++++++++++++----
> > > >  include/drm/drm_vblank.h     |  8 ++++++++
> > > >  2 files changed, 43 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > > > index 98e091175921..c3abbdca8aba 100644
> > > > --- a/drivers/gpu/drm/drm_vblank.c
> > > > +++ b/drivers/gpu/drm/drm_vblank.c
> > > > @@ -105,13 +105,20 @@ static void store_vblank(struct drm_device *dev, unsigned int pipe,
> > > >  	write_sequnlock(&vblank->seqlock);
> > > >  }
> > > >  
> > > > +static u32 drm_max_vblank_count(struct drm_device *dev, unsigned int pipe)
> > > > +{
> > > > +	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> > > > +
> > > > +	return vblank->max_vblank_count ?: dev->max_vblank_count;
> > > > +}
> > > > +
> > > >  /*
> > > >   * "No hw counter" fallback implementation of .get_vblank_counter() hook,
> > > >   * if there is no useable hardware frame counter available.
> > > >   */
> > > >  static u32 drm_vblank_no_hw_counter(struct drm_device *dev, unsigned int pipe)
> > > >  {
> > > > -	WARN_ON_ONCE(dev->max_vblank_count != 0);
> > > > +	WARN_ON_ONCE(drm_max_vblank_count(dev, pipe) != 0);
> > > >  	return 0;
> > > >  }
> > > >  
> > > > @@ -198,6 +205,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> > > >  	ktime_t t_vblank;
> > > >  	int count = DRM_TIMESTAMP_MAXRETRIES;
> > > >  	int framedur_ns = vblank->framedur_ns;
> > > > +	u32 max_vblank_count = drm_max_vblank_count(dev, pipe);
> > > >  
> > > >  	/*
> > > >  	 * Interrupts were disabled prior to this call, so deal with counter
> > > > @@ -216,9 +224,9 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> > > >  		rc = drm_get_last_vbltimestamp(dev, pipe, &t_vblank, in_vblank_irq);
> > > >  	} while (cur_vblank != __get_vblank_counter(dev, pipe) && --count > 0);
> > > >  
> > > > -	if (dev->max_vblank_count != 0) {
> > > > +	if (max_vblank_count) {
> > > >  		/* trust the hw counter when it's around */
> > > > -		diff = (cur_vblank - vblank->last) & dev->max_vblank_count;
> > > > +		diff = (cur_vblank - vblank->last) & max_vblank_count;
> > > >  	} else if (rc && framedur_ns) {
> > > >  		u64 diff_ns = ktime_to_ns(ktime_sub(t_vblank, vblank->time));
> > > >  
> > > > @@ -258,7 +266,8 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> > > >  		      pipe, vblank->count, diff, cur_vblank, vblank->last);
> > > >  
> > > >  	if (diff == 0) {
> > > > -		WARN_ON_ONCE(cur_vblank != vblank->last);
> > > > +		WARN_ON_ONCE(max_vblank_count &&
> > > > +			     cur_vblank != vblank->last);
> > > 
> > > Unrelated bugfix for this warning? Should be a separate patch I think, or
> > > I'm missing something.
> > 
> > Ah, yeah this was due to a quirk of i965gm hardware. The hw counter
> > does work until the exact point when we enable TV encoder. Thus we
> > will get non-zero values up to that point, and since the TV encoder
> > isn't yet throttling the pipe it presumably runs at the oversample
> > clock so our timestamp based estimates can give us a diff==0 even
> > though the pipe did indeed pass a vblank already. I forgot to
> > note this in the commit message.
> > 
> > I think we can handle this three ways:
> > 1. do what I do here and just let the mismatch slip through
> > 2. force i915_get_vblank_counter() to return 0 always when the
> >    TV encoder is going to be used
> > 3. don't call drm_crtc_set_max_vblank_count() before drm_vblank_on()
> >    and instead delay it until just before we enable the TV encoder
> > 
> > I think option 3 is overly complicated to consider seriously. So
> > option 1 or option 2 is what I think we should do. For whatever
> > reason I went with option 1 here, but maybe option 2 is better
> > since it would be all contained within i915...
> 
> Delay drm_crtc_vblank_on until the vblank is stable? That seems like the
> semantically clean solution to me, instead of hacking around in core code
> when drivers leak garbage out ...

We need a vblank wait before turning on the TV encoder. Chicken vs. egg.

-- 
Ville Syrj�l�
Intel

Re: [PATCH 01/15] drm/vblank: Allow dynamic per-crtc max_vblank_count

[Daniel Vetter]

On Wed, Nov 21, 2018 at 5:16 PM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Wed, Nov 21, 2018 at 04:19:36PM +0100, Daniel Vetter wrote:
> > On Wed, Nov 21, 2018 at 01:37:51PM +0200, Ville Syrjälä wrote:
> > > On Wed, Nov 21, 2018 at 10:27:27AM +0100, Daniel Vetter wrote:
> > > > On Mon, Nov 12, 2018 at 06:59:45PM +0200, Ville Syrjala wrote:
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > >
> > > > > On i965gm we need to adjust max_vblank_count dynamically
> > > > > depending on whether the TV encoder is used or not. To
> > > > > that end add a per-crtc max_vblank_count that takes
> > > > > precedence over its device wide counterpart. The driver
> > > > > can now call drm_crtc_set_max_vblank_count() to configure
> > > > > the per-crtc value before calling drm_vblank_on().
> > > > >
> > > > > Also looks like there was some discussion about exynos needing
> > > > > similar treatment.
> > > > >
> > > > > Cc: stable@vger.kernel.org
> > > > > Cc: Inki Dae <inki.dae@samsung.com>
> > > > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_vblank.c | 39 ++++++++++++++++++++++++++++++++----
> > > > >  include/drm/drm_vblank.h     |  8 ++++++++
> > > > >  2 files changed, 43 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > > > > index 98e091175921..c3abbdca8aba 100644
> > > > > --- a/drivers/gpu/drm/drm_vblank.c
> > > > > +++ b/drivers/gpu/drm/drm_vblank.c
> > > > > @@ -105,13 +105,20 @@ static void store_vblank(struct drm_device *dev, unsigned int pipe,
> > > > >         write_sequnlock(&vblank->seqlock);
> > > > >  }
> > > > >
> > > > > +static u32 drm_max_vblank_count(struct drm_device *dev, unsigned int pipe)
> > > > > +{
> > > > > +       struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> > > > > +
> > > > > +       return vblank->max_vblank_count ?: dev->max_vblank_count;
> > > > > +}
> > > > > +
> > > > >  /*
> > > > >   * "No hw counter" fallback implementation of .get_vblank_counter() hook,
> > > > >   * if there is no useable hardware frame counter available.
> > > > >   */
> > > > >  static u32 drm_vblank_no_hw_counter(struct drm_device *dev, unsigned int pipe)
> > > > >  {
> > > > > -       WARN_ON_ONCE(dev->max_vblank_count != 0);
> > > > > +       WARN_ON_ONCE(drm_max_vblank_count(dev, pipe) != 0);
> > > > >         return 0;
> > > > >  }
> > > > >
> > > > > @@ -198,6 +205,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> > > > >         ktime_t t_vblank;
> > > > >         int count = DRM_TIMESTAMP_MAXRETRIES;
> > > > >         int framedur_ns = vblank->framedur_ns;
> > > > > +       u32 max_vblank_count = drm_max_vblank_count(dev, pipe);
> > > > >
> > > > >         /*
> > > > >          * Interrupts were disabled prior to this call, so deal with counter
> > > > > @@ -216,9 +224,9 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> > > > >                 rc = drm_get_last_vbltimestamp(dev, pipe, &t_vblank, in_vblank_irq);
> > > > >         } while (cur_vblank != __get_vblank_counter(dev, pipe) && --count > 0);
> > > > >
> > > > > -       if (dev->max_vblank_count != 0) {
> > > > > +       if (max_vblank_count) {
> > > > >                 /* trust the hw counter when it's around */
> > > > > -               diff = (cur_vblank - vblank->last) & dev->max_vblank_count;
> > > > > +               diff = (cur_vblank - vblank->last) & max_vblank_count;
> > > > >         } else if (rc && framedur_ns) {
> > > > >                 u64 diff_ns = ktime_to_ns(ktime_sub(t_vblank, vblank->time));
> > > > >
> > > > > @@ -258,7 +266,8 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> > > > >                       pipe, vblank->count, diff, cur_vblank, vblank->last);
> > > > >
> > > > >         if (diff == 0) {
> > > > > -               WARN_ON_ONCE(cur_vblank != vblank->last);
> > > > > +               WARN_ON_ONCE(max_vblank_count &&
> > > > > +                            cur_vblank != vblank->last);
> > > >
> > > > Unrelated bugfix for this warning? Should be a separate patch I think, or
> > > > I'm missing something.
> > >
> > > Ah, yeah this was due to a quirk of i965gm hardware. The hw counter
> > > does work until the exact point when we enable TV encoder. Thus we
> > > will get non-zero values up to that point, and since the TV encoder
> > > isn't yet throttling the pipe it presumably runs at the oversample
> > > clock so our timestamp based estimates can give us a diff==0 even
> > > though the pipe did indeed pass a vblank already. I forgot to
> > > note this in the commit message.
> > >
> > > I think we can handle this three ways:
> > > 1. do what I do here and just let the mismatch slip through
> > > 2. force i915_get_vblank_counter() to return 0 always when the
> > >    TV encoder is going to be used
> > > 3. don't call drm_crtc_set_max_vblank_count() before drm_vblank_on()
> > >    and instead delay it until just before we enable the TV encoder
> > >
> > > I think option 3 is overly complicated to consider seriously. So
> > > option 1 or option 2 is what I think we should do. For whatever
> > > reason I went with option 1 here, but maybe option 2 is better
> > > since it would be all contained within i915...
> >
> > Delay drm_crtc_vblank_on until the vblank is stable? That seems like the
> > semantically clean solution to me, instead of hacking around in core code
> > when drivers leak garbage out ...
>
> We need a vblank wait before turning on the TV encoder. Chicken vs. egg.

Gah.

Ok I think I think having the hack here makes sense then, but split
out as a separate patch with separate justification. It's a bit a
tricky thing that deserves to be higlighted (and easier way to found
the explanation with git blame in case it's ever needed).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 01/15] drm/vblank: Allow dynamic per-crtc max_vblank_count

[Ville Syrjälä]

On Wed, Nov 21, 2018 at 05:22:49PM +0100, Daniel Vetter wrote:
> On Wed, Nov 21, 2018 at 5:16 PM Ville Syrj�l�
> <ville.syrjala@linux.intel.com> wrote:
> >
> > On Wed, Nov 21, 2018 at 04:19:36PM +0100, Daniel Vetter wrote:
> > > On Wed, Nov 21, 2018 at 01:37:51PM +0200, Ville Syrj�l� wrote:
> > > > On Wed, Nov 21, 2018 at 10:27:27AM +0100, Daniel Vetter wrote:
> > > > > On Mon, Nov 12, 2018 at 06:59:45PM +0200, Ville Syrjala wrote:
> > > > > > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > > > > >
> > > > > > On i965gm we need to adjust max_vblank_count dynamically
> > > > > > depending on whether the TV encoder is used or not. To
> > > > > > that end add a per-crtc max_vblank_count that takes
> > > > > > precedence over its device wide counterpart. The driver
> > > > > > can now call drm_crtc_set_max_vblank_count() to configure
> > > > > > the per-crtc value before calling drm_vblank_on().
> > > > > >
> > > > > > Also looks like there was some discussion about exynos needing
> > > > > > similar treatment.
> > > > > >
> > > > > > Cc: stable@vger.kernel.org
> > > > > > Cc: Inki Dae <inki.dae@samsung.com>
> > > > > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > > > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/drm_vblank.c | 39 ++++++++++++++++++++++++++++++++----
> > > > > >  include/drm/drm_vblank.h     |  8 ++++++++
> > > > > >  2 files changed, 43 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > > > > > index 98e091175921..c3abbdca8aba 100644
> > > > > > --- a/drivers/gpu/drm/drm_vblank.c
> > > > > > +++ b/drivers/gpu/drm/drm_vblank.c
> > > > > > @@ -105,13 +105,20 @@ static void store_vblank(struct drm_device *dev, unsigned int pipe,
> > > > > >         write_sequnlock(&vblank->seqlock);
> > > > > >  }
> > > > > >
> > > > > > +static u32 drm_max_vblank_count(struct drm_device *dev, unsigned int pipe)
> > > > > > +{
> > > > > > +       struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> > > > > > +
> > > > > > +       return vblank->max_vblank_count ?: dev->max_vblank_count;
> > > > > > +}
> > > > > > +
> > > > > >  /*
> > > > > >   * "No hw counter" fallback implementation of .get_vblank_counter() hook,
> > > > > >   * if there is no useable hardware frame counter available.
> > > > > >   */
> > > > > >  static u32 drm_vblank_no_hw_counter(struct drm_device *dev, unsigned int pipe)
> > > > > >  {
> > > > > > -       WARN_ON_ONCE(dev->max_vblank_count != 0);
> > > > > > +       WARN_ON_ONCE(drm_max_vblank_count(dev, pipe) != 0);
> > > > > >         return 0;
> > > > > >  }
> > > > > >
> > > > > > @@ -198,6 +205,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> > > > > >         ktime_t t_vblank;
> > > > > >         int count = DRM_TIMESTAMP_MAXRETRIES;
> > > > > >         int framedur_ns = vblank->framedur_ns;
> > > > > > +       u32 max_vblank_count = drm_max_vblank_count(dev, pipe);
> > > > > >
> > > > > >         /*
> > > > > >          * Interrupts were disabled prior to this call, so deal with counter
> > > > > > @@ -216,9 +224,9 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> > > > > >                 rc = drm_get_last_vbltimestamp(dev, pipe, &t_vblank, in_vblank_irq);
> > > > > >         } while (cur_vblank != __get_vblank_counter(dev, pipe) && --count > 0);
> > > > > >
> > > > > > -       if (dev->max_vblank_count != 0) {
> > > > > > +       if (max_vblank_count) {
> > > > > >                 /* trust the hw counter when it's around */
> > > > > > -               diff = (cur_vblank - vblank->last) & dev->max_vblank_count;
> > > > > > +               diff = (cur_vblank - vblank->last) & max_vblank_count;
> > > > > >         } else if (rc && framedur_ns) {
> > > > > >                 u64 diff_ns = ktime_to_ns(ktime_sub(t_vblank, vblank->time));
> > > > > >
> > > > > > @@ -258,7 +266,8 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> > > > > >                       pipe, vblank->count, diff, cur_vblank, vblank->last);
> > > > > >
> > > > > >         if (diff == 0) {
> > > > > > -               WARN_ON_ONCE(cur_vblank != vblank->last);
> > > > > > +               WARN_ON_ONCE(max_vblank_count &&
> > > > > > +                            cur_vblank != vblank->last);
> > > > >
> > > > > Unrelated bugfix for this warning? Should be a separate patch I think, or
> > > > > I'm missing something.
> > > >
> > > > Ah, yeah this was due to a quirk of i965gm hardware. The hw counter
> > > > does work until the exact point when we enable TV encoder. Thus we
> > > > will get non-zero values up to that point, and since the TV encoder
> > > > isn't yet throttling the pipe it presumably runs at the oversample
> > > > clock so our timestamp based estimates can give us a diff==0 even
> > > > though the pipe did indeed pass a vblank already. I forgot to
> > > > note this in the commit message.
> > > >
> > > > I think we can handle this three ways:
> > > > 1. do what I do here and just let the mismatch slip through
> > > > 2. force i915_get_vblank_counter() to return 0 always when the
> > > >    TV encoder is going to be used
> > > > 3. don't call drm_crtc_set_max_vblank_count() before drm_vblank_on()
> > > >    and instead delay it until just before we enable the TV encoder
> > > >
> > > > I think option 3 is overly complicated to consider seriously. So
> > > > option 1 or option 2 is what I think we should do. For whatever
> > > > reason I went with option 1 here, but maybe option 2 is better
> > > > since it would be all contained within i915...
> > >
> > > Delay drm_crtc_vblank_on until the vblank is stable? That seems like the
> > > semantically clean solution to me, instead of hacking around in core code
> > > when drivers leak garbage out ...
> >
> > We need a vblank wait before turning on the TV encoder. Chicken vs. egg.
> 
> Gah.
> 
> Ok I think I think having the hack here makes sense then, but split
> out as a separate patch with separate justification. It's a bit a
> tricky thing that deserves to be higlighted (and easier way to found
> the explanation with git blame in case it's ever needed).

Or we go with option 2. It doesn't seem too bad actually.

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 019cb685986c..26d86aedc7ac 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -822,11 +822,26 @@ static void i915_enable_asle_pipestat(struct drm_i915_private *dev_priv)
 static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
 {
        struct drm_i915_private *dev_priv = to_i915(dev);
+       struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
+       const struct drm_display_mode *mode = vblank->hwmode;
        i915_reg_t high_frame, low_frame;
        u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal;
-       const struct drm_display_mode *mode = &dev->vblank[pipe].hwmode;
        unsigned long irqflags;
 
+       /*
+        * On i965gm TV output the frame counter works up to
+        * the point when we enable the TV encoder. After that the
+        * frame counter ceases to work and reads zero. We need a
+        * vblank wait before enabling the TV encoder and so we
+        * have to enable vblank interrupts while the frame counter
+        * is still in a working state. However the core vblank code
+        * does not like us returning non-zero frame counter values
+        * when we've told it that we don't have a working frame
+        * counter. Thus we must stop non-zero values leaking out.
+        */
+       if (!vblank->max_vblank_count)
+               return 0;
+
        htotal = mode->crtc_htotal;
        hsync_start = mode->crtc_hsync_start;
        vbl_start = mode->crtc_vblank_start;

-- 
Ville Syrj�l�
Intel

Re: [PATCH 01/15] drm/vblank: Allow dynamic per-crtc max_vblank_count

[Daniel Vetter]

On Wed, Nov 21, 2018 at 06:46:57PM +0200, Ville Syrj�l� wrote:
> On Wed, Nov 21, 2018 at 05:22:49PM +0100, Daniel Vetter wrote:
> > On Wed, Nov 21, 2018 at 5:16 PM Ville Syrj�l�
> > <ville.syrjala@linux.intel.com> wrote:
> > >
> > > On Wed, Nov 21, 2018 at 04:19:36PM +0100, Daniel Vetter wrote:
> > > > On Wed, Nov 21, 2018 at 01:37:51PM +0200, Ville Syrj�l� wrote:
> > > > > On Wed, Nov 21, 2018 at 10:27:27AM +0100, Daniel Vetter wrote:
> > > > > > On Mon, Nov 12, 2018 at 06:59:45PM +0200, Ville Syrjala wrote:
> > > > > > > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > > > > > >
> > > > > > > On i965gm we need to adjust max_vblank_count dynamically
> > > > > > > depending on whether the TV encoder is used or not. To
> > > > > > > that end add a per-crtc max_vblank_count that takes
> > > > > > > precedence over its device wide counterpart. The driver
> > > > > > > can now call drm_crtc_set_max_vblank_count() to configure
> > > > > > > the per-crtc value before calling drm_vblank_on().
> > > > > > >
> > > > > > > Also looks like there was some discussion about exynos needing
> > > > > > > similar treatment.
> > > > > > >
> > > > > > > Cc: stable@vger.kernel.org
> > > > > > > Cc: Inki Dae <inki.dae@samsung.com>
> > > > > > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > > > > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/drm_vblank.c | 39 ++++++++++++++++++++++++++++++++----
> > > > > > >  include/drm/drm_vblank.h     |  8 ++++++++
> > > > > > >  2 files changed, 43 insertions(+), 4 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > > > > > > index 98e091175921..c3abbdca8aba 100644
> > > > > > > --- a/drivers/gpu/drm/drm_vblank.c
> > > > > > > +++ b/drivers/gpu/drm/drm_vblank.c
> > > > > > > @@ -105,13 +105,20 @@ static void store_vblank(struct drm_device *dev, unsigned int pipe,
> > > > > > >         write_sequnlock(&vblank->seqlock);
> > > > > > >  }
> > > > > > >
> > > > > > > +static u32 drm_max_vblank_count(struct drm_device *dev, unsigned int pipe)
> > > > > > > +{
> > > > > > > +       struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> > > > > > > +
> > > > > > > +       return vblank->max_vblank_count ?: dev->max_vblank_count;
> > > > > > > +}
> > > > > > > +
> > > > > > >  /*
> > > > > > >   * "No hw counter" fallback implementation of .get_vblank_counter() hook,
> > > > > > >   * if there is no useable hardware frame counter available.
> > > > > > >   */
> > > > > > >  static u32 drm_vblank_no_hw_counter(struct drm_device *dev, unsigned int pipe)
> > > > > > >  {
> > > > > > > -       WARN_ON_ONCE(dev->max_vblank_count != 0);
> > > > > > > +       WARN_ON_ONCE(drm_max_vblank_count(dev, pipe) != 0);
> > > > > > >         return 0;
> > > > > > >  }
> > > > > > >
> > > > > > > @@ -198,6 +205,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> > > > > > >         ktime_t t_vblank;
> > > > > > >         int count = DRM_TIMESTAMP_MAXRETRIES;
> > > > > > >         int framedur_ns = vblank->framedur_ns;
> > > > > > > +       u32 max_vblank_count = drm_max_vblank_count(dev, pipe);
> > > > > > >
> > > > > > >         /*
> > > > > > >          * Interrupts were disabled prior to this call, so deal with counter
> > > > > > > @@ -216,9 +224,9 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> > > > > > >                 rc = drm_get_last_vbltimestamp(dev, pipe, &t_vblank, in_vblank_irq);
> > > > > > >         } while (cur_vblank != __get_vblank_counter(dev, pipe) && --count > 0);
> > > > > > >
> > > > > > > -       if (dev->max_vblank_count != 0) {
> > > > > > > +       if (max_vblank_count) {
> > > > > > >                 /* trust the hw counter when it's around */
> > > > > > > -               diff = (cur_vblank - vblank->last) & dev->max_vblank_count;
> > > > > > > +               diff = (cur_vblank - vblank->last) & max_vblank_count;
> > > > > > >         } else if (rc && framedur_ns) {
> > > > > > >                 u64 diff_ns = ktime_to_ns(ktime_sub(t_vblank, vblank->time));
> > > > > > >
> > > > > > > @@ -258,7 +266,8 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> > > > > > >                       pipe, vblank->count, diff, cur_vblank, vblank->last);
> > > > > > >
> > > > > > >         if (diff == 0) {
> > > > > > > -               WARN_ON_ONCE(cur_vblank != vblank->last);
> > > > > > > +               WARN_ON_ONCE(max_vblank_count &&
> > > > > > > +                            cur_vblank != vblank->last);
> > > > > >
> > > > > > Unrelated bugfix for this warning? Should be a separate patch I think, or
> > > > > > I'm missing something.
> > > > >
> > > > > Ah, yeah this was due to a quirk of i965gm hardware. The hw counter
> > > > > does work until the exact point when we enable TV encoder. Thus we
> > > > > will get non-zero values up to that point, and since the TV encoder
> > > > > isn't yet throttling the pipe it presumably runs at the oversample
> > > > > clock so our timestamp based estimates can give us a diff==0 even
> > > > > though the pipe did indeed pass a vblank already. I forgot to
> > > > > note this in the commit message.
> > > > >
> > > > > I think we can handle this three ways:
> > > > > 1. do what I do here and just let the mismatch slip through
> > > > > 2. force i915_get_vblank_counter() to return 0 always when the
> > > > >    TV encoder is going to be used
> > > > > 3. don't call drm_crtc_set_max_vblank_count() before drm_vblank_on()
> > > > >    and instead delay it until just before we enable the TV encoder
> > > > >
> > > > > I think option 3 is overly complicated to consider seriously. So
> > > > > option 1 or option 2 is what I think we should do. For whatever
> > > > > reason I went with option 1 here, but maybe option 2 is better
> > > > > since it would be all contained within i915...
> > > >
> > > > Delay drm_crtc_vblank_on until the vblank is stable? That seems like the
> > > > semantically clean solution to me, instead of hacking around in core code
> > > > when drivers leak garbage out ...
> > >
> > > We need a vblank wait before turning on the TV encoder. Chicken vs. egg.
> > 
> > Gah.
> > 
> > Ok I think I think having the hack here makes sense then, but split
> > out as a separate patch with separate justification. It's a bit a
> > tricky thing that deserves to be higlighted (and easier way to found
> > the explanation with git blame in case it's ever needed).
> 
> Or we go with option 2. It doesn't seem too bad actually.
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 019cb685986c..26d86aedc7ac 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -822,11 +822,26 @@ static void i915_enable_asle_pipestat(struct drm_i915_private *dev_priv)
>  static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
>  {
>         struct drm_i915_private *dev_priv = to_i915(dev);
> +       struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> +       const struct drm_display_mode *mode = vblank->hwmode;
>         i915_reg_t high_frame, low_frame;
>         u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal;
> -       const struct drm_display_mode *mode = &dev->vblank[pipe].hwmode;
>         unsigned long irqflags;
>  
> +       /*
> +        * On i965gm TV output the frame counter works up to
> +        * the point when we enable the TV encoder. After that the
> +        * frame counter ceases to work and reads zero. We need a
> +        * vblank wait before enabling the TV encoder and so we
> +        * have to enable vblank interrupts while the frame counter
> +        * is still in a working state. However the core vblank code
> +        * does not like us returning non-zero frame counter values
> +        * when we've told it that we don't have a working frame
> +        * counter. Thus we must stop non-zero values leaking out.
> +        */
> +       if (!vblank->max_vblank_count)
> +               return 0;

Hm yeah, looks reasonable actually. And less hacks in the shared code.
-Daniel

> +
>         htotal = mode->crtc_htotal;
>         hsync_start = mode->crtc_hsync_start;
>         vbl_start = mode->crtc_vblank_start;
> 
> -- 
> Ville Syrj�l�
> Intel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH v2 01/15] drm/vblank: Allow dynamic per-crtc max_vblank_count

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

On i965gm we need to adjust max_vblank_count dynamically
depending on whether the TV encoder is used or not. To
that end add a per-crtc max_vblank_count that takes
precedence over its device wide counterpart. The driver
can now call drm_crtc_set_max_vblank_count() to configure
the per-crtc value before calling drm_vblank_on().

Also looks like there was some discussion about exynos needing
similar treatment.

v2: Drop the extra max_vblank_count!=0 check for the
    WARN(last!=current), will take care of it in i915 code (Daniel)
    WARN_ON(!inmodeset) (Daniel)
    WARN_ON(dev->max_vblank_count)
    Pimp up the docs (Daniel)

Cc: stable@vger.kernel.org
Cc: Inki Dae <inki.dae@samsung.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_vblank.c | 45 +++++++++++++++++++++++++++++++++---
 include/drm/drm_device.h     |  8 ++++++-
 include/drm/drm_vblank.h     | 22 ++++++++++++++++++
 3 files changed, 71 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 98e091175921..cde71ee95a8f 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -105,13 +105,20 @@ static void store_vblank(struct drm_device *dev, unsigned int pipe,
 	write_sequnlock(&vblank->seqlock);
 }
 
+static u32 drm_max_vblank_count(struct drm_device *dev, unsigned int pipe)
+{
+	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
+
+	return vblank->max_vblank_count ?: dev->max_vblank_count;
+}
+
 /*
  * "No hw counter" fallback implementation of .get_vblank_counter() hook,
  * if there is no useable hardware frame counter available.
  */
 static u32 drm_vblank_no_hw_counter(struct drm_device *dev, unsigned int pipe)
 {
-	WARN_ON_ONCE(dev->max_vblank_count != 0);
+	WARN_ON_ONCE(drm_max_vblank_count(dev, pipe) != 0);
 	return 0;
 }
 
@@ -198,6 +205,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
 	ktime_t t_vblank;
 	int count = DRM_TIMESTAMP_MAXRETRIES;
 	int framedur_ns = vblank->framedur_ns;
+	u32 max_vblank_count = drm_max_vblank_count(dev, pipe);
 
 	/*
 	 * Interrupts were disabled prior to this call, so deal with counter
@@ -216,9 +224,9 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
 		rc = drm_get_last_vbltimestamp(dev, pipe, &t_vblank, in_vblank_irq);
 	} while (cur_vblank != __get_vblank_counter(dev, pipe) && --count > 0);
 
-	if (dev->max_vblank_count != 0) {
+	if (max_vblank_count) {
 		/* trust the hw counter when it's around */
-		diff = (cur_vblank - vblank->last) & dev->max_vblank_count;
+		diff = (cur_vblank - vblank->last) & max_vblank_count;
 	} else if (rc && framedur_ns) {
 		u64 diff_ns = ktime_to_ns(ktime_sub(t_vblank, vblank->time));
 
@@ -1204,6 +1212,37 @@ void drm_crtc_vblank_reset(struct drm_crtc *crtc)
 }
 EXPORT_SYMBOL(drm_crtc_vblank_reset);
 
+/**
+ * drm_crtc_set_max_vblank_count - configure the hw max vblank counter value
+ * @crtc: CRTC in question
+ * @max_vblank_count: max hardware vblank counter value
+ *
+ * Update the maximum hardware vblank counter value for @crtc
+ * at runtime. Useful for hardware where the operation of the
+ * hardware vblank counter depends on the currently active
+ * display configuration.
+ *
+ * For example, if the hardware vblank counter does not work
+ * when a specific connector is active the maximum can be set
+ * to zero. And when that specific connector isn't active the
+ * maximum can again be set to the appropriate non-zero value.
+ *
+ * If used, must be called before drm_vblank_on().
+ */
+void drm_crtc_set_max_vblank_count(struct drm_crtc *crtc,
+				   u32 max_vblank_count)
+{
+	struct drm_device *dev = crtc->dev;
+	unsigned int pipe = drm_crtc_index(crtc);
+	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
+
+	WARN_ON(dev->max_vblank_count);
+	WARN_ON(!READ_ONCE(vblank->inmodeset));
+
+	vblank->max_vblank_count = max_vblank_count;
+}
+EXPORT_SYMBOL(drm_crtc_set_max_vblank_count);
+
 /**
  * drm_crtc_vblank_on - enable vblank events on a CRTC
  * @crtc: CRTC in question
diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
index 42411b3ea0c8..45e43ce9652f 100644
--- a/include/drm/drm_device.h
+++ b/include/drm/drm_device.h
@@ -184,7 +184,13 @@ struct drm_device {
 	 * races and imprecision over longer time periods, hence exposing a
 	 * hardware vblank counter is always recommended.
 	 *
-	 * If non-zeor, &drm_crtc_funcs.get_vblank_counter must be set.
+	 * This is the statically configured device wide maximum. The driver
+	 * can instead choose to use a runtime configurable per-crtc value
+	 * &drm_vblank_crtc.max_vblank_count, in which case @max_vblank_count
+	 * must be left at zero. See drm_crtc_set_max_vblank_count() on how
+	 * to use the per-crtc value.
+	 *
+	 * If non-zero, &drm_crtc_funcs.get_vblank_counter must be set.
 	 */
 	u32 max_vblank_count;           /**< size of vblank counter register */
 
diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
index 6ad9630d4f48..e528bb2f659d 100644
--- a/include/drm/drm_vblank.h
+++ b/include/drm/drm_vblank.h
@@ -128,6 +128,26 @@ struct drm_vblank_crtc {
 	 * @last: Protected by &drm_device.vbl_lock, used for wraparound handling.
 	 */
 	u32 last;
+	/**
+	 * @max_vblank_count:
+	 *
+	 * Maximum value of the vblank registers for this crtc. This value +1
+	 * will result in a wrap-around of the vblank register. It is used
+	 * by the vblank core to handle wrap-arounds.
+	 *
+	 * If set to zero the vblank core will try to guess the elapsed vblanks
+	 * between times when the vblank interrupt is disabled through
+	 * high-precision timestamps. That approach is suffering from small
+	 * races and imprecision over longer time periods, hence exposing a
+	 * hardware vblank counter is always recommended.
+	 *
+	 * This is the runtime configurable per-crtc maximum set through
+	 * drm_crtc_set_max_vblank_count(). If this is used the driver
+	 * must leave the device wide &drm_device.max_vblank_count at zero.
+	 *
+	 * If non-zero, &drm_crtc_funcs.get_vblank_counter must be set.
+	 */
+	u32 max_vblank_count;
 	/**
 	 * @inmodeset: Tracks whether the vblank is disabled due to a modeset.
 	 * For legacy driver bit 2 additionally tracks whether an additional
@@ -206,4 +226,6 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
 void drm_calc_timestamping_constants(struct drm_crtc *crtc,
 				     const struct drm_display_mode *mode);
 wait_queue_head_t *drm_crtc_vblank_waitqueue(struct drm_crtc *crtc);
+void drm_crtc_set_max_vblank_count(struct drm_crtc *crtc,
+				   u32 max_vblank_count);
 #endif
-- 
2.18.1

[PATCH v2 02/15] drm/i915: Don't try to use the hardware frame counter with i965gm TV output

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

On i965gm the hardware frame counter does not work when
the TV encoder is active. So let's not try to consult
the hardware frame counter in that case. Instead we'll
fall back to the timestamp based guesstimation method
used on gen2.

Note that the pipe timings generated by the TV encoder
are also rather peculiar. Apparently the pipe wants to
run at a much higher speed (related to the oversample
clock somehow it seems) but during the vertical active
period the TV encoder stalls the pipe every few lines
to keep its speed in check. But once the vertical
blanking period is reached the pipe gets to run at full
speed. This means our vblank timestamp estimates are
suspect. Fixing all that would require quite a bit
more work. This simple fix at least avoids the nasty
vblank timeouts that are happening currently.

Curiously the frame counter works just fine on i945gm
and gm45. I don't really understand what kind of mishap
occurred with the hardware design on i965gm. Sadly
I wasn't able to find any chicken bits etc. that would
fix the frame counter :(

v2: Move the zero vs. non-zero hw counter value handling
    into i915_get_vblank_counter() (Daniel)
    Use the per-crtc maximum exclusively, leaving the
    per-device maximum at zero

Cc: stable@vger.kernel.org
Cc: Daniel Vetter <daniel@ffwll.ch>
Fixes: 51e31d49c890 ("drm/i915: Use generic vblank wait")
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93782
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c      | 27 ++++++++++-----
 drivers/gpu/drm/i915/intel_display.c | 49 +++++++++++++++++++++++-----
 2 files changed, 59 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index d447d7d508f4..ab2d4eefef18 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -822,11 +822,26 @@ static void i915_enable_asle_pipestat(struct drm_i915_private *dev_priv)
 static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
+	const struct drm_display_mode *mode = &vblank->hwmode;
 	i915_reg_t high_frame, low_frame;
 	u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal;
-	const struct drm_display_mode *mode = &dev->vblank[pipe].hwmode;
 	unsigned long irqflags;
 
+	/*
+	 * On i965gm TV output the frame counter only works up to
+	 * the point when we enable the TV encoder. After that the
+	 * frame counter ceases to work and reads zero. We need a
+	 * vblank wait before enabling the TV encoder and so we
+	 * have to enable vblank interrupts while the frame counter
+	 * is still in a working state. However the core vblank code
+	 * does not like us returning non-zero frame counter values
+	 * when we've told it that we don't have a working frame
+	 * counter. Thus we must stop non-zero values leaking out.
+	 */
+	if (!vblank->max_vblank_count)
+		return 0;
+
 	htotal = mode->crtc_htotal;
 	hsync_start = mode->crtc_hsync_start;
 	vbl_start = mode->crtc_vblank_start;
@@ -4836,16 +4851,10 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
 	if (INTEL_GEN(dev_priv) >= 8)
 		rps->pm_intrmsk_mbz |= GEN8_PMINTR_DISABLE_REDIRECT_TO_GUC;
 
-	if (IS_GEN2(dev_priv)) {
-		/* Gen2 doesn't have a hardware frame counter */
-		dev->max_vblank_count = 0;
-	} else if (IS_G4X(dev_priv) || INTEL_GEN(dev_priv) >= 5) {
-		dev->max_vblank_count = 0xffffffff; /* full 32 bit counter */
+	if (INTEL_GEN(dev_priv) >= 5 || IS_G4X(dev_priv))
 		dev->driver->get_vblank_counter = g4x_get_vblank_counter;
-	} else {
+	else if (INTEL_GEN(dev_priv) >= 3)
 		dev->driver->get_vblank_counter = i915_get_vblank_counter;
-		dev->max_vblank_count = 0xffffff; /* only 24 bits of frame count */
-	}
 
 	/*
 	 * Opt out of the vblank disable timer on everything except gen2.
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e9f4e22b2a4e..3eafea4e598f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1758,6 +1758,7 @@ static void intel_enable_pipe(const struct intel_crtc_state *new_crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->base.crtc);
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	struct drm_vblank_crtc *vblank = &dev_priv->drm.vblank[drm_crtc_index(&crtc->base)];
 	enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
 	enum pipe pipe = crtc->pipe;
 	i915_reg_t reg;
@@ -1806,7 +1807,7 @@ static void intel_enable_pipe(const struct intel_crtc_state *new_crtc_state)
 	 * 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_priv->drm.max_vblank_count == 0)
+	if (vblank->max_vblank_count == 0)
 		intel_wait_for_pipe_scanline_moving(crtc);
 }
 
@@ -5544,6 +5545,35 @@ static void intel_encoders_post_pll_disable(struct drm_crtc *crtc,
 	}
 }
 
+static u32 intel_crtc_max_vblank_count(const struct intel_crtc_state *crtc_state)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
+
+	/*
+	 * On i965gm the hardware frame counter reads
+	 * zero when the TV encoder is enabled :(
+	 */
+	if (IS_I965GM(dev_priv) &&
+	    (crtc_state->output_types & BIT(INTEL_OUTPUT_TVOUT)))
+		return 0;
+
+	if (INTEL_GEN(dev_priv) >= 5 || IS_G4X(dev_priv))
+		return 0xffffffff; /* full 32 bit counter */
+	else if (INTEL_GEN(dev_priv) >= 3)
+		return 0xffffff; /* only 24 bits of frame count */
+	else
+		return 0; /* Gen2 doesn't have a hardware frame counter */
+}
+
+static void intel_crtc_vblank_on(const struct intel_crtc_state *crtc_state)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+
+	drm_crtc_set_max_vblank_count(&crtc->base,
+				      intel_crtc_max_vblank_count(crtc_state));
+	drm_crtc_vblank_on(&crtc->base);
+}
+
 static void ironlake_crtc_enable(struct intel_crtc_state *pipe_config,
 				 struct drm_atomic_state *old_state)
 {
@@ -5617,7 +5647,7 @@ static void ironlake_crtc_enable(struct intel_crtc_state *pipe_config,
 		ironlake_pch_enable(old_intel_state, pipe_config);
 
 	assert_vblank_disabled(crtc);
-	drm_crtc_vblank_on(crtc);
+	intel_crtc_vblank_on(pipe_config);
 
 	intel_encoders_enable(crtc, pipe_config, old_state);
 
@@ -5774,7 +5804,7 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
 		intel_ddi_set_vc_payload_alloc(pipe_config, true);
 
 	assert_vblank_disabled(crtc);
-	drm_crtc_vblank_on(crtc);
+	intel_crtc_vblank_on(pipe_config);
 
 	intel_encoders_enable(crtc, pipe_config, old_state);
 
@@ -6114,7 +6144,7 @@ static void valleyview_crtc_enable(struct intel_crtc_state *pipe_config,
 	intel_enable_pipe(pipe_config);
 
 	assert_vblank_disabled(crtc);
-	drm_crtc_vblank_on(crtc);
+	intel_crtc_vblank_on(pipe_config);
 
 	intel_encoders_enable(crtc, pipe_config, old_state);
 }
@@ -6173,7 +6203,7 @@ static void i9xx_crtc_enable(struct intel_crtc_state *pipe_config,
 	intel_enable_pipe(pipe_config);
 
 	assert_vblank_disabled(crtc);
-	drm_crtc_vblank_on(crtc);
+	intel_crtc_vblank_on(pipe_config);
 
 	intel_encoders_enable(crtc, pipe_config, old_state);
 }
@@ -12653,8 +12683,9 @@ static int intel_atomic_prepare_commit(struct drm_device *dev,
 u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
+	struct drm_vblank_crtc *vblank = &dev->vblank[drm_crtc_index(&crtc->base)];
 
-	if (!dev->max_vblank_count)
+	if (!vblank->max_vblank_count)
 		return (u32)drm_crtc_accurate_vblank_count(&crtc->base);
 
 	return dev->driver->get_vblank_counter(dev, crtc->pipe);
@@ -15736,10 +15767,12 @@ intel_modeset_setup_hw_state(struct drm_device *dev,
 	 * waits, so we need vblank interrupts restored beforehand.
 	 */
 	for_each_intel_crtc(&dev_priv->drm, crtc) {
+		crtc_state = to_intel_crtc_state(crtc->base.state);
+
 		drm_crtc_vblank_reset(&crtc->base);
 
-		if (crtc->base.state->active)
-			drm_crtc_vblank_on(&crtc->base);
+		if (crtc_state->base.active)
+			intel_crtc_vblank_on(crtc_state);
 	}
 
 	intel_sanitize_plane_mapping(dev_priv);
-- 
2.18.1

Re: [PATCH v2 01/15] drm/vblank: Allow dynamic per-crtc max_vblank_count

[Daniel Vetter]

On Tue, Nov 27, 2018 at 08:20:04PM +0200, Ville Syrjala wrote:
> From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> 
> On i965gm we need to adjust max_vblank_count dynamically
> depending on whether the TV encoder is used or not. To
> that end add a per-crtc max_vblank_count that takes
> precedence over its device wide counterpart. The driver
> can now call drm_crtc_set_max_vblank_count() to configure
> the per-crtc value before calling drm_vblank_on().
> 
> Also looks like there was some discussion about exynos needing
> similar treatment.
> 
> v2: Drop the extra max_vblank_count!=0 check for the
>     WARN(last!=current), will take care of it in i915 code (Daniel)
>     WARN_ON(!inmodeset) (Daniel)
>     WARN_ON(dev->max_vblank_count)
>     Pimp up the docs (Daniel)

Yeah I'm happy with the pile of WARN_ON here. And docs look great too.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> 
> Cc: stable@vger.kernel.org
> Cc: Inki Dae <inki.dae@samsung.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_vblank.c | 45 +++++++++++++++++++++++++++++++++---
>  include/drm/drm_device.h     |  8 ++++++-
>  include/drm/drm_vblank.h     | 22 ++++++++++++++++++
>  3 files changed, 71 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 98e091175921..cde71ee95a8f 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -105,13 +105,20 @@ static void store_vblank(struct drm_device *dev, unsigned int pipe,
>  	write_sequnlock(&vblank->seqlock);
>  }
>  
> +static u32 drm_max_vblank_count(struct drm_device *dev, unsigned int pipe)
> +{
> +	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> +
> +	return vblank->max_vblank_count ?: dev->max_vblank_count;
> +}
> +
>  /*
>   * "No hw counter" fallback implementation of .get_vblank_counter() hook,
>   * if there is no useable hardware frame counter available.
>   */
>  static u32 drm_vblank_no_hw_counter(struct drm_device *dev, unsigned int pipe)
>  {
> -	WARN_ON_ONCE(dev->max_vblank_count != 0);
> +	WARN_ON_ONCE(drm_max_vblank_count(dev, pipe) != 0);
>  	return 0;
>  }
>  
> @@ -198,6 +205,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
>  	ktime_t t_vblank;
>  	int count = DRM_TIMESTAMP_MAXRETRIES;
>  	int framedur_ns = vblank->framedur_ns;
> +	u32 max_vblank_count = drm_max_vblank_count(dev, pipe);
>  
>  	/*
>  	 * Interrupts were disabled prior to this call, so deal with counter
> @@ -216,9 +224,9 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
>  		rc = drm_get_last_vbltimestamp(dev, pipe, &t_vblank, in_vblank_irq);
>  	} while (cur_vblank != __get_vblank_counter(dev, pipe) && --count > 0);
>  
> -	if (dev->max_vblank_count != 0) {
> +	if (max_vblank_count) {
>  		/* trust the hw counter when it's around */
> -		diff = (cur_vblank - vblank->last) & dev->max_vblank_count;
> +		diff = (cur_vblank - vblank->last) & max_vblank_count;
>  	} else if (rc && framedur_ns) {
>  		u64 diff_ns = ktime_to_ns(ktime_sub(t_vblank, vblank->time));
>  
> @@ -1204,6 +1212,37 @@ void drm_crtc_vblank_reset(struct drm_crtc *crtc)
>  }
>  EXPORT_SYMBOL(drm_crtc_vblank_reset);
>  
> +/**
> + * drm_crtc_set_max_vblank_count - configure the hw max vblank counter value
> + * @crtc: CRTC in question
> + * @max_vblank_count: max hardware vblank counter value
> + *
> + * Update the maximum hardware vblank counter value for @crtc
> + * at runtime. Useful for hardware where the operation of the
> + * hardware vblank counter depends on the currently active
> + * display configuration.
> + *
> + * For example, if the hardware vblank counter does not work
> + * when a specific connector is active the maximum can be set
> + * to zero. And when that specific connector isn't active the
> + * maximum can again be set to the appropriate non-zero value.
> + *
> + * If used, must be called before drm_vblank_on().
> + */
> +void drm_crtc_set_max_vblank_count(struct drm_crtc *crtc,
> +				   u32 max_vblank_count)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	unsigned int pipe = drm_crtc_index(crtc);
> +	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> +
> +	WARN_ON(dev->max_vblank_count);
> +	WARN_ON(!READ_ONCE(vblank->inmodeset));
> +
> +	vblank->max_vblank_count = max_vblank_count;
> +}
> +EXPORT_SYMBOL(drm_crtc_set_max_vblank_count);
> +
>  /**
>   * drm_crtc_vblank_on - enable vblank events on a CRTC
>   * @crtc: CRTC in question
> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> index 42411b3ea0c8..45e43ce9652f 100644
> --- a/include/drm/drm_device.h
> +++ b/include/drm/drm_device.h
> @@ -184,7 +184,13 @@ struct drm_device {
>  	 * races and imprecision over longer time periods, hence exposing a
>  	 * hardware vblank counter is always recommended.
>  	 *
> -	 * If non-zeor, &drm_crtc_funcs.get_vblank_counter must be set.
> +	 * This is the statically configured device wide maximum. The driver
> +	 * can instead choose to use a runtime configurable per-crtc value
> +	 * &drm_vblank_crtc.max_vblank_count, in which case @max_vblank_count
> +	 * must be left at zero. See drm_crtc_set_max_vblank_count() on how
> +	 * to use the per-crtc value.
> +	 *
> +	 * If non-zero, &drm_crtc_funcs.get_vblank_counter must be set.
>  	 */
>  	u32 max_vblank_count;           /**< size of vblank counter register */
>  
> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> index 6ad9630d4f48..e528bb2f659d 100644
> --- a/include/drm/drm_vblank.h
> +++ b/include/drm/drm_vblank.h
> @@ -128,6 +128,26 @@ struct drm_vblank_crtc {
>  	 * @last: Protected by &drm_device.vbl_lock, used for wraparound handling.
>  	 */
>  	u32 last;
> +	/**
> +	 * @max_vblank_count:
> +	 *
> +	 * Maximum value of the vblank registers for this crtc. This value +1
> +	 * will result in a wrap-around of the vblank register. It is used
> +	 * by the vblank core to handle wrap-arounds.
> +	 *
> +	 * If set to zero the vblank core will try to guess the elapsed vblanks
> +	 * between times when the vblank interrupt is disabled through
> +	 * high-precision timestamps. That approach is suffering from small
> +	 * races and imprecision over longer time periods, hence exposing a
> +	 * hardware vblank counter is always recommended.
> +	 *
> +	 * This is the runtime configurable per-crtc maximum set through
> +	 * drm_crtc_set_max_vblank_count(). If this is used the driver
> +	 * must leave the device wide &drm_device.max_vblank_count at zero.
> +	 *
> +	 * If non-zero, &drm_crtc_funcs.get_vblank_counter must be set.
> +	 */
> +	u32 max_vblank_count;
>  	/**
>  	 * @inmodeset: Tracks whether the vblank is disabled due to a modeset.
>  	 * For legacy driver bit 2 additionally tracks whether an additional
> @@ -206,4 +226,6 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
>  void drm_calc_timestamping_constants(struct drm_crtc *crtc,
>  				     const struct drm_display_mode *mode);
>  wait_queue_head_t *drm_crtc_vblank_waitqueue(struct drm_crtc *crtc);
> +void drm_crtc_set_max_vblank_count(struct drm_crtc *crtc,
> +				   u32 max_vblank_count);
>  #endif
> -- 
> 2.18.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH v3 02/15] drm/i915: Don't try to use the hardware frame counter with i965gm TV output

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

On i965gm the hardware frame counter does not work when
the TV encoder is active. So let's not try to consult
the hardware frame counter in that case. Instead we'll
fall back to the timestamp based guesstimation method
used on gen2.

Note that the pipe timings generated by the TV encoder
are also rather peculiar. Apparently the pipe wants to
run at a much higher speed (related to the oversample
clock somehow it seems) but during the vertical active
period the TV encoder stalls the pipe every few lines
to keep its speed in check. But once the vertical
blanking period is reached the pipe gets to run at full
speed. This means our vblank timestamp estimates are
suspect. Fixing all that would require quite a bit
more work. This simple fix at least avoids the nasty
vblank timeouts that are happening currently.

Curiously the frame counter works just fine on i945gm
and gm45. I don't really understand what kind of mishap
occurred with the hardware design on i965gm. Sadly
I wasn't able to find any chicken bits etc. that would
fix the frame counter :(

v2: Move the zero vs. non-zero hw counter value handling
    into i915_get_vblank_counter() (Daniel)
    Use the per-crtc maximum exclusively, leaving the
    per-device maximum at zero
v3: max_vblank_count not populated yet in intel_enable_pipe()
    use intel_crtc_max_vblank_count() instead

Cc: stable@vger.kernel.org
Cc: Daniel Vetter <daniel@ffwll.ch>
Fixes: 51e31d49c890 ("drm/i915: Use generic vblank wait")
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93782
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

fix#	_slub_broken.S
---
 drivers/gpu/drm/i915/i915_irq.c      | 27 ++++++++++------
 drivers/gpu/drm/i915/intel_display.c | 48 +++++++++++++++++++++++-----
 2 files changed, 58 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index d447d7d508f4..ab2d4eefef18 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -822,11 +822,26 @@ static void i915_enable_asle_pipestat(struct drm_i915_private *dev_priv)
 static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
+	const struct drm_display_mode *mode = &vblank->hwmode;
 	i915_reg_t high_frame, low_frame;
 	u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal;
-	const struct drm_display_mode *mode = &dev->vblank[pipe].hwmode;
 	unsigned long irqflags;
 
+	/*
+	 * On i965gm TV output the frame counter only works up to
+	 * the point when we enable the TV encoder. After that the
+	 * frame counter ceases to work and reads zero. We need a
+	 * vblank wait before enabling the TV encoder and so we
+	 * have to enable vblank interrupts while the frame counter
+	 * is still in a working state. However the core vblank code
+	 * does not like us returning non-zero frame counter values
+	 * when we've told it that we don't have a working frame
+	 * counter. Thus we must stop non-zero values leaking out.
+	 */
+	if (!vblank->max_vblank_count)
+		return 0;
+
 	htotal = mode->crtc_htotal;
 	hsync_start = mode->crtc_hsync_start;
 	vbl_start = mode->crtc_vblank_start;
@@ -4836,16 +4851,10 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
 	if (INTEL_GEN(dev_priv) >= 8)
 		rps->pm_intrmsk_mbz |= GEN8_PMINTR_DISABLE_REDIRECT_TO_GUC;
 
-	if (IS_GEN2(dev_priv)) {
-		/* Gen2 doesn't have a hardware frame counter */
-		dev->max_vblank_count = 0;
-	} else if (IS_G4X(dev_priv) || INTEL_GEN(dev_priv) >= 5) {
-		dev->max_vblank_count = 0xffffffff; /* full 32 bit counter */
+	if (INTEL_GEN(dev_priv) >= 5 || IS_G4X(dev_priv))
 		dev->driver->get_vblank_counter = g4x_get_vblank_counter;
-	} else {
+	else if (INTEL_GEN(dev_priv) >= 3)
 		dev->driver->get_vblank_counter = i915_get_vblank_counter;
-		dev->max_vblank_count = 0xffffff; /* only 24 bits of frame count */
-	}
 
 	/*
 	 * Opt out of the vblank disable timer on everything except gen2.
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e9f4e22b2a4e..cb13eff78ad9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1754,6 +1754,35 @@ enum pipe intel_crtc_pch_transcoder(struct intel_crtc *crtc)
 		return crtc->pipe;
 }
 
+static u32 intel_crtc_max_vblank_count(const struct intel_crtc_state *crtc_state)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
+
+	/*
+	 * On i965gm the hardware frame counter reads
+	 * zero when the TV encoder is enabled :(
+	 */
+	if (IS_I965GM(dev_priv) &&
+	    (crtc_state->output_types & BIT(INTEL_OUTPUT_TVOUT)))
+		return 0;
+
+	if (INTEL_GEN(dev_priv) >= 5 || IS_G4X(dev_priv))
+		return 0xffffffff; /* full 32 bit counter */
+	else if (INTEL_GEN(dev_priv) >= 3)
+		return 0xffffff; /* only 24 bits of frame count */
+	else
+		return 0; /* Gen2 doesn't have a hardware frame counter */
+}
+
+static void intel_crtc_vblank_on(const struct intel_crtc_state *crtc_state)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+
+	drm_crtc_set_max_vblank_count(&crtc->base,
+				      intel_crtc_max_vblank_count(crtc_state));
+	drm_crtc_vblank_on(&crtc->base);
+}
+
 static void intel_enable_pipe(const struct intel_crtc_state *new_crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->base.crtc);
@@ -1806,7 +1835,7 @@ static void intel_enable_pipe(const struct intel_crtc_state *new_crtc_state)
 	 * 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_priv->drm.max_vblank_count == 0)
+	if (intel_crtc_max_vblank_count(new_crtc_state) == 0)
 		intel_wait_for_pipe_scanline_moving(crtc);
 }
 
@@ -5617,7 +5646,7 @@ static void ironlake_crtc_enable(struct intel_crtc_state *pipe_config,
 		ironlake_pch_enable(old_intel_state, pipe_config);
 
 	assert_vblank_disabled(crtc);
-	drm_crtc_vblank_on(crtc);
+	intel_crtc_vblank_on(pipe_config);
 
 	intel_encoders_enable(crtc, pipe_config, old_state);
 
@@ -5774,7 +5803,7 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
 		intel_ddi_set_vc_payload_alloc(pipe_config, true);
 
 	assert_vblank_disabled(crtc);
-	drm_crtc_vblank_on(crtc);
+	intel_crtc_vblank_on(pipe_config);
 
 	intel_encoders_enable(crtc, pipe_config, old_state);
 
@@ -6114,7 +6143,7 @@ static void valleyview_crtc_enable(struct intel_crtc_state *pipe_config,
 	intel_enable_pipe(pipe_config);
 
 	assert_vblank_disabled(crtc);
-	drm_crtc_vblank_on(crtc);
+	intel_crtc_vblank_on(pipe_config);
 
 	intel_encoders_enable(crtc, pipe_config, old_state);
 }
@@ -6173,7 +6202,7 @@ static void i9xx_crtc_enable(struct intel_crtc_state *pipe_config,
 	intel_enable_pipe(pipe_config);
 
 	assert_vblank_disabled(crtc);
-	drm_crtc_vblank_on(crtc);
+	intel_crtc_vblank_on(pipe_config);
 
 	intel_encoders_enable(crtc, pipe_config, old_state);
 }
@@ -12653,8 +12682,9 @@ static int intel_atomic_prepare_commit(struct drm_device *dev,
 u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
+	struct drm_vblank_crtc *vblank = &dev->vblank[drm_crtc_index(&crtc->base)];
 
-	if (!dev->max_vblank_count)
+	if (!vblank->max_vblank_count)
 		return (u32)drm_crtc_accurate_vblank_count(&crtc->base);
 
 	return dev->driver->get_vblank_counter(dev, crtc->pipe);
@@ -15736,10 +15766,12 @@ intel_modeset_setup_hw_state(struct drm_device *dev,
 	 * waits, so we need vblank interrupts restored beforehand.
 	 */
 	for_each_intel_crtc(&dev_priv->drm, crtc) {
+		crtc_state = to_intel_crtc_state(crtc->base.state);
+
 		drm_crtc_vblank_reset(&crtc->base);
 
-		if (crtc->base.state->active)
-			drm_crtc_vblank_on(&crtc->base);
+		if (crtc_state->base.active)
+			intel_crtc_vblank_on(crtc_state);
 	}
 
 	intel_sanitize_plane_mapping(dev_priv);
-- 
2.18.1

Re: [PATCH v3 02/15] drm/i915: Don't try to use the hardware frame counter with i965gm TV output

[Imre Deak]

On Tue, Nov 27, 2018 at 10:05:50PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> On i965gm the hardware frame counter does not work when
> the TV encoder is active. So let's not try to consult
> the hardware frame counter in that case. Instead we'll
> fall back to the timestamp based guesstimation method
> used on gen2.
> 
> Note that the pipe timings generated by the TV encoder
> are also rather peculiar. Apparently the pipe wants to
> run at a much higher speed (related to the oversample
> clock somehow it seems) but during the vertical active
> period the TV encoder stalls the pipe every few lines
> to keep its speed in check. But once the vertical
> blanking period is reached the pipe gets to run at full
> speed. This means our vblank timestamp estimates are
> suspect. Fixing all that would require quite a bit
> more work. This simple fix at least avoids the nasty
> vblank timeouts that are happening currently.
> 
> Curiously the frame counter works just fine on i945gm
> and gm45. I don't really understand what kind of mishap
> occurred with the hardware design on i965gm. Sadly
> I wasn't able to find any chicken bits etc. that would
> fix the frame counter :(
> 
> v2: Move the zero vs. non-zero hw counter value handling
>     into i915_get_vblank_counter() (Daniel)
>     Use the per-crtc maximum exclusively, leaving the
>     per-device maximum at zero
> v3: max_vblank_count not populated yet in intel_enable_pipe()
>     use intel_crtc_max_vblank_count() instead
> 
> Cc: stable@vger.kernel.org
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Fixes: 51e31d49c890 ("drm/i915: Use generic vblank wait")
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93782
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> fix#	_slub_broken.S
  ^ some remnant

Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_irq.c      | 27 ++++++++++------
>  drivers/gpu/drm/i915/intel_display.c | 48 +++++++++++++++++++++++-----
>  2 files changed, 58 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index d447d7d508f4..ab2d4eefef18 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -822,11 +822,26 @@ static void i915_enable_asle_pipestat(struct drm_i915_private *dev_priv)
>  static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> +	const struct drm_display_mode *mode = &vblank->hwmode;
>  	i915_reg_t high_frame, low_frame;
>  	u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal;
> -	const struct drm_display_mode *mode = &dev->vblank[pipe].hwmode;
>  	unsigned long irqflags;
>  
> +	/*
> +	 * On i965gm TV output the frame counter only works up to
> +	 * the point when we enable the TV encoder. After that the
> +	 * frame counter ceases to work and reads zero. We need a
> +	 * vblank wait before enabling the TV encoder and so we
> +	 * have to enable vblank interrupts while the frame counter
> +	 * is still in a working state. However the core vblank code
> +	 * does not like us returning non-zero frame counter values
> +	 * when we've told it that we don't have a working frame
> +	 * counter. Thus we must stop non-zero values leaking out.
> +	 */
> +	if (!vblank->max_vblank_count)
> +		return 0;
> +
>  	htotal = mode->crtc_htotal;
>  	hsync_start = mode->crtc_hsync_start;
>  	vbl_start = mode->crtc_vblank_start;
> @@ -4836,16 +4851,10 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
>  	if (INTEL_GEN(dev_priv) >= 8)
>  		rps->pm_intrmsk_mbz |= GEN8_PMINTR_DISABLE_REDIRECT_TO_GUC;
>  
> -	if (IS_GEN2(dev_priv)) {
> -		/* Gen2 doesn't have a hardware frame counter */
> -		dev->max_vblank_count = 0;
> -	} else if (IS_G4X(dev_priv) || INTEL_GEN(dev_priv) >= 5) {
> -		dev->max_vblank_count = 0xffffffff; /* full 32 bit counter */
> +	if (INTEL_GEN(dev_priv) >= 5 || IS_G4X(dev_priv))
>  		dev->driver->get_vblank_counter = g4x_get_vblank_counter;
> -	} else {
> +	else if (INTEL_GEN(dev_priv) >= 3)
>  		dev->driver->get_vblank_counter = i915_get_vblank_counter;
> -		dev->max_vblank_count = 0xffffff; /* only 24 bits of frame count */
> -	}
>  
>  	/*
>  	 * Opt out of the vblank disable timer on everything except gen2.
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e9f4e22b2a4e..cb13eff78ad9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1754,6 +1754,35 @@ enum pipe intel_crtc_pch_transcoder(struct intel_crtc *crtc)
>  		return crtc->pipe;
>  }
>  
> +static u32 intel_crtc_max_vblank_count(const struct intel_crtc_state *crtc_state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> +
> +	/*
> +	 * On i965gm the hardware frame counter reads
> +	 * zero when the TV encoder is enabled :(
> +	 */
> +	if (IS_I965GM(dev_priv) &&
> +	    (crtc_state->output_types & BIT(INTEL_OUTPUT_TVOUT)))
> +		return 0;
> +
> +	if (INTEL_GEN(dev_priv) >= 5 || IS_G4X(dev_priv))
> +		return 0xffffffff; /* full 32 bit counter */
> +	else if (INTEL_GEN(dev_priv) >= 3)
> +		return 0xffffff; /* only 24 bits of frame count */
> +	else
> +		return 0; /* Gen2 doesn't have a hardware frame counter */
> +}
> +
> +static void intel_crtc_vblank_on(const struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> +
> +	drm_crtc_set_max_vblank_count(&crtc->base,
> +				      intel_crtc_max_vblank_count(crtc_state));
> +	drm_crtc_vblank_on(&crtc->base);
> +}
> +
>  static void intel_enable_pipe(const struct intel_crtc_state *new_crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->base.crtc);
> @@ -1806,7 +1835,7 @@ static void intel_enable_pipe(const struct intel_crtc_state *new_crtc_state)
>  	 * 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_priv->drm.max_vblank_count == 0)
> +	if (intel_crtc_max_vblank_count(new_crtc_state) == 0)
>  		intel_wait_for_pipe_scanline_moving(crtc);
>  }
>  
> @@ -5617,7 +5646,7 @@ static void ironlake_crtc_enable(struct intel_crtc_state *pipe_config,
>  		ironlake_pch_enable(old_intel_state, pipe_config);
>  
>  	assert_vblank_disabled(crtc);
> -	drm_crtc_vblank_on(crtc);
> +	intel_crtc_vblank_on(pipe_config);
>  
>  	intel_encoders_enable(crtc, pipe_config, old_state);
>  
> @@ -5774,7 +5803,7 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
>  		intel_ddi_set_vc_payload_alloc(pipe_config, true);
>  
>  	assert_vblank_disabled(crtc);
> -	drm_crtc_vblank_on(crtc);
> +	intel_crtc_vblank_on(pipe_config);
>  
>  	intel_encoders_enable(crtc, pipe_config, old_state);
>  
> @@ -6114,7 +6143,7 @@ static void valleyview_crtc_enable(struct intel_crtc_state *pipe_config,
>  	intel_enable_pipe(pipe_config);
>  
>  	assert_vblank_disabled(crtc);
> -	drm_crtc_vblank_on(crtc);
> +	intel_crtc_vblank_on(pipe_config);
>  
>  	intel_encoders_enable(crtc, pipe_config, old_state);
>  }
> @@ -6173,7 +6202,7 @@ static void i9xx_crtc_enable(struct intel_crtc_state *pipe_config,
>  	intel_enable_pipe(pipe_config);
>  
>  	assert_vblank_disabled(crtc);
> -	drm_crtc_vblank_on(crtc);
> +	intel_crtc_vblank_on(pipe_config);
>  
>  	intel_encoders_enable(crtc, pipe_config, old_state);
>  }
> @@ -12653,8 +12682,9 @@ static int intel_atomic_prepare_commit(struct drm_device *dev,
>  u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->base.dev;
> +	struct drm_vblank_crtc *vblank = &dev->vblank[drm_crtc_index(&crtc->base)];
>  
> -	if (!dev->max_vblank_count)
> +	if (!vblank->max_vblank_count)
>  		return (u32)drm_crtc_accurate_vblank_count(&crtc->base);
>  
>  	return dev->driver->get_vblank_counter(dev, crtc->pipe);
> @@ -15736,10 +15766,12 @@ intel_modeset_setup_hw_state(struct drm_device *dev,
>  	 * waits, so we need vblank interrupts restored beforehand.
>  	 */
>  	for_each_intel_crtc(&dev_priv->drm, crtc) {
> +		crtc_state = to_intel_crtc_state(crtc->base.state);
> +
>  		drm_crtc_vblank_reset(&crtc->base);
>  
> -		if (crtc->base.state->active)
> -			drm_crtc_vblank_on(&crtc->base);
> +		if (crtc_state->base.active)
> +			intel_crtc_vblank_on(crtc_state);
>  	}
>  
>  	intel_sanitize_plane_mapping(dev_priv);
> -- 
> 2.18.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel