Re: [PATCH V9 26/43] drm/amd/display: Add support for sRGB EOTF in DEGAM block

[Pekka Paalanen <pekka.paalanen@collabora.com>]

[-- Attachment #1: Type: text/plain, Size: 10470 bytes --]

On Tue, 13 May 2025 16:39:51 -0400
Harry Wentland <harry.wentland@amd.com> wrote:

> 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@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@amd.com>
> >>>> Co-developed-by: Harry Wentland <harry.wentland@amd.com>
> >>>> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
> >>>> Reviewed-by: Daniel Stone <daniels@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
> > 

Hi Melissa,

sorry, that is fine. I misunderstood disabling both degamma LUT and
regamma LUT as no LUT stage being left available on a CRTC. After all,
only GAMMA_LUT and DEGAMMA_LUT exist upstream.

> > 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.

Huh. I always assumed that CRTC DEGAMMA_LUT was something in the
hardware after blending and that someone just wanted to expose that
hardware feature. I'd never have guessed that it might have applied
before blending on purpose.

The position of the CRTC CTM before or after blending does not matter.
The positions are mathematically equivalent I think.

> > 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.
> 

FWIW, people who rely on ICC profile VCGT might be quite precise about
how the VCGT is applied. I've been essentially called ignorant for
applying VCGT anywhere else than in "the" hardware LUT.

> > 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.

Sounds good to me.


Thanks,
pq

> >> 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  
> > 
> >   
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]