[PATCH 0/3] drm/i915: Some vsyncshift fixes

[PATCH 0/3] drm/i915: Some vsyncshift fixes

[ville.syrjala]

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

While reviewing Jesse's VLV vsyncshift patch I noticed we're not really
programming it correctly in many cases. This series aims to fix that.

Ville Syrjälä (3):
  drm/i915: Program VSYNCSHIFT in a more consistent manner
  drm/i915: Fix the interlace mode selection for gmch platforms
  drm/i915: Make sure vsyncshift is positive

 drivers/gpu/drm/i915/intel_display.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

-- 
1.8.3.2

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

[PATCH 1/3] drm/i915: Program VSYNCSHIFT in a more consistent manner

[ville.syrjala]

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

When interlaced sdvo output is used, vsyncshift should supposedly
be (htotal-1)/2. In reality PIPECONF/TRANSCONF will override it by
using the legacy vsyncshift interlace mode which causes the hardware
to ignore the VSYNCSHIFT register.

The only odd thing here is that on PCH platforms we program the
VSYNCSHIFT on both CPU and PCH, and it's not entirely clear if both
sides have to agree on the value or not. On the CPU side there's no
way to override the value via PIPECONF anymore, so if we want to make
the CPU side agree with the PCH side, we should probably program the
approriate value into VSYNCSHIFT manually. So let's do that, but for
now leave the PCH side to still use the legacy interlace mode in
TRANSCONF.

We can also drop the gen2 check since gen2 doesn't support interlaced
modes at all.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d633139..a9a4f6a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5377,21 +5377,23 @@ static void intel_set_pipe_timings(struct intel_crtc *intel_crtc)
 	enum transcoder cpu_transcoder = intel_crtc->config.cpu_transcoder;
 	struct drm_display_mode *adjusted_mode =
 		&intel_crtc->config.adjusted_mode;
-	uint32_t vsyncshift, crtc_vtotal, crtc_vblank_end;
+	uint32_t vsyncshift = 0, crtc_vtotal, crtc_vblank_end;
 
 	/* We need to be careful not to changed the adjusted mode, for otherwise
 	 * the hw state checker will get angry at the mismatch. */
 	crtc_vtotal = adjusted_mode->crtc_vtotal;
 	crtc_vblank_end = adjusted_mode->crtc_vblank_end;
 
-	if (!IS_GEN2(dev) && adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) {
+	if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) {
 		/* the chip adds 2 halflines automatically */
 		crtc_vtotal -= 1;
 		crtc_vblank_end -= 1;
-		vsyncshift = adjusted_mode->crtc_hsync_start
-			     - adjusted_mode->crtc_htotal / 2;
-	} else {
-		vsyncshift = 0;
+
+		if (intel_pipe_has_type(&intel_crtc->base, INTEL_OUTPUT_SDVO))
+			vsyncshift = (adjusted_mode->crtc_htotal - 1) / 2;
+		else
+			vsyncshift = adjusted_mode->crtc_hsync_start -
+				adjusted_mode->crtc_htotal / 2;
 	}
 
 	if (INTEL_INFO(dev)->gen > 3)
-- 
1.8.3.2

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

Re: [PATCH 1/3] drm/i915: Program VSYNCSHIFT in a more consistent manner

[Jesse Barnes]

On Fri, 28 Mar 2014 23:29:30 +0200
ville.syrjala@linux.intel.com wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> When interlaced sdvo output is used, vsyncshift should supposedly
> be (htotal-1)/2. In reality PIPECONF/TRANSCONF will override it by
> using the legacy vsyncshift interlace mode which causes the hardware
> to ignore the VSYNCSHIFT register.
> 
> The only odd thing here is that on PCH platforms we program the
> VSYNCSHIFT on both CPU and PCH, and it's not entirely clear if both
> sides have to agree on the value or not. On the CPU side there's no
> way to override the value via PIPECONF anymore, so if we want to make
> the CPU side agree with the PCH side, we should probably program the
> approriate value into VSYNCSHIFT manually. So let's do that, but for
> now leave the PCH side to still use the legacy interlace mode in
> TRANSCONF.
> 
> We can also drop the gen2 check since gen2 doesn't support interlaced
> modes at all.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d633139..a9a4f6a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5377,21 +5377,23 @@ static void intel_set_pipe_timings(struct intel_crtc *intel_crtc)
>  	enum transcoder cpu_transcoder = intel_crtc->config.cpu_transcoder;
>  	struct drm_display_mode *adjusted_mode =
>  		&intel_crtc->config.adjusted_mode;
> -	uint32_t vsyncshift, crtc_vtotal, crtc_vblank_end;
> +	uint32_t vsyncshift = 0, crtc_vtotal, crtc_vblank_end;
>  
>  	/* We need to be careful not to changed the adjusted mode, for otherwise
>  	 * the hw state checker will get angry at the mismatch. */
>  	crtc_vtotal = adjusted_mode->crtc_vtotal;
>  	crtc_vblank_end = adjusted_mode->crtc_vblank_end;
>  
> -	if (!IS_GEN2(dev) && adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) {
> +	if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) {
>  		/* the chip adds 2 halflines automatically */
>  		crtc_vtotal -= 1;
>  		crtc_vblank_end -= 1;
> -		vsyncshift = adjusted_mode->crtc_hsync_start
> -			     - adjusted_mode->crtc_htotal / 2;
> -	} else {
> -		vsyncshift = 0;
> +
> +		if (intel_pipe_has_type(&intel_crtc->base, INTEL_OUTPUT_SDVO))
> +			vsyncshift = (adjusted_mode->crtc_htotal - 1) / 2;
> +		else
> +			vsyncshift = adjusted_mode->crtc_hsync_start -
> +				adjusted_mode->crtc_htotal / 2;
>  	}
>  
>  	if (INTEL_INFO(dev)->gen > 3)

