Re: [PATCH V8 03/43] drm/doc/rfc: Describe why prescriptive color pipeline is needed
Thanks a lot for these docs, very well written. I especially like the
"Driver Implementer's Guide" section.
A few minor comments below, regardless:
Reviewed-by: Simon Ser
> +What problem are we solving?
> +
> +
> +We would like to support pre-, and post-blending complex color
> +transformations in display controller hardware in order to allow for
> +HW-supported HDR use-cases, as well as to provide support to
> +color-managed applications, such as video or image editors.
> +
> +It is possible to support an HDR output on HW supporting the Colorspace
> +and HDR Metadata drm_connector properties, but that requires the
> +compositor or application to render and compose the content into one
> +final buffer intended for display. Doing so is costly.
> +
> +Most modern display HW offers various 1D LUTs, 3D LUTs, matrices, and other
> +operations to support color transformations. These operations are often
> +implemented in fixed-function HW and therefore much more power efficient than
> +performing similar operations via shaders or CPU.
> +
> +We would like to make use of this HW functionality to support complex color
> +transformations with no, or minimal CPU or shader load.
I would also highlight that we need to seamlessly switch between HW
fixed-function blocks and shaders/CPU with no visible difference. Depending on
the content being displayed we might need to fallback to shaders/CPU at any
time. (A classic example would be a new notification popup preventing us from
leveraging KMS planes.)
> +An example of a drm_colorop object might look like one of these::
> +
> +/* 1D enumerated curve */
> +Color operation 42
> +├─ "TYPE": immutable enum {1D enumerated curve, 1D LUT, 3x3 matrix, 3x4
> matrix, 3D LUT, etc.} = 1D enumerated curve
> +├─ "BYPASS": bool {true, false}
> +├─ "CURVE_1D_TYPE": enum {sRGB EOTF, sRGB inverse EOTF, PQ EOTF, PQ
> inverse EOTF, …}
> +└─ "NEXT": immutable color operation ID = 43
> +
> +/* custom 4k entry 1D LUT */
> +Color operation 52
> +├─ "TYPE": immutable enum {1D enumerated curve, 1D LUT, 3x3 matrix, 3x4
> matrix, 3D LUT, etc.} = 1D LUT
> +├─ "BYPASS": bool {true, false}
> +├─ "SIZE": immutable range = 4096
> +├─ "DATA": blob
> +└─ "NEXT": immutable color operation ID = 0
> +
> +/* 17^3 3D LUT */
> +Color operation 72
> +├─ "TYPE": immutable enum {1D enumerated curve, 1D LUT, 3x3 matrix, 3x4
> matrix, 3D LUT, etc.} = 3D LUT
> +├─ "BYPASS": bool {true, false}
> +├─ "3DLUT_MODES": read-only blob of supported 3DLUT modes
> +├─ "3DLUT_MODE_INDEX": index of selected 3DLUT mode
Nit: the 3D LUT modes have been dropped from the initial patch series.
> +├─ "DATA": blob
> +└─ "NEXT": immutable color operation ID = 73
[...]
> +Color Pipeline Discovery
> +
> +
> +A DRM client wanting color management on a drm_plane will:
> +
> +1. Get the COLOR_PIPELINE property of the plane
> +2. iterate all COLOR_PIPELINE enum values
> +3. 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
> +
> +If userspace encounters an unknown or unsuitable color operation during
> +discovery it does not need to reject the entire color pipeline outright,
> +as long as the unknown or unsuitable colorop has a "BYPASS" property.
> +Drivers will ensure that a bypassed block does not have any effect.
> +
> +An example of chained properties to define an AMD pre-blending color
> +pipeline might look like this::
> +
> +Plane 10
> +├─ "TYPE" (immutable) = Primary
> +└─ "COLOR_PIPELINE": enum {0, 44} = 0
> +
> +Color operation 44
> +├─ "TYPE" (immutable) = 1D enumerated curve
> +├─ "BYPASS": bool
> +├─ "CURVE_1D_TYPE": enum {sRGB EOTF, PQ EOTF} = sRGB EOTF
> +└─ "NEXT" (immutable) = 45
> +
> +Color operation 45
> +├─ "TYPE" (immutable) = 3x4 Matrix
> +├─ "BYPASS": bool
> +├─ "DATA": blob
> +└─ "NEXT" (immutable) = 46
> +
> +Color operation 46
> +├─ "TYPE" (immutable) = 1D enumerated curve
> +├─ "BYPASS": bool
> +├─ "CURVE_1D_TYPE": enum {sRGB Inverse EOTF, PQ Inverse EOTF} = sRGB EOTF
> +└─ "NEXT" (immutable) = 47
> +
> +Color operation 47
> +├─ "TYPE" (immutable) = 1D LUT
> +├─ "SIZE": immutable range = 4096
> +├─ "DATA": blob
> +└─ "NEXT" (immutable) = 48
> +
> +Color operation 48
> +├─ "TYPE" (immutable) = 3D LUT
> +├─ "3DLUT_MODE_INDEX": 0
Ditto
> +├─ "DATA": blob
> +└─ "NEXT" (immutable) = 49
> +
> +Color operation 49
> +├─ "TYPE" (immutable) = 1D enumerated curve
> +├─ "BYPASS": bool
> +├─ "CURVE_1D_TYPE": enum {sRGB EOTF, PQ EOTF} = sRGB EOTF
> +└─ "NEXT" (immutable) = 0
> +
> +
> +Color Pipeline Programming
> +==
> +
> +Once a DRM client has found a suitable pipeline it will:
> +
> +1. Set the COL
Re: [PATCH V8 43/43] drm/colorop: Add destroy functions for color pipeline
On 3/29/25 09:48, Simon Ser wrote:
I would prefer these functions to be introduced together with the
patches adding functions to create objects and adding the new fields.
That way it's easier to check the symmetry and at no point in the
series there are memory leaks.
The object creation and new fields are introduced in different patches.
I divided this patch by introducing these functions in a patch, and 2.
adding callers when needed to avoid memory leaks.
Additionally, I would avoid using the name "cleanup", which seems to
have different semantics: for instance drm_plane_cleanup() doesn't kfree
the pointer. "destroy" seems more appropriate here.
How about the following changes, i.e., freeing pointer is moved out of
the cleanup function, and keeping the names.
@@ -173,7 +173,6 @@ static void drm_colorop_cleanup(struct drm_colorop
*colorop)
}
kfree(colorop->state);
- kfree(colorop);
}
/**
@@ -191,6 +190,7 @@ void drm_colorop_pipeline_destroy(struct drm_plane
*plane)
list_for_each_entry_safe(colorop, next, &config->colorop_list,
head) {
drm_colorop_cleanup(colorop);
+ kfree(colorop);
}
}
[PATCH V8 00/43] Color Pipeline API w/ VKMS
This is an RFC set for a color pipeline API, along with implementations
in VKMS and amdgpu. It is tested with a set of IGT tests that can be
found at [1]. The IGT tests run a pixel-by-pixel comparison with an
allowable delta variation as the goal for these transformations is
perceptual correctness, not complete pixel accuracy.
v5 of this patchset fleshed out documentation for colorops and the
various defines that are being introduced.
v6 addresses a few comments from various reviewers.
v7 simplifies 3D LUT and addresses more comments from various reviewers.
v8 fixes typo and errors and address comments from reviewers.
VKMS supports two named transfer function colorops and two matrix
colorops.
Amdgpu advertises the following pipeline for GPUs with DCN 3 or newer:
1. 1D Curve EOTF
2. 3x4 CTM
3. Multiplier
4. 1D Curve Inverse EOTF
5. 1D LUT
6. 3D LUT
7. 1D Curve EOTF
8. 1D LUT
The supported curves for the 1D Curve type are:
- sRGB EOTF and its inverse
- PQ EOTF, scaled to [0.0, 125.0] and its inverse
- BT.2020/BT.709 OETF and its inverse
Note that the 1st and 5th colorops take the EOTF or Inverse
OETF while the 3rd colorop takes the Inverse EOTF or OETF.
The 3D LUT is a 17^3 tetrahedrally interpolated LUT but the mechanism
exists for other drivers to describe their own 3D LUT capability.
This mirrors the color pipeline used by gamescope and presented by
Melissa Wen, with the exception of the DEGAM LUT, which is not currently
used. See [1]
https://indico.freedesktop.org/event/4/contributions/186/attachments/138/218/xdc2023-TheRainbowTreasureMap-MelissaWen.pdf
At this point we're hoping to see gamescope and kwin implementations
take shape. The existing pipeline should be enough to satisfy the
gamescope use-cases on the drm_plane.
In order to support YUV we'll need to add COLOR_ENCODING and COLOR_RANGE
support to the color pipeline. I have sketched these out already but
don't have it all hooked up yet. This should not hinder adoption of this
API for gaming use-cases.
We'll also want to advertise IN_FORMATS on a color pipeline as some
color pipelines won't be able to work for all IN_FORMATS on a plane.
Again, I have a sketch but no full implementation yet. This is not
currently required by the AMD color pipeline and could be added after
the merge of this set.
VKMS patches could still be improved in a few ways, though the
payoff might be limited and I would rather focus on other work
at the moment. The most obvious thing to improve would be to
eliminate the hard-coded LUTs for identity, and sRGB, and replace
them with fixed-point math instead.
There are plenty of things that I would like to see, but they could
be added after the merge of this patchset:
- COLOR_ENCODING and COLOR_RANGE
- IN_FORMATS for a color pipeline
- Is it possible to support HW which can't bypass entire pipeline?
- Can we do a LOAD / COMMIT model for LUTs (and other properties)?
- read-only scaling colorop which defines scaling taps and position
- named matrices, for things like converting YUV to RGB
- Add custom LUT colorops to VKMS
IGT tests can be found at [1] or on the igt-dev mailing list. There
have been no updates since v5 and rebase on latest main is straight-
forward.
A kernel branch can be found at [2].
[1]
https://gitlab.freedesktop.org/alex.hung/igt-gpu-tools/-/tree/amd-color-pipeline-v7
[2] https://gitlab.freedesktop.org/alex.hung/linux/-/tree/amd-color-pipeline-v8
v8:
- Change VKMS config names (Louis Chauvet)
- Remove deprecated function "drm_atomic_get_existing_colorop_state" (Louis
Chauvet)
- Remove null check in drm_colorop_set_next_property (Simon Ser)
- Remove MAX_COLOR_PIPELINES in drm (Simon Ser)
- Update kernel docs and documents for DRM_COLOROP_3D_LUT (Simon Ser)
- Add comments for dmr_color_lut (Louis Chauvet)
- Fix typos and replace DRM_ERROR and DRM_WARN_ONCE by drm_err drm_WARN_ONCE
(Louis Chauvet)
- Fix incorrect conditions in __set_colorop_in_tf_1d_curve (Leo Li)
- Add DRM_MODE_PROP_ATOMIC to drm_property_create_range (Simon Ser)
- Change "1D Curve Custom LUT" to "1D LUT" (Simon Ser)
- Return error when __set_output_tf fails (Leo Li)
- Return -EINVAL when drm_color_ctm_3x4's size mismatches (Leo Li)
- Set initialized to 0 and return when drm_lut3d_size is 0 (Harry Wentland)
- Rework tf->type = TF_TYPE_BYPASS for shaper (Harry Wentland & Leo Li)
v7:
- Simplify 3D LUT by removing lut_3d_modes and update doc accordingly (Simon
Ser)
- Add destroy / cleanup functions when color pipeline initialization
fails (Louis Chauvet)
- Fix checkpatch errors and warning (spaces, long lines, {}, and etc)
- Change lut's size from drm_colorop_state->size to drm_colorop->lut_size
and from size_property to lut_size_property
- Update some kernel updates and commit messagesa (Louis Chauvet)
- Squash "drm/colorop: Add atomic state print for drm_colorop" and
"drm/colorop: Add NEXT to colorop state print" (Simon Ser)
- Add missing MODULE_DESCRIPTION (Jeff Johnson)
v6:
- Elimina
Re: [PATCH V8 28/43] drm/colorop: Add PQ 125 EOTF and its inverse
Reviewed-by: Simon Ser
Re: [PATCH V8 03/43] drm/doc/rfc: Describe why prescriptive color pipeline is needed
On 3/29/25 09:26, Simon Ser wrote: I would also highlight that we need to seamlessly switch between HW fixed-function blocks and shaders/CPU with no visible difference. Depending on the content being displayed we might need to fallback to shaders/CPU at any time. (A classic example would be a new notification popup preventing us from leveraging KMS planes.) Yes that would be the goal. Is there any part of the docs that still need revising to clarify this?
Re: [PATCH V8 24/43] drm/amd/display: Skip color pipeline initialization for cursor plane
On 3/31/25 19:04, Shengyu Qu wrote: My understanding is that currently the driver would just report no colorop support on cursor plane and actually implement the background plane's colorop on cursor? No. Again it is up to compositors or apps to determine how color pipeline and colorops are used (or not). For example, both primary plane and overlay plane have the same color pipeline, HDR can be enabled only on overlay but not on primary. Still this is the cleanest way to let compositors know and deal with this special cursor plane behavior. Or if compositors want to use all planes with hw colorop + MPO(for power saving or sth.), they have to detect the gpu they are running on and apply a quirk for this. That's a "dirty" implementation. Unrelated to color pipeline.
