Re: [CI 9/9] drm/i915/color: Add YUV buffer support on HDR planes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Logic Error / Fatal Initialization Error: Unhandled color block in `intel_color_pipeline_plane_add_colorop`.
- [High] Hardware Configuration Error: `PLANE_COLOR_INPUT_CSC_ENABLE` is ignored for RGB framebuffers when `csc_ff_enable` is true.
- [Low] Compilation Failure: Undeclared enum value `INTEL_PLANE_CB_CSC_FF`.
- [Low] Compilation Failure: Undeclared struct member `csc_ff_enable`.
--

commit 1c6f477c5724be8b1a87f5ec970f9c13fe79be33
Author: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>

drm/i915/color: Add YUV buffer support on HDR planes

This commit adds the INTEL_PLANE_CB_CSC_FF color block as the first stage
for HDR planes to enable YUV-to-RGB color space conversion. It also updates
icl_program_input_csc() to account for color pipeline programming.

> diff --git a/drivers/gpu/drm/i915/display/intel_color_pipeline.c b/drivers/gpu/drm/i915/display/intel_color_pipeline.c
> index 6cf8080ee8000..71451a196e165 100644
> --- a/drivers/gpu/drm/i915/display/intel_color_pipeline.c
> +++ b/drivers/gpu/drm/i915/display/intel_color_pipeline.c
[ ... ]
> @@ -31,6 +31,7 @@ static const struct drm_colorop_funcs intel_colorop_funcs = {
>   * the pipeline totally unusable.
>   */
>  static const enum intel_color_block xe3plpd_primary_plane_pipeline[] = {
> +	INTEL_PLANE_CB_CSC_FF,

[Severity: Low]
Is INTEL_PLANE_CB_CSC_FF defined in this tree? It appears to be an
undeclared enum value here, which will cause a compilation failure.

>  	INTEL_PLANE_CB_PRE_CSC_LUT,
>  	INTEL_PLANE_CB_CSC,
[ ... ]
> @@ -38,6 +39,7 @@ static const enum intel_color_block xe3plpd_primary_plane_pipeline[] = {
>  };
>  
>  static const enum intel_color_block hdr_plane_pipeline[] = {
> +	INTEL_PLANE_CB_CSC_FF,

[Severity: High]
How is this new color block handled during pipeline creation? Looking at
intel_color_pipeline_plane_add_colorop(), there doesn't seem to be a case
for INTEL_PLANE_CB_CSC_FF, meaning it will hit the default path:

intel_color_pipeline_plane_add_colorop() {
    ...
    default:
        drm_err(plane->dev, "Invalid colorop id [%d]", id);
        ret = -EINVAL;
    ...
}

Will this cause pipeline creation to immediately abort and return -EINVAL
when encountering this new block?

>  	INTEL_PLANE_CB_PRE_CSC_LUT,
>  	INTEL_PLANE_CB_CSC,
[ ... ]
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 5cda1ab90e40f..c6ec77c38cc50 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
[ ... ]
> @@ -1616,7 +1616,8 @@ icl_plane_update_noarm(struct intel_dsb *dsb,
>  	intel_de_write_dsb(display, dsb, PLANE_COLOR_CTL(pipe, plane_id),
>  			   plane_color_ctl);
>  
> -	if (fb->format->is_yuv && icl_is_hdr_plane(display, plane_id))
> +	if (icl_is_hdr_plane(display, plane_id) &&
> +	    (fb->format->is_yuv || plane_state->hw.csc_ff_enable))

[Severity: Low]
Does struct intel_plane_state contain csc_ff_enable in its hw member?
This looks like an undeclared struct member that will break the build.

[Severity: High]
When csc_ff_enable is explicitly requested for RGB buffers, will the
hardware actually enable the CSC block?

In glk_plane_color_ctl(), the enable bit is only set if the framebuffer
format is YUV:

glk_plane_color_ctl() {
    ...
    } else if (fb->format->is_yuv) {
        plane_color_ctl |= PLANE_COLOR_INPUT_CSC_ENABLE;
    ...
}

Does this mean the fixed-function CSC hardware block remains disabled in
PLANE_COLOR_CTL when we have an RGB framebuffer with csc_ff_enable set to
true?

>  		icl_program_input_csc(dsb, plane, plane_state);
>  
>  	skl_write_plane_wm(dsb, plane, crtc_state);

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