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@xxxxxxx>
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@xxxxxxx>
Co-developed-by: Alex Hung <alex.hung@xxxxxxx>
Signed-off-by: Alex Hung <alex.hung@xxxxxxx>
---
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@xxxxxxxxxxx>
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
