Re: [CI v4 06/14] drm/i915/display: Program input CSC on SDR planes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Hardware YUV-to-RGB conversion (Input CSC) is incorrectly disabled for HDR planes when a color pipeline is active.
- [Medium] For SDR planes, when a color pipeline is active, the driver fails to set the PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE bit for full-range YCbCr-to-RGB matrix conversions.
--

--- Patch [6]: [CI v4 06/14] drm/i915/display: Program input CSC on SDR planes ---
commit 840c01156b413fde2fac882f08e842c50840876f
Author: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
Subject: drm/i915/display: Program input CSC on SDR planes

This commit programs the SDR plane fixed-function CSC when a color pipeline
is active. It also ensures the legacy color_encoding/color_range properties
remain mutually exclusive with the color pipeline uAPI.

> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index da2df5be9848d..0e951b36cccad 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
[ ... ]
> @@ -1240,33 +1240,63 @@ static u32 glk_plane_color_ctl_crtc(const struct intel_crtc_state *crtc_state)
[ ... ]
>  static u32 glk_plane_color_ctl_input_csc(const struct intel_plane_state *plane_state)
>  {
>  	struct intel_display *display = to_intel_display(plane_state);
>  	const struct drm_framebuffer *fb = plane_state->hw.fb;
>  	struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
> +	bool color_pipeline = plane_state->uapi.state &&
> +		plane_state->uapi.state->plane_color_pipeline;
>  	u32 ctl = 0;
>  
> -	if (!fb->format->is_yuv)
> -		return 0;
> +	if (!color_pipeline) {
> +		if (!fb->format->is_yuv)
> +			return 0;
>  
> -	if (!icl_is_hdr_plane(display, plane->id)) {
> -		switch (plane_state->hw.color_encoding) {
> -		case DRM_COLOR_YCBCR_BT709:
> -			ctl |= PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709;
> -			break;
> -		case DRM_COLOR_YCBCR_BT2020:
> -			ctl |= PLANE_COLOR_CSC_MODE_YUV2020_TO_RGB2020;
> -			break;
> -		default:
> -			ctl |= PLANE_COLOR_CSC_MODE_YUV601_TO_RGB601;
> +		if (!icl_is_hdr_plane(display, plane->id)) {
> +			switch (plane_state->hw.color_encoding) {
[ ... ]
> +			default:
> +				ctl |= PLANE_COLOR_CSC_MODE_YUV601_TO_RGB601;
> +			}
> +		} else {
> +			ctl |= PLANE_COLOR_INPUT_CSC_ENABLE;
>  		}
> -	} else {
> -		ctl |= PLANE_COLOR_INPUT_CSC_ENABLE;
> -	}
>  
> -	if (plane_state->hw.color_range == DRM_COLOR_YCBCR_FULL_RANGE)
> -		ctl |= PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE;
> +		if (plane_state->hw.color_range == DRM_COLOR_YCBCR_FULL_RANGE)
> +			ctl |= PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE;

[Severity: High]
Does this logic inadvertently disable hardware YUV-to-RGB conversion for HDR
planes when a color pipeline is active?

When color_pipeline is true for an HDR plane, the logic in
glk_plane_color_ctl_input_csc() skips the (!color_pipeline) branch entirely.
However, the else-if block below only handles SDR planes, which causes the
function to return 0 and fail to set the PLANE_COLOR_INPUT_CSC_ENABLE bit
for HDR planes.

Since the hardware coefficients might still be programmed in
icl_plane_update_noarm():

	if (fb->format->is_yuv && icl_is_hdr_plane(display, plane_id))
		icl_program_input_csc(dsb, plane, plane_state);

Could this allow raw YUV data to flow directly into the RGB pipeline without
conversion, causing visual corruption when a YUV framebuffer is assigned to
an HDR plane?

> +	} else if (!icl_is_hdr_plane(display, plane->id)) {
> +		if (plane_state->hw.csc_ff_enable)
> +			ctl |= intel_csc_ff_type_to_csc_mode(plane_state->hw.csc_ff_type);

[Severity: Medium]
For SDR planes with an active color pipeline, does this miss setting the
PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE bit for full-range YCbCr-to-RGB
conversions?

If full-range YCbCr input is processed without this bit, would the hardware's
default limited-to-full range correction be applied before the full-range
matrix, causing color distortion?

Even if this path is currently unreachable in practice because the color
pipeline property isn't exposed on SDR planes, should the disable bit still
be explicitly set when appropriate to prevent future issues?

> +	}
>  
>  	return ctl;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713051516.4087420-1-chaitanya.kumar.borah@intel.com?part=6