My only concern here is that some chip might try to use a nonzero
vsyncshift for a non-interlaced mode.  But that should be easy to
bisect to if so, so

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/3] drm/i915: Fix the interlace mode selection for gmch platforms

[ville.syrjala]

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

PIPECONF_INTERLACE_W_FIELD_INDICATION is only meant to be used for sdvo
since it implies a slightly weird vsync shift of htotal/2. For everything
else we should use PIPECONF_INTERLACE_W_SYNC_SHIFT and let the value in
the VSYNCSHIFT register take effect.

The only exception is gen3 simply because VSYNCSHIFT didn't exist yet.
Gen2 doesn't support interlaced modes at all, so we can drop the
explicit gen2 checks.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a9a4f6a..3ab40e3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5543,13 +5543,13 @@ static void i9xx_set_pipeconf(struct intel_crtc *intel_crtc)
 		}
 	}
 
-	if (IS_VALLEYVIEW(dev) &&
-	    intel_crtc->config.adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
-		pipeconf |= PIPECONF_INTERLACE_W_SYNC_SHIFT;
-	else if (!IS_GEN2(dev) &&
-		 intel_crtc->config.adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
-		pipeconf |= PIPECONF_INTERLACE_W_FIELD_INDICATION;
-	else
+	if (intel_crtc->config.adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE) {
+		if (INTEL_INFO(dev)->gen < 4 ||
+		    intel_pipe_has_type(&intel_crtc->base, INTEL_OUTPUT_SDVO))
+			pipeconf |= PIPECONF_INTERLACE_W_FIELD_INDICATION;
+		else
+			pipeconf |= PIPECONF_INTERLACE_W_SYNC_SHIFT;
+	} else
 		pipeconf |= PIPECONF_PROGRESSIVE;
 
 	if (IS_VALLEYVIEW(dev) && intel_crtc->config.limited_color_range)
-- 
1.8.3.2

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

Re: [PATCH 2/3] drm/i915: Fix the interlace mode selection for gmch platforms

[Jesse Barnes]

On Fri, 28 Mar 2014 23:29:31 +0200
ville.syrjala@linux.intel.com wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> PIPECONF_INTERLACE_W_FIELD_INDICATION is only meant to be used for sdvo
> since it implies a slightly weird vsync shift of htotal/2. For everything
> else we should use PIPECONF_INTERLACE_W_SYNC_SHIFT and let the value in
> the VSYNCSHIFT register take effect.
> 
> The only exception is gen3 simply because VSYNCSHIFT didn't exist yet.
> Gen2 doesn't support interlaced modes at all, so we can drop the
> explicit gen2 checks.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a9a4f6a..3ab40e3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5543,13 +5543,13 @@ static void i9xx_set_pipeconf(struct intel_crtc *intel_crtc)
>  		}
>  	}
>  
> -	if (IS_VALLEYVIEW(dev) &&
> -	    intel_crtc->config.adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
> -		pipeconf |= PIPECONF_INTERLACE_W_SYNC_SHIFT;
> -	else if (!IS_GEN2(dev) &&
> -		 intel_crtc->config.adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
> -		pipeconf |= PIPECONF_INTERLACE_W_FIELD_INDICATION;
> -	else
> +	if (intel_crtc->config.adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE) {
> +		if (INTEL_INFO(dev)->gen < 4 ||
> +		    intel_pipe_has_type(&intel_crtc->base, INTEL_OUTPUT_SDVO))
> +			pipeconf |= PIPECONF_INTERLACE_W_FIELD_INDICATION;
> +		else
> +			pipeconf |= PIPECONF_INTERLACE_W_SYNC_SHIFT;
> +	} else
>  		pipeconf |= PIPECONF_PROGRESSIVE;
>  
>  	if (IS_VALLEYVIEW(dev) && intel_crtc->config.limited_color_range)

Hooray for SDVO.  I really hope no one tries to do that on VLV...
(afaik it's unsupported but who knows the hw might work if someone
solders such a board together).

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/3] drm/i915: Make sure vsyncshift is positive

[ville.syrjala]

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

If vsyncshift comes out as negative, add one htotal to it to get the
corresponding positive value.

This is rather theoretical as it would require a mode where the
hsync+back porch is very long and the active+front porch very short.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3ab40e3..7d3b18b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5377,7 +5377,8 @@ static void intel_set_pipe_timings(struct intel_crtc *intel_crtc)
 	enum transcoder cpu_transcoder = intel_crtc->config.cpu_transcoder;
 	struct drm_display_mode *adjusted_mode =
 		&intel_crtc->config.adjusted_mode;
-	uint32_t vsyncshift = 0, crtc_vtotal, crtc_vblank_end;
+	uint32_t crtc_vtotal, crtc_vblank_end;
+	int vsyncshift = 0;
 
 	/* We need to be careful not to changed the adjusted mode, for otherwise
 	 * the hw state checker will get angry at the mismatch. */
@@ -5394,6 +5395,8 @@ static void intel_set_pipe_timings(struct intel_crtc *intel_crtc)
 		else
 			vsyncshift = adjusted_mode->crtc_hsync_start -
 				adjusted_mode->crtc_htotal / 2;
+		if (vsyncshift < 0)
+			vsyncshift += adjusted_mode->crtc_htotal;
 	}
 
 	if (INTEL_INFO(dev)->gen > 3)
