On 2023-08-28 04:17, Pekka Paalanen wrote:
> On Fri, 25 Aug 2023 13:29:44 -0100
> Melissa Wen <mwen@xxxxxxxxxx> wrote:
>
>> On 08/22, Pekka Paalanen wrote:
>>> On Thu, 10 Aug 2023 15:02:59 -0100
>>> Melissa Wen <mwen@xxxxxxxxxx> 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@xxxxxxx>
>>>> Signed-off-by: Melissa Wen <mwen@xxxxxxxxxx>
>>>> ---
>>>> .../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,
>>>> °amma_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@xxxxxxxxxx/
>> [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;
>>>
>
