[PATCH 1/3] drm/i915: set up PIPECONF explicitly on ilk-ivb

[PATCH 1/3] drm/i915: set up PIPECONF explicitly on ilk-ivb

[Daniel Vetter]

Dragging random garbage along from the BIOS isn't a good idea, since
we really only support exactly what we've set up.

In the specific case for the bug reporter the BIOS used the 10bit
gamma table, but since we only support an 8bit table the dark colors
ended up all wrong and the light ones all unadjusted.

Note that this has a nice implication for fastboot, it essentially
means that we have quite a bit more state to check and compare before
we can decide whether fastboot is possible.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=65593
Reported-and-Tested-by: Thomas Hebb <tommyhebb@gmail.com>
Cc: stable@vger.kernel.org
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 85e023f..01f26b03 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5298,9 +5298,8 @@ static void ironlake_set_pipeconf(struct drm_crtc *crtc)
 	int pipe = intel_crtc->pipe;
 	uint32_t val;
 
-	val = I915_READ(PIPECONF(pipe));
+	val = 0;
 
-	val &= ~PIPECONF_BPC_MASK;
 	switch (intel_crtc->config.pipe_bpp) {
 	case 18:
 		val |= PIPECONF_6BPC;
@@ -5319,11 +5318,9 @@ static void ironlake_set_pipeconf(struct drm_crtc *crtc)
 		BUG();
 	}
 
-	val &= ~(PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_MASK);
 	if (intel_crtc->config.dither)
 		val |= (PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_SP);
 
