[PATCH V9 26/43] drm/amd/display: Add support for sRGB EOTF in DEGAM block
Harry Wentland
harry.wentland at amd.com
Tue May 13 20:39:51 UTC 2025
On 2025-05-13 11:36, Melissa Wen wrote:
> On 05/13, Pekka Paalanen wrote:
>> On Mon, 12 May 2025 15:50:17 -0300
>> Melissa Wen <mwen at igalia.com> wrote:
>>
>>> On 04/29, Alex Hung wrote:
>>>> Expose one 1D curve colorop with support for
>>>> DRM_COLOROP_1D_CURVE_SRGB_EOTF and program HW to perform
>>>> the sRGB transform when the colorop is not in bypass.
>>>>
>>>> With this change the following IGT test passes:
>>>> kms_colorop --run plane-XR30-XR30-srgb_eotf
>>>>
>>>> The color pipeline now consists of a single colorop:
>>>> 1. 1D curve colorop w/ sRGB EOTF
>>>>
>>>> Signed-off-by: Alex Hung <alex.hung at amd.com>
>>>> Co-developed-by: Harry Wentland <harry.wentland at amd.com>
>>>> Signed-off-by: Harry Wentland <harry.wentland at amd.com>
>>>> Reviewed-by: Daniel Stone <daniels at collabora.com>
>>>> ---
>>>> V9:
>>>> - Update function names by _plane_ (Chaitanya Kumar Borah)
>>>> - Update replace cleanup code by drm_colorop_pipeline_destroy (Simon Ser)
>>>>
>>>> v8:
>>>> - Fix incorrect && by || in __set_colorop_in_tf_1d_curve (Leo Li)
>>>>
>>>> v7:
>>>> - Fix checkpatch warnings
>>>> - Change switch "{ }" position
>>>> - Delete double ";"
>>>> - Delete "{ }" for single-line if-statement
>>>> - Add a new line at EOF
>>>> - Change SPDX-License-Identifier: GPL-2.0+ from // to /* */
>>>>
>>>> v6:
>>>> - cleanup if colorop alloc or init fails
>>>>
>>>> .../gpu/drm/amd/display/amdgpu_dm/Makefile | 3 +-
>>>> .../amd/display/amdgpu_dm/amdgpu_dm_color.c | 86 +++++++++++++++++++
>>>> .../amd/display/amdgpu_dm/amdgpu_dm_colorop.c | 69 +++++++++++++++
>>>> .../amd/display/amdgpu_dm/amdgpu_dm_colorop.h | 34 ++++++++
>>>> .../amd/display/amdgpu_dm/amdgpu_dm_plane.c | 10 +++
>>>> 5 files changed, 201 insertions(+), 1 deletion(-)
>>>> create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.c
>>>> create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.h
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile b/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
>>>> index ab2a97e354da..46158d67ab12 100644
>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
>>>> @@ -38,7 +38,8 @@ AMDGPUDM = \
>>>> amdgpu_dm_pp_smu.o \
>>>> amdgpu_dm_psr.o \
>>>> amdgpu_dm_replay.o \
>>>> - amdgpu_dm_wb.o
>>>> + amdgpu_dm_wb.o \
>>>> + amdgpu_dm_colorop.o
>>>>
>>>> ifdef CONFIG_DRM_AMD_DC_FP
>>>> AMDGPUDM += dc_fpu.o
>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
>>>> index ebabfe3a512f..0b513ab5050f 100644
>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
>>>> @@ -668,6 +668,18 @@ amdgpu_tf_to_dc_tf(enum amdgpu_transfer_function tf)
>>>> }
>>>> }
>>>>
>>>> +static enum dc_transfer_func_predefined
>>>> +amdgpu_colorop_tf_to_dc_tf(enum drm_colorop_curve_1d_type tf)
>>>> +{
>>>> + switch (tf) {
>>>> + case DRM_COLOROP_1D_CURVE_SRGB_EOTF:
>>>> + case DRM_COLOROP_1D_CURVE_SRGB_INV_EOTF:
>>>> + return TRANSFER_FUNCTION_SRGB;
>>>> + default:
>>>> + return TRANSFER_FUNCTION_LINEAR;
>>>> + }
>>>> +}
>>>> +
>>>> static void __to_dc_lut3d_color(struct dc_rgb *rgb,
>>>> const struct drm_color_lut lut,
>>>> int bit_precision)
>>>> @@ -1137,6 +1149,59 @@ __set_dm_plane_degamma(struct drm_plane_state *plane_state,
>>>> return 0;
>>>> }
>>>>
>>>> +static int
>>>> +__set_colorop_in_tf_1d_curve(struct dc_plane_state *dc_plane_state,
>>>> + struct drm_colorop_state *colorop_state)
>>>> +{
>>>> + struct dc_transfer_func *tf = &dc_plane_state->in_transfer_func;
>>>> + struct drm_colorop *colorop = colorop_state->colorop;
>>>> + struct drm_device *drm = colorop->dev;
>>>> +
>>>> + if (colorop->type != DRM_COLOROP_1D_CURVE ||
>>>> + colorop_state->curve_1d_type != DRM_COLOROP_1D_CURVE_SRGB_EOTF)
>>>> + return -EINVAL;
>>>> +
>>>> + if (colorop_state->bypass) {
>>>> + tf->type = TF_TYPE_BYPASS;
>>>> + tf->tf = TRANSFER_FUNCTION_LINEAR;
>>>> + return 0;
>>>> + }
>>>> +
>>>> + drm_dbg(drm, "Degamma colorop with ID: %d\n", colorop->base.id);
>>>> +
>>>> + tf->type = TF_TYPE_PREDEFINED;
>>>> + tf->tf = amdgpu_colorop_tf_to_dc_tf(colorop_state->curve_1d_type);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int
>>>> +__set_dm_plane_colorop_degamma(struct drm_plane_state *plane_state,
>>>> + struct dc_plane_state *dc_plane_state,
>>>> + struct drm_colorop *colorop)
>>>> +{
>>>> + struct drm_colorop *old_colorop;
>>>> + struct drm_colorop_state *colorop_state = NULL, *new_colorop_state;
>>>> + struct drm_atomic_state *state = plane_state->state;
>>>> + int i = 0;
>>>> +
>>>> + old_colorop = colorop;
>>>> +
>>>> + /* 1st op: 1d curve - degamma */
>>>> + for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) {
>>>> + if (new_colorop_state->colorop == old_colorop &&
>>>> + new_colorop_state->curve_1d_type == DRM_COLOROP_1D_CURVE_SRGB_EOTF) {
>>>> + colorop_state = new_colorop_state;
>>>> + break;
>>>> + }
>>>> + }
>>>> +
>>>> + if (!colorop_state)
>>>> + return -EINVAL;
>>>> +
>>>> + return __set_colorop_in_tf_1d_curve(dc_plane_state, colorop_state);
>>>
>>> I wonder what will happen if plane degamma isn't set, but CRTC degamma
>>> LUT or legacy CRTC regamma LUT (with its implicity sRGB degamma) is used
>>> together with other plane color ops.
>>>
>>> I can imagine the mess, so I think CRTC degamma LUT and legacy CRTC
>>> regamma LUT should be somehow entirely disabled (or rejected) if plane
>>> color pipeline is in use.
>>
>> Hi Melissa,
>>
>> if using a plane color pipeline means that a CRTC LUT cannot be used, it
>> will severely limit the usefulness of the whole KMS color processing. In
>> Weston's case it would prohibit *all* KMS off-loading when color
>> management is in use.
>>
>> Weston chooses to do composition and blending in an optical space. This
>> means that plane color pipelines are required to convert incoming
>> pixels into the optical space, and a CRTC LUT (a CRTC color pipeline in
>> the future) is required to convert from the optical space to the
>> monitor signalling (electrical space).
>
> Hi Pekka,
>
> IIRC, Weston needs one post-blending 1D LUT and with my suggestion the
> atomic CRTC regamma LUT works fine and can be this 1D LUT.
> So, instead of an atomic post-blending/CRTC color pipeline with:
>
> [blending] -> CRTC 1D LUT -> CRTC CTM -> CRTC 1D LUT
>
> when plane color pipeline is in use, the driver accepts only:
>
> [blending] -> CRTC CTM -> CRTC 1D LUT
>
> If AMD driver continues accepting/exposing CRTC degamma LUT plus plane
> color pipeline, and userpace wants something like:
>
> Plane shaper LUT -> Plane 3D LUT -> Plane Blnd LUT -> [blending] -> **CRTC** degamma LUT
>
> I understand that this weird sequence is what will actually happen:
>
> **CRTC** degamma LUT -> Plane shaper LUT -> Plane 3D LUT -> Plane Blnd LUT -> [blending]
>
Right, and this is quirky behavior by the AMD driver because
historically we only had the CRTC LUTs and had to use the CRTC
degamma LUT to program degamma, which technically needs to happen
pre-blending, i.e., on the plane.
The degamma LUT never really made sense on a crtc, but here
we are.
> Because userspace doesn't care if this is a "degamma" or "regamma" LUT
> and they will probably pick the first 1D LUT in the post-blending color
> pipeline, which currently means CRTC degamma LUT. So, better if it takes
Ideally a userspace implementation would be the correct LUT
but not sure that would always happen.
> the CRTC regamma LUT that is actually a post-blending 1D LUT. Where I
> expected this result:
>
> Plane shaper LUT -> PLane 3D LUT -> PLane Blnd LUT -> [blending] -> CRTC regamma LUT
>
> You can vizualize better this degamma issue in this diagram:
> https://raw.githubusercontent.com/melissawen/melissawen.github.io/master/img/xdc-2023-colors-talk/rainbow-treasure-xdc-2023-17.png
>
> In the current driver-specific implementation, driver rejects atomic commits
> in case of collision, i.e., Plane degamma LUT + CRTC degamma LUT, but
> IIRC, the driver doesn't handle it if it doesn't clash.
>
Yeah, I agree the most straight-forward way to avoid problems
going forward is to not advertise the CRTC degamma LUT for HW
generations where we'll advertise the plane pipeline.
I talked to Alex to make that change.
Harry
>>
>> I don't know what "with its implicity sRGB degamma" means, but there
>> cannot be any implicit curves at all. The driver has no knowledge of
>> how the framebuffer pixels are encoded, nor about what the blending
>> space should be.
>
> It targets the legacy CRTC regamma LUT (256 size), not the atomic CRTC
> color pipeline.
>
> Melissa
>
>>
>>
>> Thanks,
>> pq
>
>
More information about the wayland-devel
mailing list