[V7 07/45] drm/colorop: Add 1D Curve subtype
Louis Chauvet
louis.chauvet at bootlin.com
Fri Feb 28 09:24:12 UTC 2025
Le 28/02/2025 à 02:07, Alex Hung a écrit :
>
>
> On 2/25/25 03:13, Louis Chauvet wrote:
>>
>>
>> Le 20/12/2024 à 05:33, Alex Hung a écrit :
>>> From: Harry Wentland <harry.wentland at 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.
>>>
>>> Signed-off-by: Harry Wentland <harry.wentland at amd.com>
>>> Co-developed-by: Alex Hung <alex.hung at amd.com>
>>> Signed-off-by: Alex Hung <alex.hung at amd.com>
>>> ---
>>> v5:
>>> - Add drm_get_colorop_curve_1d_type_name in header
>>> - Add drm_colorop_init
>>> - Set default curve
>>> - Add kernel docs
>>>
>>> v4:
>>> - Use drm_colorop_curve_1d_type_enum_list to get name (Pekka)
>>> - Create separate init function for 1D curve
>>> - Pass supported TFs into 1D curve init function
>>>
>>> drivers/gpu/drm/drm_atomic_uapi.c | 18 ++--
>>> drivers/gpu/drm/drm_colorop.c | 134 ++++++++++++++++++++++++++++++
>>> include/drm/drm_colorop.h | 60 +++++++++++++
>>> 3 files changed, 207 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/
>>> drm_atomic_uapi.c
>>> index 59fc25b59100..9a5dbf0a1306 100644
>>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>>> @@ -648,11 +648,17 @@ static int
>>> drm_atomic_colorop_set_property(struct drm_colorop *colorop,
>>> struct drm_colorop_state *state, struct drm_file *file_priv,
>>> struct drm_property *property, uint64_t val)
>>> {
>>> - drm_dbg_atomic(colorop->dev,
>>> - "[COLOROP:%d] unknown property [PROP:%d:%s]]\n",
>>> - colorop->base.id,
>>> - property->base.id, property->name);
>>> - return -EINVAL;
>>> + if (property == colorop->curve_1d_type_property) {
>>> + state->curve_1d_type = val;
>>> + } else {
>>> + drm_dbg_atomic(colorop->dev,
>>> + "[COLOROP:%d:%d] unknown property [PROP:%d:%s]]\n",
>>> + colorop->base.id, colorop->type,
>>> + property->base.id, property->name);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + return 0;
>>> }
>>> static int
>>> @@ -662,6 +668,8 @@ drm_atomic_colorop_get_property(struct drm_colorop
>>> *colorop,
>>> {
>>> if (property == colorop->type_property) {
>>> *val = colorop->type;
>>> + } else if (property == colorop->curve_1d_type_property) {
>>> + *val = state->curve_1d_type;
>>> } else {
>>> return -EINVAL;
>>> }
>>> diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/
>>> drm_colorop.c
>>> index 1459a28c7e7b..a42de0aa48e1 100644
>>> --- a/drivers/gpu/drm/drm_colorop.c
>>> +++ b/drivers/gpu/drm/drm_colorop.c
>>> @@ -31,6 +31,123 @@
>>> #include "drm_crtc_internal.h"
>>> +static const struct drm_prop_enum_list drm_colorop_type_enum_list[] = {
>>> + { DRM_COLOROP_1D_CURVE, "1D Curve" },
>>> +};
>>> +
>>> +static const char * const colorop_curve_1d_type_names[] = {
>>> + [DRM_COLOROP_1D_CURVE_SRGB_EOTF] = "sRGB EOTF",
>>> + [DRM_COLOROP_1D_CURVE_SRGB_INV_EOTF] = "sRGB Inverse EOTF",
>>> +};
>>> +
>>> +
>>> +/* Init Helpers */
>>> +
>>> +static int drm_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++;
>>> +
>>> + /* 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));
>>
>> I think this function belongs to the previous patch "Add TYPE property".
>
> This function is only called by the first colorop. Some pieces of the
> code in this function are introduced with the first colorop (1D curve)
> so it makes sense to include it here.
True! I did not saw it, you can keep it here indeed
>>
>>> +
>>> + if (!prop)
>>> + return -ENOMEM;
>>> +
>>> + colorop->type_property = prop;
>>> +
>>> + drm_object_attach_property(&colorop->base,
>>> + colorop->type_property,
>>> + colorop->type);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +/**
>>> + * drm_colorop_curve_1d_init - Initialize a DRM_COLOROP_1D_CURVE
>>> + *
>>> + * @dev: DRM device
>>> + * @colorop: The drm_colorop object to initialize
>>> + * @plane: The associated drm_plane
>>> + * @supported_tfs: A bitfield of supported drm_colorop_curve_1d_init
>>> enum values,
>>> + * created using BIT(curve_type) and combined with
>>> the OR '|'
>>> + * operator.
>>> + * @return zero on success, -E value on failure
>>> + */
>>> +int drm_colorop_curve_1d_init(struct drm_device *dev, struct
>>> drm_colorop *colorop,
>>> + struct drm_plane *plane, u64 supported_tfs)
>>> +{
>>> + struct drm_prop_enum_list enum_list[DRM_COLOROP_1D_CURVE_COUNT];
>>> + int i, len;
>>> +
>>> + struct drm_property *prop;
>>> + int ret;
>>> +
>>> + if (!supported_tfs) {
>>> + drm_err(dev,
>>> + "No supported TFs for new 1D curve colorop on [PLANE:%d:
>>> %s]\n",
>>> + plane->base.id, plane->name);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + if ((supported_tfs & -BIT(DRM_COLOROP_1D_CURVE_COUNT)) != 0) {
>>> + drm_err(dev, "Unknown TF provided on [PLANE:%d:%s]\n",
>>> + plane->base.id, plane->name);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + ret = drm_colorop_init(dev, colorop, plane, DRM_COLOROP_1D_CURVE);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + len = 0;
>>> + for (i = 0; i < DRM_COLOROP_1D_CURVE_COUNT; i++) {
>>> + if ((supported_tfs & BIT(i)) == 0)
>>> + continue;
>>> +
>>> + enum_list[len].type = i;
>>> + enum_list[len].name = colorop_curve_1d_type_names[i];
>>> + len++;
>>> + }
>>> +
>>> + if (WARN_ON(len <= 0))
>>> + return -EINVAL;
>>> +
>>> +
>>> + /* initialize 1D curve only attribute */
>>> + prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
>>> "CURVE_1D_TYPE",
>>> + enum_list, len);
>>> + if (!prop)
>>> + return -ENOMEM;
>>> +
>>> + colorop->curve_1d_type_property = prop;
>>> + drm_object_attach_property(&colorop->base, colorop-
>>>> curve_1d_type_property,
>>> + enum_list[0].type);
>>> + drm_colorop_reset(colorop);
>>> +
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL(drm_colorop_curve_1d_init);
>>> +
>>> static void __drm_atomic_helper_colorop_duplicate_state(struct
>>> drm_colorop *colorop,
>>> struct drm_colorop_state *state)
>>> {
>>> @@ -70,7 +187,16 @@ void drm_colorop_atomic_destroy_state(struct
>>> drm_colorop *colorop,
>>> static void __drm_colorop_state_reset(struct drm_colorop_state
>>> *colorop_state,
>>> struct drm_colorop *colorop)
>>> {
>>> + u64 val;
>>> +
>>> colorop_state->colorop = colorop;
>>> +
>>> + if (colorop->curve_1d_type_property) {
>>> + drm_object_property_get_default_value(&colorop->base,
>>> + colorop->curve_1d_type_property,
>>> + &val);
>>> + colorop_state->curve_1d_type = val;
>>> + }
>>> }
>>> /**
>>> @@ -114,3 +240,11 @@ const char *drm_get_colorop_type_name(enum
>>> drm_colorop_type type
>>> return colorop_type_name[type];
>>
>> Probably a dumb question: can't we use drm_colorop_type_enum_list
>> instead of colorop_type_name in the drm_get_colorop_type_name function?
>> So we will avoid duplicating the string "1D Curve".
>
> Using drm_colorop_type_enum_list in drm_get_colorop_type_name needs
> enumerating the list every time (extra CPU cycles) unlike using
> colorop_type_name[].
Why do you want to iterate over the list? See [1]/[2], it seems to be a
common patter to simply order the enum list and use the index in it.
Furthermore, I don't expect this function to be called often, so a list
enumeration doesn't seem expensive (maybe called almost never? The only
usage I found in your series is __drm_state_dump).
I don't have a strong opinion on this, so:
Reviewed-by: Louis Chauvet <louis.chauvet at bootlin.com>
Thanks!
Louis Chauvet
[1]:https://elixir.bootlin.com/linux/v6.13.4/source/drivers/gpu/drm/drm_connector.c#L1238-L1259
[2]:https://elixir.bootlin.com/linux/v6.13.4/source/drivers/gpu/drm/drm_connector.c#L972-L994
>>
>>> }
>>
>> Thanks,
>> Louis Chauvet
>>
>> [...]
>>
>
--
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
More information about the wayland-devel
mailing list