-	val &= ~PIPECONF_INTERLACE_MASK;
 	if (intel_crtc->config.adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
 		val |= PIPECONF_INTERLACED_ILK;
 	else
@@ -5331,8 +5328,6 @@ static void ironlake_set_pipeconf(struct drm_crtc *crtc)
 
 	if (intel_crtc->config.limited_color_range)
 		val |= PIPECONF_COLOR_RANGE_SELECT;
-	else
-		val &= ~PIPECONF_COLOR_RANGE_SELECT;
 
 	I915_WRITE(PIPECONF(pipe), val);
 	POSTING_READ(PIPECONF(pipe));
-- 
1.7.11.7

[PATCH 2/3] drm/i915: set up PIPECONF explicitly for i9xx/vlv platforms

[Daniel Vetter]

Same reasons as for the previous patch, just no bug report about
anything going wrong yet: We only support exactly the mode we program,
so don't leave any stale BIOS state behind.

Again this will be fun to properly track for fastboot.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 01f26b03..4ca0273 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4686,7 +4686,7 @@ static void i9xx_set_pipeconf(struct intel_crtc *intel_crtc)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t pipeconf;
 
-	pipeconf = I915_READ(PIPECONF(intel_crtc->pipe));
+	pipeconf = 0;
 
 	if (intel_crtc->pipe == 0 && INTEL_INFO(dev)->gen < 4) {
 		/* Enable pixel doubling when the dot clock is > 90% of the (display)
@@ -4698,15 +4698,10 @@ static void i9xx_set_pipeconf(struct intel_crtc *intel_crtc)
 		if (intel_crtc->config.requested_mode.clock >
 		    dev_priv->display.get_display_clock_speed(dev) * 9 / 10)
 			pipeconf |= PIPECONF_DOUBLE_WIDE;
-		else
-			pipeconf &= ~PIPECONF_DOUBLE_WIDE;
 	}
 
 	/* only g4x and later have fancy bpc/dither controls */
 	if (IS_G4X(dev) || IS_VALLEYVIEW(dev)) {
-		pipeconf &= ~(PIPECONF_BPC_MASK |
-			      PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_MASK);
-
 		/* Bspec claims that we can't use dithering for 30bpp pipes. */
 		if (intel_crtc->config.dither && intel_crtc->config.pipe_bpp != 30)
 			pipeconf |= PIPECONF_DITHER_EN |
@@ -4734,23 +4729,17 @@ static void i9xx_set_pipeconf(struct intel_crtc *intel_crtc)
 			pipeconf |= PIPECONF_CXSR_DOWNCLOCK;
 		} else {
 			DRM_DEBUG_KMS("disabling CxSR downclocking\n");
-			pipeconf &= ~PIPECONF_CXSR_DOWNCLOCK;
 		}
 	}
 
-	pipeconf &= ~PIPECONF_INTERLACE_MASK;
 	if (!IS_GEN2(dev) &&
 	    intel_crtc->config.adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
 		pipeconf |= PIPECONF_INTERLACE_W_FIELD_INDICATION;
 	else
 		pipeconf |= PIPECONF_PROGRESSIVE;
 
-	if (IS_VALLEYVIEW(dev)) {
-		if (intel_crtc->config.limited_color_range)
-			pipeconf |= PIPECONF_COLOR_RANGE_SELECT;
-		else
-			pipeconf &= ~PIPECONF_COLOR_RANGE_SELECT;
-	}
+	if (IS_VALLEYVIEW(dev) && intel_crtc->config.limited_color_range)
+		pipeconf |= PIPECONF_COLOR_RANGE_SELECT;
 
 	I915_WRITE(PIPECONF(intel_crtc->pipe), pipeconf);
 	POSTING_READ(PIPECONF(intel_crtc->pipe));
-- 
1.7.11.7

[PATCH 3/3] drm/i915: explicitly set up PIPECONF (and gamma table) on haswell

[Daniel Vetter]

Again we don't really support different settings, so don't let the
BIOS sneak stuff through.

Since the motivation for this patch series is to ensure we have the
correct gamma table mode selected also add the required write to the
GAMMA_MODE register to select the 8bit legacy table.

And since I find lowercase letters in #defines offensive, also
bikeshed those.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_reg.h      | 6 +++---
 drivers/gpu/drm/i915/intel_display.c | 7 ++++---
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 01e8783..8136b00 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3670,9 +3670,9 @@
 #define _GAMMA_MODE_B		0x4ac80
 #define GAMMA_MODE(pipe) _PIPE(pipe, _GAMMA_MODE_A, _GAMMA_MODE_B)
 #define GAMMA_MODE_MODE_MASK	(3 << 0)
-#define GAMMA_MODE_MODE_8bit	(0 << 0)
-#define GAMMA_MODE_MODE_10bit	(1 << 0)
-#define GAMMA_MODE_MODE_12bit	(2 << 0)
+#define GAMMA_MODE_MODE_8BIT	(0 << 0)
+#define GAMMA_MODE_MODE_10BIT	(1 << 0)
+#define GAMMA_MODE_MODE_12BIT	(2 << 0)
 #define GAMMA_MODE_MODE_SPLIT	(3 << 0)
 
 /* interrupts */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4ca0273..e1184eb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5393,13 +5393,11 @@ static void haswell_set_pipeconf(struct drm_crtc *crtc)
 	enum transcoder cpu_transcoder = intel_crtc->config.cpu_transcoder;
 	uint32_t val;
 
-	val = I915_READ(PIPECONF(cpu_transcoder));
+	val = 0;
 
-	val &= ~(PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_MASK);
 	if (intel_crtc->config.dither)
 		val |= (PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_SP);
 
-	val &= ~PIPECONF_INTERLACE_MASK_HSW;
 	if (intel_crtc->config.adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
 		val |= PIPECONF_INTERLACED_ILK;
 	else
@@ -5407,6 +5405,9 @@ static void haswell_set_pipeconf(struct drm_crtc *crtc)
 
 	I915_WRITE(PIPECONF(cpu_transcoder), val);
 	POSTING_READ(PIPECONF(cpu_transcoder));
+
+	I915_WRITE(GAMMA_MODE(intel_crtc->pipe), GAMMA_MODE_MODE_8BIT);
+	POSTING_READ(GAMMA_MODE(intel_crtc->pipe));
 }
 
 static bool ironlake_compute_clocks(struct drm_crtc *crtc,
-- 
1.7.11.7

Re: [PATCH 3/3] drm/i915: explicitly set up PIPECONF (and gamma table) on haswell

[Ville Syrjälä]

On Thu, Jun 13, 2013 at 12:54:59AM +0200, Daniel Vetter wrote:
> Again we don't really support different settings, so don't let the
> BIOS sneak stuff through.
> 
> Since the motivation for this patch series is to ensure we have the
> correct gamma table mode selected also add the required write to the
> GAMMA_MODE register to select the 8bit legacy table.
> 
> And since I find lowercase letters in #defines offensive, also
> bikeshed those.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      | 6 +++---
>  drivers/gpu/drm/i915/intel_display.c | 7 ++++---
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 01e8783..8136b00 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3670,9 +3670,9 @@
>  #define _GAMMA_MODE_B		0x4ac80
>  #define GAMMA_MODE(pipe) _PIPE(pipe, _GAMMA_MODE_A, _GAMMA_MODE_B)
>  #define GAMMA_MODE_MODE_MASK	(3 << 0)
> -#define GAMMA_MODE_MODE_8bit	(0 << 0)
> -#define GAMMA_MODE_MODE_10bit	(1 << 0)
> -#define GAMMA_MODE_MODE_12bit	(2 << 0)
> +#define GAMMA_MODE_MODE_8BIT	(0 << 0)
> +#define GAMMA_MODE_MODE_10BIT	(1 << 0)
> +#define GAMMA_MODE_MODE_12BIT	(2 << 0)
>  #define GAMMA_MODE_MODE_SPLIT	(3 << 0)
>  
>  /* interrupts */
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4ca0273..e1184eb 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5393,13 +5393,11 @@ static void haswell_set_pipeconf(struct drm_crtc *crtc)
>  	enum transcoder cpu_transcoder = intel_crtc->config.cpu_transcoder;
>  	uint32_t val;
>  
> -	val = I915_READ(PIPECONF(cpu_transcoder));
> +	val = 0;
>  
> -	val &= ~(PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_MASK);
>  	if (intel_crtc->config.dither)
>  		val |= (PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_SP);
>  
> -	val &= ~PIPECONF_INTERLACE_MASK_HSW;
>  	if (intel_crtc->config.adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
>  		val |= PIPECONF_INTERLACED_ILK;
>  	else
> @@ -5407,6 +5405,9 @@ static void haswell_set_pipeconf(struct drm_crtc *crtc)
>  
>  	I915_WRITE(PIPECONF(cpu_transcoder), val);
>  	POSTING_READ(PIPECONF(cpu_transcoder));
> +
> +	I915_WRITE(GAMMA_MODE(intel_crtc->pipe), GAMMA_MODE_MODE_8BIT);
> +	POSTING_READ(GAMMA_MODE(intel_crtc->pipe));

Why the POSTING_READ()? In fact, why do we have any posting reads in
xxx_set_pipeconf()?

Otherwise, for the series:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  }
>  
>  static bool ironlake_compute_clocks(struct drm_crtc *crtc,
> -- 
> 1.7.11.7
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] [PATCH 1/3] drm/i915: set up PIPECONF explicitly on ilk-ivb

[Chris Wilson]

On Thu, Jun 13, 2013 at 12:54:57AM +0200, Daniel Vetter wrote:
> Dragging random garbage along from the BIOS isn't a good idea, since
> we really only support exactly what we've set up.

And hopefully we will get failures in the BIOS -> KMS transition making
it much clearer what was missed. Rather than some random modeset in the
future where we switch pipes or outputs or configuration or the weather
changes.

There were some bits that we don't set in pipeconf (mostly for frame
offset and other esoteric features) that look like they might be used
one day, but as above, we already have problems if they are needed.

For the series,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

However, I don't think I would be so bold as to tag this for stable.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [Intel-gfx] [PATCH 1/3] drm/i915: set up PIPECONF explicitly on ilk-ivb

[Daniel Vetter]

On Thu, Jun 13, 2013 at 10:00:18AM +0100, Chris Wilson wrote:
> On Thu, Jun 13, 2013 at 12:54:57AM +0200, Daniel Vetter wrote:
> > Dragging random garbage along from the BIOS isn't a good idea, since
> > we really only support exactly what we've set up.
> 
> And hopefully we will get failures in the BIOS -> KMS transition making
> it much clearer what was missed. Rather than some random modeset in the
> future where we switch pipes or outputs or configuration or the weather
> changes.
> 
> There were some bits that we don't set in pipeconf (mostly for frame
> offset and other esoteric features) that look like they might be used
> one day, but as above, we already have problems if they are needed.
> 
> For the series,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Thanks for the review, all merged to dinq.

> However, I don't think I would be so bold as to tag this for stable.

Well I wouldn't still be maintainer without the occasional mad case of
stupid^Wbold ;-)

