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

["Nícolas F. R. A. Prado" <nfraprado@collabora.com>]

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?

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;
> +}