> -----Original Message-----
> From: Kandpal, Suraj <[email protected]>
> Sent: Tuesday, January 6, 2026 9:25 AM
> To: Borah, Chaitanya Kumar <[email protected]>; dri-
> [email protected]; [email protected]; intel-
> [email protected]; [email protected]
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; Shankar, Uma
> <[email protected]>; [email protected];
> [email protected]; Roper, Matthew D <[email protected]>;
> Nautiyal, Ankit K <[email protected]>
> Subject: RE: [PATCH 13/13] drm/i915/color: Add failure handling in plane color
> pipeline init
>
> > 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.
Yeah the refactor created some weirdness but logically looks ok.
Check if it can be made more readable. Maybe break it in 2 patches if required,
one to introduce the helper and next using the new helper.
> > 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;
> }
This can be explored, we can create pipeline wise array entries and traverse
them in loop
to make it clearer and more concise. To create a specific pipeline, we can just
pass the array.
Overall changes in the series look good to me. Thanks for working on fixing the
leaks.
Regards,
Uma Shankar
> >
> > - 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
