Re: [DO_NOT_REVIEW v2 1/2] drm/i915/color: Add color pipeline support for SDR planes

[sashiko-bot@kernel.org]

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

New issues:
- [Low] The commit references undefined DRM core UAPI structures, enums, and functions, causing a compilation failure.

Pre-existing issues:
- [High] Unchecked memory allocation via `kasprintf` leads to a NULL pointer dereference when creating the color pipeline property.
--

commit c28893e0173187dd4a064073c1ffc14ec288c788
Author: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
drm/i915/color: Add color pipeline support for SDR planes

This commit adds color pipeline support for SDR planes in the i915 driver.
It introduces fixed matrix handling for YUV to RGB conversions and exposes
color pipelines for SDR planes alongside the existing HDR support.

Link: https://lore.kernel.org/intel-gfx/20260804123107.2256124-1-chaitanya.kumar.borah@intel.com/

> diff --git a/drivers/gpu/drm/i915/display/intel_color_pipeline.c b/drivers/gpu/drm/i915/display/intel_color_pipeline.c
> index 6cf8080ee8000..38cfd6ed585d0 100644
> --- a/drivers/gpu/drm/i915/display/intel_color_pipeline.c
> +++ b/drivers/gpu/drm/i915/display/intel_color_pipeline.c
> @@ -92,6 +105,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);
> +		break;

[Severity: Low]
Does this code rely on out-of-tree patches? The function
drm_plane_colorop_fixed_matrix_init does not appear to exist in the current
DRM core tree, which causes a build failure in
intel_color_pipeline_plane_add_colorop when compiling the kernel with this
patch applied.

[ ... ]

> @@ -126,9 +145,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: High]
This is a pre-existing issue, but later in _intel_color_pipeline_plane_init,
kasprintf is used to allocate the name property for the color pipeline:

    list->name = kasprintf(GFP_KERNEL, "Color Pipeline %d",
                           colorop[0]->base.base.id);
    return 0;

If memory allocation fails under pressure and kasprintf returns a NULL pointer,
the function still returns 0 (success). This NULL pointer is subsequently
passed to drm_property_add_enum via drm_plane_create_color_pipeline_property.

Since drm_property_add_enum calls strlen on the name, would this lead to a
NULL pointer dereference and kernel panic during driver initialization?

> diff --git a/drivers/gpu/drm/i915/display/intel_plane.c b/drivers/gpu/drm/i915/display/intel_plane.c
> index d0f99a87c42e8..25ca049009efb 100644
> --- a/drivers/gpu/drm/i915/display/intel_plane.c
> +++ b/drivers/gpu/drm/i915/display/intel_plane.c
> @@ -462,6 +462,42 @@ intel_plane_colorop_replace_blob(struct intel_plane_state *plane_state,
>  	return false;
>  }
>  
> +static u32
> +fixedmatrix_colorop_to_encoding(enum drm_colorop_fixed_matrix_type fm_type)
> +{
> +	switch (fm_type) {
> +	case DRM_COLOROP_FM_YCBCR709_FULL_RGB:

[Severity: Low]
Are the new format macros defined in the DRM core? Using
DRM_COLOROP_FM_YCBCR709_FULL_RGB (and the related macros below) results in a
compilation failure in fixedmatrix_colorop_to_encoding because the values
appear to be undefined in the current tree.

[ ... ]

> @@ -485,11 +522,23 @@ intel_plane_color_copy_uapi_to_hw_state(struct intel_atomic_state *state,
>  	while (iter_colorop) {
>  		for_each_new_colorop_in_state(&state->base, colorop, new_colorop_state, i) {
>  			if (new_colorop_state->colorop == iter_colorop) {
> -				blob = new_colorop_state->bypass ? NULL : new_colorop_state->data;
>  				intel_colorop = to_intel_colorop(colorop);
> -				changed |= intel_plane_colorop_replace_blob(plane_state,
> +				if (intel_colorop->id == INTEL_PLANE_CB_CSC_FF) {
> +					fm_type = new_colorop_state->fixed_matrix_type;

[Severity: Low]
Does drm_colorop_state include a fixed_matrix_type field? Accessing
new_colorop_state->fixed_matrix_type in intel_plane_color_copy_uapi_to_hw_state
causes a build failure because the field is not present in the struct
drm_colorop_state definition in the DRM UAPI headers.

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