On 2023-09-13 07:29, Pekka Paalanen wrote:
> On Fri, 8 Sep 2023 11:02:26 -0400
> Harry Wentland <harry.wentland@xxxxxxx> 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
>
> Hi Harry,
>
> it's really nice to see this!
>
Thanks for the feedback. I'm just putting a v2 together with comments
(partially) addressed.
> Sebastian started on the backward/forward compatibility, so I'll
> comment on everything else here, and leave the compatibility for that
> thread.
>
>> 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 Plane Property
>> +=============================
>> +
>> +Because we don't have existing KMS color properties in the pre-blending
>> +portion of display pipelines (i.e. on drm_planes) we are introducing
>> +color pipelines here first. Eventually we'll want to use the same
>> +concept for the post-blending portion, i.e. drm_crtcs.
>
> This paragraph might fit better in a cover letter.
>
>> +
>> +Color Pipelines are created by a driver and advertised via a new
>> +COLOR_PIPELINE enum property on each plane. Values of the property
>> +always include '0', which is the default and means all color processing
>> +is disabled. Additional values will be the object IDs of the first
>> +drm_colorop in a pipeline. A driver can create and advertise none, one,
>> +or more possible color pipelines. A DRM client will select a color
>> +pipeline by setting the COLOR PIPELINE to the respective value.
>> +
>> +In the case where drivers have custom support for pre-blending color
>> +processing those drivers shall reject atomic commits that are trying to
>> +set both the custom color properties, as well as the COLOR_PIPELINE
>
> s/set/use/ because one of them could be carried-over state from
> previous commits while not literally set in this one.
>
>> +property.
>> +
>> +An example of a COLOR_PIPELINE property on a plane might look like this::
>> +
>> + Plane 10
>> + ├─ "type": immutable enum {Overlay, Primary, Cursor} = Primary
>> + ├─ …
>> + └─ "color_pipeline": enum {0, 42, 52} = 0
>
> Enum values are string names. I presume the intention here is that the
> strings will never need to be parsed, and the uint64_t is always equal
> to the string representation, right?
>
> That needs a statement here. It differs from all previous uses of
> enums, and e.g. requires a little bit of new API in libweston's
> DRM-backend to handle since it has its own enums referring to the
> string names that get mapped to the uint64_t per owning KMS object.
>
I'm currently putting the DRM object ID in the "value" and use the
"name" as a descriptive name.
> struct drm_mode_property_enum {
> __u64 value;
> char name[DRM_PROP_NAME_LEN];
> };
This works well in IGT and gives us a nice descriptive name for
debugging, but I could consider changing this if it'd simplify
people's lives.
>> +
>> +
>> +Color Pipeline Discovery
>> +========================
>> +
>> +A DRM client wanting color management on a drm_plane will:
>> +
>> +1. Read all drm_colorop objects
>
> What does this do?
We probably don't need this, and with it we probably don't need
the new IOCTLs. I added this to align with IGT's current init
procedure where it reads all DRM core objects, like planes, etc.,
before using them. But realistically we can just look at the
colorop ID from the COLOR_PIPELINE property and then retrieve
the other colorops through the NEXT pointer.
>
>> +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::
>> +
>> + Plane 10
>> + ├─ "type": immutable enum {Overlay, Primary, Cursor} = Primary
>> + └─ "color_pipeline": enum {0, 42} = 0
>> + Color operation 42 (input CSC)
>
> I presume the string "(input CSC)" does not come from KMS, and is
> actually just a comment added here by hand?
>
Exactly. It only exists as a comment here. I'll remove it.
Harry
>
> Thanks,
> pq
>
>> + ├─ "type": enum {Bypass, Matrix} = Matrix
>> + ├─ "matrix_data": blob
>> + └─ "next": immutable color operation ID = 43
>> + 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
>> + 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
>> +
>> +
>> +References
>> +==========
>> +
>> +1. https://lore.kernel.org/dri-devel/QMers3awXvNCQlyhWdTtsPwkp5ie9bze_hD5nAccFW7a_RXlWjYB7MoUW_8CKLT2bSQwIXVi5H6VULYIxCdgvryZoAoJnC5lZgyK1QWn488=@emersion.fr/
>> \ No newline at end of file
>
