On Thu, Nov 29, 2018 at 08:21:42PM +0530, Uma Shankar wrote:
> Enable ICL pipe csc hardware. CSC block is enabled
> in CSC_MODE register instead of PLANE_COLOR_CTL.
>
> v2: Addressed Maarten's review comments.
>
> v3: Addressed Matt's review comments. Removed rmw patterns
> as suggested by Matt.
>
> Signed-off-by: Uma Shankar <[email protected]>
> ---
> drivers/gpu/drm/i915/i915_reg.h | 2 ++
> drivers/gpu/drm/i915/intel_color.c | 28 +++++++++++++++++++++++-----
> drivers/gpu/drm/i915/intel_drv.h | 3 +++
> 3 files changed, 28 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index b0147bf..6470008 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -9824,6 +9824,8 @@ enum skl_power_gate {
> #define _PIPE_A_CSC_COEFF_RV_GV 0x49020
> #define _PIPE_A_CSC_COEFF_BV 0x49024
> #define _PIPE_A_CSC_MODE 0x49028
> +#define CSC_ENABLE (1 << 31)
> +#define 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)
> diff --git a/drivers/gpu/drm/i915/intel_color.c
> b/drivers/gpu/drm/i915/intel_color.c
> index 7c8c996..414e1ca 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -108,10 +108,14 @@ static u64 *ctm_mult_by_limited(u64 *result, const u64
> *input)
> return result;
> }
>
> -static void ilk_load_ycbcr_conversion_matrix(struct intel_crtc *intel_crtc)
> +static void ilk_load_ycbcr_conversion_matrix(struct drm_crtc_state
> *crtc_state)
It's generally preferred to use Intel types for these lower-level
functions rather than DRM types. It looks like a lot of the preexisting
color management code breaks from this convention, so we might want to
also clean that up in the preexisting code as a separate patch.
> {
> + struct drm_crtc *crtc = crtc_state->crtc;
> + struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> int pipe = intel_crtc->pipe;
> - struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
> + struct intel_crtc_state *intel_crtc_state =
> + to_intel_crtc_state(crtc_state);
>
> I915_WRITE(PIPE_CSC_PREOFF_HI(pipe), 0);
> I915_WRITE(PIPE_CSC_PREOFF_ME(pipe), 0);
> @@ -129,7 +133,11 @@ static void ilk_load_ycbcr_conversion_matrix(struct
> intel_crtc *intel_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)
> + intel_crtc_state->csc_mode |= CSC_ENABLE;
Shouldn't fixed RGB->YUV be done with OUTPUT_CSC_ENABLE now that we have
two CSC's to work with instead of just one? I.e., I think we can leave
the first pipe CSC for the userspace-defined transformation (or identity
if none is provided) and then enable the output CSC if we need to
convert to YUV and/or compress to limited range.
Matt
> + else
> + intel_crtc_state->csc_mode = 0;
> }
>
> static void ilk_load_csc_matrix(struct drm_crtc_state *crtc_state)
> @@ -151,7 +159,7 @@ static void ilk_load_csc_matrix(struct drm_crtc_state
> *crtc_state)
>
> if (intel_crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420 ||
> intel_crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444) {
> - ilk_load_ycbcr_conversion_matrix(intel_crtc);
> + ilk_load_ycbcr_conversion_matrix(crtc_state);
> return;
> } else if (crtc_state->ctm) {
> struct drm_color_ctm *ctm = crtc_state->ctm->data;
> @@ -239,7 +247,10 @@ static void ilk_load_csc_matrix(struct drm_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)
> + intel_crtc_state->csc_mode |= CSC_ENABLE;
> + else
> + intel_crtc_state->csc_mode = 0;
> } else {
> uint32_t mode = CSC_MODE_YUV_TO_RGB;
>
> @@ -305,9 +316,15 @@ void intel_color_set_csc(struct drm_crtc_state
> *crtc_state)
> {
> struct drm_device *dev = crtc_state->crtc->dev;
> struct drm_i915_private *dev_priv = to_i915(dev);
> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->crtc);
> + struct intel_crtc_state *intel_crtc_state =
> + to_intel_crtc_state(crtc_state);
> + enum pipe pipe = intel_crtc->pipe;
>
> if (dev_priv->display.load_csc_matrix)
> dev_priv->display.load_csc_matrix(crtc_state);
> +
> + I915_WRITE(PIPE_CSC_MODE(pipe), intel_crtc_state->csc_mode);
> }
>
> /* Loads the legacy palette/gamma unit for the CRTC. */
> @@ -734,6 +751,7 @@ void intel_color_init(struct drm_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;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index a62d77b..e7b77c1 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -922,6 +922,9 @@ struct intel_crtc_state {
> /* Gamma mode programmed on the pipe */
> uint32_t gamma_mode;
>
> + /* CSC mode programmed on the pipe */
> + uint32_t csc_mode;
> +
> /* bitmask of visible planes (enum plane_id) */
> u8 active_planes;
> u8 nv12_planes;
> --
> 1.9.1
>
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
