Re: [PATCH v2 19/34] drm/amd/display: decouple steps for mapping CRTC degamma to DC plane

[Harry Wentland <harry.wentland@amd.com>]

On 2023-08-28 04:17, Pekka Paalanen wrote:
> On Fri, 25 Aug 2023 13:29:44 -0100
> Melissa Wen <mwen@igalia.com> wrote:
> 
>> On 08/22, Pekka Paalanen wrote:
>>> On Thu, 10 Aug 2023 15:02:59 -0100
>>> Melissa Wen <mwen@igalia.com> wrote:
>>>   
>>>> The next patch adds pre-blending degamma to AMD color mgmt pipeline, but
>>>> pre-blending degamma caps (DPP) is currently in use to provide DRM CRTC
>>>> atomic degamma or implict degamma on legacy gamma. Detach degamma usage
>>>> regarging CRTC color properties to manage plane and CRTC color
>>>> correction combinations.
>>>>
>>>> Reviewed-by: Harry Wentland <harry.wentland@amd.com>
>>>> Signed-off-by: Melissa Wen <mwen@igalia.com>
>>>> ---
>>>>  .../amd/display/amdgpu_dm/amdgpu_dm_color.c   | 59 +++++++++++++------
>>>>  1 file changed, 41 insertions(+), 18 deletions(-)
>>>>
>>>> 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 68e9f2c62f2e..74eb02655d96 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
>>>> @@ -764,20 +764,9 @@ int amdgpu_dm_update_crtc_color_mgmt(struct dm_crtc_state *crtc)
>>>>  	return 0;
>>>>  }
>>>>  
>>>> -/**
>>>> - * amdgpu_dm_update_plane_color_mgmt: Maps DRM color management to DC plane.
>>>> - * @crtc: amdgpu_dm crtc state
>>>> - * @dc_plane_state: target DC surface
>>>> - *
>>>> - * Update the underlying dc_stream_state's input transfer function (ITF) in
>>>> - * preparation for hardware commit. The transfer function used depends on
>>>> - * the preparation done on the stream for color management.
>>>> - *
>>>> - * Returns:
>>>> - * 0 on success. -ENOMEM if mem allocation fails.
>>>> - */
>>>> -int amdgpu_dm_update_plane_color_mgmt(struct dm_crtc_state *crtc,
>>>> -				      struct dc_plane_state *dc_plane_state)
>>>> +static int
>>>> +map_crtc_degamma_to_dc_plane(struct dm_crtc_state *crtc,
>>>> +			     struct dc_plane_state *dc_plane_state)
>>>>  {
>>>>  	const struct drm_color_lut *degamma_lut;
>>>>  	enum dc_transfer_func_predefined tf = TRANSFER_FUNCTION_SRGB;
>>>> @@ -800,8 +789,7 @@ int amdgpu_dm_update_plane_color_mgmt(struct dm_crtc_state *crtc,
>>>>  						 &degamma_size);
>>>>  		ASSERT(degamma_size == MAX_COLOR_LUT_ENTRIES);
>>>>  
>>>> -		dc_plane_state->in_transfer_func->type =
>>>> -			TF_TYPE_DISTRIBUTED_POINTS;
>>>> +		dc_plane_state->in_transfer_func->type = TF_TYPE_DISTRIBUTED_POINTS;
>>>>  
>>>>  		/*
>>>>  		 * This case isn't fully correct, but also fairly
>>>> @@ -837,7 +825,7 @@ int amdgpu_dm_update_plane_color_mgmt(struct dm_crtc_state *crtc,
>>>>  				   degamma_lut, degamma_size);
>>>>  		if (r)
>>>>  			return r;
>>>> -	} else if (crtc->cm_is_degamma_srgb) {
>>>> +	} else {
>>>>  		/*
>>>>  		 * For legacy gamma support we need the regamma input
>>>>  		 * in linear space. Assume that the input is sRGB.
>>>> @@ -847,8 +835,43 @@ int amdgpu_dm_update_plane_color_mgmt(struct dm_crtc_state *crtc,
>>>>  
>>>>  		if (tf != TRANSFER_FUNCTION_SRGB &&
>>>>  		    !mod_color_calculate_degamma_params(NULL,
>>>> -			    dc_plane_state->in_transfer_func, NULL, false))
>>>> +							dc_plane_state->in_transfer_func,
>>>> +							NULL, false))
>>>>  			return -ENOMEM;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +/**
>>>> + * amdgpu_dm_update_plane_color_mgmt: Maps DRM color management to DC plane.
>>>> + * @crtc: amdgpu_dm crtc state
>>>> + * @dc_plane_state: target DC surface
>>>> + *
>>>> + * Update the underlying dc_stream_state's input transfer function (ITF) in
>>>> + * preparation for hardware commit. The transfer function used depends on
>>>> + * the preparation done on the stream for color management.
>>>> + *
>>>> + * Returns:
>>>> + * 0 on success. -ENOMEM if mem allocation fails.
>>>> + */
>>>> +int amdgpu_dm_update_plane_color_mgmt(struct dm_crtc_state *crtc,
>>>> +				      struct dc_plane_state *dc_plane_state)
>>>> +{
>>>> +	bool has_crtc_cm_degamma;
>>>> +	int ret;
>>>> +
>>>> +	has_crtc_cm_degamma = (crtc->cm_has_degamma || crtc->cm_is_degamma_srgb);
>>>> +	if (has_crtc_cm_degamma){
>>>> +		/* AMD HW doesn't have post-blending degamma caps. When DRM
>>>> +		 * CRTC atomic degamma is set, we maps it to DPP degamma block
>>>> +		 * (pre-blending) or, on legacy gamma, we use DPP degamma to
>>>> +		 * linearize (implicit degamma) from sRGB/BT709 according to
>>>> +		 * the input space.  
>>>
>>> Uhh, you can't just move degamma before blending if KMS userspace
>>> wants it after blending. That would be incorrect behaviour. If you
>>> can't implement it correctly, reject it.
>>>
>>> I hope that magical unexpected linearization is not done with atomic,
>>> either.
>>>
>>> Or maybe this is all a lost cause, and only the new color-op pipeline
>>> UAPI will actually work across drivers.  
>>
>> I agree that crtc degamma is an optional property and should be not
>> exposed if not available.  I did something in this line for DCE that has
>> no degamma block[1].  Then, AMD DDX driver stopped to advertise atomic
>> API for DCE, that was not correct too[2].
> 
> Did AMD go through all the trouble of making their Xorg DDX use KMS
> atomic, even after the kernel took it away from X due to modesetting
> DDX screwing it up? I'm surprised, what did that achieve?
> 
> I saw that between 5.15 and 6.1 amdgpu stopped advertising CRTC DEGAMMA
> on my card, which seems like the right thing to do if there is no
> hardware for it.
> 
>> But I see it as a lost cause that will only be fixed in a new generic
>> color API. I don't think we should change it using the current DRM CRTC
>> API with driver-specific props.
>>
>> [1] https://lore.kernel.org/amd-gfx/20221103184500.14450-1-mwen@igalia.com/
>> [2] https://gitlab.freedesktop.org/xorg/driver/xf86-video-amdgpu/-/issues/67
> 
> Oh well.
> 
> Is the old CRTC GAMMA property still "safe" to use in that it is
> definitely always after blending?
> 

CRTC GAMMA is always post-blending. DEGAMMA and CTM are not always
post-blending and need to be fixed (or removed).

Harry

> 
> Thanks,
> pq
> 
>>>> +		 */
>>>> +		ret = map_crtc_degamma_to_dc_plane(crtc, dc_plane_state);
>>>> +		if (ret)
>>>> +			return ret;
>>>>  	} else {
>>>>  		/* ...Otherwise we can just bypass the DGM block. */
>>>>  		dc_plane_state->in_transfer_func->type = TF_TYPE_BYPASS;  
>>>   
>