[PATCH v2 1/5] drm/i915: Correctly handle limited range YCbCr data on VLV/CHV

[PATCH v2 1/5] drm/i915: Correctly handle limited range YCbCr data on VLV/CHV

[ville.syrjala]

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

Turns out the VLV/CHV fixed function sprite CSC expects full range
data as input. We've been feeding it limited range data to it all
along. To expand the data out to full range we'll use the color
correction registers (brightness, contrast, and saturation).

On CHV pipe B we were actually doing the right thing already because we
progammed the custom CSC matrix to do expect limited range input. Now
that well pre-expand the data out with the color correction unit, we
need to change the CSC matrix to operate with full range input instead.

This should make the sprite output of the other pipes match the sprite
output of pipe B reasonably well. Looking at the resulting pipe CRCs,
there can be a slight difference in the output, but as I don't know
the formula used by the fixed function CSC of the other pipes, I don't
think it's worth the effort to try to match the output exactly. It
might not even be possible due to difference in internal precision etc.

One slight caveat here is that the color correction registers are single
bufferred, so we should really be updating them during vblank, but we
still don't have a mechanism for that, so just toss in another FIXME.

v2: Rebase

Cc: Jyri Sarha <jsarha@ti.com>
Cc: "Tang, Jun" <jun.tang@intel.com>
Reported-by: "Tang, Jun" <jun.tang@intel.com>
Cc: stable@vger.kernel.org
Fixes: 7f1f3851feb0 ("drm/i915: sprite support for ValleyView v4")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h     | 10 +++++
 drivers/gpu/drm/i915/intel_sprite.c | 74 ++++++++++++++++++++++++++++---------
 2 files changed, 66 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index b6d69e289974..ce7c0dc79cf7 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5777,6 +5777,12 @@ enum {
 #define _SPATILEOFF		(VLV_DISPLAY_BASE + 0x721a4)
 #define _SPACONSTALPHA		(VLV_DISPLAY_BASE + 0x721a8)
 #define   SP_CONST_ALPHA_ENABLE		(1<<31)
+#define _SPACLRC0		(VLV_DISPLAY_BASE + 0x721d0)
+#define   SP_CONTRAST(x)		((x) << 18) /* u3.6 */
+#define   SP_BRIGHTNESS(x)		((x) & 0xff) /* s8 */
+#define _SPACLRC1		(VLV_DISPLAY_BASE + 0x721d4)
+#define   SP_SH_SIN(x)			(((x) & 0x7ff) << 16) /* s4.7 */
+#define   SP_SH_COS(x)			(x) /* u3.7 */
 #define _SPAGAMC		(VLV_DISPLAY_BASE + 0x721f4)
 
 #define _SPBCNTR		(VLV_DISPLAY_BASE + 0x72280)
@@ -5790,6 +5796,8 @@ enum {
 #define _SPBKEYMAXVAL		(VLV_DISPLAY_BASE + 0x722a0)
 #define _SPBTILEOFF		(VLV_DISPLAY_BASE + 0x722a4)
 #define _SPBCONSTALPHA		(VLV_DISPLAY_BASE + 0x722a8)
+#define _SPBCLRC0		(VLV_DISPLAY_BASE + 0x722d0)
+#define _SPBCLRC1		(VLV_DISPLAY_BASE + 0x722d4)
 #define _SPBGAMC		(VLV_DISPLAY_BASE + 0x722f4)
 
 #define _MMIO_VLV_SPR(pipe, plane_id, reg_a, reg_b) \
@@ -5806,6 +5814,8 @@ enum {
 #define SPKEYMAXVAL(pipe, plane_id)	_MMIO_VLV_SPR((pipe), (plane_id), _SPAKEYMAXVAL, _SPBKEYMAXVAL)
 #define SPTILEOFF(pipe, plane_id)	_MMIO_VLV_SPR((pipe), (plane_id), _SPATILEOFF, _SPBTILEOFF)
 #define SPCONSTALPHA(pipe, plane_id)	_MMIO_VLV_SPR((pipe), (plane_id), _SPACONSTALPHA, _SPBCONSTALPHA)
+#define SPCLRC0(pipe, plane_id)		_MMIO_VLV_SPR((pipe), (plane_id), _SPACLRC0, _SPBCLRC0)
+#define SPCLRC1(pipe, plane_id)		_MMIO_VLV_SPR((pipe), (plane_id), _SPACLRC1, _SPBCLRC1)
 #define SPGAMC(pipe, plane_id)		_MMIO_VLV_SPR((pipe), (plane_id), _SPAGAMC, _SPBGAMC)
 
 /*
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 0c650c2cbca8..2ce3b3c6ffa8 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -325,44 +325,80 @@ skl_disable_plane(struct intel_plane *plane, struct intel_crtc *crtc)
 }
 
 static void
-chv_update_csc(struct intel_plane *plane, uint32_t format)
+chv_update_csc(const struct intel_plane_state *plane_state)
 {
+	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
 	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
+	const struct drm_framebuffer *fb = plane_state->base.fb;
 	enum plane_id plane_id = plane->id;
 
 	/* Seems RGB data bypasses the CSC always */
-	if (!format_is_yuv(format))
+	if (!format_is_yuv(fb->format->format))
 		return;
 
 	/*
-	 * BT.601 limited range YCbCr -> full range RGB
+	 * BT.601 full range YCbCr -> full range RGB
 	 *
-	 * |r|   | 6537 4769     0|   |cr  |
-	 * |g| = |-3330 4769 -1605| x |y-64|
-	 * |b|   |    0 4769  8263|   |cb  |
+	 * |r|   | 5743 4096     0|   |cr|
+	 * |g| = |-2925 4096 -1410| x |y |
+	 * |b|   |    0 4096  7258|   |cb|
 	 *
-	 * Cb and Cr apparently come in as signed already, so no
-	 * need for any offset. For Y we need to remove the offset.
+	 * Cb and Cr apparently come in as signed already,
+	 * and we get full range data in on account of CLRC0/1
 	 */
-	I915_WRITE_FW(SPCSCYGOFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(-64));
+	I915_WRITE_FW(SPCSCYGOFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(0));
 	I915_WRITE_FW(SPCSCCBOFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(0));
 	I915_WRITE_FW(SPCSCCROFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(0));
 
-	I915_WRITE_FW(SPCSCC01(plane_id), SPCSC_C1(4769) | SPCSC_C0(6537));
-	I915_WRITE_FW(SPCSCC23(plane_id), SPCSC_C1(-3330) | SPCSC_C0(0));
-	I915_WRITE_FW(SPCSCC45(plane_id), SPCSC_C1(-1605) | SPCSC_C0(4769));
-	I915_WRITE_FW(SPCSCC67(plane_id), SPCSC_C1(4769) | SPCSC_C0(0));
-	I915_WRITE_FW(SPCSCC8(plane_id), SPCSC_C0(8263));
+	I915_WRITE_FW(SPCSCC01(plane_id), SPCSC_C1(4096) | SPCSC_C0(5743));
+	I915_WRITE_FW(SPCSCC23(plane_id), SPCSC_C1(-2925) | SPCSC_C0(0));
+	I915_WRITE_FW(SPCSCC45(plane_id), SPCSC_C1(-1410) | SPCSC_C0(4096));
+	I915_WRITE_FW(SPCSCC67(plane_id), SPCSC_C1(4096) | SPCSC_C0(0));
+	I915_WRITE_FW(SPCSCC8(plane_id), SPCSC_C0(7258));
 
-	I915_WRITE_FW(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(940) | SPCSC_IMIN(64));
-	I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
-	I915_WRITE_FW(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
+	I915_WRITE_FW(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(1023) | SPCSC_IMIN(0));
+	I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
+	I915_WRITE_FW(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
 
 	I915_WRITE_FW(SPCSCYGOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
 	I915_WRITE_FW(SPCSCCBOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
 	I915_WRITE_FW(SPCSCCROCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
 }
 
+static void
+vlv_update_clrc(const struct intel_plane_state *plane_state)
+{
+	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
+	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
+	const struct drm_framebuffer *fb = plane_state->base.fb;
+	enum pipe pipe = plane->pipe;
+	enum plane_id plane_id = plane->id;
+	int con, bri, sh_sin, sh_cos;
+
+	if (format_is_yuv(fb->format->format)) {
+		/*
+		 * expand limited range to full range.
+		 * contrast is applied first, then brightness
+		 */
+		con = ((255 << 7) / 219 + 1) >> 1;
+		bri = -((16 << 1) * 255 / 219 + 1) >> 1;
+		sh_sin = 0;
+		sh_cos = (((128 << 8) / 112) + 1) >> 1;
+	} else {
+		/* pass-through everything */
+		con = 1 << 6;
+		bri = 0;
+		sh_sin = 0;
+		sh_cos = 1 << 7;
+	}
+
+	/* FIXME these register are single buffered :( */
+	I915_WRITE_FW(SPCLRC0(pipe, plane_id),
+		      SP_CONTRAST(con) | SP_BRIGHTNESS(bri));
+	I915_WRITE_FW(SPCLRC1(pipe, plane_id),
+		      SP_SH_SIN(sh_sin) | SP_SH_COS(sh_cos));
+}
+
 static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
 			  const struct intel_plane_state *plane_state)
 {
@@ -456,8 +492,10 @@ vlv_update_plane(struct intel_plane *plane,
 
 	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
 
+	vlv_update_clrc(plane_state);
+
 	if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B)
-		chv_update_csc(plane, fb->format->format);
+		chv_update_csc(plane_state);
 
 	if (key->flags) {
 		I915_WRITE_FW(SPKEYMINVAL(pipe, plane_id), key->min_value);
-- 
2.13.0

Re: [PATCH v2 1/5] drm/i915: Correctly handle limited range YCbCr data on VLV/CHV

[Sharma, Shashank]

Regards

Shashank


On 6/9/2017 2:03 AM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Turns out the VLV/CHV fixed function sprite CSC expects full range
> data as input. We've been feeding it limited range data to it all
> along. To expand the data out to full range we'll use the color
> correction registers (brightness, contrast, and saturation).
>
> On CHV pipe B we were actually doing the right thing already because we
> progammed the custom CSC matrix to do expect limited range input. Now
> that well pre-expand the data out with the color correction unit, we
> need to change the CSC matrix to operate with full range input instead.
>
> This should make the sprite output of the other pipes match the sprite
> output of pipe B reasonably well. Looking at the resulting pipe CRCs,
> there can be a slight difference in the output, but as I don't know
> the formula used by the fixed function CSC of the other pipes, I don't
> think it's worth the effort to try to match the output exactly. It
> might not even be possible due to difference in internal precision etc.
>
> One slight caveat here is that the color correction registers are single
> bufferred, so we should really be updating them during vblank, but we
> still don't have a mechanism for that, so just toss in another FIXME.
>
> v2: Rebase
>
> Cc: Jyri Sarha <jsarha@ti.com>
> Cc: "Tang, Jun" <jun.tang@intel.com>
> Reported-by: "Tang, Jun" <jun.tang@intel.com>
> Cc: stable@vger.kernel.org
> Fixes: 7f1f3851feb0 ("drm/i915: sprite support for ValleyView v4")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_reg.h     | 10 +++++
>   drivers/gpu/drm/i915/intel_sprite.c | 74 ++++++++++++++++++++++++++++---------
>   2 files changed, 66 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index b6d69e289974..ce7c0dc79cf7 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5777,6 +5777,12 @@ enum {
>   #define _SPATILEOFF		(VLV_DISPLAY_BASE + 0x721a4)
>   #define _SPACONSTALPHA		(VLV_DISPLAY_BASE + 0x721a8)
>   #define   SP_CONST_ALPHA_ENABLE		(1<<31)
> +#define _SPACLRC0		(VLV_DISPLAY_BASE + 0x721d0)
> +#define   SP_CONTRAST(x)		((x) << 18) /* u3.6 */
> +#define   SP_BRIGHTNESS(x)		((x) & 0xff) /* s8 */
> +#define _SPACLRC1		(VLV_DISPLAY_BASE + 0x721d4)
> +#define   SP_SH_SIN(x)			(((x) & 0x7ff) << 16) /* s4.7 */
> +#define   SP_SH_COS(x)			(x) /* u3.7 */
>   #define _SPAGAMC		(VLV_DISPLAY_BASE + 0x721f4)
>   
>   #define _SPBCNTR		(VLV_DISPLAY_BASE + 0x72280)
> @@ -5790,6 +5796,8 @@ enum {
>   #define _SPBKEYMAXVAL		(VLV_DISPLAY_BASE + 0x722a0)
>   #define _SPBTILEOFF		(VLV_DISPLAY_BASE + 0x722a4)
>   #define _SPBCONSTALPHA		(VLV_DISPLAY_BASE + 0x722a8)
> +#define _SPBCLRC0		(VLV_DISPLAY_BASE + 0x722d0)
> +#define _SPBCLRC1		(VLV_DISPLAY_BASE + 0x722d4)
>   #define _SPBGAMC		(VLV_DISPLAY_BASE + 0x722f4)
>   
>   #define _MMIO_VLV_SPR(pipe, plane_id, reg_a, reg_b) \
> @@ -5806,6 +5814,8 @@ enum {
>   #define SPKEYMAXVAL(pipe, plane_id)	_MMIO_VLV_SPR((pipe), (plane_id), _SPAKEYMAXVAL, _SPBKEYMAXVAL)
>   #define SPTILEOFF(pipe, plane_id)	_MMIO_VLV_SPR((pipe), (plane_id), _SPATILEOFF, _SPBTILEOFF)
>   #define SPCONSTALPHA(pipe, plane_id)	_MMIO_VLV_SPR((pipe), (plane_id), _SPACONSTALPHA, _SPBCONSTALPHA)
> +#define SPCLRC0(pipe, plane_id)		_MMIO_VLV_SPR((pipe), (plane_id), _SPACLRC0, _SPBCLRC0)
> +#define SPCLRC1(pipe, plane_id)		_MMIO_VLV_SPR((pipe), (plane_id), _SPACLRC1, _SPBCLRC1)
>   #define SPGAMC(pipe, plane_id)		_MMIO_VLV_SPR((pipe), (plane_id), _SPAGAMC, _SPBGAMC)
>   
>   /*
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 0c650c2cbca8..2ce3b3c6ffa8 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -325,44 +325,80 @@ skl_disable_plane(struct intel_plane *plane, struct intel_crtc *crtc)
>   }
>   
>   static void
> -chv_update_csc(struct intel_plane *plane, uint32_t format)
> +chv_update_csc(const struct intel_plane_state *plane_state)
>   {
> +	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
>   	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> +	const struct drm_framebuffer *fb = plane_state->base.fb;
>   	enum plane_id plane_id = plane->id;
>   
>   	/* Seems RGB data bypasses the CSC always */
> -	if (!format_is_yuv(format))
> +	if (!format_is_yuv(fb->format->format))
>   		return;
>   
>   	/*
> -	 * BT.601 limited range YCbCr -> full range RGB
> +	 * BT.601 full range YCbCr -> full range RGB
>   	 *
> -	 * |r|   | 6537 4769     0|   |cr  |
> -	 * |g| = |-3330 4769 -1605| x |y-64|
> -	 * |b|   |    0 4769  8263|   |cb  |
> +	 * |r|   | 5743 4096     0|   |cr|
> +	 * |g| = |-2925 4096 -1410| x |y |
> +	 * |b|   |    0 4096  7258|   |cb|
>   	 *
> -	 * Cb and Cr apparently come in as signed already, so no
> -	 * need for any offset. For Y we need to remove the offset.
> +	 * Cb and Cr apparently come in as signed already,
> +	 * and we get full range data in on account of CLRC0/1
>   	 */
> -	I915_WRITE_FW(SPCSCYGOFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(-64));
> +	I915_WRITE_FW(SPCSCYGOFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(0));
>   	I915_WRITE_FW(SPCSCCBOFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(0));
>   	I915_WRITE_FW(SPCSCCROFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(0));
>   
> -	I915_WRITE_FW(SPCSCC01(plane_id), SPCSC_C1(4769) | SPCSC_C0(6537));
> -	I915_WRITE_FW(SPCSCC23(plane_id), SPCSC_C1(-3330) | SPCSC_C0(0));
> -	I915_WRITE_FW(SPCSCC45(plane_id), SPCSC_C1(-1605) | SPCSC_C0(4769));
> -	I915_WRITE_FW(SPCSCC67(plane_id), SPCSC_C1(4769) | SPCSC_C0(0));
> -	I915_WRITE_FW(SPCSCC8(plane_id), SPCSC_C0(8263));
> +	I915_WRITE_FW(SPCSCC01(plane_id), SPCSC_C1(4096) | SPCSC_C0(5743));
> +	I915_WRITE_FW(SPCSCC23(plane_id), SPCSC_C1(-2925) | SPCSC_C0(0));
> +	I915_WRITE_FW(SPCSCC45(plane_id), SPCSC_C1(-1410) | SPCSC_C0(4096));
> +	I915_WRITE_FW(SPCSCC67(plane_id), SPCSC_C1(4096) | SPCSC_C0(0));
> +	I915_WRITE_FW(SPCSCC8(plane_id), SPCSC_C0(7258));
>   
> -	I915_WRITE_FW(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(940) | SPCSC_IMIN(64));
> -	I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
> -	I915_WRITE_FW(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
> +	I915_WRITE_FW(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(1023) | SPCSC_IMIN(0));
> +	I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
> +	I915_WRITE_FW(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
>   
>   	I915_WRITE_FW(SPCSCYGOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
>   	I915_WRITE_FW(SPCSCCBOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
>   	I915_WRITE_FW(SPCSCCROCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
>   }
>   
> +static void
> +vlv_update_clrc(const struct intel_plane_state *plane_state)
> +{
> +	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> +	const struct drm_framebuffer *fb = plane_state->base.fb;
> +	enum pipe pipe = plane->pipe;
> +	enum plane_id plane_id = plane->id;
> +	int con, bri, sh_sin, sh_cos;
> +
con = contrast bri = brightness for better reading ?
> +	if (format_is_yuv(fb->format->format)) {
> +		/*
> +		 * expand limited range to full range.
> +		 * contrast is applied first, then brightness
> +		 */
> +		con = ((255 << 7) / 219 + 1) >> 1;
> +		bri = -((16 << 1) * 255 / 219 + 1) >> 1;
> +		sh_sin = 0;
> +		sh_cos = (((128 << 8) / 112) + 1) >> 1;
> +	} else {
> +		/* pass-through everything */
> +		con = 1 << 6;
> +		bri = 0;
> +		sh_sin = 0;
> +		sh_cos = 1 << 7;
> +	}
contrast and brightness are mostly used for color / display correction. 
Would it be better to create a
plane level property for the same, and attach into color management 
framework ? In this way, userspace
can also play around.

- Shashank
> +
> +	/* FIXME these register are single buffered :( */
> +	I915_WRITE_FW(SPCLRC0(pipe, plane_id),
> +		      SP_CONTRAST(con) | SP_BRIGHTNESS(bri));
> +	I915_WRITE_FW(SPCLRC1(pipe, plane_id),
> +		      SP_SH_SIN(sh_sin) | SP_SH_COS(sh_cos));
> +}
> +
>   static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
>   			  const struct intel_plane_state *plane_state)
>   {
> @@ -456,8 +492,10 @@ vlv_update_plane(struct intel_plane *plane,
>   
>   	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>   
> +	vlv_update_clrc(plane_state);
> +
>   	if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B)
> -		chv_update_csc(plane, fb->format->format);
> +		chv_update_csc(plane_state);
>   
>   	if (key->flags) {
>   		I915_WRITE_FW(SPKEYMINVAL(pipe, plane_id), key->min_value);

Re: [PATCH v2 1/5] drm/i915: Correctly handle limited range YCbCr data on VLV/CHV

[Ville Syrjälä]

On Thu, Jun 15, 2017 at 06:08:43PM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 6/9/2017 2:03 AM, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> >
> > Turns out the VLV/CHV fixed function sprite CSC expects full range
> > data as input. We've been feeding it limited range data to it all
> > along. To expand the data out to full range we'll use the color
> > correction registers (brightness, contrast, and saturation).
> >
> > On CHV pipe B we were actually doing the right thing already because we
> > progammed the custom CSC matrix to do expect limited range input. Now
> > that well pre-expand the data out with the color correction unit, we
> > need to change the CSC matrix to operate with full range input instead.
> >
> > This should make the sprite output of the other pipes match the sprite
> > output of pipe B reasonably well. Looking at the resulting pipe CRCs,
> > there can be a slight difference in the output, but as I don't know
> > the formula used by the fixed function CSC of the other pipes, I don't
> > think it's worth the effort to try to match the output exactly. It
> > might not even be possible due to difference in internal precision etc.
> >
> > One slight caveat here is that the color correction registers are single
> > bufferred, so we should really be updating them during vblank, but we
> > still don't have a mechanism for that, so just toss in another FIXME.
> >
> > v2: Rebase
> >
> > Cc: Jyri Sarha <jsarha@ti.com>
> > Cc: "Tang, Jun" <jun.tang@intel.com>
> > Reported-by: "Tang, Jun" <jun.tang@intel.com>
> > Cc: stable@vger.kernel.org
> > Fixes: 7f1f3851feb0 ("drm/i915: sprite support for ValleyView v4")
> > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_reg.h     | 10 +++++
> >   drivers/gpu/drm/i915/intel_sprite.c | 74 ++++++++++++++++++++++++++++---------
> >   2 files changed, 66 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index b6d69e289974..ce7c0dc79cf7 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5777,6 +5777,12 @@ enum {
> >   #define _SPATILEOFF		(VLV_DISPLAY_BASE + 0x721a4)
> >   #define _SPACONSTALPHA		(VLV_DISPLAY_BASE + 0x721a8)
> >   #define   SP_CONST_ALPHA_ENABLE		(1<<31)
> > +#define _SPACLRC0		(VLV_DISPLAY_BASE + 0x721d0)
> > +#define   SP_CONTRAST(x)		((x) << 18) /* u3.6 */
> > +#define   SP_BRIGHTNESS(x)		((x) & 0xff) /* s8 */
> > +#define _SPACLRC1		(VLV_DISPLAY_BASE + 0x721d4)
> > +#define   SP_SH_SIN(x)			(((x) & 0x7ff) << 16) /* s4.7 */
> > +#define   SP_SH_COS(x)			(x) /* u3.7 */
> >   #define _SPAGAMC		(VLV_DISPLAY_BASE + 0x721f4)
> >   
> >   #define _SPBCNTR		(VLV_DISPLAY_BASE + 0x72280)
> > @@ -5790,6 +5796,8 @@ enum {
> >   #define _SPBKEYMAXVAL		(VLV_DISPLAY_BASE + 0x722a0)
> >   #define _SPBTILEOFF		(VLV_DISPLAY_BASE + 0x722a4)
> >   #define _SPBCONSTALPHA		(VLV_DISPLAY_BASE + 0x722a8)
> > +#define _SPBCLRC0		(VLV_DISPLAY_BASE + 0x722d0)
> > +#define _SPBCLRC1		(VLV_DISPLAY_BASE + 0x722d4)
> >   #define _SPBGAMC		(VLV_DISPLAY_BASE + 0x722f4)
> >   
> >   #define _MMIO_VLV_SPR(pipe, plane_id, reg_a, reg_b) \
> > @@ -5806,6 +5814,8 @@ enum {
> >   #define SPKEYMAXVAL(pipe, plane_id)	_MMIO_VLV_SPR((pipe), (plane_id), _SPAKEYMAXVAL, _SPBKEYMAXVAL)
> >   #define SPTILEOFF(pipe, plane_id)	_MMIO_VLV_SPR((pipe), (plane_id), _SPATILEOFF, _SPBTILEOFF)
> >   #define SPCONSTALPHA(pipe, plane_id)	_MMIO_VLV_SPR((pipe), (plane_id), _SPACONSTALPHA, _SPBCONSTALPHA)
> > +#define SPCLRC0(pipe, plane_id)		_MMIO_VLV_SPR((pipe), (plane_id), _SPACLRC0, _SPBCLRC0)
> > +#define SPCLRC1(pipe, plane_id)		_MMIO_VLV_SPR((pipe), (plane_id), _SPACLRC1, _SPBCLRC1)
> >   #define SPGAMC(pipe, plane_id)		_MMIO_VLV_SPR((pipe), (plane_id), _SPAGAMC, _SPBGAMC)
> >   
> >   /*
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index 0c650c2cbca8..2ce3b3c6ffa8 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -325,44 +325,80 @@ skl_disable_plane(struct intel_plane *plane, struct intel_crtc *crtc)
> >   }
> >   
> >   static void
> > -chv_update_csc(struct intel_plane *plane, uint32_t format)
> > +chv_update_csc(const struct intel_plane_state *plane_state)
> >   {
> > +	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> >   	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > +	const struct drm_framebuffer *fb = plane_state->base.fb;
> >   	enum plane_id plane_id = plane->id;
> >   
> >   	/* Seems RGB data bypasses the CSC always */
> > -	if (!format_is_yuv(format))
> > +	if (!format_is_yuv(fb->format->format))
> >   		return;
> >   
> >   	/*
> > -	 * BT.601 limited range YCbCr -> full range RGB
> > +	 * BT.601 full range YCbCr -> full range RGB
> >   	 *
> > -	 * |r|   | 6537 4769     0|   |cr  |
> > -	 * |g| = |-3330 4769 -1605| x |y-64|
> > -	 * |b|   |    0 4769  8263|   |cb  |
> > +	 * |r|   | 5743 4096     0|   |cr|
> > +	 * |g| = |-2925 4096 -1410| x |y |
> > +	 * |b|   |    0 4096  7258|   |cb|
> >   	 *
> > -	 * Cb and Cr apparently come in as signed already, so no
> > -	 * need for any offset. For Y we need to remove the offset.
> > +	 * Cb and Cr apparently come in as signed already,
> > +	 * and we get full range data in on account of CLRC0/1
> >   	 */
> > -	I915_WRITE_FW(SPCSCYGOFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(-64));
> > +	I915_WRITE_FW(SPCSCYGOFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(0));
> >   	I915_WRITE_FW(SPCSCCBOFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(0));
> >   	I915_WRITE_FW(SPCSCCROFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(0));
> >   
> > -	I915_WRITE_FW(SPCSCC01(plane_id), SPCSC_C1(4769) | SPCSC_C0(6537));
> > -	I915_WRITE_FW(SPCSCC23(plane_id), SPCSC_C1(-3330) | SPCSC_C0(0));
> > -	I915_WRITE_FW(SPCSCC45(plane_id), SPCSC_C1(-1605) | SPCSC_C0(4769));
> > -	I915_WRITE_FW(SPCSCC67(plane_id), SPCSC_C1(4769) | SPCSC_C0(0));
> > -	I915_WRITE_FW(SPCSCC8(plane_id), SPCSC_C0(8263));
> > +	I915_WRITE_FW(SPCSCC01(plane_id), SPCSC_C1(4096) | SPCSC_C0(5743));
> > +	I915_WRITE_FW(SPCSCC23(plane_id), SPCSC_C1(-2925) | SPCSC_C0(0));
> > +	I915_WRITE_FW(SPCSCC45(plane_id), SPCSC_C1(-1410) | SPCSC_C0(4096));
> > +	I915_WRITE_FW(SPCSCC67(plane_id), SPCSC_C1(4096) | SPCSC_C0(0));
> > +	I915_WRITE_FW(SPCSCC8(plane_id), SPCSC_C0(7258));
> >   
> > -	I915_WRITE_FW(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(940) | SPCSC_IMIN(64));
> > -	I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
> > -	I915_WRITE_FW(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
> > +	I915_WRITE_FW(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(1023) | SPCSC_IMIN(0));
> > +	I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
> > +	I915_WRITE_FW(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
> >   
> >   	I915_WRITE_FW(SPCSCYGOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
> >   	I915_WRITE_FW(SPCSCCBOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
> >   	I915_WRITE_FW(SPCSCCROCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
> >   }
> >   
> > +static void
> > +vlv_update_clrc(const struct intel_plane_state *plane_state)
> > +{
> > +	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > +	const struct drm_framebuffer *fb = plane_state->base.fb;
> > +	enum pipe pipe = plane->pipe;
> > +	enum plane_id plane_id = plane->id;
> > +	int con, bri, sh_sin, sh_cos;
> > +
> con = contrast bri = brightness for better reading ?

Sure, why not.

> > +	if (format_is_yuv(fb->format->format)) {
> > +		/*
> > +		 * expand limited range to full range.
> > +		 * contrast is applied first, then brightness
> > +		 */
> > +		con = ((255 << 7) / 219 + 1) >> 1;
> > +		bri = -((16 << 1) * 255 / 219 + 1) >> 1;
> > +		sh_sin = 0;
> > +		sh_cos = (((128 << 8) / 112) + 1) >> 1;
> > +	} else {
> > +		/* pass-through everything */
> > +		con = 1 << 6;
> > +		bri = 0;
> > +		sh_sin = 0;
> > +		sh_cos = 1 << 7;
> > +	}
> contrast and brightness are mostly used for color / display correction. 
> Would it be better to create a
> plane level property for the same, and attach into color management 
> framework ? In this way, userspace
> can also play around.

I did have such plans long ago, but I'm not sure there's much point in
doing that since we would only be able to support these propeerties
on a very limited subset of planes on a very limited subset of platforms.

> 
> - Shashank
> > +
> > +	/* FIXME these register are single buffered :( */
> > +	I915_WRITE_FW(SPCLRC0(pipe, plane_id),
> > +		      SP_CONTRAST(con) | SP_BRIGHTNESS(bri));
> > +	I915_WRITE_FW(SPCLRC1(pipe, plane_id),
> > +		      SP_SH_SIN(sh_sin) | SP_SH_COS(sh_cos));
> > +}
> > +
> >   static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
> >   			  const struct intel_plane_state *plane_state)
> >   {
> > @@ -456,8 +492,10 @@ vlv_update_plane(struct intel_plane *plane,
> >   
> >   	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> >   
> > +	vlv_update_clrc(plane_state);
> > +
> >   	if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B)
> > -		chv_update_csc(plane, fb->format->format);
> > +		chv_update_csc(plane_state);
> >   
> >   	if (key->flags) {
> >   		I915_WRITE_FW(SPKEYMINVAL(pipe, plane_id), key->min_value);

-- 
Ville Syrj�l�
Intel OTC

[PATCH v3 1/5] drm/i915: Correctly handle limited range YCbCr data on VLV/CHV

[ville.syrjala]

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

Turns out the VLV/CHV fixed function sprite CSC expects full range
data as input. We've been feeding it limited range data to it all
along. To expand the data out to full range we'll use the color
correction registers (brightness, contrast, and saturation).

On CHV pipe B we were actually doing the right thing already because we
progammed the custom CSC matrix to do expect limited range input. Now
that well pre-expand the data out with the color correction unit, we
need to change the CSC matrix to operate with full range input instead.

This should make the sprite output of the other pipes match the sprite
output of pipe B reasonably well. Looking at the resulting pipe CRCs,
there can be a slight difference in the output, but as I don't know
the formula used by the fixed function CSC of the other pipes, I don't
think it's worth the effort to try to match the output exactly. It
might not even be possible due to difference in internal precision etc.

One slight caveat here is that the color correction registers are single
bufferred, so we should really be updating them during vblank, but we
still don't have a mechanism for that, so just toss in another FIXME.

v2: Rebase
v3: s/bri/brightness/ s/con/contrast/ (Shashank)

Cc: Shashank Sharma <shashank.sharma@intel.com>
Cc: Jyri Sarha <jsarha@ti.com>
Cc: "Tang, Jun" <jun.tang@intel.com>
Reported-by: "Tang, Jun" <jun.tang@intel.com>
Cc: stable@vger.kernel.org
Fixes: 7f1f3851feb0 ("drm/i915: sprite support for ValleyView v4")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h     | 10 +++++
 drivers/gpu/drm/i915/intel_sprite.c | 74 ++++++++++++++++++++++++++++---------
 2 files changed, 66 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index c8647cfa81ba..290322588f56 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5974,6 +5974,12 @@ enum {
 #define _SPATILEOFF		(VLV_DISPLAY_BASE + 0x721a4)
 #define _SPACONSTALPHA		(VLV_DISPLAY_BASE + 0x721a8)
 #define   SP_CONST_ALPHA_ENABLE		(1<<31)
+#define _SPACLRC0		(VLV_DISPLAY_BASE + 0x721d0)
+#define   SP_CONTRAST(x)		((x) << 18) /* u3.6 */
+#define   SP_BRIGHTNESS(x)		((x) & 0xff) /* s8 */
+#define _SPACLRC1		(VLV_DISPLAY_BASE + 0x721d4)
+#define   SP_SH_SIN(x)			(((x) & 0x7ff) << 16) /* s4.7 */
+#define   SP_SH_COS(x)			(x) /* u3.7 */
 #define _SPAGAMC		(VLV_DISPLAY_BASE + 0x721f4)
 
 #define _SPBCNTR		(VLV_DISPLAY_BASE + 0x72280)
@@ -5987,6 +5993,8 @@ enum {
 #define _SPBKEYMAXVAL		(VLV_DISPLAY_BASE + 0x722a0)
 #define _SPBTILEOFF		(VLV_DISPLAY_BASE + 0x722a4)
 #define _SPBCONSTALPHA		(VLV_DISPLAY_BASE + 0x722a8)
+#define _SPBCLRC0		(VLV_DISPLAY_BASE + 0x722d0)
+#define _SPBCLRC1		(VLV_DISPLAY_BASE + 0x722d4)
 #define _SPBGAMC		(VLV_DISPLAY_BASE + 0x722f4)
 
 #define _MMIO_VLV_SPR(pipe, plane_id, reg_a, reg_b) \
@@ -6003,6 +6011,8 @@ enum {
 #define SPKEYMAXVAL(pipe, plane_id)	_MMIO_VLV_SPR((pipe), (plane_id), _SPAKEYMAXVAL, _SPBKEYMAXVAL)
 #define SPTILEOFF(pipe, plane_id)	_MMIO_VLV_SPR((pipe), (plane_id), _SPATILEOFF, _SPBTILEOFF)
 #define SPCONSTALPHA(pipe, plane_id)	_MMIO_VLV_SPR((pipe), (plane_id), _SPACONSTALPHA, _SPBCONSTALPHA)
+#define SPCLRC0(pipe, plane_id)		_MMIO_VLV_SPR((pipe), (plane_id), _SPACLRC0, _SPBCLRC0)
+#define SPCLRC1(pipe, plane_id)		_MMIO_VLV_SPR((pipe), (plane_id), _SPACLRC1, _SPBCLRC1)
 #define SPGAMC(pipe, plane_id)		_MMIO_VLV_SPR((pipe), (plane_id), _SPAGAMC, _SPBGAMC)
 
 /*
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 0c650c2cbca8..4462408cc835 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -325,44 +325,80 @@ skl_disable_plane(struct intel_plane *plane, struct intel_crtc *crtc)
 }
 
 static void
-chv_update_csc(struct intel_plane *plane, uint32_t format)
+chv_update_csc(const struct intel_plane_state *plane_state)
 {
+	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
 	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
+	const struct drm_framebuffer *fb = plane_state->base.fb;
 	enum plane_id plane_id = plane->id;
 
 	/* Seems RGB data bypasses the CSC always */
-	if (!format_is_yuv(format))
+	if (!format_is_yuv(fb->format->format))
 		return;
 
 	/*
-	 * BT.601 limited range YCbCr -> full range RGB
+	 * BT.601 full range YCbCr -> full range RGB
 	 *
-	 * |r|   | 6537 4769     0|   |cr  |
-	 * |g| = |-3330 4769 -1605| x |y-64|
-	 * |b|   |    0 4769  8263|   |cb  |
+	 * |r|   | 5743 4096     0|   |cr|
+	 * |g| = |-2925 4096 -1410| x |y |
+	 * |b|   |    0 4096  7258|   |cb|
 	 *
-	 * Cb and Cr apparently come in as signed already, so no
-	 * need for any offset. For Y we need to remove the offset.
+	 * Cb and Cr apparently come in as signed already,
+	 * and we get full range data in on account of CLRC0/1
 	 */
-	I915_WRITE_FW(SPCSCYGOFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(-64));
+	I915_WRITE_FW(SPCSCYGOFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(0));
 	I915_WRITE_FW(SPCSCCBOFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(0));
 	I915_WRITE_FW(SPCSCCROFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(0));
 
-	I915_WRITE_FW(SPCSCC01(plane_id), SPCSC_C1(4769) | SPCSC_C0(6537));
-	I915_WRITE_FW(SPCSCC23(plane_id), SPCSC_C1(-3330) | SPCSC_C0(0));
-	I915_WRITE_FW(SPCSCC45(plane_id), SPCSC_C1(-1605) | SPCSC_C0(4769));
-	I915_WRITE_FW(SPCSCC67(plane_id), SPCSC_C1(4769) | SPCSC_C0(0));
-	I915_WRITE_FW(SPCSCC8(plane_id), SPCSC_C0(8263));
+	I915_WRITE_FW(SPCSCC01(plane_id), SPCSC_C1(4096) | SPCSC_C0(5743));
+	I915_WRITE_FW(SPCSCC23(plane_id), SPCSC_C1(-2925) | SPCSC_C0(0));
+	I915_WRITE_FW(SPCSCC45(plane_id), SPCSC_C1(-1410) | SPCSC_C0(4096));
+	I915_WRITE_FW(SPCSCC67(plane_id), SPCSC_C1(4096) | SPCSC_C0(0));
+	I915_WRITE_FW(SPCSCC8(plane_id), SPCSC_C0(7258));
 
-	I915_WRITE_FW(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(940) | SPCSC_IMIN(64));
-	I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
-	I915_WRITE_FW(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
+	I915_WRITE_FW(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(1023) | SPCSC_IMIN(0));
+	I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
+	I915_WRITE_FW(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
 
 	I915_WRITE_FW(SPCSCYGOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
 	I915_WRITE_FW(SPCSCCBOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
 	I915_WRITE_FW(SPCSCCROCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
 }
 
+static void
+vlv_update_clrc(const struct intel_plane_state *plane_state)
+{
+	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
+	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
+	const struct drm_framebuffer *fb = plane_state->base.fb;
+	enum pipe pipe = plane->pipe;
+	enum plane_id plane_id = plane->id;
+	int contrast, brightness, sh_sin, sh_cos;
+
+	if (format_is_yuv(fb->format->format)) {
+		/*
+		 * expand limited range to full range.
+		 * contrast is applied first, then brightness
+		 */
+		contrast = ((255 << 7) / 219 + 1) >> 1;
+		brightness = -((16 << 1) * 255 / 219 + 1) >> 1;
+		sh_sin = 0;
+		sh_cos = (((128 << 8) / 112) + 1) >> 1;
+	} else {
+		/* pass-through everything */
+		contrast = 1 << 6;
+		brightness = 0;
+		sh_sin = 0;
+		sh_cos = 1 << 7;
+	}
+
+	/* FIXME these register are single buffered :( */
+	I915_WRITE_FW(SPCLRC0(pipe, plane_id),
+		      SP_CONTRAST(contrast) | SP_BRIGHTNESS(brightness));
+	I915_WRITE_FW(SPCLRC1(pipe, plane_id),
+		      SP_SH_SIN(sh_sin) | SP_SH_COS(sh_cos));
+}
+
 static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
 			  const struct intel_plane_state *plane_state)
 {
@@ -456,8 +492,10 @@ vlv_update_plane(struct intel_plane *plane,
 
 	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
 
+	vlv_update_clrc(plane_state);
+
 	if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B)
-		chv_update_csc(plane, fb->format->format);
+		chv_update_csc(plane_state);
 
 	if (key->flags) {
 		I915_WRITE_FW(SPKEYMINVAL(pipe, plane_id), key->min_value);
-- 
2.13.0

Re: [PATCH v3 1/5] drm/i915: Correctly handle limited range YCbCr data on VLV/CHV

[Sharma, Shashank]

Regards
Shashank

On 6/20/2017 7:02 PM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Turns out the VLV/CHV fixed function sprite CSC expects full range
> data as input. We've been feeding it limited range data to it all
> along. To expand the data out to full range we'll use the color
> correction registers (brightness, contrast, and saturation).
>
> On CHV pipe B we were actually doing the right thing already because we
> progammed the custom CSC matrix to do expect limited range input. Now
> that well pre-expand the data out with the color correction unit, we
> need to change the CSC matrix to operate with full range input instead.
>
> This should make the sprite output of the other pipes match the sprite
> output of pipe B reasonably well. Looking at the resulting pipe CRCs,
> there can be a slight difference in the output, but as I don't know
> the formula used by the fixed function CSC of the other pipes, I don't
> think it's worth the effort to try to match the output exactly. It
> might not even be possible due to difference in internal precision etc.
>
> One slight caveat here is that the color correction registers are single
> bufferred, so we should really be updating them during vblank, but we
> still don't have a mechanism for that, so just toss in another FIXME.
>
> v2: Rebase
> v3: s/bri/brightness/ s/con/contrast/ (Shashank)
>
> Cc: Shashank Sharma <shashank.sharma@intel.com>
> Cc: Jyri Sarha <jsarha@ti.com>
> Cc: "Tang, Jun" <jun.tang@intel.com>
> Reported-by: "Tang, Jun" <jun.tang@intel.com>
> Cc: stable@vger.kernel.org
> Fixes: 7f1f3851feb0 ("drm/i915: sprite support for ValleyView v4")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_reg.h     | 10 +++++
>   drivers/gpu/drm/i915/intel_sprite.c | 74 ++++++++++++++++++++++++++++---------
>   2 files changed, 66 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index c8647cfa81ba..290322588f56 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5974,6 +5974,12 @@ enum {
>   #define _SPATILEOFF		(VLV_DISPLAY_BASE + 0x721a4)
>   #define _SPACONSTALPHA		(VLV_DISPLAY_BASE + 0x721a8)
>   #define   SP_CONST_ALPHA_ENABLE		(1<<31)
> +#define _SPACLRC0		(VLV_DISPLAY_BASE + 0x721d0)
> +#define   SP_CONTRAST(x)		((x) << 18) /* u3.6 */
protection for higher reserved bits ? From 27-31 ? something like ((x & 
1FF) << 18) ?
> +#define   SP_BRIGHTNESS(x)		((x) & 0xff) /* s8 */
> +#define _SPACLRC1		(VLV_DISPLAY_BASE + 0x721d4)
> +#define   SP_SH_SIN(x)			(((x) & 0x7ff) << 16) /* s4.7 */
> +#define   SP_SH_COS(x)			(x) /* u3.7 */
>   

#define   SP_SH_COS(x)			(x & 3FF) /* u3.7 */ ?

> #define _SPAGAMC		(VLV_DISPLAY_BASE + 0x721f4)
>   
>   #define _SPBCNTR		(VLV_DISPLAY_BASE + 0x72280)
> @@ -5987,6 +5993,8 @@ enum {
>   #define _SPBKEYMAXVAL		(VLV_DISPLAY_BASE + 0x722a0)
>   #define _SPBTILEOFF		(VLV_DISPLAY_BASE + 0x722a4)
>   #define _SPBCONSTALPHA		(VLV_DISPLAY_BASE + 0x722a8)
> +#define _SPBCLRC0		(VLV_DISPLAY_BASE + 0x722d0)
> +#define _SPBCLRC1		(VLV_DISPLAY_BASE + 0x722d4)
>   #define _SPBGAMC		(VLV_DISPLAY_BASE + 0x722f4)
>   
>   #define _MMIO_VLV_SPR(pipe, plane_id, reg_a, reg_b) \
> @@ -6003,6 +6011,8 @@ enum {
>   #define SPKEYMAXVAL(pipe, plane_id)	_MMIO_VLV_SPR((pipe), (plane_id), _SPAKEYMAXVAL, _SPBKEYMAXVAL)
>   #define SPTILEOFF(pipe, plane_id)	_MMIO_VLV_SPR((pipe), (plane_id), _SPATILEOFF, _SPBTILEOFF)
>   #define SPCONSTALPHA(pipe, plane_id)	_MMIO_VLV_SPR((pipe), (plane_id), _SPACONSTALPHA, _SPBCONSTALPHA)
> +#define SPCLRC0(pipe, plane_id)		_MMIO_VLV_SPR((pipe), (plane_id), _SPACLRC0, _SPBCLRC0)
> +#define SPCLRC1(pipe, plane_id)		_MMIO_VLV_SPR((pipe), (plane_id), _SPACLRC1, _SPBCLRC1)
>   #define SPGAMC(pipe, plane_id)		_MMIO_VLV_SPR((pipe), (plane_id), _SPAGAMC, _SPBGAMC)
>   
>   /*
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 0c650c2cbca8..4462408cc835 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -325,44 +325,80 @@ skl_disable_plane(struct intel_plane *plane, struct intel_crtc *crtc)
>   }
>   
>   static void
> -chv_update_csc(struct intel_plane *plane, uint32_t format)
> +chv_update_csc(const struct intel_plane_state *plane_state)
>   {
> +	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
>   	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> +	const struct drm_framebuffer *fb = plane_state->base.fb;
>   	enum plane_id plane_id = plane->id;
>   
>   	/* Seems RGB data bypasses the CSC always */
> -	if (!format_is_yuv(format))
> +	if (!format_is_yuv(fb->format->format))
>   		return;
>   
>   	/*
> -	 * BT.601 limited range YCbCr -> full range RGB
> +	 * BT.601 full range YCbCr -> full range RGB
>   	 *
> -	 * |r|   | 6537 4769     0|   |cr  |
> -	 * |g| = |-3330 4769 -1605| x |y-64|
> -	 * |b|   |    0 4769  8263|   |cb  |
> +	 * |r|   | 5743 4096     0|   |cr|
> +	 * |g| = |-2925 4096 -1410| x |y |
> +	 * |b|   |    0 4096  7258|   |cb|
>   	 *
> -	 * Cb and Cr apparently come in as signed already, so no
> -	 * need for any offset. For Y we need to remove the offset.
> +	 * Cb and Cr apparently come in as signed already,
> +	 * and we get full range data in on account of CLRC0/1
>   	 */
> -	I915_WRITE_FW(SPCSCYGOFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(-64));
> +	I915_WRITE_FW(SPCSCYGOFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(0));
>   	I915_WRITE_FW(SPCSCCBOFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(0));
>   	I915_WRITE_FW(SPCSCCROFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(0));
>   
> -	I915_WRITE_FW(SPCSCC01(plane_id), SPCSC_C1(4769) | SPCSC_C0(6537));
> -	I915_WRITE_FW(SPCSCC23(plane_id), SPCSC_C1(-3330) | SPCSC_C0(0));
> -	I915_WRITE_FW(SPCSCC45(plane_id), SPCSC_C1(-1605) | SPCSC_C0(4769));
> -	I915_WRITE_FW(SPCSCC67(plane_id), SPCSC_C1(4769) | SPCSC_C0(0));
> -	I915_WRITE_FW(SPCSCC8(plane_id), SPCSC_C0(8263));
> +	I915_WRITE_FW(SPCSCC01(plane_id), SPCSC_C1(4096) | SPCSC_C0(5743));
> +	I915_WRITE_FW(SPCSCC23(plane_id), SPCSC_C1(-2925) | SPCSC_C0(0));
> +	I915_WRITE_FW(SPCSCC45(plane_id), SPCSC_C1(-1410) | SPCSC_C0(4096));
> +	I915_WRITE_FW(SPCSCC67(plane_id), SPCSC_C1(4096) | SPCSC_C0(0));
> +	I915_WRITE_FW(SPCSCC8(plane_id), SPCSC_C0(7258));
>   
> -	I915_WRITE_FW(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(940) | SPCSC_IMIN(64));
> -	I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
> -	I915_WRITE_FW(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
> +	I915_WRITE_FW(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(1023) | SPCSC_IMIN(0));
> +	I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
> +	I915_WRITE_FW(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
>   
>   	I915_WRITE_FW(SPCSCYGOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
>   	I915_WRITE_FW(SPCSCCBOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
>   	I915_WRITE_FW(SPCSCCROCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
>   }
>   
> +static void
> +vlv_update_clrc(const struct intel_plane_state *plane_state)
> +{
> +	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> +	const struct drm_framebuffer *fb = plane_state->base.fb;
> +	enum pipe pipe = plane->pipe;
> +	enum plane_id plane_id = plane->id;
> +	int contrast, brightness, sh_sin, sh_cos;
> +
> +	if (format_is_yuv(fb->format->format)) {
> +		/*
> +		 * expand limited range to full range.
> +		 * contrast is applied first, then brightness
> +		 */
I would be happy to see some comment explaining the Brightness/Contrast 
calculation magic nos, or may be a hint for other developers.
> +		contrast = ((255 << 7) / 219 + 1) >> 1;
> +		brightness = -((16 << 1) * 255 / 219 + 1) >> 1;
> +		sh_sin = 0;
> +		sh_cos = (((128 << 8) / 112) + 1) >> 1;
> +	} else {
> +		/* pass-through everything */
> +		contrast = 1 << 6;
> +		brightness = 0;
> +		sh_sin = 0;
> +		sh_cos = 1 << 7;
> +	}
> +
> +	/* FIXME these register are single buffered :( */
> +	I915_WRITE_FW(SPCLRC0(pipe, plane_id),
> +		      SP_CONTRAST(contrast) | SP_BRIGHTNESS(brightness));
> +	I915_WRITE_FW(SPCLRC1(pipe, plane_id),
> +		      SP_SH_SIN(sh_sin) | SP_SH_COS(sh_cos));
> +}
> +
>   static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
>   			  const struct intel_plane_state *plane_state)
>   {
> @@ -456,8 +492,10 @@ vlv_update_plane(struct intel_plane *plane,
>   
>   	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>   
> +	vlv_update_clrc(plane_state);
> +
>   	if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B)
> -		chv_update_csc(plane, fb->format->format);
> +		chv_update_csc(plane_state);
>   
>   	if (key->flags) {
>   		I915_WRITE_FW(SPKEYMINVAL(pipe, plane_id), key->min_value);

Re: [PATCH v3 1/5] drm/i915: Correctly handle limited range YCbCr data on VLV/CHV

[Ville Syrjälä]

On Tue, Jun 20, 2017 at 07:27:54PM +0530, Sharma, Shashank wrote:
> Regards
> Shashank
> 
> On 6/20/2017 7:02 PM, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> >
> > Turns out the VLV/CHV fixed function sprite CSC expects full range
> > data as input. We've been feeding it limited range data to it all
> > along. To expand the data out to full range we'll use the color
> > correction registers (brightness, contrast, and saturation).
> >
> > On CHV pipe B we were actually doing the right thing already because we
> > progammed the custom CSC matrix to do expect limited range input. Now
> > that well pre-expand the data out with the color correction unit, we
> > need to change the CSC matrix to operate with full range input instead.
> >
> > This should make the sprite output of the other pipes match the sprite
> > output of pipe B reasonably well. Looking at the resulting pipe CRCs,
> > there can be a slight difference in the output, but as I don't know
> > the formula used by the fixed function CSC of the other pipes, I don't
> > think it's worth the effort to try to match the output exactly. It
> > might not even be possible due to difference in internal precision etc.
> >
> > One slight caveat here is that the color correction registers are single
> > bufferred, so we should really be updating them during vblank, but we
> > still don't have a mechanism for that, so just toss in another FIXME.
> >
> > v2: Rebase
> > v3: s/bri/brightness/ s/con/contrast/ (Shashank)
> >
> > Cc: Shashank Sharma <shashank.sharma@intel.com>
> > Cc: Jyri Sarha <jsarha@ti.com>
> > Cc: "Tang, Jun" <jun.tang@intel.com>
> > Reported-by: "Tang, Jun" <jun.tang@intel.com>
> > Cc: stable@vger.kernel.org
> > Fixes: 7f1f3851feb0 ("drm/i915: sprite support for ValleyView v4")
> > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_reg.h     | 10 +++++
> >   drivers/gpu/drm/i915/intel_sprite.c | 74 ++++++++++++++++++++++++++++---------
> >   2 files changed, 66 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index c8647cfa81ba..290322588f56 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5974,6 +5974,12 @@ enum {
> >   #define _SPATILEOFF		(VLV_DISPLAY_BASE + 0x721a4)
> >   #define _SPACONSTALPHA		(VLV_DISPLAY_BASE + 0x721a8)
> >   #define   SP_CONST_ALPHA_ENABLE		(1<<31)
> > +#define _SPACLRC0		(VLV_DISPLAY_BASE + 0x721d0)
> > +#define   SP_CONTRAST(x)		((x) << 18) /* u3.6 */
> protection for higher reserved bits ? From 27-31 ? something like ((x & 
> 1FF) << 18) ?

It's unsigned, so no need.

> > +#define   SP_BRIGHTNESS(x)		((x) & 0xff) /* s8 */
> > +#define _SPACLRC1		(VLV_DISPLAY_BASE + 0x721d4)
> > +#define   SP_SH_SIN(x)			(((x) & 0x7ff) << 16) /* s4.7 */
> > +#define   SP_SH_COS(x)			(x) /* u3.7 */
> >   
> 
> #define   SP_SH_COS(x)			(x & 3FF) /* u3.7 */ ?

Also unsigned

> 
> > #define _SPAGAMC		(VLV_DISPLAY_BASE + 0x721f4)
> >   
> >   #define _SPBCNTR		(VLV_DISPLAY_BASE + 0x72280)
> > @@ -5987,6 +5993,8 @@ enum {
> >   #define _SPBKEYMAXVAL		(VLV_DISPLAY_BASE + 0x722a0)
> >   #define _SPBTILEOFF		(VLV_DISPLAY_BASE + 0x722a4)
> >   #define _SPBCONSTALPHA		(VLV_DISPLAY_BASE + 0x722a8)
> > +#define _SPBCLRC0		(VLV_DISPLAY_BASE + 0x722d0)
> > +#define _SPBCLRC1		(VLV_DISPLAY_BASE + 0x722d4)
> >   #define _SPBGAMC		(VLV_DISPLAY_BASE + 0x722f4)
> >   
> >   #define _MMIO_VLV_SPR(pipe, plane_id, reg_a, reg_b) \
> > @@ -6003,6 +6011,8 @@ enum {
> >   #define SPKEYMAXVAL(pipe, plane_id)	_MMIO_VLV_SPR((pipe), (plane_id), _SPAKEYMAXVAL, _SPBKEYMAXVAL)
> >   #define SPTILEOFF(pipe, plane_id)	_MMIO_VLV_SPR((pipe), (plane_id), _SPATILEOFF, _SPBTILEOFF)
> >   #define SPCONSTALPHA(pipe, plane_id)	_MMIO_VLV_SPR((pipe), (plane_id), _SPACONSTALPHA, _SPBCONSTALPHA)
> > +#define SPCLRC0(pipe, plane_id)		_MMIO_VLV_SPR((pipe), (plane_id), _SPACLRC0, _SPBCLRC0)
> > +#define SPCLRC1(pipe, plane_id)		_MMIO_VLV_SPR((pipe), (plane_id), _SPACLRC1, _SPBCLRC1)
> >   #define SPGAMC(pipe, plane_id)		_MMIO_VLV_SPR((pipe), (plane_id), _SPAGAMC, _SPBGAMC)
> >   
> >   /*
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index 0c650c2cbca8..4462408cc835 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -325,44 +325,80 @@ skl_disable_plane(struct intel_plane *plane, struct intel_crtc *crtc)
> >   }
> >   
> >   static void
> > -chv_update_csc(struct intel_plane *plane, uint32_t format)
> > +chv_update_csc(const struct intel_plane_state *plane_state)
> >   {
> > +	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> >   	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > +	const struct drm_framebuffer *fb = plane_state->base.fb;
> >   	enum plane_id plane_id = plane->id;
> >   
> >   	/* Seems RGB data bypasses the CSC always */
> > -	if (!format_is_yuv(format))
> > +	if (!format_is_yuv(fb->format->format))
> >   		return;
> >   
> >   	/*
> > -	 * BT.601 limited range YCbCr -> full range RGB
> > +	 * BT.601 full range YCbCr -> full range RGB
> >   	 *
> > -	 * |r|   | 6537 4769     0|   |cr  |
> > -	 * |g| = |-3330 4769 -1605| x |y-64|
> > -	 * |b|   |    0 4769  8263|   |cb  |
> > +	 * |r|   | 5743 4096     0|   |cr|
> > +	 * |g| = |-2925 4096 -1410| x |y |
> > +	 * |b|   |    0 4096  7258|   |cb|
> >   	 *
> > -	 * Cb and Cr apparently come in as signed already, so no
> > -	 * need for any offset. For Y we need to remove the offset.
> > +	 * Cb and Cr apparently come in as signed already,
> > +	 * and we get full range data in on account of CLRC0/1
> >   	 */
> > -	I915_WRITE_FW(SPCSCYGOFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(-64));
> > +	I915_WRITE_FW(SPCSCYGOFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(0));
> >   	I915_WRITE_FW(SPCSCCBOFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(0));
> >   	I915_WRITE_FW(SPCSCCROFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(0));
> >   
> > -	I915_WRITE_FW(SPCSCC01(plane_id), SPCSC_C1(4769) | SPCSC_C0(6537));
> > -	I915_WRITE_FW(SPCSCC23(plane_id), SPCSC_C1(-3330) | SPCSC_C0(0));
> > -	I915_WRITE_FW(SPCSCC45(plane_id), SPCSC_C1(-1605) | SPCSC_C0(4769));
> > -	I915_WRITE_FW(SPCSCC67(plane_id), SPCSC_C1(4769) | SPCSC_C0(0));
> > -	I915_WRITE_FW(SPCSCC8(plane_id), SPCSC_C0(8263));
> > +	I915_WRITE_FW(SPCSCC01(plane_id), SPCSC_C1(4096) | SPCSC_C0(5743));
> > +	I915_WRITE_FW(SPCSCC23(plane_id), SPCSC_C1(-2925) | SPCSC_C0(0));
> > +	I915_WRITE_FW(SPCSCC45(plane_id), SPCSC_C1(-1410) | SPCSC_C0(4096));
> > +	I915_WRITE_FW(SPCSCC67(plane_id), SPCSC_C1(4096) | SPCSC_C0(0));
> > +	I915_WRITE_FW(SPCSCC8(plane_id), SPCSC_C0(7258));
> >   
> > -	I915_WRITE_FW(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(940) | SPCSC_IMIN(64));
> > -	I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
> > -	I915_WRITE_FW(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
> > +	I915_WRITE_FW(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(1023) | SPCSC_IMIN(0));
> > +	I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
> > +	I915_WRITE_FW(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
> >   
> >   	I915_WRITE_FW(SPCSCYGOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
> >   	I915_WRITE_FW(SPCSCCBOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
> >   	I915_WRITE_FW(SPCSCCROCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
> >   }
> >   
> > +static void
> > +vlv_update_clrc(const struct intel_plane_state *plane_state)
> > +{
> > +	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > +	const struct drm_framebuffer *fb = plane_state->base.fb;
> > +	enum pipe pipe = plane->pipe;
> > +	enum plane_id plane_id = plane->id;
> > +	int contrast, brightness, sh_sin, sh_cos;
> > +
> > +	if (format_is_yuv(fb->format->format)) {
> > +		/*
> > +		 * expand limited range to full range.
> > +		 * contrast is applied first, then brightness
> > +		 */
> I would be happy to see some comment explaining the Brightness/Contrast 
> calculation magic nos, or may be a hint for other developers.

Y: 16-235 * contrast + brightness -> ~0->255
CbCr: no hue adjustemnt -> 0 degree angle
      scale to expand CbCr range from -112-112 to -128-128
      sh_sin = sin(0) * 128/112 = 0
      sh_cos = cos(0) * 128/112 = whatever

> > +		contrast = ((255 << 7) / 219 + 1) >> 1;
> > +		brightness = -((16 << 1) * 255 / 219 + 1) >> 1;
> > +		sh_sin = 0;
> > +		sh_cos = (((128 << 8) / 112) + 1) >> 1;
> > +	} else {
> > +		/* pass-through everything */
> > +		contrast = 1 << 6;
> > +		brightness = 0;
> > +		sh_sin = 0;
> > +		sh_cos = 1 << 7;
> > +	}
> > +
> > +	/* FIXME these register are single buffered :( */
> > +	I915_WRITE_FW(SPCLRC0(pipe, plane_id),
> > +		      SP_CONTRAST(contrast) | SP_BRIGHTNESS(brightness));
> > +	I915_WRITE_FW(SPCLRC1(pipe, plane_id),
> > +		      SP_SH_SIN(sh_sin) | SP_SH_COS(sh_cos));
> > +}
> > +
> >   static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
> >   			  const struct intel_plane_state *plane_state)
> >   {
> > @@ -456,8 +492,10 @@ vlv_update_plane(struct intel_plane *plane,
> >   
> >   	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> >   
> > +	vlv_update_clrc(plane_state);
> > +
> >   	if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B)
> > -		chv_update_csc(plane, fb->format->format);
> > +		chv_update_csc(plane_state);
> >   
> >   	if (key->flags) {
> >   		I915_WRITE_FW(SPKEYMINVAL(pipe, plane_id), key->min_value);

-- 
Ville Syrj�l�
Intel OTC

Re: [PATCH v3 1/5] drm/i915: Correctly handle limited range YCbCr data on VLV/CHV

[Sharma, Shashank]

Regards

Shashank


On 6/20/2017 8:04 PM, Ville Syrj�l� wrote:
> On Tue, Jun 20, 2017 at 07:27:54PM +0530, Sharma, Shashank wrote:
>> Regards
>> Shashank
>>
>> On 6/20/2017 7:02 PM, ville.syrjala@linux.intel.com wrote:
>>> From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
>>>
>>> Turns out the VLV/CHV fixed function sprite CSC expects full range
>>> data as input. We've been feeding it limited range data to it all
>>> along. To expand the data out to full range we'll use the color
>>> correction registers (brightness, contrast, and saturation).
>>>
>>> On CHV pipe B we were actually doing the right thing already because we
>>> progammed the custom CSC matrix to do expect limited range input. Now
>>> that well pre-expand the data out with the color correction unit, we
>>> need to change the CSC matrix to operate with full range input instead.
>>>
>>> This should make the sprite output of the other pipes match the sprite
>>> output of pipe B reasonably well. Looking at the resulting pipe CRCs,
>>> there can be a slight difference in the output, but as I don't know
>>> the formula used by the fixed function CSC of the other pipes, I don't
>>> think it's worth the effort to try to match the output exactly. It
>>> might not even be possible due to difference in internal precision etc.
>>>
>>> One slight caveat here is that the color correction registers are single
>>> bufferred, so we should really be updating them during vblank, but we
>>> still don't have a mechanism for that, so just toss in another FIXME.
>>>
>>> v2: Rebase
>>> v3: s/bri/brightness/ s/con/contrast/ (Shashank)
>>>
>>> Cc: Shashank Sharma <shashank.sharma@intel.com>
>>> Cc: Jyri Sarha <jsarha@ti.com>
>>> Cc: "Tang, Jun" <jun.tang@intel.com>
>>> Reported-by: "Tang, Jun" <jun.tang@intel.com>
>>> Cc: stable@vger.kernel.org
>>> Fixes: 7f1f3851feb0 ("drm/i915: sprite support for ValleyView v4")
>>> Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/i915_reg.h     | 10 +++++
>>>    drivers/gpu/drm/i915/intel_sprite.c | 74 ++++++++++++++++++++++++++++---------
>>>    2 files changed, 66 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>> index c8647cfa81ba..290322588f56 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -5974,6 +5974,12 @@ enum {
>>>    #define _SPATILEOFF		(VLV_DISPLAY_BASE + 0x721a4)
>>>    #define _SPACONSTALPHA		(VLV_DISPLAY_BASE + 0x721a8)
>>>    #define   SP_CONST_ALPHA_ENABLE		(1<<31)
>>> +#define _SPACLRC0		(VLV_DISPLAY_BASE + 0x721d0)
>>> +#define   SP_CONTRAST(x)		((x) << 18) /* u3.6 */
>> protection for higher reserved bits ? From 27-31 ? something like ((x &
>> 1FF) << 18) ?
> It's unsigned, so no need.
Humm .. doesn't stops me to pass a bigger value, which will be shifted 
towards reserved bits ?
>
>>> +#define   SP_BRIGHTNESS(x)		((x) & 0xff) /* s8 */
>>> +#define _SPACLRC1		(VLV_DISPLAY_BASE + 0x721d4)
>>> +#define   SP_SH_SIN(x)			(((x) & 0x7ff) << 16) /* s4.7 */
>>> +#define   SP_SH_COS(x)			(x) /* u3.7 */
>>>    
>> #define   SP_SH_COS(x)			(x & 3FF) /* u3.7 */ ?
> Also unsigned
>
>>> #define _SPAGAMC		(VLV_DISPLAY_BASE + 0x721f4)
>>>    
>>>    #define _SPBCNTR		(VLV_DISPLAY_BASE + 0x72280)
>>> @@ -5987,6 +5993,8 @@ enum {
>>>    #define _SPBKEYMAXVAL		(VLV_DISPLAY_BASE + 0x722a0)
>>>    #define _SPBTILEOFF		(VLV_DISPLAY_BASE + 0x722a4)
>>>    #define _SPBCONSTALPHA		(VLV_DISPLAY_BASE + 0x722a8)
>>> +#define _SPBCLRC0		(VLV_DISPLAY_BASE + 0x722d0)
>>> +#define _SPBCLRC1		(VLV_DISPLAY_BASE + 0x722d4)
>>>    #define _SPBGAMC		(VLV_DISPLAY_BASE + 0x722f4)
>>>    
>>>    #define _MMIO_VLV_SPR(pipe, plane_id, reg_a, reg_b) \
>>> @@ -6003,6 +6011,8 @@ enum {
>>>    #define SPKEYMAXVAL(pipe, plane_id)	_MMIO_VLV_SPR((pipe), (plane_id), _SPAKEYMAXVAL, _SPBKEYMAXVAL)
>>>    #define SPTILEOFF(pipe, plane_id)	_MMIO_VLV_SPR((pipe), (plane_id), _SPATILEOFF, _SPBTILEOFF)
>>>    #define SPCONSTALPHA(pipe, plane_id)	_MMIO_VLV_SPR((pipe), (plane_id), _SPACONSTALPHA, _SPBCONSTALPHA)
>>> +#define SPCLRC0(pipe, plane_id)		_MMIO_VLV_SPR((pipe), (plane_id), _SPACLRC0, _SPBCLRC0)
>>> +#define SPCLRC1(pipe, plane_id)		_MMIO_VLV_SPR((pipe), (plane_id), _SPACLRC1, _SPBCLRC1)
>>>    #define SPGAMC(pipe, plane_id)		_MMIO_VLV_SPR((pipe), (plane_id), _SPAGAMC, _SPBGAMC)
>>>    
>>>    /*
>>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>>> index 0c650c2cbca8..4462408cc835 100644
>>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>>> @@ -325,44 +325,80 @@ skl_disable_plane(struct intel_plane *plane, struct intel_crtc *crtc)
>>>    }
>>>    
>>>    static void
>>> -chv_update_csc(struct intel_plane *plane, uint32_t format)
>>> +chv_update_csc(const struct intel_plane_state *plane_state)
>>>    {
>>> +	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
>>>    	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>>> +	const struct drm_framebuffer *fb = plane_state->base.fb;
>>>    	enum plane_id plane_id = plane->id;
>>>    
>>>    	/* Seems RGB data bypasses the CSC always */
>>> -	if (!format_is_yuv(format))
>>> +	if (!format_is_yuv(fb->format->format))
>>>    		return;
>>>    
>>>    	/*
>>> -	 * BT.601 limited range YCbCr -> full range RGB
>>> +	 * BT.601 full range YCbCr -> full range RGB
>>>    	 *
>>> -	 * |r|   | 6537 4769     0|   |cr  |
>>> -	 * |g| = |-3330 4769 -1605| x |y-64|
>>> -	 * |b|   |    0 4769  8263|   |cb  |
>>> +	 * |r|   | 5743 4096     0|   |cr|
>>> +	 * |g| = |-2925 4096 -1410| x |y |
>>> +	 * |b|   |    0 4096  7258|   |cb|
>>>    	 *
>>> -	 * Cb and Cr apparently come in as signed already, so no
>>> -	 * need for any offset. For Y we need to remove the offset.
>>> +	 * Cb and Cr apparently come in as signed already,
>>> +	 * and we get full range data in on account of CLRC0/1
>>>    	 */
>>> -	I915_WRITE_FW(SPCSCYGOFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(-64));
>>> +	I915_WRITE_FW(SPCSCYGOFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(0));
>>>    	I915_WRITE_FW(SPCSCCBOFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(0));
>>>    	I915_WRITE_FW(SPCSCCROFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(0));
>>>    
>>> -	I915_WRITE_FW(SPCSCC01(plane_id), SPCSC_C1(4769) | SPCSC_C0(6537));
>>> -	I915_WRITE_FW(SPCSCC23(plane_id), SPCSC_C1(-3330) | SPCSC_C0(0));
>>> -	I915_WRITE_FW(SPCSCC45(plane_id), SPCSC_C1(-1605) | SPCSC_C0(4769));
>>> -	I915_WRITE_FW(SPCSCC67(plane_id), SPCSC_C1(4769) | SPCSC_C0(0));
>>> -	I915_WRITE_FW(SPCSCC8(plane_id), SPCSC_C0(8263));
>>> +	I915_WRITE_FW(SPCSCC01(plane_id), SPCSC_C1(4096) | SPCSC_C0(5743));
>>> +	I915_WRITE_FW(SPCSCC23(plane_id), SPCSC_C1(-2925) | SPCSC_C0(0));
>>> +	I915_WRITE_FW(SPCSCC45(plane_id), SPCSC_C1(-1410) | SPCSC_C0(4096));
>>> +	I915_WRITE_FW(SPCSCC67(plane_id), SPCSC_C1(4096) | SPCSC_C0(0));
>>> +	I915_WRITE_FW(SPCSCC8(plane_id), SPCSC_C0(7258));
>>>    
>>> -	I915_WRITE_FW(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(940) | SPCSC_IMIN(64));
>>> -	I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
>>> -	I915_WRITE_FW(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
>>> +	I915_WRITE_FW(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(1023) | SPCSC_IMIN(0));
>>> +	I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
>>> +	I915_WRITE_FW(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
>>>    
>>>    	I915_WRITE_FW(SPCSCYGOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
>>>    	I915_WRITE_FW(SPCSCCBOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
>>>    	I915_WRITE_FW(SPCSCCROCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
>>>    }
>>>    
>>> +static void
>>> +vlv_update_clrc(const struct intel_plane_state *plane_state)
>>> +{
>>> +	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
>>> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>>> +	const struct drm_framebuffer *fb = plane_state->base.fb;
>>> +	enum pipe pipe = plane->pipe;
>>> +	enum plane_id plane_id = plane->id;
>>> +	int contrast, brightness, sh_sin, sh_cos;
>>> +
>>> +	if (format_is_yuv(fb->format->format)) {
>>> +		/*
>>> +		 * expand limited range to full range.
>>> +		 * contrast is applied first, then brightness
>>> +		 */
>> I would be happy to see some comment explaining the Brightness/Contrast
>> calculation magic nos, or may be a hint for other developers.
> Y: 16-235 * contrast + brightness -> ~0->255
> CbCr: no hue adjustemnt -> 0 degree angle
>        scale to expand CbCr range from -112-112 to -128-128
>        sh_sin = sin(0) * 128/112 = 0
>        sh_cos = cos(0) * 128/112 = whatever
Yep, (fortunately I am aware of this formula :-)), this same in form of 
a comment ?
With of without this comment, feel free to use:
R-B: Shashank Sharma <shashank.sharma@intel.com>
>>> +		contrast = ((255 << 7) / 219 + 1) >> 1;
>>> +		brightness = -((16 << 1) * 255 / 219 + 1) >> 1;
>>> +		sh_sin = 0;
>>> +		sh_cos = (((128 << 8) / 112) + 1) >> 1;
>>> +	} else {
>>> +		/* pass-through everything */
>>> +		contrast = 1 << 6;
>>> +		brightness = 0;
>>> +		sh_sin = 0;
>>> +		sh_cos = 1 << 7;
>>> +	}
>>> +
>>> +	/* FIXME these register are single buffered :( */
>>> +	I915_WRITE_FW(SPCLRC0(pipe, plane_id),
>>> +		      SP_CONTRAST(contrast) | SP_BRIGHTNESS(brightness));
>>> +	I915_WRITE_FW(SPCLRC1(pipe, plane_id),
>>> +		      SP_SH_SIN(sh_sin) | SP_SH_COS(sh_cos));
>>> +}
>>> +
>>>    static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
>>>    			  const struct intel_plane_state *plane_state)
>>>    {
>>> @@ -456,8 +492,10 @@ vlv_update_plane(struct intel_plane *plane,
>>>    
>>>    	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>>>    
>>> +	vlv_update_clrc(plane_state);
>>> +
>>>    	if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B)
>>> -		chv_update_csc(plane, fb->format->format);
>>> +		chv_update_csc(plane_state);
>>>    
>>>    	if (key->flags) {
>>>    		I915_WRITE_FW(SPKEYMINVAL(pipe, plane_id), key->min_value);

Re: [PATCH v3 1/5] drm/i915: Correctly handle limited range YCbCr data on VLV/CHV

[Ville Syrjälä]

On Tue, Jun 20, 2017 at 08:13:42PM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 6/20/2017 8:04 PM, Ville Syrj�l� wrote:
> > On Tue, Jun 20, 2017 at 07:27:54PM +0530, Sharma, Shashank wrote:
> >> Regards
> >> Shashank
> >>
> >> On 6/20/2017 7:02 PM, ville.syrjala@linux.intel.com wrote:
> >>> From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> >>>
> >>> Turns out the VLV/CHV fixed function sprite CSC expects full range
> >>> data as input. We've been feeding it limited range data to it all
> >>> along. To expand the data out to full range we'll use the color
> >>> correction registers (brightness, contrast, and saturation).
> >>>
> >>> On CHV pipe B we were actually doing the right thing already because we
> >>> progammed the custom CSC matrix to do expect limited range input. Now
> >>> that well pre-expand the data out with the color correction unit, we
> >>> need to change the CSC matrix to operate with full range input instead.
> >>>
> >>> This should make the sprite output of the other pipes match the sprite
> >>> output of pipe B reasonably well. Looking at the resulting pipe CRCs,
> >>> there can be a slight difference in the output, but as I don't know
> >>> the formula used by the fixed function CSC of the other pipes, I don't
> >>> think it's worth the effort to try to match the output exactly. It
> >>> might not even be possible due to difference in internal precision etc.
> >>>
> >>> One slight caveat here is that the color correction registers are single
> >>> bufferred, so we should really be updating them during vblank, but we
> >>> still don't have a mechanism for that, so just toss in another FIXME.
> >>>
> >>> v2: Rebase
> >>> v3: s/bri/brightness/ s/con/contrast/ (Shashank)
> >>>
> >>> Cc: Shashank Sharma <shashank.sharma@intel.com>
> >>> Cc: Jyri Sarha <jsarha@ti.com>
> >>> Cc: "Tang, Jun" <jun.tang@intel.com>
> >>> Reported-by: "Tang, Jun" <jun.tang@intel.com>
> >>> Cc: stable@vger.kernel.org
> >>> Fixes: 7f1f3851feb0 ("drm/i915: sprite support for ValleyView v4")
> >>> Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> >>> ---
> >>>    drivers/gpu/drm/i915/i915_reg.h     | 10 +++++
> >>>    drivers/gpu/drm/i915/intel_sprite.c | 74 ++++++++++++++++++++++++++++---------
> >>>    2 files changed, 66 insertions(+), 18 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >>> index c8647cfa81ba..290322588f56 100644
> >>> --- a/drivers/gpu/drm/i915/i915_reg.h
> >>> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >>> @@ -5974,6 +5974,12 @@ enum {
> >>>    #define _SPATILEOFF		(VLV_DISPLAY_BASE + 0x721a4)
> >>>    #define _SPACONSTALPHA		(VLV_DISPLAY_BASE + 0x721a8)
> >>>    #define   SP_CONST_ALPHA_ENABLE		(1<<31)
> >>> +#define _SPACLRC0		(VLV_DISPLAY_BASE + 0x721d0)
> >>> +#define   SP_CONTRAST(x)		((x) << 18) /* u3.6 */
> >> protection for higher reserved bits ? From 27-31 ? something like ((x &
> >> 1FF) << 18) ?
> > It's unsigned, so no need.
> Humm .. doesn't stops me to pass a bigger value, which will be shifted 
> towards reserved bits ?

We don't hand hold developers quite that much. I did have some ideas in
the past that we might want to have some optional debug checks in these
things to trigger WARNs if a value overflows its bits. But I never got
as far as implementing anything like that.

> >
> >>> +#define   SP_BRIGHTNESS(x)		((x) & 0xff) /* s8 */
> >>> +#define _SPACLRC1		(VLV_DISPLAY_BASE + 0x721d4)
> >>> +#define   SP_SH_SIN(x)			(((x) & 0x7ff) << 16) /* s4.7 */
> >>> +#define   SP_SH_COS(x)			(x) /* u3.7 */
> >>>    
> >> #define   SP_SH_COS(x)			(x & 3FF) /* u3.7 */ ?
> > Also unsigned
> >
> >>> #define _SPAGAMC		(VLV_DISPLAY_BASE + 0x721f4)
> >>>    
> >>>    #define _SPBCNTR		(VLV_DISPLAY_BASE + 0x72280)
> >>> @@ -5987,6 +5993,8 @@ enum {
> >>>    #define _SPBKEYMAXVAL		(VLV_DISPLAY_BASE + 0x722a0)
> >>>    #define _SPBTILEOFF		(VLV_DISPLAY_BASE + 0x722a4)
> >>>    #define _SPBCONSTALPHA		(VLV_DISPLAY_BASE + 0x722a8)
> >>> +#define _SPBCLRC0		(VLV_DISPLAY_BASE + 0x722d0)
> >>> +#define _SPBCLRC1		(VLV_DISPLAY_BASE + 0x722d4)
> >>>    #define _SPBGAMC		(VLV_DISPLAY_BASE + 0x722f4)
> >>>    
> >>>    #define _MMIO_VLV_SPR(pipe, plane_id, reg_a, reg_b) \
> >>> @@ -6003,6 +6011,8 @@ enum {
> >>>    #define SPKEYMAXVAL(pipe, plane_id)	_MMIO_VLV_SPR((pipe), (plane_id), _SPAKEYMAXVAL, _SPBKEYMAXVAL)
> >>>    #define SPTILEOFF(pipe, plane_id)	_MMIO_VLV_SPR((pipe), (plane_id), _SPATILEOFF, _SPBTILEOFF)
> >>>    #define SPCONSTALPHA(pipe, plane_id)	_MMIO_VLV_SPR((pipe), (plane_id), _SPACONSTALPHA, _SPBCONSTALPHA)
> >>> +#define SPCLRC0(pipe, plane_id)		_MMIO_VLV_SPR((pipe), (plane_id), _SPACLRC0, _SPBCLRC0)
> >>> +#define SPCLRC1(pipe, plane_id)		_MMIO_VLV_SPR((pipe), (plane_id), _SPACLRC1, _SPBCLRC1)
> >>>    #define SPGAMC(pipe, plane_id)		_MMIO_VLV_SPR((pipe), (plane_id), _SPAGAMC, _SPBGAMC)
> >>>    
> >>>    /*
> >>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> >>> index 0c650c2cbca8..4462408cc835 100644
> >>> --- a/drivers/gpu/drm/i915/intel_sprite.c
> >>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> >>> @@ -325,44 +325,80 @@ skl_disable_plane(struct intel_plane *plane, struct intel_crtc *crtc)
> >>>    }
> >>>    
> >>>    static void
> >>> -chv_update_csc(struct intel_plane *plane, uint32_t format)
> >>> +chv_update_csc(const struct intel_plane_state *plane_state)
> >>>    {
> >>> +	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> >>>    	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> >>> +	const struct drm_framebuffer *fb = plane_state->base.fb;
> >>>    	enum plane_id plane_id = plane->id;
> >>>    
> >>>    	/* Seems RGB data bypasses the CSC always */
> >>> -	if (!format_is_yuv(format))
> >>> +	if (!format_is_yuv(fb->format->format))
> >>>    		return;
> >>>    
> >>>    	/*
> >>> -	 * BT.601 limited range YCbCr -> full range RGB
> >>> +	 * BT.601 full range YCbCr -> full range RGB
> >>>    	 *
> >>> -	 * |r|   | 6537 4769     0|   |cr  |
> >>> -	 * |g| = |-3330 4769 -1605| x |y-64|
> >>> -	 * |b|   |    0 4769  8263|   |cb  |
> >>> +	 * |r|   | 5743 4096     0|   |cr|
> >>> +	 * |g| = |-2925 4096 -1410| x |y |
> >>> +	 * |b|   |    0 4096  7258|   |cb|
> >>>    	 *
> >>> -	 * Cb and Cr apparently come in as signed already, so no
> >>> -	 * need for any offset. For Y we need to remove the offset.
> >>> +	 * Cb and Cr apparently come in as signed already,
> >>> +	 * and we get full range data in on account of CLRC0/1
> >>>    	 */
> >>> -	I915_WRITE_FW(SPCSCYGOFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(-64));
> >>> +	I915_WRITE_FW(SPCSCYGOFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(0));
> >>>    	I915_WRITE_FW(SPCSCCBOFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(0));
> >>>    	I915_WRITE_FW(SPCSCCROFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(0));
> >>>    
> >>> -	I915_WRITE_FW(SPCSCC01(plane_id), SPCSC_C1(4769) | SPCSC_C0(6537));
> >>> -	I915_WRITE_FW(SPCSCC23(plane_id), SPCSC_C1(-3330) | SPCSC_C0(0));
> >>> -	I915_WRITE_FW(SPCSCC45(plane_id), SPCSC_C1(-1605) | SPCSC_C0(4769));
> >>> -	I915_WRITE_FW(SPCSCC67(plane_id), SPCSC_C1(4769) | SPCSC_C0(0));
> >>> -	I915_WRITE_FW(SPCSCC8(plane_id), SPCSC_C0(8263));
> >>> +	I915_WRITE_FW(SPCSCC01(plane_id), SPCSC_C1(4096) | SPCSC_C0(5743));
> >>> +	I915_WRITE_FW(SPCSCC23(plane_id), SPCSC_C1(-2925) | SPCSC_C0(0));
> >>> +	I915_WRITE_FW(SPCSCC45(plane_id), SPCSC_C1(-1410) | SPCSC_C0(4096));
> >>> +	I915_WRITE_FW(SPCSCC67(plane_id), SPCSC_C1(4096) | SPCSC_C0(0));
> >>> +	I915_WRITE_FW(SPCSCC8(plane_id), SPCSC_C0(7258));
> >>>    
> >>> -	I915_WRITE_FW(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(940) | SPCSC_IMIN(64));
> >>> -	I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
> >>> -	I915_WRITE_FW(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
> >>> +	I915_WRITE_FW(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(1023) | SPCSC_IMIN(0));
> >>> +	I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
> >>> +	I915_WRITE_FW(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
> >>>    
> >>>    	I915_WRITE_FW(SPCSCYGOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
> >>>    	I915_WRITE_FW(SPCSCCBOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
> >>>    	I915_WRITE_FW(SPCSCCROCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
> >>>    }
> >>>    
> >>> +static void
> >>> +vlv_update_clrc(const struct intel_plane_state *plane_state)
> >>> +{
> >>> +	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> >>> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> >>> +	const struct drm_framebuffer *fb = plane_state->base.fb;
> >>> +	enum pipe pipe = plane->pipe;
> >>> +	enum plane_id plane_id = plane->id;
> >>> +	int contrast, brightness, sh_sin, sh_cos;
> >>> +
> >>> +	if (format_is_yuv(fb->format->format)) {
> >>> +		/*
> >>> +		 * expand limited range to full range.
> >>> +		 * contrast is applied first, then brightness
> >>> +		 */
> >> I would be happy to see some comment explaining the Brightness/Contrast
> >> calculation magic nos, or may be a hint for other developers.
> > Y: 16-235 * contrast + brightness -> ~0->255
> > CbCr: no hue adjustemnt -> 0 degree angle
> >        scale to expand CbCr range from -112-112 to -128-128
> >        sh_sin = sin(0) * 128/112 = 0
> >        sh_cos = cos(0) * 128/112 = whatever
> Yep, (fortunately I am aware of this formula :-)), this same in form of 
> a comment ?

Perhaps. If I can write it in a way that doesn't just confuse people
more :P

> With of without this comment, feel free to use:
> R-B: Shashank Sharma <shashank.sharma@intel.com>
> >>> +		contrast = ((255 << 7) / 219 + 1) >> 1;
> >>> +		brightness = -((16 << 1) * 255 / 219 + 1) >> 1;
> >>> +		sh_sin = 0;
> >>> +		sh_cos = (((128 << 8) / 112) + 1) >> 1;
> >>> +	} else {
> >>> +		/* pass-through everything */
> >>> +		contrast = 1 << 6;
> >>> +		brightness = 0;
> >>> +		sh_sin = 0;
> >>> +		sh_cos = 1 << 7;
> >>> +	}
> >>> +
> >>> +	/* FIXME these register are single buffered :( */
> >>> +	I915_WRITE_FW(SPCLRC0(pipe, plane_id),
> >>> +		      SP_CONTRAST(contrast) | SP_BRIGHTNESS(brightness));
> >>> +	I915_WRITE_FW(SPCLRC1(pipe, plane_id),
> >>> +		      SP_SH_SIN(sh_sin) | SP_SH_COS(sh_cos));
> >>> +}
> >>> +
> >>>    static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
> >>>    			  const struct intel_plane_state *plane_state)
> >>>    {
> >>> @@ -456,8 +492,10 @@ vlv_update_plane(struct intel_plane *plane,
> >>>    
> >>>    	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> >>>    
> >>> +	vlv_update_clrc(plane_state);
> >>> +
> >>>    	if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B)
> >>> -		chv_update_csc(plane, fb->format->format);
> >>> +		chv_update_csc(plane_state);
> >>>    
> >>>    	if (key->flags) {
> >>>    		I915_WRITE_FW(SPKEYMINVAL(pipe, plane_id), key->min_value);

-- 
Ville Syrj�l�
Intel OTC