Re: [Intel-gfx] [PATCH v2] drm/i915: support two CSC module on gen11 and later

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

On Fri, Jan 15, 2021 at 01:46:04PM +0800, Lee Shawn C wrote:
> There are two CSC on pipeline on gen11 and later platform.
> User space application is allowed to enable CTM and RGB
> to YCbCr coversion at the same time now.
> 
> v2: check csc capability in {}_color_check function.
> 
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Cooper Chiou <cooper.chiou@intel.com>
> Cc: Shankar Uma <uma.shankar@intel.com>
> 
> Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_color.c   | 45 ++++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_display.c | 13 ------
>  2 files changed, 45 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> index 172d398081ee..22edcd0c9ad5 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -1322,10 +1322,35 @@ static u32 i9xx_gamma_mode(struct intel_crtc_state *crtc_state)
>  		return GAMMA_MODE_MODE_10BIT; /* i965+ only */
>  }
>  
> +static int check_csc(const struct intel_crtc_state *pipe_config)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(pipe_config->uapi.crtc->dev);
> +
> +	if ((INTEL_GEN(dev_priv) < 11) &&
> +	    (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420 ||
> +	     pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR444) &&
> +	     pipe_config->hw.ctm) {

IMO trying to make this generic to all platforms is just making it
more complicated for no good reason. For now I'd just inline this
directly into the two places where we need it.

And if we write it as 

if (crtc_state->output_format != RGB &&
    crtc_state->hw.ctm) {
	drm_dbg_kms(...);
	return -EINVAL;
}

it will nicely match the code that calculates csc_enable.

> +		/*
> +		 * 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.
> +		 */
> +		drm_dbg_kms(&dev_priv->drm,
> +			    "YCBCR420 and CTM together are not possible\n");

Should just say YCbCr here since it's not necessarily 4:2:0 subsampled.

> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static int i9xx_color_check(struct intel_crtc_state *crtc_state)
>  {
>  	int ret;
>  
> +	ret = check_csc(crtc_state);
> +	if (ret)
> +		return ret;
> +
>  	ret = check_luts(crtc_state);
>  	if (ret)
>  		return ret;
> @@ -1374,6 +1399,10 @@ static int chv_color_check(struct intel_crtc_state *crtc_state)
>  {
>  	int ret;
>  
> +	ret = check_csc(crtc_state);
> +	if (ret)
> +		return ret;
> +
>  	ret = check_luts(crtc_state);
>  	if (ret)
>  		return ret;
> @@ -1427,6 +1456,10 @@ static int ilk_color_check(struct intel_crtc_state *crtc_state)
>  {
>  	int ret;
>  
> +	ret = check_csc(crtc_state);
> +	if (ret)
> +		return ret;
> +
>  	ret = check_luts(crtc_state);
>  	if (ret)
>  		return ret;
> @@ -1488,6 +1521,10 @@ static int ivb_color_check(struct intel_crtc_state *crtc_state)
>  	bool limited_color_range = ilk_csc_limited_range(crtc_state);
>  	int ret;
>  
> +	ret = check_csc(crtc_state);
> +	if (ret)
> +		return ret;
> +
>  	ret = check_luts(crtc_state);
>  	if (ret)
>  		return ret;
> @@ -1527,6 +1564,10 @@ static int glk_color_check(struct intel_crtc_state *crtc_state)
>  {
>  	int ret;
>  
> +	ret = check_csc(crtc_state);
> +	if (ret)
> +		return ret;
> +
>  	ret = check_luts(crtc_state);
>  	if (ret)
>  		return ret;
> @@ -1592,6 +1633,10 @@ static int icl_color_check(struct intel_crtc_state *crtc_state)
>  {
>  	int ret;
>  
> +	ret = check_csc(crtc_state);
> +	if (ret)
> +		return ret;
> +
>  	ret = check_luts(crtc_state);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 66990e48c0d4..e60cbe8b0203 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -7835,19 +7835,6 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>  		return -EINVAL;
>  	}
>  
> -	if ((pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420 ||
> -	     pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR444) &&
> -	     pipe_config->hw.ctm) {
> -		/*
> -		 * 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.
> -		 */
> -		drm_dbg_kms(&dev_priv->drm,
> -			    "YCBCR420 and CTM together are not possible\n");
> -		return -EINVAL;
> -	}
> -
>  	/*
>  	 * Pipe horizontal size must be even in:
>  	 * - DVO ganged mode
> -- 
> 2.17.1

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