Re: [Intel-gfx] [PATCH 06/12] drm/i915: Utilize crtc_state->csc on chv

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

On 3/29/2023 7:19 PM, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Store the chv cgm csc matrix in the crtc state as well. We
> shall store it in the same place where we store the ilk+
> pipe csc matrix (as opposed to the output csc matrix).
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_color.c | 35 +++++++++++++---------
>   1 file changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> index a6f7eba59e12..7e8820583942 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -431,10 +431,10 @@ static void icl_load_csc_matrix(const struct intel_crtc_state *crtc_state)
>   		icl_update_output_csc(crtc, &crtc_state->output_csc);
>   }
>   
> -static void chv_cgm_csc_convert_ctm(struct intel_csc_matrix *csc,
> -				    const struct drm_property_blob *blob)
> +static void chv_cgm_csc_convert_ctm(const struct intel_crtc_state *crtc_state,
> +				    struct intel_csc_matrix *csc)
>   {
> -	const struct drm_color_ctm *ctm = blob->data;
> +	const struct drm_color_ctm *ctm = crtc_state->hw.ctm->data;
>   	int i;
>   
>   	for (i = 0; i < 9; i++) {
> @@ -457,24 +457,30 @@ static void chv_cgm_csc_convert_ctm(struct intel_csc_matrix *csc,
>   }
>   
>   static void chv_load_cgm_csc(struct intel_crtc *crtc,
> -			     const struct drm_property_blob *blob)
> +			     const struct intel_csc_matrix *csc)
>   {
>   	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
>   	enum pipe pipe = crtc->pipe;
> -	struct intel_csc_matrix tmp;
> -
> -	chv_cgm_csc_convert_ctm(&tmp, blob);
>   
>   	intel_de_write_fw(i915, CGM_PIPE_CSC_COEFF01(pipe),
> -			  tmp.coeff[1] << 16 | tmp.coeff[0]);
> +			  csc->coeff[1] << 16 | csc->coeff[0]);
>   	intel_de_write_fw(i915, CGM_PIPE_CSC_COEFF23(pipe),
> -			  tmp.coeff[3] << 16 | tmp.coeff[2]);
> +			  csc->coeff[3] << 16 | csc->coeff[2]);
>   	intel_de_write_fw(i915, CGM_PIPE_CSC_COEFF45(pipe),
> -			  tmp.coeff[5] << 16 | tmp.coeff[4]);
> +			  csc->coeff[5] << 16 | csc->coeff[4]);
>   	intel_de_write_fw(i915, CGM_PIPE_CSC_COEFF67(pipe),
> -			  tmp.coeff[7] << 16 | tmp.coeff[6]);
> +			  csc->coeff[7] << 16 | csc->coeff[6]);
>   	intel_de_write_fw(i915, CGM_PIPE_CSC_COEFF8(pipe),
> -			  tmp.coeff[8]);
> +			  csc->coeff[8]);
> +}
> +
> +static void chv_assign_csc(struct intel_crtc_state *crtc_state)
> +{
> +	if (crtc_state->hw.ctm) {
> +		chv_cgm_csc_convert_ctm(crtc_state, &crtc_state->csc);
> +	} else {
> +		intel_csc_clear(&crtc_state->csc);
> +	}

It does make sense to have a block for csc and another for output_csc, 
analogous to the load part.

We are anyway adding drm_Warn later, so braces can be dropped here and 
introduced in patch adding warn_ons

Otherwise the Patch looks good to me.

Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>


>   }
>   
>   /* convert hw value with given bit_precision to lut property val */
> @@ -1442,10 +1448,9 @@ static void chv_load_luts(const struct intel_crtc_state *crtc_state)
>   	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
>   	const struct drm_property_blob *pre_csc_lut = crtc_state->pre_csc_lut;
>   	const struct drm_property_blob *post_csc_lut = crtc_state->post_csc_lut;
> -	const struct drm_property_blob *ctm = crtc_state->hw.ctm;
>   
>   	if (crtc_state->cgm_mode & CGM_PIPE_MODE_CSC)
> -		chv_load_cgm_csc(crtc, ctm);
> +		chv_load_cgm_csc(crtc, &crtc_state->csc);
>   
>   	if (crtc_state->cgm_mode & CGM_PIPE_MODE_DEGAMMA)
>   		chv_load_cgm_degamma(crtc, pre_csc_lut);
> @@ -1872,6 +1877,8 @@ static int chv_color_check(struct intel_crtc_state *crtc_state)
>   
>   	intel_assign_luts(crtc_state);
>   
> +	chv_assign_csc(crtc_state);
> +
>   	crtc_state->preload_luts = chv_can_preload_luts(crtc_state);
>   
>   	return 0;