> Subject: [PATCH v2 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. > > v2: > - Refactor code to avoid repetition (Suraj) > > Signed-off-by: Chaitanya Kumar Borah <[email protected]> > --- > .../drm/i915/display/intel_color_pipeline.c | 164 +++++++++++++----- > 1 file changed, 117 insertions(+), 47 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..1b8d504fa9f2 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 > > @@ -17,70 +20,137 @@ static const struct drm_colorop_funcs > intel_colorop_funcs = { > .destroy = intel_colorop_destroy, > }; > > +/* > + * 3DLUT can be bound to all three HDR planes. However, even with the > +latest > + * color pipeline UAPI, there is no good way to represent a HW block > +which > + * can be shared/attached at different stages of the pipeline. So right > +now, > + * we expose 3DLUT only attached with the primary plane. > + * > + * That way we don't confuse the userspace with opaque commit failures > + * on trying to enable it on multiple planes which would otherwise make > + * the pipeline totally unusable. > + */ > +static const enum intel_color_block nvl_primary_plane_pipeline[] = {
Use the official code name that would be xe3plpd Otherwise LGTM, Reviewed-by: Suraj Kandpal <[email protected]> > + INTEL_PLANE_CB_PRE_CSC_LUT, > + INTEL_PLANE_CB_CSC, > + INTEL_PLANE_CB_3DLUT, > + INTEL_PLANE_CB_POST_CSC_LUT, > +}; > + > +static const enum intel_color_block hdr_plane_pipeline[] = { > + INTEL_PLANE_CB_PRE_CSC_LUT, > + INTEL_PLANE_CB_CSC, > + INTEL_PLANE_CB_POST_CSC_LUT, > +}; > + > +static bool plane_has_3dlut(struct intel_display *display, enum pipe pipe, > + struct drm_plane *plane) > +{ > + return (DISPLAY_VER(display) >= 35 && > + intel_color_crtc_has_3dlut(display, pipe) && > + plane->type == DRM_PLANE_TYPE_PRIMARY); } > + > 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) > { > 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); > - > - if (ret) > - return ret; > - > - list->type = colorop->base.base.id; > - > - /* TODO: handle failures and clean up */ > - prev_op = &colorop->base; > - > - 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; > - > - drm_colorop_set_next_property(prev_op, &colorop->base); > - prev_op = &colorop->base; > - > - 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 = 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); > - if (ret) > - return ret; > - > - drm_colorop_set_next_property(prev_op, &colorop->base); > - > - prev_op = &colorop->base; > + 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; > } > > - 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; > + goto cleanup; > > - drm_colorop_set_next_property(prev_op, &colorop->base); > + if (prev) > + drm_colorop_set_next_property(&prev->base, &colorop- > >base); > > - list->name = kasprintf(GFP_KERNEL, "Color Pipeline %d", list->type); > + return colorop; > + > +cleanup: > + intel_colorop_destroy(&colorop->base); > + return ERR_PTR(ret); > +} > + > +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]; > + struct intel_colorop *prev = NULL; > + const enum intel_color_block *pipeline; > + int pipeline_len; > + int ret = 0; > + int i; > + > + if (plane_has_3dlut(display, pipe, plane)) { > + pipeline = nvl_primary_plane_pipeline; > + pipeline_len = ARRAY_SIZE(nvl_primary_plane_pipeline); > + } else { > + pipeline = hdr_plane_pipeline; > + pipeline_len = ARRAY_SIZE(hdr_plane_pipeline); > + } > + > + for (i = 0; i < pipeline_len; i++) { > + colorop[i] = intel_color_pipeline_plane_add_colorop(plane, > prev, > + > pipeline[i]); > + if (IS_ERR(colorop[i])) { > + ret = PTR_ERR(colorop[i]); > + goto cleanup; > + } > + > + prev = colorop[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
