Re: [PATCH 13/13] drm/i915/color: Add failure handling in plane color pipeline init

["Borah, Chaitanya Kumar" <chaitanya.kumar.borah@intel.com>]

On 1/6/2026 9:25 AM, Kandpal, Suraj wrote:
>> Subject: [PATCH 13/13] drm/i915/color: Add failure handling in plane color
>> pipeline init
>>
>> The plane color pipeline initialization built up multiple colorop blocks inline,
>> but did not reliably clean up partially constructed pipelines when an
>> intermediate step failed. This could lead to leaked colorop objects and fragile
>> error handling as the pipeline grows.
>>
>> Refactor the pipeline construction to use a common helper for adding colorop
>> blocks. This centralizes allocation, initialization, and teardown logic, allowing
>> the caller to reliably unwind all previously created colorops on failure.
>>
>> Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
>> ---
>>   .../drm/i915/display/intel_color_pipeline.c   | 142 ++++++++++++------
>>   1 file changed, 100 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_color_pipeline.c
>> b/drivers/gpu/drm/i915/display/intel_color_pipeline.c
>> index 8fecc53540ba..035ec3f022cd 100644
>> --- a/drivers/gpu/drm/i915/display/intel_color_pipeline.c
>> +++ b/drivers/gpu/drm/i915/display/intel_color_pipeline.c
>> @@ -2,6 +2,8 @@
>>   /*
>>    * Copyright © 2025 Intel Corporation
>>    */
>> +#include <drm/drm_print.h>
>> +
>>   #include "intel_color.h"
>>   #include "intel_colorop.h"
>>   #include "intel_color_pipeline.h"
>> @@ -10,6 +12,7 @@
>>   #include "skl_universal_plane.h"
>>
>>   #define MAX_COLOR_PIPELINES 1
>> +#define MAX_COLOROP 4
>>   #define PLANE_DEGAMMA_SIZE 128
>>   #define PLANE_GAMMA_SIZE 32
>>
>> @@ -18,69 +21,124 @@ static const struct drm_colorop_funcs
>> intel_colorop_funcs = {  };
>>
>>   static
>> -int _intel_color_pipeline_plane_init(struct drm_plane *plane, struct
>> drm_prop_enum_list *list,
>> -				     enum pipe pipe)
>> +struct intel_colorop *intel_color_pipeline_plane_add_colorop(struct
>> drm_plane *plane,
>> +							     struct
>> intel_colorop *prev,
>> +							     enum
>> intel_color_block id)
>>   {
> 
> Seems like you just added a new function and then changed the function
> _intel_color_pipeline_plane_init but git format-patch messed this patch up bit
> Maybe try use --patience option to make this patch more readable.
>   

I used it on the v2 but did not help much :/ May be I am missing something.

>>   	struct drm_device *dev = plane->dev;
>> -	struct intel_display *display = to_intel_display(dev);
>> -	struct drm_colorop *prev_op;
>>   	struct intel_colorop *colorop;
>>   	int ret;
>>
>> -	colorop = intel_colorop_create(INTEL_PLANE_CB_PRE_CSC_LUT);
>> -
>> -	ret = drm_plane_colorop_curve_1d_lut_init(dev, &colorop->base,
>> plane, &intel_colorop_funcs,
>> -						  PLANE_DEGAMMA_SIZE,
>> -
>> DRM_COLOROP_LUT1D_INTERPOLATION_LINEAR,
>> -
>> DRM_COLOROP_FLAG_ALLOW_BYPASS);
>> +	colorop = intel_colorop_create(id);
>> +
>> +	if (IS_ERR(colorop))
>> +		return colorop;
>> +
>> +	switch (id) {
>> +	case INTEL_PLANE_CB_PRE_CSC_LUT:
>> +		ret = drm_plane_colorop_curve_1d_lut_init(dev,
>> +							  &colorop->base,
>> plane,
>> +
>> &intel_colorop_funcs,
>> +
>> PLANE_DEGAMMA_SIZE,
>> +
>> DRM_COLOROP_LUT1D_INTERPOLATION_LINEAR,
>> +
>> DRM_COLOROP_FLAG_ALLOW_BYPASS);
>> +		break;
>> +	case INTEL_PLANE_CB_CSC:
>> +		ret = drm_plane_colorop_ctm_3x4_init(dev, &colorop->base,
>> plane,
>> +						     &intel_colorop_funcs,
>> +
>> DRM_COLOROP_FLAG_ALLOW_BYPASS);
>> +		break;
>> +	case INTEL_PLANE_CB_3DLUT:
>> +		ret = drm_plane_colorop_3dlut_init(dev, &colorop->base,
>> plane,
>> +						   &intel_colorop_funcs, 17,
>> +
>> DRM_COLOROP_LUT3D_INTERPOLATION_TETRAHEDRAL,
>> +						   true);
>> +		break;
>> +	case INTEL_PLANE_CB_POST_CSC_LUT:
>> +		ret = drm_plane_colorop_curve_1d_lut_init(dev, &colorop-
>>> base, plane,
>> +
>> &intel_colorop_funcs,
>> +
>> PLANE_GAMMA_SIZE,
>> +
>> DRM_COLOROP_LUT1D_INTERPOLATION_LINEAR,
>> +
>> DRM_COLOROP_FLAG_ALLOW_BYPASS);
>> +		break;
>> +	default:
>> +		drm_err(plane->dev, "Invalid colorop id [%d]", id);
>> +		ret = -EINVAL;
>> +	}
>>
>>   	if (ret)
>> -		return ret;
>> +		goto cleanup;
>>
>> -	list->type = colorop->base.base.id;
>> +	if (prev)
>> +		drm_colorop_set_next_property(&prev->base, &colorop-
>>> base);
>>
>> -	/* TODO: handle failures and clean up */
>> -	prev_op = &colorop->base;
>> +	return colorop;
>>
>> -	colorop = intel_colorop_create(INTEL_PLANE_CB_CSC);
>> -	ret = drm_plane_colorop_ctm_3x4_init(dev, &colorop->base, plane,
>> &intel_colorop_funcs,
>> -
>> DRM_COLOROP_FLAG_ALLOW_BYPASS);
>> -	if (ret)
>> -		return ret;
>> +cleanup:
>> +	intel_colorop_destroy(&colorop->base);
>> +	return ERR_PTR(ret);
>> +}
>>
>> -	drm_colorop_set_next_property(prev_op, &colorop->base);
>> -	prev_op = &colorop->base;
>> +static
>> +int _intel_color_pipeline_plane_init(struct drm_plane *plane, struct
>> drm_prop_enum_list *list,
>> +				     enum pipe pipe)
>> +{
>> +	struct drm_device *dev = plane->dev;
>> +	struct intel_display *display = to_intel_display(dev);
>> +	struct intel_colorop *colorop[MAX_COLOROP];
>> +	int ret = 0;
>> +	int i = 0;
>>
>> -	if (DISPLAY_VER(display) >= 35 &&
>> -	    intel_color_crtc_has_3dlut(display, pipe) &&
>> -	    plane->type == DRM_PLANE_TYPE_PRIMARY) {
>> -		colorop = intel_colorop_create(INTEL_PLANE_CB_3DLUT);
>> +	colorop[i] = intel_color_pipeline_plane_add_colorop(plane, NULL,
>> +
>> INTEL_PLANE_CB_PRE_CSC_LUT);
>> +
>> +	if (IS_ERR(colorop[i])) {
>> +		ret = PTR_ERR(colorop[i]);
>> +		goto cleanup;
>> +	}
>> +	i++;
> 
> I see a lot of repeated code maybe we can get this done via a loop
> static const enum intel_colorop_type pipeline[] = {
>          INTEL_PLANE_CB_PRE_CSC_LUT,
>          INTEL_PLANE_CB_CSC,
>          INTEL_PLANE_CB_3DLUT,
>          INTEL_PLANE_CB_POST_CSC_LUT,
> };
> 
> for (n = 0; n < ARRAY_SIZE(pipeline); n++) {
>          if (pipeline[n] == INTEL_PLANE_CB_3DLUT &&
>              (DISPLAY_VER(display) < 35 ||
>               plane->type != DRM_PLANE_TYPE_PRIMARY ||
>               !intel_color_crtc_has_3dlut(display, pipe)))
>                  continue;
> 
>          ret = add_plane_colorop(plane, colorop, &i, &prev, pipeline[n]);
>          if (ret)
>                  goto cleanup;
> }
> 
> Where add_plane_colorop is
> 
> static int
> add_plane_colorop(struct drm_plane *plane,
>                    struct intel_colorop **colorop,
>                    int *i,
>                    struct intel_colorop **prev,
>                    enum intel_colorop_type type)
> {
>          colorop[*i] = intel_color_pipeline_plane_add_colorop(plane, *prev, type);
>          if (IS_ERR(colorop[*i]))
>                  return PTR_ERR(colorop[*i]);
> 
>          *prev = colorop[*i];
>          (*i)++;
> 
>          return 0;
> }

Sounds like a good idea, thank you. Implemented a version of this in v2

https://lore.kernel.org/intel-gfx/20260109081728.478844-14-chaitanya.kumar.borah@intel.com/T/#u

> 
> Regards,
> Suraj Kandpal
> 
>>
>> -		ret = drm_plane_colorop_3dlut_init(dev, &colorop->base,
>> plane,
>> -						   &intel_colorop_funcs, 17,
>> -
>> DRM_COLOROP_LUT3D_INTERPOLATION_TETRAHEDRAL,
>> -						   true);
>> -		if (ret)
>> -			return ret;
>>
>> -		drm_colorop_set_next_property(prev_op, &colorop->base);
>> +	colorop[i] = intel_color_pipeline_plane_add_colorop(plane, colorop[i -
>> 1],
>> +
>> INTEL_PLANE_CB_CSC);
>>
>> -		prev_op = &colorop->base;
>> +	if (IS_ERR(colorop[i])) {
>> +		ret = PTR_ERR(colorop[i]);
>> +		goto cleanup;
>>   	}
>>
>> -	colorop = intel_colorop_create(INTEL_PLANE_CB_POST_CSC_LUT);
>> -	ret = drm_plane_colorop_curve_1d_lut_init(dev, &colorop->base,
>> plane, &intel_colorop_funcs,
>> -						  PLANE_GAMMA_SIZE,
>> -
>> DRM_COLOROP_LUT1D_INTERPOLATION_LINEAR,
>> -
>> DRM_COLOROP_FLAG_ALLOW_BYPASS);
>> -	if (ret)
>> -		return ret;
>> +	i++;
>>
>> -	drm_colorop_set_next_property(prev_op, &colorop->base);
>> +	if (DISPLAY_VER(display) >= 35 &&
>> +	    intel_color_crtc_has_3dlut(display, pipe) &&
>> +	    plane->type == DRM_PLANE_TYPE_PRIMARY) {
>> +		colorop[i] = intel_color_pipeline_plane_add_colorop(plane,
>> colorop[i - 1],
>> +
>> INTEL_PLANE_CB_3DLUT);
>> +
>> +		if (IS_ERR(colorop[i])) {
>> +			ret = PTR_ERR(colorop[i]);
>> +			goto cleanup;
>> +		}
>> +		i++;
>> +	}
>>
>> -	list->name = kasprintf(GFP_KERNEL, "Color Pipeline %d", list->type);
>> +	colorop[i] = intel_color_pipeline_plane_add_colorop(plane, colorop[i -
>> 1],
>> +
>> INTEL_PLANE_CB_POST_CSC_LUT);
>> +	if (IS_ERR(colorop[i])) {
>> +		ret = PTR_ERR(colorop[i]);
>> +		goto cleanup;
>> +	}
>> +	i++;
>> +
>> +	list->type = colorop[0]->base.base.id;
>> +	list->name = kasprintf(GFP_KERNEL, "Color Pipeline %d",
>> +colorop[0]->base.base.id);
>>
>>   	return 0;
>> +
>> +cleanup:
>> +	while (--i >= 0)
>> +		intel_colorop_destroy(&colorop[i]->base);
>> +	return ret;
>>   }
>>
>>   int intel_color_pipeline_plane_init(struct drm_plane *plane, enum pipe pipe)
>> --
>> 2.25.1
>