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@xxxxxxxxx>
> ---
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
