[Intel-gfx] [v5 5/6] drm/i915/icl: Enable ICL Pipe CSC block

[Matt Roper]

On Tue, Jan 08, 2019 at 01:07:32PM +0530, Uma Shankar wrote:
> Enable ICL pipe csc hardware. CSC block is enabled
> in CSC_MODE register instead of PLANE_COLOR_CTL.
> 
> ToDO: Extend the ABI to accept 32 bit coefficient values
> instead of 16bit for future platforms.
> 
> v2: Addressed Maarten's review comments.
> 
> v3: Addressed Matt's review comments. Removed rmw patterns
> as suggested by Matt.
> 
> v4: Addressed Matt's review comments.
> 
> v5: Addressed Ville's review comments.
> 
> Signed-off-by: Uma Shankar <uma.shankar at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h    | 10 +++++++---
>  drivers/gpu/drm/i915/intel_color.c | 12 ++++++++++--
>  2 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index f29eef7..5a262c0 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -9861,10 +9861,14 @@ enum skl_power_gate {
>  #define _PIPE_A_CSC_COEFF_BU	0x4901c
>  #define _PIPE_A_CSC_COEFF_RV_GV	0x49020
>  #define _PIPE_A_CSC_COEFF_BV	0x49024
> +
>  #define _PIPE_A_CSC_MODE	0x49028
> -#define   CSC_BLACK_SCREEN_OFFSET	(1 << 2)
> -#define   CSC_POSITION_BEFORE_GAMMA	(1 << 1)
> -#define   CSC_MODE_YUV_TO_RGB		(1 << 0)
> +#define  ICL_CSC_ENABLE			(1 << 31)
> +#define  ICL_OUTPUT_CSC_ENABLE		(1 << 30)
> +#define  CSC_BLACK_SCREEN_OFFSET	(1 << 2)
> +#define  CSC_POSITION_BEFORE_GAMMA	(1 << 1)
> +#define  CSC_MODE_YUV_TO_RGB		(1 << 0)
> +
>  #define _PIPE_A_CSC_PREOFF_HI	0x49030
>  #define _PIPE_A_CSC_PREOFF_ME	0x49034
>  #define _PIPE_A_CSC_PREOFF_LO	0x49038
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 9cd4646..c3e4ff6 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -134,7 +134,11 @@ static void ilk_load_ycbcr_conversion_matrix(struct intel_crtc *crtc)
>  	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);
> +
> +	if (INTEL_GEN(dev_priv) >= 11)
> +		I915_WRITE(PIPE_CSC_MODE(pipe), ICL_OUTPUT_CSC_ENABLE);

For gen11+, shouldn't we be programming OUTPUT_CSC_COEFF instead of
CSC_COEFF if we set this bit?

> +	else
> +		I915_WRITE(PIPE_CSC_MODE(pipe), 0);
>  }
>  
>  static void ilk_load_csc_matrix(struct intel_crtc_state *crtc_state)
> @@ -242,7 +246,10 @@ static void ilk_load_csc_matrix(struct intel_crtc_state *crtc_state)
>  		I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), postoff);
>  		I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), postoff);
>  
> -		I915_WRITE(PIPE_CSC_MODE(pipe), 0);
> +		if (INTEL_GEN(dev_priv) >= 11)
> +			I915_WRITE(PIPE_CSC_MODE(pipe), ICL_CSC_ENABLE);
> +		else
> +			I915_WRITE(PIPE_CSC_MODE(pipe), 0);

I might be misinterpreting how the hardware works, but my impression was
that we had two distinct CSC units now on gen11+.  So we could use the
traditional CSC registers to hold the userspace-provided CTM and the new
output CSC registers to hold the fixed RGB->YUV matrix, and they could
both be used at the same time if necessary.  It looks like this function
is still assuming that the two are mutually exclusive and that if we
need RGB->YUV we never bother programming the userspace matrix.  Is that
intentional or an oversight?  Aside from the register definitions
themselves, the bspec seems to be pretty sparse on how the whole
pipeline goes together...


Matt

>  	} else {
>  		uint32_t mode = CSC_MODE_YUV_TO_RGB;
>  
> @@ -700,6 +707,7 @@ void intel_color_init(struct intel_crtc *crtc)
>  		dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
>  		dev_priv->display.load_luts = glk_load_luts;
>  	} else if (IS_ICELAKE(dev_priv)) {
> +		dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
>  		dev_priv->display.load_luts = icl_load_luts;
>  	} else {
>  		dev_priv->display.load_luts = i9xx_load_luts;
> -- 
> 1.9.1
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795