On Fri Aug 15, 2025 at 5:50 AM CEST, Alex Hung wrote:
> We've previously introduced DRM_COLOROP_1D_CURVE for
> pre-defined 1D curves. But we also have HW that supports
> custom curves and userspace needs the ability to pass
> custom curves, aka LUTs.
>
> This patch introduces a new colorop type, called
> DRM_COLOROP_1D_LUT that provides a SIZE property which
> is used by a driver to advertise the supported SIZE
> of the LUT, as well as a DATA property which userspace
> uses to set the LUT.
>
> DATA and size function in the same way as current drm_crtc
> GAMMA and DEGAMMA LUTs.
>
> Reviewed-by: Simon Ser <cont...@emersion.fr>
> Signed-off-by: Alex Hung <alex.h...@amd.com>
> Co-developed-by: Harry Wentland <harry.wentl...@amd.com>
> Signed-off-by: Harry Wentland <harry.wentl...@amd.com>
> Reviewed-by: Daniel Stone <dani...@collabora.com>
> ---
> v11:
> - Update names from *_lut_32_* to *_lut32_* (Simon Ser)
>
> v10:
> - 1D LUT API is now using 32BIT RGB with drm_color_lut_32 (Uma Shankar)
>
> v9:
> - Update function names by _plane_ (Chaitanya Kumar Borah)
>
> v8:
> - Add DRM_MODE_PROP_ATOMIC to drm_property_create_range (Simon Ser)
> - Change "1D Curve Custom LUT" to "1D LUT" (Simon Ser)
>
> v7:
> - Change "size" to "lut_size" (this affects multiple following commits)
> - Move "lut_size" from drm_colorop_state to drm_colorop
> - Modify other files accordingly (i.e. from drm_colorop_state->size
> to drm_colorop->lut_size)
>
> v5:
> - Add kernel doc
> - Define SIZE in similar manner to GAMMA_SIZE on drm_crtc (Melissa)
>
> drivers/gpu/drm/drm_atomic.c | 4 +++
> drivers/gpu/drm/drm_atomic_uapi.c | 5 ++++
> drivers/gpu/drm/drm_colorop.c | 43 +++++++++++++++++++++++++++++++
> include/drm/drm_colorop.h | 16 ++++++++++++
> include/uapi/drm/drm_mode.h | 14 ++++++++++
> 5 files changed, 82 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 1f47949aa10b..27eba485fe2b 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -793,6 +793,10 @@ static void drm_atomic_colorop_print_state(struct
> drm_printer *p,
> drm_printf(p, "\tcurve_1d_type=%s\n",
>
> drm_get_colorop_curve_1d_type_name(state->curve_1d_type));
> break;
> + case DRM_COLOROP_1D_LUT:
> + drm_printf(p, "\tsize=%d\n", colorop->lut_size);
> + drm_printf(p, "\tdata blob id=%d\n", state->data ?
> state->data->base.id : 0);
> + break;
> case DRM_COLOROP_CTM_3X4:
> drm_printf(p, "\tdata blob id=%d\n", state->data ?
> state->data->base.id : 0);
> break;
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index fe59dd1f2c07..093635d43ea3 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -697,6 +697,9 @@ static int drm_atomic_color_set_data_property(struct
> drm_colorop *colorop,
> bool replaced = false;
>
> switch (colorop->type) {
> + case DRM_COLOROP_1D_LUT:
> + size = colorop->lut_size * sizeof(struct drm_color_lut32);
> + break;
> case DRM_COLOROP_CTM_3X4:
> size = sizeof(struct drm_color_ctm_3x4);
> break;
> @@ -746,6 +749,8 @@ drm_atomic_colorop_get_property(struct drm_colorop
> *colorop,
> *val = state->bypass;
> } else if (property == colorop->curve_1d_type_property) {
> *val = state->curve_1d_type;
> + } else if (property == colorop->lut_size_property) {
> + *val = colorop->lut_size;
> } else if (property == colorop->data_property) {
> *val = (state->data) ? state->data->base.id : 0;
> } else {
> diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
> index c245a3ff45d3..e799a87f25fe 100644
> --- a/drivers/gpu/drm/drm_colorop.c
> +++ b/drivers/gpu/drm/drm_colorop.c
> @@ -64,6 +64,7 @@
>
> static const struct drm_prop_enum_list drm_colorop_type_enum_list[] = {
> { DRM_COLOROP_1D_CURVE, "1D Curve" },
> + { DRM_COLOROP_1D_LUT, "1D LUT" },
> { DRM_COLOROP_CTM_3X4, "3x4 Matrix"},
> };
>
> @@ -265,6 +266,47 @@ static int drm_colorop_create_data_prop(struct
> drm_device *dev, struct drm_color
> return 0;
> }
>
> +/**
> + * drm_plane_colorop_curve_1d_lut_init - Initialize a DRM_COLOROP_1D_LUT
> + *
> + * @dev: DRM device
> + * @colorop: The drm_colorop object to initialize
> + * @plane: The associated drm_plane
> + * @lut_size: LUT size supported by driver
> + * @return zero on success, -E value on failure
> + */
> +int drm_plane_colorop_curve_1d_lut_init(struct drm_device *dev, struct
> drm_colorop *colorop,
> + struct drm_plane *plane, uint32_t
> lut_size)
> +{
> + struct drm_property *prop;
> + int ret;
> +
> + ret = drm_plane_colorop_init(dev, colorop, plane, DRM_COLOROP_1D_LUT);
> + if (ret)
> + return ret;
> +
> + /* initialize 1D LUT only attribute */
> + /* LUT size */
> + prop = drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE |
> DRM_MODE_PROP_ATOMIC,
> + "SIZE", 0, UINT_MAX);
> + if (!prop)
> + return -ENOMEM;
> +
> + colorop->lut_size_property = prop;
I'm a bit confused here. The property itself is just called "SIZE" which
looks very similar to the generic "DATA" property. However, it is
assigned to `lut_size_property`.
Is this meant to be to be a generic property where the exact usage
depends on the type of the color op (like "DATA"), or is this meant to
be specific to LUTs (in which case the generic name is misleading)?
I also tried to find the user space documentation for all the properties
but could not find them. The only thing I could find was the kernel
documentation of
struct drm_property *lut_size_property;
Which says "Size property for custom LUT from userspace."
> + drm_object_attach_property(&colorop->base, colorop->lut_size_property,
> lut_size);
> + colorop->lut_size = lut_size;
> +
> + /* data */
> + ret = drm_colorop_create_data_prop(dev, colorop);
> + if (ret)
> + return ret;
> +
> + drm_colorop_reset(colorop);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_plane_colorop_curve_1d_lut_init);
> +
> int drm_plane_colorop_ctm_3x4_init(struct drm_device *dev, struct
> drm_colorop *colorop,
> struct drm_plane *plane)
> {
> @@ -373,6 +415,7 @@ void drm_colorop_reset(struct drm_colorop *colorop)
>
> static const char * const colorop_type_name[] = {
> [DRM_COLOROP_1D_CURVE] = "1D Curve",
> + [DRM_COLOROP_1D_LUT] = "1D LUT",
> [DRM_COLOROP_CTM_3X4] = "3x4 Matrix",
> };
>
> diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h
> index c6d2b289e3cf..fe662e0f89aa 100644
> --- a/include/drm/drm_colorop.h
> +++ b/include/drm/drm_colorop.h
> @@ -259,6 +259,13 @@ struct drm_colorop {
> */
> struct drm_property *bypass_property;
>
> + /**
> + * @lut_size:
> + *
> + * Number of entries of the custom LUT. This should be read-only.
> + */
> + uint32_t lut_size;
> +
> /**
> * @curve_1d_type_property:
> *
> @@ -266,6 +273,13 @@ struct drm_colorop {
> */
> struct drm_property *curve_1d_type_property;
>
> + /**
> + * @lut_size_property:
> + *
> + * Size property for custom LUT from userspace.
> + */
> + struct drm_property *lut_size_property;
> +
> /**
> * @data_property:
> *
> @@ -312,6 +326,8 @@ void drm_colorop_pipeline_destroy(struct drm_device *dev);
>
> int drm_plane_colorop_curve_1d_init(struct drm_device *dev, struct
> drm_colorop *colorop,
> struct drm_plane *plane, u64 supported_tfs);
> +int drm_plane_colorop_curve_1d_lut_init(struct drm_device *dev, struct
> drm_colorop *colorop,
> + struct drm_plane *plane, uint32_t
> lut_size);
> int drm_plane_colorop_ctm_3x4_init(struct drm_device *dev, struct
> drm_colorop *colorop,
> struct drm_plane *plane);
>
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index bcc4e9845881..24fd52e16953 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -902,6 +902,20 @@ enum drm_colorop_type {
> */
> DRM_COLOROP_1D_CURVE,
>
> + /**
> + * @DRM_COLOROP_1D_LUT:
> + *
> + * enum string "1D LUT"
> + *
> + * A simple 1D LUT of uniformly spaced &drm_color_lut32 entries,
> + * packed into a blob via the DATA property. The driver's
> + * expected LUT size is advertised via the SIZE property.
> + *
> + * The DATA blob is an array of struct drm_color_lut32 with size
> + * of "lut_size".
> + */
> + DRM_COLOROP_1D_LUT,
> +
> /**
> * @DRM_COLOROP_CTM_3X4:
> *
