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

["Borah, Chaitanya Kumar" <chaitanya.kumar.borah@intel.com>]

On 12/17/2025 6:36 AM, Alex Hung wrote:
> 
> 
> 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.
> 

There is one caveat though drm_colorop_pipeline_destroy() destroys all 
the colorop in the mode_config. That means, that pipeline creation for 
any plane fails, all colorops (even from successfully created pipelines) 
are destroyed too. However, that may well be your intention here, I just 
thought I would point it out.

For i915/xe, I decided to destroy colorops only for the failed pipeline.

https://lore.kernel.org/intel-gfx/20251219065614.190834-1-chaitanya.kumar.borah@intel.com/T/#m143bb249df288cf88123d5d66283918f3317ecc2

Anyway, all these are low probability scenarios.

==
Chaitanya

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