Re: [PATCH 05/13] drm: Allow driver-managed destruction of colorop objects

[Borah, Chaitanya Kumar]

On 1/7/2026 5:32 AM, Alex Hung wrote:
It is a nitpick, but is it better to have patch 6 first, and then 
separate changes to drm to a single patch, and move the changes in 
amdgpu, vkms and i915 in the following corresponding patches (7, 8 and 9)?

We can avoid passing NULL to struct drm_colorop_funcs *funcs.

Thank you very much, Alex, for the review.

I structured the patches this way to keep each patch buildable and to 
keep the existing behavior intact until all changes are in place.

Once the prototypes of the init functions are changed, all call sites 
need to be updated. I couldn’t find a way to keep the DRM changes 
isolated while also preserving per-patch buildability. If I am missing 
something here, please let me know. I can respin accordingly.

In any case, I can move patch 6 ahead of this one.

==
Chaitanya


Otherwise it looks good to me.

Reviewed-by: Alex Hung <alex.h...@amd.com>

On 12/18/25 23:56, Chaitanya Kumar Borah wrote:
Some drivers might want to embed struct drm_colorop inside
driver-specific objects, similar to planes or CRTCs. In such
cases, freeing only the drm_colorop is incorrect.

Add a drm_colorop_funcs callback to allow drivers to provide a destroy
hook that cleans up the full enclosing object. Make changes in helper
functions to accept helper functions as argument. Pass NULL for now
to retain current behavior.

Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.bo...@intel.com>
---
  .../amd/display/amdgpu_dm/amdgpu_dm_colorop.c | 18 ++++++-----
  drivers/gpu/drm/drm_colorop.c                 | 31 +++++++++++++------
  .../drm/i915/display/intel_color_pipeline.c   |  8 ++---
  drivers/gpu/drm/vkms/vkms_colorop.c           | 10 +++---
  include/drm/drm_colorop.h                     | 30 +++++++++++++++---
  5 files changed, 66 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.c
index a2de3bba8346..dfdb4fb4219f 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.c
@@ -72,7 +72,7 @@ int amdgpu_dm_initialize_default_pipeline(struct 
drm_plane *plane, struct drm_pr
          goto cleanup;
      }
-    ret = drm_plane_colorop_curve_1d_init(dev, ops[i], plane,
+    ret = drm_plane_colorop_curve_1d_init(dev, ops[i], plane, NULL,
                            amdgpu_dm_supported_degam_tfs,
                            DRM_COLOROP_FLAG_ALLOW_BYPASS);
      if (ret)
@@ -89,7 +89,7 @@ int amdgpu_dm_initialize_default_pipeline(struct 
drm_plane *plane, struct drm_pr
          goto cleanup;
      }
-    ret = drm_plane_colorop_mult_init(dev, ops[i], plane, 
DRM_COLOROP_FLAG_ALLOW_BYPASS);
+    ret = drm_plane_colorop_mult_init(dev, ops[i], plane, NULL, 
DRM_COLOROP_FLAG_ALLOW_BYPASS);
      if (ret)
          goto cleanup;
@@ -104,7 +104,8 @@ int amdgpu_dm_initialize_default_pipeline(struct 
drm_plane *plane, struct drm_pr
          goto cleanup;
      }
-    ret = drm_plane_colorop_ctm_3x4_init(dev, ops[i], plane, 
DRM_COLOROP_FLAG_ALLOW_BYPASS);
+    ret = drm_plane_colorop_ctm_3x4_init(dev, ops[i], plane, NULL,
+                         DRM_COLOROP_FLAG_ALLOW_BYPASS);
      if (ret)
          goto cleanup;
@@ -120,7 +121,7 @@ int amdgpu_dm_initialize_default_pipeline(struct 
drm_plane *plane, struct drm_pr
              goto cleanup;
          }
-        ret = drm_plane_colorop_curve_1d_init(dev, ops[i], plane,
+        ret = drm_plane_colorop_curve_1d_init(dev, ops[i], plane, NULL,
                          amdgpu_dm_supported_shaper_tfs,
                          DRM_COLOROP_FLAG_ALLOW_BYPASS);
          if (ret)
@@ -137,7 +138,8 @@ int amdgpu_dm_initialize_default_pipeline(struct 
drm_plane *plane, struct drm_pr
              goto cleanup;
          }
-        ret = drm_plane_colorop_curve_1d_lut_init(dev, ops[i], plane, 
MAX_COLOR_LUT_ENTRIES,
+        ret = drm_plane_colorop_curve_1d_lut_init(dev, ops[i], plane, 
NULL,
+                            MAX_COLOR_LUT_ENTRIES,
                              DRM_COLOROP_LUT1D_INTERPOLATION_LINEAR,
                              DRM_COLOROP_FLAG_ALLOW_BYPASS);
          if (ret)
@@ -154,7 +156,7 @@ int amdgpu_dm_initialize_default_pipeline(struct 
drm_plane *plane, struct drm_pr
              goto cleanup;
          }
-        ret = drm_plane_colorop_3dlut_init(dev, ops[i], plane, 
LUT3D_SIZE,
+        ret = drm_plane_colorop_3dlut_init(dev, ops[i], plane, NULL, 
LUT3D_SIZE,
                      DRM_COLOROP_LUT3D_INTERPOLATION_TETRAHEDRAL,
                      DRM_COLOROP_FLAG_ALLOW_BYPASS);
          if (ret)
@@ -172,7 +174,7 @@ int amdgpu_dm_initialize_default_pipeline(struct 
drm_plane *plane, struct drm_pr
          goto cleanup;
      }
-    ret = drm_plane_colorop_curve_1d_init(dev, ops[i], plane,
+    ret = drm_plane_colorop_curve_1d_init(dev, ops[i], plane, NULL,
                            amdgpu_dm_supported_blnd_tfs,
                            DRM_COLOROP_FLAG_ALLOW_BYPASS);
      if (ret)
@@ -189,7 +191,7 @@ int amdgpu_dm_initialize_default_pipeline(struct 
drm_plane *plane, struct drm_pr
          goto cleanup;
      }
-    ret = drm_plane_colorop_curve_1d_lut_init(dev, ops[i], plane, 
MAX_COLOR_LUT_ENTRIES,
+    ret = drm_plane_colorop_curve_1d_lut_init(dev, ops[i], plane, 
NULL, MAX_COLOR_LUT_ENTRIES,
                            DRM_COLOROP_LUT1D_INTERPOLATION_LINEAR,
                            DRM_COLOROP_FLAG_ALLOW_BYPASS);
      if (ret)
diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/ 
drm_colorop.c
index 44eb823585d2..efe61bdd9b24 100644
--- a/drivers/gpu/drm/drm_colorop.c
+++ b/drivers/gpu/drm/drm_colorop.c
@@ -93,7 +93,8 @@ static const struct drm_prop_enum_list 
drm_colorop_lut3d_interpolation_list[] =
  /* Init Helpers */
  static int drm_plane_colorop_init(struct drm_device *dev, struct 
drm_colorop *colorop,
-                  struct drm_plane *plane, enum drm_colorop_type type,
+                  struct drm_plane *plane, const struct 
drm_colorop_funcs *funcs,
+                  enum drm_colorop_type type,
                    uint32_t flags)
  {
      struct drm_mode_config *config = &dev->mode_config;
@@ -109,6 +110,7 @@ static int drm_plane_colorop_init(struct 
drm_device *dev, struct drm_colorop *co
      colorop->type = type;
      colorop->plane = plane;
      colorop->next = NULL;
+    colorop->funcs = funcs;
      list_add_tail(&colorop->head, &config->colorop_list);
      colorop->index = config->num_colorop++;
@@ -203,6 +205,7 @@ EXPORT_SYMBOL(drm_colorop_pipeline_destroy);
   * @dev: DRM device
   * @colorop: The drm_colorop object to initialize
   * @plane: The associated drm_plane
+ * @funcs: control functions for the new colorop
   * @supported_tfs: A bitfield of supported 
drm_plane_colorop_curve_1d_init enum values,
   *                 created using BIT(curve_type) and combined with 
the OR '|'
   *                 operator.
@@ -210,7 +213,8 @@ EXPORT_SYMBOL(drm_colorop_pipeline_destroy);
   * @return zero on success, -E value on failure
   */
  int drm_plane_colorop_curve_1d_init(struct drm_device *dev, struct 
drm_colorop *colorop,
-                    struct drm_plane *plane, u64 supported_tfs, 
uint32_t flags)
+                    struct drm_plane *plane, const struct 
drm_colorop_funcs *funcs,
+                    u64 supported_tfs, uint32_t flags)
  {
      struct drm_prop_enum_list enum_list[DRM_COLOROP_1D_CURVE_COUNT];
      int i, len;
@@ -231,7 +235,7 @@ int drm_plane_colorop_curve_1d_init(struct 
drm_device *dev, struct drm_colorop *
          return -EINVAL;
      }
-    ret = drm_plane_colorop_init(dev, colorop, plane, 
DRM_COLOROP_1D_CURVE, flags);
+    ret = drm_plane_colorop_init(dev, colorop, plane, funcs, 
DRM_COLOROP_1D_CURVE, flags);
      if (ret)
          return ret;
@@ -288,20 +292,23 @@ static int drm_colorop_create_data_prop(struct 
drm_device *dev, struct drm_color
   * @dev: DRM device
   * @colorop: The drm_colorop object to initialize
   * @plane: The associated drm_plane
+ * @funcs: control functions for new colorop
   * @lut_size: LUT size supported by driver
   * @interpolation: 1D LUT interpolation type
   * @flags: bitmask of misc, see DRM_COLOROP_FLAG_* defines.
   * @return zero on success, -E value on failure
   */
  int drm_plane_colorop_curve_1d_lut_init(struct drm_device *dev, 
struct drm_colorop *colorop,
-                    struct drm_plane *plane, uint32_t lut_size,
+                    struct drm_plane *plane,
+                    const struct drm_colorop_funcs *funcs,
+                    uint32_t lut_size,
                      enum drm_colorop_lut1d_interpolation_type 
interpolation,
                      uint32_t flags)
  {
      struct drm_property *prop;
      int ret;
-    ret = drm_plane_colorop_init(dev, colorop, plane, 
DRM_COLOROP_1D_LUT, flags);
+    ret = drm_plane_colorop_init(dev, colorop, plane, funcs, 
DRM_COLOROP_1D_LUT, flags);
      if (ret)
          return ret;
@@ -339,11 +346,12 @@ int drm_plane_colorop_curve_1d_lut_init(struct 
drm_device *dev, struct drm_color
  EXPORT_SYMBOL(drm_plane_colorop_curve_1d_lut_init);
  int drm_plane_colorop_ctm_3x4_init(struct drm_device *dev, struct 
drm_colorop *colorop,
-                   struct drm_plane *plane, uint32_t flags)
+                   struct drm_plane *plane, const struct 
drm_colorop_funcs *funcs,
+                   uint32_t flags)
  {
      int ret;
-    ret = drm_plane_colorop_init(dev, colorop, plane, 
DRM_COLOROP_CTM_3X4, flags);
+    ret = drm_plane_colorop_init(dev, colorop, plane, funcs, 
DRM_COLOROP_CTM_3X4, flags);
      if (ret)
          return ret;
@@ -363,16 +371,18 @@ EXPORT_SYMBOL(drm_plane_colorop_ctm_3x4_init);
   * @dev: DRM device
   * @colorop: The drm_colorop object to initialize
   * @plane: The associated drm_plane
+ * @funcs: control functions for the new colorop
   * @flags: bitmask of misc, see DRM_COLOROP_FLAG_* defines.
   * @return zero on success, -E value on failure
   */
  int drm_plane_colorop_mult_init(struct drm_device *dev, struct 
drm_colorop *colorop,
-                struct drm_plane *plane, uint32_t flags)
+                struct drm_plane *plane, const struct 
drm_colorop_funcs *funcs,
+                uint32_t flags)
  {
      struct drm_property *prop;
      int ret;
-    ret = drm_plane_colorop_init(dev, colorop, plane, 
DRM_COLOROP_MULTIPLIER, flags);
+    ret = drm_plane_colorop_init(dev, colorop, plane, funcs, 
DRM_COLOROP_MULTIPLIER, flags);
      if (ret)
          return ret;
@@ -391,6 +401,7 @@ EXPORT_SYMBOL(drm_plane_colorop_mult_init);
  int drm_plane_colorop_3dlut_init(struct drm_device *dev, struct 
drm_colorop *colorop,
                   struct drm_plane *plane,
+                 const struct drm_colorop_funcs *funcs,
                   uint32_t lut_size,
                   enum drm_colorop_lut3d_interpolation_type 
interpolation,
                   uint32_t flags)
@@ -398,7 +409,7 @@ int drm_plane_colorop_3dlut_init(struct drm_device 
*dev, struct drm_colorop *col
      struct drm_property *prop;
      int ret;
-    ret = drm_plane_colorop_init(dev, colorop, plane, 
DRM_COLOROP_3D_LUT, flags);
+    ret = drm_plane_colorop_init(dev, colorop, plane, funcs, 
DRM_COLOROP_3D_LUT, flags);
      if (ret)
          return ret;
diff --git a/drivers/gpu/drm/i915/display/intel_color_pipeline.c b/ 
drivers/gpu/drm/i915/display/intel_color_pipeline.c
index 04af552b3648..d3d73d60727c 100644
--- a/drivers/gpu/drm/i915/display/intel_color_pipeline.c
+++ b/drivers/gpu/drm/i915/display/intel_color_pipeline.c
@@ -25,7 +25,7 @@ int _intel_color_pipeline_plane_init(struct 
drm_plane *plane, struct drm_prop_en
      colorop = intel_colorop_create(INTEL_PLANE_CB_PRE_CSC_LUT);
-    ret = drm_plane_colorop_curve_1d_lut_init(dev, &colorop->base, 
plane,
+    ret = drm_plane_colorop_curve_1d_lut_init(dev, &colorop->base, 
plane, NULL,
                            PLANE_DEGAMMA_SIZE,
                            DRM_COLOROP_LUT1D_INTERPOLATION_LINEAR,
                            DRM_COLOROP_FLAG_ALLOW_BYPASS);
@@ -39,7 +39,7 @@ int _intel_color_pipeline_plane_init(struct 
drm_plane *plane, struct drm_prop_en
      prev_op = &colorop->base;
      colorop = intel_colorop_create(INTEL_PLANE_CB_CSC);
-    ret = drm_plane_colorop_ctm_3x4_init(dev, &colorop->base, plane,
+    ret = drm_plane_colorop_ctm_3x4_init(dev, &colorop->base, plane, 
NULL,
                           DRM_COLOROP_FLAG_ALLOW_BYPASS);
      if (ret)
          return ret;
@@ -52,7 +52,7 @@ int _intel_color_pipeline_plane_init(struct 
drm_plane *plane, struct drm_prop_en
          plane->type == DRM_PLANE_TYPE_PRIMARY) {
          colorop = intel_colorop_create(INTEL_PLANE_CB_3DLUT);
-        ret = drm_plane_colorop_3dlut_init(dev, &colorop->base, 
plane, 17,
+        ret = drm_plane_colorop_3dlut_init(dev, &colorop->base, 
plane, NULL, 17,
                             DRM_COLOROP_LUT3D_INTERPOLATION_TETRAHEDRAL,
                             true);
          if (ret)
@@ -64,7 +64,7 @@ int _intel_color_pipeline_plane_init(struct 
drm_plane *plane, struct drm_prop_en
      }
      colorop = intel_colorop_create(INTEL_PLANE_CB_POST_CSC_LUT);
-    ret = drm_plane_colorop_curve_1d_lut_init(dev, &colorop->base, 
plane,
+    ret = drm_plane_colorop_curve_1d_lut_init(dev, &colorop->base, 
plane, NULL,
                            PLANE_GAMMA_SIZE,
                            DRM_COLOROP_LUT1D_INTERPOLATION_LINEAR,
                            DRM_COLOROP_FLAG_ALLOW_BYPASS);
diff --git a/drivers/gpu/drm/vkms/vkms_colorop.c b/drivers/gpu/drm/ 
vkms/vkms_colorop.c
index d03a1f2e9c41..9e9dd0494628 100644
--- a/drivers/gpu/drm/vkms/vkms_colorop.c
+++ b/drivers/gpu/drm/vkms/vkms_colorop.c
@@ -31,7 +31,7 @@ static int vkms_initialize_color_pipeline(struct 
drm_plane *plane, struct drm_pr
          goto cleanup;
      }
-    ret = drm_plane_colorop_curve_1d_init(dev, ops[i], plane, 
supported_tfs,
+    ret = drm_plane_colorop_curve_1d_init(dev, ops[i], plane, NULL, 
supported_tfs,
                            DRM_COLOROP_FLAG_ALLOW_BYPASS);
      if (ret)
          goto cleanup;
@@ -48,7 +48,8 @@ static int vkms_initialize_color_pipeline(struct 
drm_plane *plane, struct drm_pr
          goto cleanup;
      }
-    ret = drm_plane_colorop_ctm_3x4_init(dev, ops[i], plane, 
DRM_COLOROP_FLAG_ALLOW_BYPASS);
+    ret = drm_plane_colorop_ctm_3x4_init(dev, ops[i], plane, NULL,
+                         DRM_COLOROP_FLAG_ALLOW_BYPASS);
      if (ret)
          goto cleanup;
@@ -64,7 +65,8 @@ static int vkms_initialize_color_pipeline(struct 
drm_plane *plane, struct drm_pr
          goto cleanup;
      }
-    ret = drm_plane_colorop_ctm_3x4_init(dev, ops[i], plane, 
DRM_COLOROP_FLAG_ALLOW_BYPASS);
+    ret = drm_plane_colorop_ctm_3x4_init(dev, ops[i], plane, NULL,
+                         DRM_COLOROP_FLAG_ALLOW_BYPASS);
      if (ret)
          goto cleanup;
@@ -80,7 +82,7 @@ static int vkms_initialize_color_pipeline(struct 
drm_plane *plane, struct drm_pr
          goto cleanup;
      }
-    ret = drm_plane_colorop_curve_1d_init(dev, ops[i], plane, 
supported_tfs,
+    ret = drm_plane_colorop_curve_1d_init(dev, ops[i], plane, NULL, 
supported_tfs,
                            DRM_COLOROP_FLAG_ALLOW_BYPASS);
      if (ret)
          goto cleanup;
diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h
index a3a32f9f918c..45d1b1d3faf9 100644
--- a/include/drm/drm_colorop.h
+++ b/include/drm/drm_colorop.h
@@ -187,6 +187,19 @@ struct drm_colorop_state {
      struct drm_atomic_state *state;
  };
+/**
+ * struct drm_colorop_funcs - driver colorop control functions
+ */
+struct drm_colorop_funcs {
+    /**
+     * @destroy:
+     *
+     * Clean up colorop resources. This is called at driver unload time
+     * through drm_mode_config_cleanup()
+     */
+    void (*destroy)(struct drm_colorop *colorop);
+};
+
  /**
   * struct drm_colorop - DRM color operation control structure
   *
@@ -362,6 +375,8 @@ struct drm_colorop {
       */
      struct drm_property *next_property;
+    /** @funcs: colorop control functions */
+    const struct drm_colorop_funcs *funcs;
  };
  #define obj_to_colorop(x) container_of(x, struct drm_colorop, base)
@@ -390,17 +405,22 @@ void drm_colorop_pipeline_destroy(struct 
drm_device *dev);
  void drm_colorop_cleanup(struct drm_colorop *colorop);
  int drm_plane_colorop_curve_1d_init(struct drm_device *dev, struct 
drm_colorop *colorop,
-                    struct drm_plane *plane, u64 supported_tfs, 
uint32_t flags);
+                    struct drm_plane *plane, const struct 
drm_colorop_funcs *funcs,
+                    u64 supported_tfs, uint32_t flags);
  int drm_plane_colorop_curve_1d_lut_init(struct drm_device *dev, 
struct drm_colorop *colorop,
-                    struct drm_plane *plane, uint32_t lut_size,
+                    struct drm_plane *plane,
+                    const struct drm_colorop_funcs *funcs,
+                    uint32_t lut_size,
                      enum drm_colorop_lut1d_interpolation_type 
interpolation,
                      uint32_t flags);
  int drm_plane_colorop_ctm_3x4_init(struct drm_device *dev, struct 
drm_colorop *colorop,
-                   struct drm_plane *plane, uint32_t flags);
+                   struct drm_plane *plane, const struct 
drm_colorop_funcs *funcs,
+                   uint32_t flags);
  int drm_plane_colorop_mult_init(struct drm_device *dev, struct 
drm_colorop *colorop,
-                struct drm_plane *plane, uint32_t flags);
+                struct drm_plane *plane, const struct 
drm_colorop_funcs *funcs,
+                uint32_t flags);
  int drm_plane_colorop_3dlut_init(struct drm_device *dev, struct 
drm_colorop *colorop,
-                 struct drm_plane *plane,
+                 struct drm_plane *plane, const struct 
drm_colorop_funcs *funcs,
                   uint32_t lut_size,
                   enum drm_colorop_lut3d_interpolation_type 
interpolation,
                   uint32_t flags);