-- 
1.8.3.2

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

Re: [PATCH 3/3] drm/i915: Make sure vsyncshift is positive

[Jesse Barnes]

On Fri, 28 Mar 2014 23:29:32 +0200
ville.syrjala@linux.intel.com wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> If vsyncshift comes out as negative, add one htotal to it to get the
> corresponding positive value.
> 
> This is rather theoretical as it would require a mode where the
> hsync+back porch is very long and the active+front porch very short.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3ab40e3..7d3b18b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5377,7 +5377,8 @@ static void intel_set_pipe_timings(struct intel_crtc *intel_crtc)
>  	enum transcoder cpu_transcoder = intel_crtc->config.cpu_transcoder;
>  	struct drm_display_mode *adjusted_mode =
>  		&intel_crtc->config.adjusted_mode;
> -	uint32_t vsyncshift = 0, crtc_vtotal, crtc_vblank_end;
> +	uint32_t crtc_vtotal, crtc_vblank_end;
> +	int vsyncshift = 0;
>  
>  	/* We need to be careful not to changed the adjusted mode, for otherwise
>  	 * the hw state checker will get angry at the mismatch. */
> @@ -5394,6 +5395,8 @@ static void intel_set_pipe_timings(struct intel_crtc *intel_crtc)
>  		else
>  			vsyncshift = adjusted_mode->crtc_hsync_start -
>  				adjusted_mode->crtc_htotal / 2;
> +		if (vsyncshift < 0)
> +			vsyncshift += adjusted_mode->crtc_htotal;
>  	}
>  
>  	if (INTEL_INFO(dev)->gen > 3)


Funky indeed.  I wonder if we should congratulate the user if we detect
this configuration.  "Achievement unlocked: funky mode timings!".

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Make sure vsyncshift is positive

[Daniel Vetter]

On Fri, Mar 28, 2014 at 03:11:55PM -0700, Jesse Barnes wrote:
> On Fri, 28 Mar 2014 23:29:32 +0200
> ville.syrjala@linux.intel.com wrote:
> 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > If vsyncshift comes out as negative, add one htotal to it to get the
> > corresponding positive value.
> > 
> > This is rather theoretical as it would require a mode where the
> > hsync+back porch is very long and the active+front porch very short.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 3ab40e3..7d3b18b 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5377,7 +5377,8 @@ static void intel_set_pipe_timings(struct intel_crtc *intel_crtc)
> >  	enum transcoder cpu_transcoder = intel_crtc->config.cpu_transcoder;
> >  	struct drm_display_mode *adjusted_mode =
> >  		&intel_crtc->config.adjusted_mode;
> > -	uint32_t vsyncshift = 0, crtc_vtotal, crtc_vblank_end;
> > +	uint32_t crtc_vtotal, crtc_vblank_end;
> > +	int vsyncshift = 0;
> >  
> >  	/* We need to be careful not to changed the adjusted mode, for otherwise
> >  	 * the hw state checker will get angry at the mismatch. */
> > @@ -5394,6 +5395,8 @@ static void intel_set_pipe_timings(struct intel_crtc *intel_crtc)
> >  		else
> >  			vsyncshift = adjusted_mode->crtc_hsync_start -
> >  				adjusted_mode->crtc_htotal / 2;
> > +		if (vsyncshift < 0)
> > +			vsyncshift += adjusted_mode->crtc_htotal;
> >  	}
> >  
> >  	if (INTEL_INFO(dev)->gen > 3)
> 
> 
> Funky indeed.  I wonder if we should congratulate the user if we detect
> this configuration.  "Achievement unlocked: funky mode timings!".

Would be a neat bounty-hunt sprinkling this all over ...
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

All 3 merged, thanks for patches&review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch