Re: [RFC PATCH 01/10] drm/doc/rfc: Describe why prescriptive color pipeline is needed

[Pekka Paalanen <ppaalanen@gmail.com>]

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

On Tue, 10 Oct 2023 15:13:46 -0100
Melissa Wen <mwen@igalia.com> wrote:

> O 09/08, Harry Wentland wrote:
> > Signed-off-by: Harry Wentland <harry.wentland@amd.com>
> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
> > Cc: Simon Ser <contact@emersion.fr>
> > Cc: Harry Wentland <harry.wentland@amd.com>
> > Cc: Melissa Wen <mwen@igalia.com>
> > Cc: Jonas Ådahl <jadahl@redhat.com>
> > Cc: Sebastian Wick <sebastian.wick@redhat.com>
> > Cc: Shashank Sharma <shashank.sharma@amd.com>
> > Cc: Alexander Goins <agoins@nvidia.com>
> > Cc: Joshua Ashton <joshua@froggi.es>
> > Cc: Michel Dänzer <mdaenzer@redhat.com>
> > Cc: Aleix Pol <aleixpol@kde.org>
> > Cc: Xaver Hugl <xaver.hugl@gmail.com>
> > Cc: Victoria Brekenfeld <victoria@system76.com>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: Uma Shankar <uma.shankar@intel.com>
> > Cc: Naseer Ahmed <quic_naseer@quicinc.com>
> > Cc: Christopher Braga <quic_cbraga@quicinc.com>
> > ---
> >  Documentation/gpu/rfc/color_pipeline.rst | 278 +++++++++++++++++++++++
> >  1 file changed, 278 insertions(+)
> >  create mode 100644 Documentation/gpu/rfc/color_pipeline.rst
> > 
> > diff --git a/Documentation/gpu/rfc/color_pipeline.rst b/Documentation/gpu/rfc/color_pipeline.rst
> > new file mode 100644
> > index 000000000000..bfa4a8f12087
> > --- /dev/null
> > +++ b/Documentation/gpu/rfc/color_pipeline.rst

...

> > +Color Pipeline Discovery
> > +========================
> > +
> > +A DRM client wanting color management on a drm_plane will:
> > +
> > +1. Read all drm_colorop objects
> > +2. Get the COLOR_PIPELINE property of the plane
> > +3. iterate all COLOR_PIPELINE enum values
> > +4. for each enum value walk the color pipeline (via the NEXT pointers)
> > +   and see if the available color operations are suitable for the
> > +   desired color management operations
> > +
> > +An example of chained properties to define an AMD pre-blending color
> > +pipeline might look like this::  
> 
> Hi Harry,
> 
> Thanks for sharing this proposal. Overall I think it's very aligned with
> Simon's description of the generic KMS color API. I think it's a good
> start point and we can refine over iterations. My general questions have
> already been pointed out by Sebastian and Pekka (mainly regarding the
> BYPASS property).
> 
> I still have some doubts on how to fit these set of colorops with some
> AMD corners cases as below:
> 
> > +
> > +    Plane 10
> > +    ├─ "type": immutable enum {Overlay, Primary, Cursor} = Primary
> > +    └─ "color_pipeline": enum {0, 42} = 0
> > +    Color operation 42 (input CSC)
> > +    ├─ "type": enum {Bypass, Matrix} = Matrix
> > +    ├─ "matrix_data": blob
> > +    └─ "next": immutable color operation ID = 43  
> 
> IIUC, for input CSC, there are currently two possiblities, or we use
> `drm_plane_color_encoding` and `drm_plane_color range` to get
> pre-defined coefficients or we set a custom matrix here, right? If so, I
> think we need some kind of pre-defined matrix option?
> 
> Also, with this new plane API in place, I understand that we will
> already need think on how to deal with the mixing between old drm color
> properties (color encoding and color range) and these new way of setting
> plane color properties. IIUC, Pekka asked a related question about it
> when talking about CRTC automatic RGB->YUV (?) 

I didn't realize color encoding and color range KMS plane properties
even existed. There is even color space on rockchip!

https://drmdb.emersion.fr/properties?object-type=4008636142

That list has even more conflicts: DEGAMMA_MODE, EOTF, FEATURE,
NV_INPUT_COLORSPACE, SCALING_FILTER, WATERMARK, alpha, GLOBAL_ALPHA,
brightness, colorkey, contrast, and more. I hope most of them are
actually from downstream drivers.

