Re: [CI 1/9] drm/i915/color: Add CSC on SDR plane color pipeline

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] The patch relies on core DRM functions and enum values that are not defined anywhere in the kernel tree.
- [Medium] The newly added SDR plane color pipeline is entirely unreachable because the caller function restricts pipeline initialization exclusively to HDR planes.
- [Medium] The fixed-function CSC colorop state is exposed to userspace, but the driver completely ignores its configuration when programming the hardware.

Pre-existing issues:
- [High] The error cleanup path for colorop initialization calls `drm_colorop_cleanup` on partially initialized objects, causing a NULL pointer dereference.
- [High] Missing error handling for `kasprintf` memory allocation failure leads to a kernel panic.
--

commit d4c7800e456ca3d700da2c92787c702c23a7fcd5
Author: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
Subject: drm/i915/color: Add CSC on SDR plane color pipeline

This commit introduces a fixed-function CSC block to the SDR plane color
pipeline as a DRM_COLOROP_FIXED_MATRIX color operation. It adds the new
pipeline definition and initializes it for SDR planes during setup.

> diff --git a/drivers/gpu/drm/i915/display/intel_color_pipeline.c b/drivers/gpu/drm/i915/display/intel_color_pipeline.c
> index 6cf8080ee8000..efd4375c43318 100644
> --- a/drivers/gpu/drm/i915/display/intel_color_pipeline.c
> +++ b/drivers/gpu/drm/i915/display/intel_color_pipeline.c
[ ... ]
> @@ -43,6 +43,18 @@ static const enum intel_color_block hdr_plane_pipeline[] = {
>  	INTEL_PLANE_CB_POST_CSC_LUT,
>  };
>  
> +static const enum intel_color_block sdr_plane_yuv_pipeline[] = {
> +	INTEL_PLANE_CB_CSC_FF,
> +};
> +
> +static const u64 intel_plane_supported_csc_ff =
> +		BIT(DRM_COLOROP_FM_YCBCR601_FULL_RGB) |
> +		BIT(DRM_COLOROP_FM_YCBCR601_LIMITED_RGB) |
> +		BIT(DRM_COLOROP_FM_YCBCR709_FULL_RGB) |
> +		BIT(DRM_COLOROP_FM_YCBCR709_LIMITED_RGB) |
> +		BIT(DRM_COLOROP_FM_YCBCR2020_NC_FULL_RGB) |
> +		BIT(DRM_COLOROP_FM_YCBCR2020_NC_LIMITED_RGB);

[Severity: High]
Are these enum values (e.g., DRM_COLOROP_FM_YCBCR601_FULL_RGB) defined
in the kernel tree? It appears they are not defined anywhere, which would
cause a compilation failure. Is there a prerequisite patch missing from
this series?

[ ... ]
> @@ -92,6 +104,12 @@ struct intel_colorop *intel_color_pipeline_plane_add_colorop(struct drm_plane *p
>  							  DRM_COLOROP_LUT1D_INTERPOLATION_LINEAR,
>  							  DRM_COLOROP_FLAG_ALLOW_BYPASS);
>  		break;
> +	case INTEL_PLANE_CB_CSC_FF:
> +		ret = drm_plane_colorop_fixed_matrix_init(dev, &colorop->base, plane,
> +							  &intel_colorop_funcs,
> +							  intel_plane_supported_csc_ff,
> +							  DRM_COLOROP_FLAG_ALLOW_BYPASS);

[Severity: High]
Is drm_plane_colorop_fixed_matrix_init() implemented in another patch?
This core DRM function does not seem to be defined in the tree, which will
cause a build failure.

[Severity: High]
This isn't a bug introduced by this patch, but can the error path for this 
initialization lead to a NULL pointer dereference? 

If drm_plane_colorop_fixed_matrix_init() fails early, it jumps to the
cleanup label:

	if (ret)
		goto cleanup;
    ...
cleanup:
	intel_colorop_destroy(&colorop->base);

This calls drm_colorop_cleanup(), which dereferences colorop->dev:

void drm_colorop_cleanup(struct drm_colorop *colorop)
{
	struct drm_device *dev = colorop->dev;
	struct drm_mode_config *config = &dev->mode_config;

Since colorop was just allocated with kzalloc and colorop->dev might not
be set yet if initialization fails early, could dev be NULL here?

> +		break;
>  	default:
>  		drm_err(plane->dev, "Invalid colorop id [%d]", id);
>  		ret = -EINVAL;
> @@ -126,9 +144,12 @@ int _intel_color_pipeline_plane_init(struct drm_plane *plane, struct drm_prop_en
>  	if (plane_has_3dlut(display, pipe, plane)) {
>  		pipeline = xe3plpd_primary_plane_pipeline;
>  		pipeline_len = ARRAY_SIZE(xe3plpd_primary_plane_pipeline);
> -	} else {
> +	} else if (icl_is_hdr_plane(display, to_intel_plane(plane)->id)) {
>  		pipeline = hdr_plane_pipeline;
>  		pipeline_len = ARRAY_SIZE(hdr_plane_pipeline);
> +	} else {
> +		pipeline = sdr_plane_yuv_pipeline;
> +		pipeline_len = ARRAY_SIZE(sdr_plane_yuv_pipeline);
>  	}

[Severity: Medium]
Will this newly added SDR pipeline logic ever be reached? The caller
function intel_color_pipeline_plane_init() unconditionally restricts
pipeline initialization to HDR planes with an early return:

int intel_color_pipeline_plane_init(...)
{
    ...
	/* Currently expose pipeline only for HDR planes */
	if (!icl_is_hdr_plane(display, to_intel_plane(plane)->id))
		return 0;

	ret = _intel_color_pipeline_plane_init(plane, &pipelines[len], pipe);
    ...
}

Does this early return make the sdr_plane_yuv_pipeline dead code?

[Severity: High]
This isn't a bug introduced by this patch, but does the kasprintf() call 
slightly below this block in _intel_color_pipeline_plane_init() need a 
NULL check?

	list->name = kasprintf(GFP_KERNEL, "Color Pipeline %d", ...);
    return 0;

If kasprintf() fails due to OOM and returns NULL, the function still
returns 0 (success). Could this propagate a NULL pointer to 
drm_property_add_enum(), leading to a kernel panic when it tries to read
the name length?

[Severity: Medium]
Once the fixed-function CSC colorop state (INTEL_PLANE_CB_CSC_FF) is set 
by userspace, does the driver correctly apply it to the hardware?

Looking at intel_plane_color_copy_uapi_to_hw_state(), there appears to be no
logic in intel_plane_colorop_replace_blob() to extract the new fixed matrix 
configuration. Furthermore, the hardware programming in glk_plane_color_ctl()
continues to rely on the legacy hw.color_encoding field:

static u32 glk_plane_color_ctl(const struct intel_plane_state *plane_state)
{
    ...
	if (fb->format->is_yuv && !icl_is_hdr_plane(display, plane->id)) {
		switch (plane_state->hw.color_encoding) {
		case DRM_COLOR_YCBCR_BT709:
    ...

Could this result in the hardware being programmed with the legacy state
instead of the newly requested color pipeline configuration?

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