Re: [PATCH V13 06/51] drm/colorop: Add 1D Curve subtype

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

On 12/16/25 11:19, Nícolas F. R. A. Prado wrote:
> On Fri, 2025-11-14 at 17:01 -0700, Alex Hung wrote:
>> From: Harry Wentland <harry.wentland@amd.com>
>>
>> Add a new drm_colorop with DRM_COLOROP_1D_CURVE with two subtypes:
>> DRM_COLOROP_1D_CURVE_SRGB_EOTF and
>> DRM_COLOROP_1D_CURVE_SRGB_INV_EOTF.
>>
>> Reviewed-by: Simon Ser <contact@emersion.fr>
>> Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com>
>> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
>> Co-developed-by: Alex Hung <alex.hung@amd.com>
>> Signed-off-by: Alex Hung <alex.hung@amd.com>
>> Reviewed-by: Daniel Stone <daniels@collabora.com>
>> Reviewed-by: Melissa Wen <mwen@igalia.com>
>> Reviewed-by: Sebastian Wick <sebastian.wick@redhat.com>
>> ---
> [..]
>> diff --git a/drivers/gpu/drm/drm_colorop.c
>> b/drivers/gpu/drm/drm_colorop.c
>> index 1459a28c7e7b..6fbc3c284d33 100644
>> --- a/drivers/gpu/drm/drm_colorop.c
>> +++ b/drivers/gpu/drm/drm_colorop.c
> [..]
>> +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_mode_config *config = &dev->mode_config;
>> +	struct drm_property *prop;
>> +	int ret = 0;
>> +
>> +	ret = drm_mode_object_add(dev, &colorop->base,
>> DRM_MODE_OBJECT_COLOROP);
>> +	if (ret)
>> +		return ret;
>> +
>> +	colorop->base.properties = &colorop->properties;
>> +	colorop->dev = dev;
>> +	colorop->type = type;
>> +	colorop->plane = plane;
>> +
>> +	list_add_tail(&colorop->head, &config->colorop_list);
>> +	colorop->index = config->num_colorop++;
> 
> Hi Alex,
> 
> I know this series has already been merged, but I was looking through
> the code together with Ariel and we noticed that while this init
> function adds the colorop to the list in the drm_mode_config, it
> doesn't remove it in the error paths below, and I believe it should.
> 
> Does that make sense?
> 

Hi Nicolas,

drm_colorop_pipeline_destroy() calls drm_colorop_cleanup() to delete it. 
After drm_colorop_pipeline_destroy is called the entire pipeline will be 
freed.

void drm_colorop_cleanup(struct drm_colorop *colorop)
{
	...
	list_del(&colorop->head);
	config->num_colorop--;
	...
}

For example, amdgpu calls drm_plane_colorop_*_init functions (which call 
drm_plane_colorop_init themselves) to create a pipeline. If any of 
colorop creation fails, amdgpu calls drm_colorop_pipeline_destroy to 
destroy the entire pipeline.

In the end, we either have a good pipeline or none.

> Thanks,
> Nicolas
> 
>> +
>> +	/* add properties */
>> +
>> +	/* type */
>> +	prop = drm_property_create_enum(dev,
>> +					DRM_MODE_PROP_IMMUTABLE,
>> +					"TYPE",
>> drm_colorop_type_enum_list,
>> +					ARRAY_SIZE(drm_colorop_type_
>> enum_list));
>> +
>> +	if (!prop)
>> +		return -ENOMEM;
>> +
>> +	colorop->type_property = prop;
>> +
>> +	drm_object_attach_property(&colorop->base,
>> +				   colorop->type_property,
>> +				   colorop->type);
>> +
>> +	return ret;
>> +}