Re: [PATCH 09/11] drm/colorop: Introduce 3x3 Matrix

[Pekka Paalanen <pekka.paalanen@collabora.com>]

[-- Attachment #1: Type: text/plain, Size: 5346 bytes --]

On Tue, 23 Dec 2025 16:44:50 -0300
Nícolas F. R. A. Prado <nfraprado@collabora.com> wrote:

> Introduce a 3x3 Matrix colorop analogous to the 3x4 Matrix colorop, with
> the difference of not supporting offset coefficients.
> 
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> ---
>  drivers/gpu/drm/drm_atomic.c      |  1 +
>  drivers/gpu/drm/drm_atomic_uapi.c |  3 +++
>  drivers/gpu/drm/drm_colorop.c     | 21 +++++++++++++++++++++
>  include/drm/drm_colorop.h         |  3 +++
>  include/uapi/drm/drm_mode.h       | 16 ++++++++++++++++
>  5 files changed, 44 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 6d3ea8056b60..bf4a31c02b70 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -802,6 +802,7 @@ static void drm_atomic_colorop_print_state(struct drm_printer *p,
>  		drm_printf(p, "\tdata blob id=%d\n", state->data ? state->data->base.id : 0);
>  		break;
>  	case DRM_COLOROP_CTM_3X4:
> +	case DRM_COLOROP_CTM_3X3:
>  		drm_printf(p, "\tdata blob id=%d\n", state->data ? state->data->base.id : 0);
>  		break;
>  	case DRM_COLOROP_MULTIPLIER:
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 7320db4b8489..7a70e894a2ef 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -709,6 +709,9 @@ static int drm_atomic_color_set_data_property(struct drm_colorop *colorop,
>  		size = colorop->size * colorop->size * colorop->size *
>  		       sizeof(struct drm_color_lut32);
>  		break;
> +	case DRM_COLOROP_CTM_3X3:
> +		size = sizeof(struct drm_color_ctm);
> +		break;
>  	default:
>  		/* should never get here */
>  		return -EINVAL;
> diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
> index a19e03fb9c7c..51c1a0726dfa 100644
> --- a/drivers/gpu/drm/drm_colorop.c
> +++ b/drivers/gpu/drm/drm_colorop.c
> @@ -68,6 +68,7 @@ static const struct drm_prop_enum_list drm_colorop_type_enum_list[] = {
>  	{ DRM_COLOROP_CTM_3X4, "3x4 Matrix"},
>  	{ DRM_COLOROP_MULTIPLIER, "Multiplier"},
>  	{ DRM_COLOROP_3D_LUT, "3D LUT"},
> +	{ DRM_COLOROP_CTM_3X3, "3x3 Matrix"},
>  };
>  
>  static const char * const colorop_curve_1d_type_names[] = {
> @@ -377,6 +378,26 @@ int drm_plane_colorop_ctm_3x4_init(struct drm_device *dev, struct drm_colorop *c
>  }
>  EXPORT_SYMBOL(drm_plane_colorop_ctm_3x4_init);
>  
> +int drm_plane_colorop_ctm_3x3_init(struct drm_device *dev, struct drm_colorop *colorop,
> +				   struct drm_plane *plane, const struct drm_colorop_funcs *funcs,
> +				   uint32_t flags)
> +{
> +	int ret;
> +
> +	ret = drm_plane_colorop_init(dev, colorop, plane, funcs, DRM_COLOROP_CTM_3X3, flags);
> +	if (ret)
> +		return ret;
> +
> +	ret = drm_colorop_create_data_prop(dev, colorop);
> +	if (ret)
> +		return ret;
> +
> +	drm_colorop_reset(colorop);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_plane_colorop_ctm_3x3_init);
> +
>  /**
>   * drm_plane_colorop_mult_init - Initialize a DRM_COLOROP_MULTIPLIER
>   *
> diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h
> index 8ec98521607d..ee7fa0eb5dbf 100644
> --- a/include/drm/drm_colorop.h
> +++ b/include/drm/drm_colorop.h
> @@ -426,6 +426,9 @@ int drm_plane_colorop_curve_1d_lut_init(struct drm_device *dev, struct drm_color
>  int drm_plane_colorop_ctm_3x4_init(struct drm_device *dev, struct drm_colorop *colorop,
>  				   struct drm_plane *plane, const struct drm_colorop_funcs *funcs,
>  				   uint32_t flags);
> +int drm_plane_colorop_ctm_3x3_init(struct drm_device *dev, struct drm_colorop *colorop,
> +				   struct drm_plane *plane, const struct drm_colorop_funcs *funcs,
> +				   uint32_t flags);
>  int drm_plane_colorop_mult_init(struct drm_device *dev, struct drm_colorop *colorop,
>  				struct drm_plane *plane, const struct drm_colorop_funcs *funcs,
>  				uint32_t flags);
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index cbbbfc1dfe2b..b894b19eb9f8 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -964,6 +964,22 @@ enum drm_colorop_type {
>  	 *         color = lut3d[index]
>  	 */
>  	DRM_COLOROP_3D_LUT,
> +
> +	/**
> +	 * @DRM_COLOROP_CTM_3X3:
> +	 *
> +	 * enum string "3x3 Matrix"
> +	 *
> +	 * A 3x3 matrix. Its values are specified via the
> +	 * &drm_color_ctm struct provided via the DATA property.
> +	 *
> +	 * The DATA blob is a float[9]:
> +	 * out   matrix          in
> +	 * | R |   | 0  1  2 |   | R |
> +	 * | G | = | 3  4  5 | x | G |
> +	 * | B |   | 6  7  8 |   | B |
> +	 */
> +	DRM_COLOROP_CTM_3X3,
>  };
>  
>  /**
> 

Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>

FWIW, the statement "The DATA blob is a float[9]" is incorrect, but the
same wording exists already with the DRM_COLOROP_CTM_3X4. The data type
is not a float, nor is it a float reinterpreted as __u64 (they are not
even the same size). Documentation for the structs explains the correct
type, which is a fixed-point number.

The struct types document also the layout, so there is no need to
repeat the layout here. It may be worth mentioning that the blob must
contain exactly one instance of the struct.


Thanks,
pq


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]