> 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 <[email protected]>
> ---
> .../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.
> 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;
}
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
