[18/18] drm/i915: Add CSC correction for BDW/SKL/BXT

Commit Message

Sharma, Shashank Aug. 6, 2015, 4:38 p.m. UTC

From: Kausal Malladi <[e-mail]>

BDW/SKL/BXT support Color Space Conversion (CSC) using a 3x3 matrix
that needs to be programmed into respective CSC registers.

This patch does the following:
1. Adds the core function to program CSC correction values for
   BDW/SKL/BXT platform
2. Adds CSC correction macros/defines

Signed-off-by: Shashank Sharma <[e-mail]>
Signed-off-by: Kausal Malladi <[e-mail]>
---
 drivers/gpu/drm/i915/i915_reg.h            |  5 ++
 drivers/gpu/drm/i915/intel_color_manager.c | 90 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_color_manager.h | 17 ++++++
 3 files changed, 112 insertions(+)

Comments

Rob Bradford Sept. 30, 2015, 5:49 p.m. UTC | #1

Hi Shashank, some feedback on the CSC code below.


On Thu, 2015-08-06 at 22:08 +0530, Shashank Sharma wrote:
> From: Kausal Malladi <[e-mail]>
> 
> BDW/SKL/BXT support Color Space Conversion (CSC) using a 3x3 matrix
> that needs to be programmed into respective CSC registers.
> 
> This patch does the following:
> 1. Adds the core function to program CSC correction values for
>    BDW/SKL/BXT platform
> 2. Adds CSC correction macros/defines
> 

*snip* 

> +s16 get_csc_s2_7_format(s64 csc_value)
> +{
> +	s32 csc_int_value;
> +	u32 csc_fract_value;
> +	s16 csc_s2_7_format;
> +
> +	if (csc_value >= 0) {
> +		csc_value += GEN9_CSC_FRACT_ROUNDOFF;
> +		if (csc_value > GEN9_CSC_COEFF_MAX)
> +			csc_value = GEN9_CSC_COEFF_MAX;
> +	} else {
> +		csc_value = -csc_value;
> +		csc_value += GEN9_CSC_FRACT_ROUNDOFF;
> +		if (csc_value > GEN9_CSC_COEFF_MAX + 1)
> +			csc_value = GEN9_CSC_COEFF_MAX + 1;
> +		csc_value = -csc_value;
> +	}
> +
> +	csc_int_value = csc_value >> GEN9_CSC_COEFF_SHIFT;
> +	csc_int_value <<= GEN9_CSC_COEFF_INT_SHIFT;
> +	if (csc_value < 0)
> +		csc_int_value |= CSC_COEFF_SIGN;
> +	csc_fract_value = csc_value;
> +	csc_fract_value >>= GEN9_CSC_COEFF_FRACT_SHIFT;
> +	csc_s2_7_format = csc_int_value | csc_fract_value;
> +
> +	return csc_s2_7_format;
> +}

I'm afraid this isn't the right way to calculate the coefficients for
BDW. It doesn't use a fixed s2.7 format rather a limited floating point
(according to the BSpec.) 

I think it's perfectly reasonable to make the decision only to support
the values in that range but then you still need to modify the above
code to set the exponent part to 110b and also shift for the reserved
bits.

