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: 6397 bytes --]

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

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.


Thanks,
pq

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