Re: [PATCH v3 01/11] drm/colorop: Add DRM_COLOROP_FIXED_MATRIX

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

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

On Tue, 21 Jul 2026 15:27:55 -0400
Harry Wentland <harry.wentland@amd.com> wrote:

> On 2026-07-03 06:52, Pekka Paalanen wrote:
> > On Tue, 23 Jun 2026 12:48:02 -0400
> > Harry Wentland <harry.wentland@amd.com> wrote:
> >   
> >> From: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
> >>
> >> Introduce DRM_COLOROP_FIXED_MATRIX, a new colorop type representing a
> >> hardware that performs a fixed matrix operation.
> >>
> >> Unlike CTM-based colorops, this block does not expose programmable
> >> coefficients. Instead, userspace selects one of the predefined
> >> hardware modes via a new FIXED_MATRIX_TYPE enum property. Supported modes
> >> include common YCbCr->RGB and RGB709->RGB2020 conversions.
> >>
> >> v2:
> >>  - Naming changes (Pekka)
> >>
> >> v3:
> >>  - Fix NC matrix enum name and string (Melissa)
> >>  - Rebase
> >>
> >> Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
> >> Reviewed-by: Melissa Wen <mwen@igalia.com>
> >> Reviewed-by: Harry Wentland <harry.wentland@amd.com>
> >> ---
> >>  drivers/gpu/drm/drm_atomic.c      |   4 ++
> >>  drivers/gpu/drm/drm_atomic_uapi.c |   4 ++
> >>  drivers/gpu/drm/drm_colorop.c     | 106 ++++++++++++++++++++++++++++++
> >>  include/drm/drm_colorop.h         |  84 +++++++++++++++++++++++
> >>  include/uapi/drm/drm_mode.h       |  12 ++++
> >>  5 files changed, 210 insertions(+)
> >>  
> > 
> > Hi,
> > 
> > the UAPI looks good.
> > 
> > Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> > 
> > It's a little bit unfortunate that the colorop UAPI documentation is
> > embedded into kernel-private enum types' documentation.
> >   
> 
> Agreed. This goes beyond this patchset. I'll have a look at a separate
> series to see if we can fix that.
> 
> > Is RGB always full-range? The documentation could be more explicit
> > about that. Maybe the documentation should spell out the matrix
> > contents just to be clear? There are more than one form of limited
> > range IIRC (HDMI, JPEG, SDI). Or maybe JPEG was a different type of
> > full-range?
> >   
> 
> It's always full-range. Agreed about spelling out the matrix contents.
> 
> >> +	 * enum string "RGB709 to RGB2020"
> >> +	 *
> >> +	 * Selects the fixed-function CSC preset that converts RGB
> >> +	 * (BT.709) colorimetry to RGB (BT.2020).  
> > 
> > Why is this called a "CSC preset" rather than a matrix? This confuses
> > me.
> >   
> 
> Will fix that up.
> 
> I was taking Chaitanya's patch as-is and adding my changes in Patch 2
> but with all these changes I'll end up squashing the two commits
> together.
> 
> > Was there a conclusion from discussions around compatible pixel formats
> > for a specific choice of a fixed matrix? How will userspace discover
> > that, or will it be based on documentation and enforced as a common
> > requirement over all drivers?
> > 
> > ISTR some hardware requiring an YUV pixel format to be able to use the
> > YCbCr conversion matrices, or vice versa. Otherwise there is no reason
> > to have that limitation.
> >   
> 
> We talked about this at the Hackfest (offline most likely) and the
> consensus is that there's no reason to limit this to certain pixel formats.
> We should treat it as a simple matrix that operates on the 3 input channels.
> If userspace decides to select a CSC matrix for an RGB buffer it's assumed
> that that's intentional and userspace knows what it's doing.

Excellent!

Thanks, Harry!
-pq

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