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

["Nícolas F. R. A. Prado" <nfraprado@collabora.com>]

On Fri, 2026-02-06 at 11:27 +0200, Pekka Paalanen wrote:
> 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.

Makes sense, thanks! I'll fix that in v2.

-- 
Thanks,

Nícolas