I think they should be forbidden to be used together with the new
pipeline UAPI. Mixing does not work in the long run, it would be
undefined at what position do the old properties apply in a pipeline.

Apparently, we already need a DRM client cap for the new color pipeline
UAPI to hide these legacy things.


This is different from "CRTC automatic RGB->YUV", because the CRTC
thing is chosen silently by the driver and there is nothing after it.
Those old plane properties are explicitly programmed by userspace.


Thanks,
pq

> > +    Color operation 43
> > +    ├─ "type": enum {Scaling} = Scaling
> > +    └─ "next": immutable color operation ID = 44
> > +    Color operation 44 (DeGamma)
> > +    ├─ "type": enum {Bypass, 1D curve} = 1D curve
> > +    ├─ "1d_curve_type": enum {sRGB, PQ, …} = sRGB
> > +    └─ "next": immutable color operation ID = 45
> > +    Color operation 45 (gamut remap)
> > +    ├─ "type": enum {Bypass, Matrix} = Matrix
> > +    ├─ "matrix_data": blob
> > +    └─ "next": immutable color operation ID = 46
> > +    Color operation 46 (shaper LUT RAM)
> > +    ├─ "type": enum {Bypass, 1D curve} = 1D curve
> > +    ├─ "1d_curve_type": enum {LUT} = LUT
> > +    ├─ "lut_size": immutable range = 4096
> > +    ├─ "lut_data": blob
> > +    └─ "next": immutable color operation ID = 47  
> 
> For shaper and blend LUT RAM, that the driver supports pre-defined
> curves and custom LUT at the same time but the resulted LUT is a
> combination of those, how to make this behavior clear? Could this
> behavior be described by two colorop in a row: for example, one for
> shaper TF and,just after, another for shaper LUT or would it not be the
> right representation?
> 
> > +    Color operation 47 (3D LUT RAM)
> > +    ├─ "type": enum {Bypass, 3D LUT} = 3D LUT
> > +    ├─ "lut_size": immutable range = 17
> > +    ├─ "lut_data": blob
> > +    └─ "next": immutable color operation ID = 48
> > +    Color operation 48 (blend gamma)
> > +    ├─ "type": enum {Bypass, 1D curve} = 1D curve
> > +    ├─ "1d_curve_type": enum {LUT, sRGB, PQ, …} = LUT
> > +    ├─ "lut_size": immutable range = 4096
> > +    ├─ "lut_data": blob
> > +    └─ "next": immutable color operation ID = 0
> > +
> > +
> > +Color Pipeline Programming
> > +==========================
> > +
> > +Once a DRM client has found a suitable pipeline it will:
> > +
> > +1. Set the COLOR_PIPELINE enum value to the one pointing at the first
> > +   drm_colorop object of the desired pipeline
> > +2. Set the properties for all drm_colorop objects in the pipeline to the
> > +   desired values, setting BYPASS to true for unused drm_colorop blocks,
> > +   and false for enabled drm_colorop blocks
> > +3. Perform atomic_check/commit as desired
> > +
> > +To configure the pipeline for an HDR10 PQ plane and blending in linear
> > +space, a compositor might perform an atomic commit with the following
> > +property values::
> > +
> > +    Plane 10
> > +    └─ "color_pipeline" = 42
> > +    Color operation 42 (input CSC)
> > +    └─ "bypass" = true
> > +    Color operation 44 (DeGamma)
> > +    └─ "bypass" = true
> > +    Color operation 45 (gamut remap)
> > +    └─ "bypasse" = true
> > +    Color operation 46 (shaper LUT RAM)
> > +    └─ "bypass" = true
> > +    Color operation 47 (3D LUT RAM)
> > +    └─ "lut_data" = Gamut mapping + tone mapping + night mode
> > +    Color operation 48 (blend gamma)
> > +    └─ "1d_curve_type" = PQ inverse EOTF  
> 
> Isn't it a PQ EOTF for blend gamma?
> 
> Best Regards,
> 
> Melissa
> 
> > +
> > +
> > +References
> > +==========
> > +
> > +1. https://lore.kernel.org/dri-devel/QMers3awXvNCQlyhWdTtsPwkp5ie9bze_hD5nAccFW7a_RXlWjYB7MoUW_8CKLT2bSQwIXVi5H6VULYIxCdgvryZoAoJnC5lZgyK1QWn488=@emersion.fr/
> > \ No newline at end of file
> > -- 
> > 2.42.0
> >   


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