Re: [PATCH] drm/colorop: Fix blob property reference tracking in state lifecycle

[Alex Hung <alex.hung@amd.com>]

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

On 3/12/26 14:41, Harry Wentland wrote:
> The colorop state blob property handling had memory leaks during state
> duplication, destruction, and reset operations. The implementation
> failed to follow the established pattern from drm_crtc's handling of
> DEGAMMA/GAMMA blob properties.
> 
> Issues fixed:
> - drm_colorop_atomic_destroy_state() was freeing state memory without
>    releasing the blob reference, causing a leak
> - drm_colorop_reset() was directly freeing old state with kfree()
>    instead of properly destroying it, leaking blob references
> - drm_colorop_cleanup() had duplicate blob cleanup code
> 
> Changes:
> - Add __drm_atomic_helper_colorop_destroy_state() helper to properly
>    release blob references before freeing state memory
> - Update drm_colorop_atomic_destroy_state() to call the helper
> - Fix drm_colorop_reset() to use drm_colorop_atomic_destroy_state()
>    for proper cleanup of old state
> - Simplify drm_colorop_cleanup() to use the common destruction path
> 
> This matches the well-tested pattern used by drm_crtc since 2016 and
> ensures proper reference counting throughout the state lifecycle.
> 
> Co-developed by Claude Sonnet 4.5.
> 
> Fixes: cfc27680ee20 ("drm/colorop: Introduce new drm_colorop mode object")
> Cc: Simon Ser <contact@emersion.fr>
> Cc: Alex Hung <alex.hung@amd.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Daniel Stone <daniels@collabora.com>
> Cc: Melissa Wen <mwen@igalia.com>
> Cc: Sebastian Wick <sebastian.wick@redhat.com>
> Cc: Uma Shankar <uma.shankar@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Louis Chauvet <louis.chauvet@bootlin.com>
> Cc: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
> Cc: <stable@vger.kernel.org> #v6.19+
> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
> ---
>   drivers/gpu/drm/drm_colorop.c | 26 +++++++++++++++++++-------
>   1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
> index f421c623b3f0..647cf881f413 100644
> --- a/drivers/gpu/drm/drm_colorop.c
> +++ b/drivers/gpu/drm/drm_colorop.c
> @@ -171,12 +171,8 @@ void drm_colorop_cleanup(struct drm_colorop *colorop)
>   	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);
> +	if (colorop->state)
> +		drm_colorop_atomic_destroy_state(colorop, colorop->state);
>   }
>   EXPORT_SYMBOL(drm_colorop_cleanup);
>   
> @@ -485,9 +481,23 @@ drm_atomic_helper_colorop_duplicate_state(struct drm_colorop *colorop)
>   	return state;
>   }
>   
> +/**
> + * __drm_atomic_helper_colorop_destroy_state - release colorop state
> + * @state: colorop state object to release
> + *
> + * Releases all resources stored in the colorop state without actually freeing
> + * the memory of the colorop state. This is useful for drivers that subclass the
> + * colorop state.
> + */
> +static void __drm_atomic_helper_colorop_destroy_state(struct drm_colorop_state *state)
> +{
> +	drm_property_blob_put(state->data);
> +}
> +
>   void drm_colorop_atomic_destroy_state(struct drm_colorop *colorop,
>   				      struct drm_colorop_state *state)
>   {
> +	__drm_atomic_helper_colorop_destroy_state(state);
>   	kfree(state);
>   }
>   
> @@ -538,7 +548,9 @@ static void __drm_colorop_reset(struct drm_colorop *colorop,
>   
>   void drm_colorop_reset(struct drm_colorop *colorop)
>   {
> -	kfree(colorop->state);
> +	if (colorop->state)
> +		drm_colorop_atomic_destroy_state(colorop, colorop->state);
> +
>   	colorop->state = kzalloc_obj(*colorop->state);
>   
>   	if (colorop->state)