Re: [Intel-gfx] [PATCH v4 4/6] drm/i915: prepare csc unit for YCBCR420 output

[Imre Deak]

On Mon, Jul 17, 2017 at 08:06:25PM +0530, Shashank Sharma wrote:
> To support ycbcr output, we need a pipe CSC block to do
> RGB->YCBCR conversion.
> 
> Current Intel platforms have only one pipe CSC unit, so
> we can either do color correction using it, or we can perform
> RGB->YCBCR conversion.
> 
> This function adds a csc handler, which uses recommended bspec
> values to perform RGB->YCBCR conversion (target color space BT709)
> 
> V2: Rebase
> V3: Rebase
> V4: Rebase
> V5: Addressed review comments from Ander
> - Remove extra line added in the patch
> - Add the spec details in the commit message
> - Combine two if(cond) while calling intel_crtc_compute_config
> V6: Handle YCBCR420 outputs only (Ville)
> V7: Addressed review comments from Ville:
> - Add description about target colorspace
> - Remove the comments from CSC function
> - DRM_DEBUG->DEBUG_KMS for atomic failure due to CSC unit busy
> - Remove unnecessary debug message about YCBCR420 possibe
> V8: Addressed review comments from Ville:
> - Remove extra comment, not required.
> - Do not add extra variable for CTM, reuse pipe_config
> Added r-b from Ville
> 
> Cc: Ville Syrjala 
> Cc: Daniel Vetter 
> Cc: Ander Conselvan de Oliveira 
> 
> Reviewed-by: Ville Syrjala 
> Signed-off-by: Shashank Sharma 
> ---
>  drivers/gpu/drm/i915/intel_color.c   | 46 
> +++-
>  drivers/gpu/drm/i915/intel_display.c | 11 +
>  2 files changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_color.c 
> b/drivers/gpu/drm/i915/intel_color.c
> index f85d575..8698691 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -41,6 +41,22 @@
>  
>  #define LEGACY_LUT_LENGTH(sizeof(struct drm_color_lut) * 256)
>  
> +/* Post offset values for RGB->YCBCR conversion */
> +#define POSTOFF_RGB_TO_YUV_HI 0x800
> +#define POSTOFF_RGB_TO_YUV_ME 0x100
> +#define POSTOFF_RGB_TO_YUV_LO 0x800
> +
> +/*
> + * These values are direct register values specified in the Bspec,
> + * for RGB->YUV conversion matrix (colorspace BT709)
> + */
> +#define CSC_RGB_TO_YUV_RU_GU 0x2ba809d8
> +#define CSC_RGB_TO_YUV_BU 0x37e8
> +#define CSC_RGB_TO_YUV_RY_GY 0x1e089cc0
> +#define CSC_RGB_TO_YUV_BY 0xb528
> +#define CSC_RGB_TO_YUV_RV_GV 0xbce89ad8
> +#define CSC_RGB_TO_YUV_BV 0x1e08
> +
>  /*
>   * Extract the CSC coefficient from a CTM coefficient (in U32.32 fixed point
>   * format). This macro takes the coefficient we want transformed and the
> @@ -91,6 +107,31 @@ static void ctm_mult_by_limited(uint64_t *result, int64_t 
> *input)
>   }
>  }
>  
> +void i9xx_load_ycbcr_conversion_matrix(struct intel_crtc *intel_crtc)
> +{
> + int pipe = intel_crtc->pipe;
> + struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
> +
> +

Extra w/s.

> + I915_WRITE(PIPE_CSC_PREOFF_HI(pipe), 0);
> + I915_WRITE(PIPE_CSC_PREOFF_ME(pipe), 0);
> + I915_WRITE(PIPE_CSC_PREOFF_LO(pipe), 0);
> +
> + I915_WRITE(PIPE_CSC_COEFF_RU_GU(pipe), CSC_RGB_TO_YUV_RU_GU);
> + I915_WRITE(PIPE_CSC_COEFF_BU(pipe), CSC_RGB_TO_YUV_BU);
> +
> + I915_WRITE(PIPE_CSC_COEFF_RY_GY(pipe), CSC_RGB_TO_YUV_RY_GY);
> + I915_WRITE(PIPE_CSC_COEFF_BY(pipe), CSC_RGB_TO_YUV_BY);
> +
> + I915_WRITE(PIPE_CSC_COEFF_RV_GV(pipe), CSC_RGB_TO_YUV_RV_GV);
> + I915_WRITE(PIPE_CSC_COEFF_BV(pipe), CSC_RGB_TO_YUV_BV);
> +
> + I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), POSTOFF_RGB_TO_YUV_HI);
> + I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), POSTOFF_RGB_TO_YUV_ME);
> + I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), POSTOFF_RGB_TO_YUV_LO);
> + I915_WRITE(PIPE_CSC_MODE(pipe), 0);
> +}
> +
>  /* Set up the pipe CSC unit. */
>  static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state)
>  {
> @@ -101,7 +142,10 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state 
> *crtc_state)
>   uint16_t coeffs[9] = { 0, };
>   struct intel_crtc_state *intel_crtc_state = 
> to_intel_crtc_state(crtc_state);
>  
> - if (crtc_state->ctm) {
> + if (intel_crtc_state->ycbcr420) {
> + i9xx_load_ycbcr_conversion_matrix(intel_crtc);
> + return;
> + } else if (crtc_state->ctm) {
>   struct drm_color_ctm *ctm =
>   (struct drm_color_ctm *)crtc_state->ctm->data;
>   uint64_t input[9] = { 0, };
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index 788502a..fb60811 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6282,6 +6282,17 @@ static int intel_crtc_compute_config(struct intel_crtc 
> *crtc,
>   return -EINVAL;
>   }
>  
> + if (pipe_config->ycbcr420 && pipe_config->base.ctm) {
> +

Extra w/s.

> + /*
> +

[Intel-gfx] [PATCH v4 4/6] drm/i915: prepare csc unit for YCBCR420 output

[Shashank Sharma]

To support ycbcr output, we need a pipe CSC block to do
RGB->YCBCR conversion.

Current Intel platforms have only one pipe CSC unit, so
we can either do color correction using it, or we can perform
RGB->YCBCR conversion.

This function adds a csc handler, which uses recommended bspec
values to perform RGB->YCBCR conversion (target color space BT709)

V2: Rebase
V3: Rebase
V4: Rebase
V5: Addressed review comments from Ander
- Remove extra line added in the patch
- Add the spec details in the commit message
- Combine two if(cond) while calling intel_crtc_compute_config
V6: Handle YCBCR420 outputs only (Ville)
V7: Addressed review comments from Ville:
- Add description about target colorspace
- Remove the comments from CSC function
- DRM_DEBUG->DEBUG_KMS for atomic failure due to CSC unit busy
- Remove unnecessary debug message about YCBCR420 possibe
V8: Addressed review comments from Ville:
- Remove extra comment, not required.
- Do not add extra variable for CTM, reuse pipe_config
Added r-b from Ville

Cc: Ville Syrjala 
Cc: Daniel Vetter 
Cc: Ander Conselvan de Oliveira 

Reviewed-by: Ville Syrjala 
Signed-off-by: Shashank Sharma 
---
 drivers/gpu/drm/i915/intel_color.c   | 46 +++-
 drivers/gpu/drm/i915/intel_display.c | 11 +
 2 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_color.c 
b/drivers/gpu/drm/i915/intel_color.c
index f85d575..8698691 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -41,6 +41,22 @@
 
 #define LEGACY_LUT_LENGTH  (sizeof(struct drm_color_lut) * 256)
 
+/* Post offset values for RGB->YCBCR conversion */
+#define POSTOFF_RGB_TO_YUV_HI 0x800
+#define POSTOFF_RGB_TO_YUV_ME 0x100
+#define POSTOFF_RGB_TO_YUV_LO 0x800
+
+/*
+ * These values are direct register values specified in the Bspec,
+ * for RGB->YUV conversion matrix (colorspace BT709)
+ */
+#define CSC_RGB_TO_YUV_RU_GU 0x2ba809d8
+#define CSC_RGB_TO_YUV_BU 0x37e8
+#define CSC_RGB_TO_YUV_RY_GY 0x1e089cc0
+#define CSC_RGB_TO_YUV_BY 0xb528
+#define CSC_RGB_TO_YUV_RV_GV 0xbce89ad8
+#define CSC_RGB_TO_YUV_BV 0x1e08
+
 /*
  * Extract the CSC coefficient from a CTM coefficient (in U32.32 fixed point
  * format). This macro takes the coefficient we want transformed and the
@@ -91,6 +107,31 @@ static void ctm_mult_by_limited(uint64_t *result, int64_t 
*input)
}
 }
 
+void i9xx_load_ycbcr_conversion_matrix(struct intel_crtc *intel_crtc)
+{
+   int pipe = intel_crtc->pipe;
+   struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
+
+
+   I915_WRITE(PIPE_CSC_PREOFF_HI(pipe), 0);
+   I915_WRITE(PIPE_CSC_PREOFF_ME(pipe), 0);
+   I915_WRITE(PIPE_CSC_PREOFF_LO(pipe), 0);
+
+   I915_WRITE(PIPE_CSC_COEFF_RU_GU(pipe), CSC_RGB_TO_YUV_RU_GU);
+   I915_WRITE(PIPE_CSC_COEFF_BU(pipe), CSC_RGB_TO_YUV_BU);
+
+   I915_WRITE(PIPE_CSC_COEFF_RY_GY(pipe), CSC_RGB_TO_YUV_RY_GY);
+   I915_WRITE(PIPE_CSC_COEFF_BY(pipe), CSC_RGB_TO_YUV_BY);
+
+   I915_WRITE(PIPE_CSC_COEFF_RV_GV(pipe), CSC_RGB_TO_YUV_RV_GV);
+   I915_WRITE(PIPE_CSC_COEFF_BV(pipe), CSC_RGB_TO_YUV_BV);
+
+   I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), POSTOFF_RGB_TO_YUV_HI);
+   I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), POSTOFF_RGB_TO_YUV_ME);
+   I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), POSTOFF_RGB_TO_YUV_LO);
+   I915_WRITE(PIPE_CSC_MODE(pipe), 0);
+}
+
 /* Set up the pipe CSC unit. */
 static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state)
 {
@@ -101,7 +142,10 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state 
*crtc_state)
uint16_t coeffs[9] = { 0, };
struct intel_crtc_state *intel_crtc_state = 
to_intel_crtc_state(crtc_state);
 
-   if (crtc_state->ctm) {
+   if (intel_crtc_state->ycbcr420) {
+   i9xx_load_ycbcr_conversion_matrix(intel_crtc);
+   return;
+   } else if (crtc_state->ctm) {
struct drm_color_ctm *ctm =
(struct drm_color_ctm *)crtc_state->ctm->data;
uint64_t input[9] = { 0, };
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 788502a..fb60811 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6282,6 +6282,17 @@ static int intel_crtc_compute_config(struct intel_crtc 
*crtc,
return -EINVAL;
}
 
+   if (pipe_config->ycbcr420 && pipe_config->base.ctm) {
+
+   /*
+* There is only one pipe CSC unit per pipe, and we need that
+* for output conversion from RGB->YCBCR. So if CTM is already
+* applied we can't support YCBCR420 output.
+*/
+   DRM_DEBUG_KMS("YCBCR420 and CTM