On 05/13, Pekka Paalanen wrote:
> On Mon, 12 May 2025 15:50:17 -0300
> Melissa Wen <m...@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.h...@amd.com>
> > > Co-developed-by: Harry Wentland <harry.wentl...@amd.com>
> > > Signed-off-by: Harry Wentland <harry.wentl...@amd.com>
> > > Reviewed-by: Daniel Stone <dani...@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]
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
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.
>
> 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