> +int gen9_set_csc(struct drm_device *dev, struct drm_property_blob
> *blob,
> +		struct drm_crtc *crtc)
> +{

*snip*

> +	/* Write csc coeff to csc regs */
> +	for (i = 0, j = 0; i < CSC_MAX_VALS; i++) {
> +		word = get_csc_s2_7_format(csc_data->ctm_coeff[i]);
> +		word = word << GEN9_CSC_SHIFT;
> +		if (i % 3 != 2)
> +			word = word |
> +				get_csc_s2_7_format(csc_data
> ->ctm_coeff[i]);
> +
> +		I915_WRITE(reg + j, word);
> +		j = j + 4;
> +	}

*snip*

I'm not sure the above loop is great style, vs explicitly describing
each of the register updates. But for the above code to come close to
working you need to increment i before ORing the second packed value.

Rob

Patch

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 92233ce..31d7ff8 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7943,6 +7943,9 @@  enum skl_disp_power_wells {
 #define PAL_PREC_DATA_A				0x4A404
 #define PAL_PREC_DATA_B				0x4AC04
 #define PAL_PREC_DATA_C				0x4B404
+#define CSC_COEFF_A				0x49010
+#define CSC_COEFF_B				0x49110
+#define CSC_COEFF_C				0x49210
 
 #define _PIPE_CGM_CONTROL(pipe) \
 	(_PIPE3(pipe, PIPEA_CGM_CONTROL, PIPEB_CGM_CONTROL, PIPEC_CGM_CONTROL))
@@ -7957,5 +7960,7 @@  enum skl_disp_power_wells {
 	(_PIPE3(pipe, PAL_PREC_INDEX_A, PAL_PREC_INDEX_B, PAL_PREC_INDEX_C))
 #define _PREC_PAL_DATA(pipe) \
 	(_PIPE3(pipe, PAL_PREC_DATA_A, PAL_PREC_DATA_B, PAL_PREC_DATA_C))
+#define _PIPE_CSC_COEFF(pipe) \
+	(_PIPE3(pipe, CSC_COEFF_A, CSC_COEFF_B, CSC_COEFF_C))
 
 #endif /* _I915_REG_H_ */
diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
index 9f9fb1a..fc08e67 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.c
+++ b/drivers/gpu/drm/i915/intel_color_manager.c
@@ -27,6 +27,94 @@ 
 
 #include "intel_color_manager.h"
 
+s16 get_csc_s2_7_format(s64 csc_value)
+{
+	s32 csc_int_value;
+	u32 csc_fract_value;
+	s16 csc_s2_7_format;
+
+	if (csc_value >= 0) {
+		csc_value += GEN9_CSC_FRACT_ROUNDOFF;
+		if (csc_value > GEN9_CSC_COEFF_MAX)
+			csc_value = GEN9_CSC_COEFF_MAX;
+	} else {
+		csc_value = -csc_value;
+		csc_value += GEN9_CSC_FRACT_ROUNDOFF;
+		if (csc_value > GEN9_CSC_COEFF_MAX + 1)
+			csc_value = GEN9_CSC_COEFF_MAX + 1;
+		csc_value = -csc_value;
+	}
+
+	csc_int_value = csc_value >> GEN9_CSC_COEFF_SHIFT;
+	csc_int_value <<= GEN9_CSC_COEFF_INT_SHIFT;
+	if (csc_value < 0)
+		csc_int_value |= CSC_COEFF_SIGN;
+	csc_fract_value = csc_value;
+	csc_fract_value >>= GEN9_CSC_COEFF_FRACT_SHIFT;
+	csc_s2_7_format = csc_int_value | csc_fract_value;
+
+	return csc_s2_7_format;
+}
+
+int gen9_set_csc(struct drm_device *dev, struct drm_property_blob *blob,
+		struct drm_crtc *crtc)
+{
+	struct drm_ctm *csc_data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 reg, plane_ctl;
+	enum pipe pipe;
+	enum plane plane;
+	s32 word;
+	int i, j;
+
+	if (!blob) {
+		DRM_ERROR("NULL Blob\n");
+		return -EINVAL;
+	}
+
+	if (blob->length != sizeof(struct drm_ctm)) {
+		DRM_ERROR("Invalid length of data received\n");
+		return -EINVAL;
+	}
+
+	csc_data = (struct drm_ctm *)blob->data;
+	if (csc_data->version != GEN9_CSC_DATA_STRUCT_VERSION) {
+		DRM_ERROR("Invalid CSC Data struct version\n");
+		return -EINVAL;
+	}
+
+	pipe = to_intel_crtc(crtc)->pipe;
+	plane = to_intel_crtc(crtc)->plane;
+
+	plane_ctl = I915_READ(PLANE_CTL(pipe, plane));
+	plane_ctl |= PLANE_CTL_PIPE_CSC_ENABLE;
+
+	I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl);
+
+	reg = _PIPE_CSC_COEFF(pipe);
+
+	/* Write csc coeff to csc regs */
+	for (i = 0, j = 0; i < CSC_MAX_VALS; i++) {
+		word = get_csc_s2_7_format(csc_data->ctm_coeff[i]);
+		word = word << GEN9_CSC_SHIFT;
+		if (i % 3 != 2)
+			word = word |
+				get_csc_s2_7_format(csc_data->ctm_coeff[i]);
+
+		I915_WRITE(reg + j, word);
+		j = j + 4;
+	}
+
+	DRM_DEBUG_DRIVER("All CSC values written to registers\n");
+
+	/* Enable CSC functionality */
+	reg = _PIPE_CGM_CONTROL(pipe);
+	I915_WRITE(reg, I915_READ(reg) | CGM_CSC_EN);
+	DRM_DEBUG_DRIVER("CSC enabled on Pipe %c\n", pipe_name(pipe));
+
+	return 0;
+}
+
 s16 get_csc_s3_12_format(s64 csc_value)
 {
 	s32 csc_int_value;
@@ -720,6 +808,8 @@  void intel_color_manager_crtc_commit(struct drm_device *dev,
 		/* CSC correction */
 		if (IS_CHERRYVIEW(dev))
 			ret = chv_set_csc(dev, blob, crtc);
+		else if (IS_BROADWELL(dev) || IS_GEN9(dev))
+			ret = gen9_set_csc(dev, blob, crtc);
 
 		if (ret)
 			DRM_ERROR("set CSC correction failed\n");
diff --git a/drivers/gpu/drm/i915/intel_color_manager.h b/drivers/gpu/drm/i915/intel_color_manager.h
index ca89f25..83e9bc7 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.h
+++ b/drivers/gpu/drm/i915/intel_color_manager.h
@@ -93,6 +93,23 @@ 
 #define CSC_COEFF_SIGN				(1 << 31)
 #define CHV_CSC_COEFF_FRACT_SHIFT		20
 #define CSC_MAX_VALS				9
+/* Gen 9 */
+#define GEN9_CSC_DATA_STRUCT_VERSION		1
+/*
+ * Fractional part is 32 bit, and we need only 7 MSBs for programming
+ * into registers. ROUNDOFF is required to minimize loss of precision.
+ */
+#define GEN9_CSC_FRACT_ROUNDOFF			(1 << 24)
+/*
+ * CSC values are 64-bit values. For GEN9, the maximum CSC value that
+ * user can program is 3.99999..., which can be represented in fixed point
+ * S31.32 format like this, with all fractional bits as 1
+ */
+#define GEN9_CSC_COEFF_MAX			0x00000003FFFFFFFF
+#define GEN9_CSC_COEFF_SHIFT			32
+#define GEN9_CSC_COEFF_INT_SHIFT		29
+#define GEN9_CSC_COEFF_FRACT_SHIFT		25
+#define GEN9_CSC_SHIFT				16
 
 /* CHV CGM Block */
 #define CGM_GAMMA_EN				(1 << 2)