On Wed, Oct 11, 2023 at 12:20 PM Pekka Paalanen <ppaalanen@xxxxxxxxx> wrote:
>
> On Tue, 10 Oct 2023 15:13:46 -0100
> Melissa Wen <mwen@xxxxxxxxxx> wrote:
>
> > O 09/08, Harry Wentland wrote:
> > > Signed-off-by: Harry Wentland <harry.wentland@xxxxxxx>
> > > Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
> > > Cc: Pekka Paalanen <pekka.paalanen@xxxxxxxxxxxxx>
> > > Cc: Simon Ser <contact@xxxxxxxxxxx>
> > > Cc: Harry Wentland <harry.wentland@xxxxxxx>
> > > Cc: Melissa Wen <mwen@xxxxxxxxxx>
> > > Cc: Jonas Ådahl <jadahl@xxxxxxxxxx>
> > > Cc: Sebastian Wick <sebastian.wick@xxxxxxxxxx>
> > > Cc: Shashank Sharma <shashank.sharma@xxxxxxx>
> > > Cc: Alexander Goins <agoins@xxxxxxxxxx>
> > > Cc: Joshua Ashton <joshua@xxxxxxxxx>
> > > Cc: Michel Dänzer <mdaenzer@xxxxxxxxxx>
> > > Cc: Aleix Pol <aleixpol@xxxxxxx>
> > > Cc: Xaver Hugl <xaver.hugl@xxxxxxxxx>
> > > Cc: Victoria Brekenfeld <victoria@xxxxxxxxxxxx>
> > > Cc: Daniel Vetter <daniel@xxxxxxxx>
> > > Cc: Uma Shankar <uma.shankar@xxxxxxxxx>
> > > Cc: Naseer Ahmed <quic_naseer@xxxxxxxxxxx>
> > > Cc: Christopher Braga <quic_cbraga@xxxxxxxxxxx>
> > > ---
> > > 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?
Seems reasonable. If they are mutually exclusive you might want to
expose 2 different pipelines for it.
> > 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 (?)
Indeed, good catch! I listed some of them in my proposal more than one
year ago but completely forgot about them already.
>
> 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.
Agreed. We'll need one cap for planes and one in the future for CRTCs then.
>
> 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
> > >
>
