Re: [PATCH 13/20] drm/i915: prepare csc unit for YCBCR420 output

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Jul 10, 2017 at 04:48:41PM +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)
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_color.c   | 47 +++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_display.c | 17 +++++++++++++
>  2 files changed, 63 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 306c6b0..8a5d211 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -41,6 +41,19 @@
>  
>  #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
> +
> +/* Direct spec values for RGB->YUV conversion matrix */

Are these BT.601 or BT.709 or something else?

> +#define CSC_RGB_TO_YUV_RU_GU 0x2ba809d8
> +#define CSC_RGB_TO_YUV_BU 0x37e80000
> +#define CSC_RGB_TO_YUV_RY_GY 0x1e089cc0
> +#define CSC_RGB_TO_YUV_BY 0xb5280000
> +#define CSC_RGB_TO_YUV_RV_GV 0xbce89ad8
> +#define CSC_RGB_TO_YUV_BV 0x1e080000

IIRC Ander didn't like these, and neither do I. I'd much prefer to reuse
the code we alreayd have for dealing with the CSC. Except I think that's
pretty broken in places so I guess we can go with this for now and try
to clean up the color management stuff later.

> +
>  /*
>   * 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 +104,35 @@ 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);
> +
> +	/* We don't use high values for conversion */

I don't understand what this comment is trying so say.

> +	I915_WRITE(PIPE_CSC_PREOFF_HI(pipe), 0);
> +	I915_WRITE(PIPE_CSC_PREOFF_ME(pipe), 0);
> +	I915_WRITE(PIPE_CSC_PREOFF_LO(pipe), 0);
> +
> +	/* Program direct spec values for RGB to YCBCR conversion matrix */
> +	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);
> +
> +	/* Spec postoffset values */
> +	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);
> +
> +	/* CSC mode before gamma */
> +	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 +143,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 b4a6415..c5ff568 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6288,6 +6288,23 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>  		return -EINVAL;
>  	}
>  
> +	/* YCBCR420 feasibility check */
> +	if (pipe_config->ycbcr420) {
> +		struct drm_crtc_state *drm_state = &pipe_config->base;
> +
> +		/*
> +		 * 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.
> +		 */
> +		if (drm_state->ctm) {
> +			DRM_ERROR("YCBCR420 and CTM is not possible\n");

DRM_DEBUG_KMS

> +			return -EINVAL;
> +		}
> +
> +		DRM_DEBUG_KMS("YCBCR420 output is possible from CRTC\n");

Seems like a fairly pointless debug print.

> +	}
> +
>  	/*
>  	 * Pipe horizontal size must be even in:
>  	 * - DVO ganged mode
> -- 
> 2.7.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx