Re: [PATCH] drm/i915: Workaround VLV/CHV DSI scanline counter hardware fail

[Mika Kahola <mika.kahola@intel.com>]

The patch does what it says.

Tested-by: Mika Kahola <mika.kahola@intel.com>
Reviewed-by: Mika Kahola <mika.kahola@intel.com>

On Thu, 2016-12-15 at 19:47 +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The scanline counter is bonkers on VLV/CHV DSI. The scanline counter
> increment is not lined up with the start of vblank like it is on
> every other platform and output type. This causes problems for
> both the vblank timestamping and atomic update vblank evasion.
> 
> On my FFRD8 machine at least, the scanline counter increment
> happens about 1/3 of a scanline ahead of the start of vblank (which
> is where all register latching happens still). That means we can't
> trust the scanline counter to tell us whether we're in vblank or not
> while we're on that particular line. In order to keep vblank
> timestamping in working condition when called from the vblank irq,
> we'll leave scanline_offset at one, which means that the entire
> line containing the start of vblank is considered to be inside
> the vblank.
> 
> For the vblank evasion we'll need to consider that entire line
> to be bad, since we can't tell whether the registers already
> got latched or not. And we can't actually use the start of vblank
> interrupt to get us past that line as the interrupt would fire
> too soon, and then we'd up waiting for the next start of vblank
> instead. One way around that would using the frame start
> interrupt instead since that wouldn't fire until the next
> scanline, but that would require some bigger changes in the
> interrupt code. So for simplicity we'll just poll until we get
> past the bad line.
> 
> v2: Adjust the comments a bit
> 
> Cc: stable@vger.kernel.org
> Cc: Jonas Aaberg <cja@gmx.net>
> Tested-by: Jonas Aaberg <cja@gmx.net>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99086
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  9 +++++++++
>  drivers/gpu/drm/i915/intel_sprite.c  | 21 +++++++++++++++++++++
>  2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 9cc5dbfc314b..159b2eb9766b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13852,6 +13852,15 @@ static void update_scanline_offset(struct
> intel_crtc *crtc)
>  	 * type. For DP ports it behaves like most other platforms,
> but on HDMI
>  	 * there's an extra 1 line difference. So we need to add two
> instead of
>  	 * one to the value.
> +	 *
> +	 * On VLV/CHV DSI the scanline counter would appear to
> increment
> +	 * approx. 1/3 of a scanline before start of vblank.
> Unfortunately
> +	 * that means we can't tell whether we're in vblank or not
> while
> +	 * we're on that particular line. We must still set
> scanline_offset
> +	 * to 1 so that the vblank timestamps come out correct when
> we query
> +	 * the scanline counter from within the vblank interrupt
> handler.
> +	 * However if queried just before the start of vblank we'll
> get an
> +	 * answer that's slightly in the future.
>  	 */
>  	if (IS_GEN2(dev_priv)) {
>  		const struct drm_display_mode *adjusted_mode =
> &crtc->config->base.adjusted_mode;
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> b/drivers/gpu/drm/i915/intel_sprite.c
> index 7031bc733d97..fb0e0d8e893a 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -81,10 +81,13 @@ int intel_usecs_to_scanlines(const struct
> drm_display_mode *adjusted_mode,
>   */
>  void intel_pipe_update_start(struct intel_crtc *crtc)
>  {
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	const struct drm_display_mode *adjusted_mode = &crtc-
> >config->base.adjusted_mode;
>  	long timeout = msecs_to_jiffies_timeout(1);
>  	int scanline, min, max, vblank_start;
>  	wait_queue_head_t *wq = drm_crtc_vblank_waitqueue(&crtc-
> >base);
> +	bool need_vlv_dsi_wa = (IS_VALLEYVIEW(dev_priv) ||
> IS_CHERRYVIEW(dev_priv)) &&
> +		intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DSI);
>  	DEFINE_WAIT(wait);
>  
>  	vblank_start = adjusted_mode->crtc_vblank_start;
> @@ -136,6 +139,24 @@ void intel_pipe_update_start(struct intel_crtc
> *crtc)
>  
>  	drm_crtc_vblank_put(&crtc->base);
>  
> +	/*
> +	 * On VLV/CHV DSI the scanline counter would appear to
> +	 * increment approx. 1/3 of a scanline before start of
> vblank.
> +	 * The registers still get latched at start of vblank
> however.
> +	 * This means we must not write any registers on the first
> +	 * line of vblank (since not the whole line is actually in
> +	 * vblank). And unfortunately we can't use the interrupt to
> +	 * wait here since it will fire too soon. We could use the
> +	 * frame start interrupt instead since it will fire after
> the
> +	 * critical scanline, but that would require more changes
> +	 * in the interrupt code. So for now we'll just do the nasty
> +	 * thing and poll for the bad scanline to pass us by.
> +	 *
> +	 * FIXME figure out if BXT+ DSI suffers from this as well
> +	 */
> +	while (need_vlv_dsi_wa && scanline == vblank_start)
> +		scanline = intel_get_crtc_scanline(crtc);
> +
>  	crtc->debug.scanline_start = scanline;
>  	crtc->debug.start_vbl_time = ktime_get();
>  	crtc->debug.start_vbl_count =
> intel_crtc_get_vblank_counter(crtc);
-- 
Mika Kahola - Intel OTC

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx