[Intel-gfx] [PATCH] drm/i915/icl: Fix checks for userspace ctm + ycbcr output

[Ville Syrjälä]

On Fri, Feb 15, 2019 at 03:35:41PM -0800, Matt Roper wrote:
> We recently added support for gen11's new "output csc" which can be used
> independently of the regular pipe CSC.  The idea is that this new output
> csc allows us to use a userspace-provided color transformation matrix at
> the same time we drive a YCBCR display mode requiring RGB->YUV
> conversion.  However when landing that support we only updated the color
> management code and overlooked that there was an additional atomic check
> in the modeset path (in intel_crtc_compute_config()) that still assumes
> we only have a single CSC unit to work with.  Let's update that check to
> only apply to pre-gen11 platforms and move it into the color management
> code to ensure it gets called on all commits, not just modesets.
> 
> Also, if we're *only* using the output CSC and not the pipe CSC, there's
> no need to set crtc_state->csc_enable, so let's also not consider the
> output format when setting csc_enable on gen11+ platforms.
> 
> Fixes: a91de580541c ("drm/i915/icl: Enable pipe output csc")
> Cc: Uma Shankar <uma.shankar at intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> ---
> It might also be worth moving the full->limited range RGB conversion
> from the pipe CSC to the output CSC on gen11 at some point.
> 
>  drivers/gpu/drm/i915/intel_color.c   | 16 +++++++++++++---
>  drivers/gpu/drm/i915/intel_display.c | 12 ------------
>  2 files changed, 13 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index da7a07d5ccea..b47e6d1aaaec 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -780,9 +780,10 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
>  	    IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
>  		limited_color_range = crtc_state->limited_color_range;
>  
> -	crtc_state->csc_enable =
> -		crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB ||
> -		crtc_state->base.ctm || limited_color_range;
> +	crtc_state->csc_enable = crtc_state->base.ctm || limited_color_range;
> +	if (INTEL_GEN(dev_priv) < 11)
> +		crtc_state->csc_enable |=
> +			crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB;
>  
>  	ret = intel_color_add_affected_planes(crtc_state);
>  	if (ret)
> @@ -822,6 +823,15 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
>  			crtc_state->csc_mode |= ICL_OUTPUT_CSC_ENABLE;
>  
>  		crtc_state->csc_mode |= ICL_CSC_ENABLE;
> +	} else if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB &&
> +		   crtc_state->base.ctm) {
> +		/*
> +		 * There is only one pipe CSC unit per pipe on pre-gen11, and
> +		 * we need that for output conversion from RGB->YCBCR. So if
> +		 * CTM is already applied we can't support YCBCR output.
> +		 */
> +		DRM_DEBUG_KMS("YCBCR420 and CTM together are not possible\n");
> +		return -EINVAL;

BTW we have quite a few illegal combinations we're not currently
catching. I hacked together some checks for them here:
https://github.com/vsyrjala/linux/commit/0bc55aef05489a7e782bc14b7d5070eea52ce5ea
and then I'm lifting some of those restrictions for pre-glk here
https://github.com/vsyrjala/linux/commit/8db929ee8104c1ee640157d4ea8be15ea27451c5

Sadly on glk lost the flexibility to reorder the csc vs. a single LUT.
I think what we need to do there is to use the gamma LUT to do the
limited range compression, and for the YCbCr case we might just have
to load the software gamma LUT into the hw degamma LUT as best we can.
Otherwise the user may just get a totally unexpected fail when we
automagically decide that limited range/YCbCr output is required.

>  	}
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index afa21daaae51..81bfdbd99092 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6855,18 +6855,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->base.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_DEBUG_KMS("YCBCR420 and CTM together are not possible\n");
> -		return -EINVAL;
> -	}
> -
>  	/*
>  	 * Pipe horizontal size must be even in:
>  	 * - DVO ganged mode
> -- 
> 2.14.5

-- 
Ville Syrjälä
Intel