But since I've decided to push the bugfix through -next we should have
plenty of warning time in case it has a bad effect somewhere.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/3] drm/i915: explicitly set up PIPECONF (and gamma table) on haswell

[Daniel Vetter]

On Thu, Jun 13, 2013 at 10:57:52AM +0300, Ville Syrjälä wrote:
> On Thu, Jun 13, 2013 at 12:54:59AM +0200, Daniel Vetter wrote:
> > Again we don't really support different settings, so don't let the
> > BIOS sneak stuff through.
> > 
> > Since the motivation for this patch series is to ensure we have the
> > correct gamma table mode selected also add the required write to the
> > GAMMA_MODE register to select the 8bit legacy table.
> > 
> > And since I find lowercase letters in #defines offensive, also
> > bikeshed those.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h      | 6 +++---
> >  drivers/gpu/drm/i915/intel_display.c | 7 ++++---
> >  2 files changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 01e8783..8136b00 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -3670,9 +3670,9 @@
> >  #define _GAMMA_MODE_B		0x4ac80
> >  #define GAMMA_MODE(pipe) _PIPE(pipe, _GAMMA_MODE_A, _GAMMA_MODE_B)
> >  #define GAMMA_MODE_MODE_MASK	(3 << 0)
> > -#define GAMMA_MODE_MODE_8bit	(0 << 0)
> > -#define GAMMA_MODE_MODE_10bit	(1 << 0)
> > -#define GAMMA_MODE_MODE_12bit	(2 << 0)
> > +#define GAMMA_MODE_MODE_8BIT	(0 << 0)
> > +#define GAMMA_MODE_MODE_10BIT	(1 << 0)
> > +#define GAMMA_MODE_MODE_12BIT	(2 << 0)
> >  #define GAMMA_MODE_MODE_SPLIT	(3 << 0)
> >  
> >  /* interrupts */
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 4ca0273..e1184eb 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5393,13 +5393,11 @@ static void haswell_set_pipeconf(struct drm_crtc *crtc)
> >  	enum transcoder cpu_transcoder = intel_crtc->config.cpu_transcoder;
> >  	uint32_t val;
> >  
> > -	val = I915_READ(PIPECONF(cpu_transcoder));
> > +	val = 0;
> >  
> > -	val &= ~(PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_MASK);
> >  	if (intel_crtc->config.dither)
> >  		val |= (PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_SP);
> >  
> > -	val &= ~PIPECONF_INTERLACE_MASK_HSW;
> >  	if (intel_crtc->config.adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
> >  		val |= PIPECONF_INTERLACED_ILK;
> >  	else
> > @@ -5407,6 +5405,9 @@ static void haswell_set_pipeconf(struct drm_crtc *crtc)
> >  
> >  	I915_WRITE(PIPECONF(cpu_transcoder), val);
> >  	POSTING_READ(PIPECONF(cpu_transcoder));
> > +
> > +	I915_WRITE(GAMMA_MODE(intel_crtc->pipe), GAMMA_MODE_MODE_8BIT);
> > +	POSTING_READ(GAMMA_MODE(intel_crtc->pipe));
> 
> Why the POSTING_READ()? In fact, why do we have any posting reads in
> xxx_set_pipeconf()?

Cargo-culting at its best ;-) But I think cleaning that up is best left to
another series ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch