[V8,43/43] drm/colorop: Add destroy functions for color pipeline

Commit Message

Alex Hung March 26, 2025, 11:47 p.m. UTC

The functions are to clean up color pipeline when a device driver
fails to create its color pipeline.

Signed-off-by: Alex Hung <[e-mail]>
---
 .../amd/display/amdgpu_dm/amdgpu_dm_colorop.c |  3 +-
 drivers/gpu/drm/drm_colorop.c                 | 41 +++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_colorop.c           |  3 +-
 include/drm/drm_colorop.h                     |  2 +
 4 files changed, 45 insertions(+), 4 deletions(-)

Comments

Simon Ser March 29, 2025, 3:48 p.m. UTC | #1

I would prefer these functions to be introduced together with the
patches adding functions to create objects and adding the new fields.
That way it's easier to check the symmetry and at no point in the
series there are memory leaks.

Additionally, I would avoid using the name "cleanup", which seems to
have different semantics: for instance drm_plane_cleanup() doesn't kfree
the pointer. "destroy" seems more appropriate here.

Alex Hung April 1, 2025, 2:42 a.m. UTC | #2

On 3/29/25 09:48, Simon Ser wrote:
> I would prefer these functions to be introduced together with the
> patches adding functions to create objects and adding the new fields.
> That way it's easier to check the symmetry and at no point in the
> series there are memory leaks.

The object creation and new fields are introduced in different patches. 
I divided this patch by introducing these functions in a patch, and 2. 
adding callers when needed to avoid memory leaks.

> 
> Additionally, I would avoid using the name "cleanup", which seems to
> have different semantics: for instance drm_plane_cleanup() doesn't kfree
> the pointer. "destroy" seems more appropriate here.

How about the following changes, i.e., freeing pointer is moved out of 
the cleanup function, and keeping the names.

@@ -173,7 +173,6 @@ static void drm_colorop_cleanup(struct drm_colorop 
*colorop)
         }

         kfree(colorop->state);
-       kfree(colorop);
  }

  /**
@@ -191,6 +190,7 @@ void drm_colorop_pipeline_destroy(struct drm_plane 
*plane)

         list_for_each_entry_safe(colorop, next, &config->colorop_list, 
head) {
                 drm_colorop_cleanup(colorop);
+               kfree(colorop);
         }
  }

Patch

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 e03e6044f937..80173f00dfd0 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
@@ -200,8 +200,7 @@  int amdgpu_dm_initialize_default_pipeline(struct drm_plane *plane, struct drm_pr
 	return 0;
 
 cleanup:
-	for (; i >= 0; i--)
-		kfree(ops[i]);
+	drm_colorop_pipeline_destroy(plane);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
index 224c6be237d2..5845527201d2 100644
--- a/drivers/gpu/drm/drm_colorop.c
+++ b/drivers/gpu/drm/drm_colorop.c
@@ -154,6 +154,47 @@  static int drm_colorop_init(struct drm_device *dev, struct drm_colorop *colorop,
 	return ret;
 }
 
+/**
+ * drm_colorop_cleanup - Cleanup a drm_colorop object in color_pipeline
+ *
+ * @colorop: The drm_colorop object to be cleaned
+ */
+static void drm_colorop_cleanup(struct drm_colorop *colorop)
+{
+	struct drm_device *dev = colorop->dev;
+	struct drm_mode_config *config = &dev->mode_config;
+
+	list_del(&colorop->head);
+	config->num_colorop--;
+
+	if (colorop->state && colorop->state->data) {
+		drm_property_blob_put(colorop->state->data);
+		colorop->state->data = NULL;
+	}
+
+	kfree(colorop->state);
+	kfree(colorop);
+}
+
+/**
+ * drm_colorop_pipeline_destroy - Helper for color pipeline destruction
+ *
+ * @plane: - The drm_plane structure containing the color_pipeline
+ *
+ * Provides a default color pipeline destroy handler for a planes.
+ */
+void drm_colorop_pipeline_destroy(struct drm_plane *plane)
+{
+	struct drm_device *dev = plane->dev;
+	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_colorop *colorop, *next;
+
+	list_for_each_entry_safe(colorop, next, &config->colorop_list, head) {
+		drm_colorop_cleanup(colorop);
+	}
+}
+EXPORT_SYMBOL(drm_colorop_pipeline_destroy);
+
 /**
  * drm_colorop_curve_1d_init - Initialize a DRM_COLOROP_1D_CURVE
  *
diff --git a/drivers/gpu/drm/vkms/vkms_colorop.c b/drivers/gpu/drm/vkms/vkms_colorop.c
index a2f548a4b84d..801e7ad6ad79 100644
--- a/drivers/gpu/drm/vkms/vkms_colorop.c
+++ b/drivers/gpu/drm/vkms/vkms_colorop.c
@@ -89,8 +89,7 @@  static int vkms_initialize_color_pipeline(struct drm_plane *plane, struct drm_pr
 	return 0;
 
 cleanup:
-	for (; i >= 0; i--)
-		kfree(ops[i]);
+	drm_colorop_pipeline_destroy(plane);
 
 	return ret;
 }
diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h
index e999d5ceb8a5..e1f7c91e8db1 100644
--- a/include/drm/drm_colorop.h
+++ b/include/drm/drm_colorop.h
@@ -366,6 +366,8 @@  static inline struct drm_colorop *drm_colorop_find(struct drm_device *dev,
 	return mo ? obj_to_colorop(mo) : NULL;
 }
 
+void drm_colorop_pipeline_destroy(struct drm_plane *plane);
+
 int drm_colorop_curve_1d_init(struct drm_device *dev, struct drm_colorop *colorop,
 			      struct drm_plane *plane, u64 supported_tfs,
 			      bool allow_bypass);