[PATCH v2 19/34] drm/amd/display: decouple steps for mapping CRTC degamma to DC plane
Harry Wentland
harry.wentland at amd.com
Wed Sep 6 14:52:32 UTC 2023
On 2023-08-29 04:51, Pekka Paalanen wrote:
> On Mon, 28 Aug 2023 12:56:04 -0100
> Melissa Wen <mwen at igalia.com> wrote:
>
>> On 08/28, Pekka Paalanen wrote:
>>> On Mon, 28 Aug 2023 09:45:44 +0100
>>> Joshua Ashton <joshua at froggi.es> wrote:
>>>
>>>> Degamma has always been on the plane on AMD. CRTC DEGAMMA_LUT has actually
>>>> just been applying it to every plane pre-blend.
>>>
>>> I've never seen that documented anywhere.
>>>
>>> It has seemed obvious, that since we have KMS objects for planes and
>>> CRTCs, stuff on the CRTC does not do plane stuff before blending. That
>>> also has not been documented in the past, but it seemed the most
>>> logical choice.
>>>
>>> Even today
>>> https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#color-management-properties
>>> make no mention of whether they apply before or after blending.
>>
>> It's mentioned in the next section:
>> https://dri.freedesktop.org/docs/drm/gpu/amdgpu/display/display-manager.html#dc-color-capabilities-between-dcn-generations
>> In hindsight, maybe it isn't the best place...
>
> That is driver-specific documentation. As a userspace dev, I'd never
> look at driver-specific documentation, because I'm interested in the
> KMS UAPI which is supposed to be generic, and therefore documented with
> the DRM "core".
>
> Maybe kernel reviewers also never look at driver-specific docs to find
> attempts at redefining common KMS properties?
>
> (I still don't know which definition is prevalent.)
>
>>>
>>>> Degamma makes no sense after blending anyway.
>>>
>>> If the goal is to allow blending in optical or other space, you are
>>> correct. However, APIs do not need to make sense to exist, like most of
>>> the options of "Colorspace" connector property.
>>>
>>> I have always thought the CRTC DEGAMMA only exists to allow the CRTC
>>> CTM to work in linear or other space.
>>>
>>> I have at times been puzzled by what the DEGAMMA and CTM are actually
>>> good for.
>>>
>>>> The entire point is for it to happen before blending to blend in linear
>>>> space. Otherwise DEGAMMA_LUT and REGAMMA_LUT are the exact same thing...
>>>
>>> The CRTC CTM is between CRTC DEGAMMA and CRTC GAMMA, meaning they are
>>> not interchangeable.
>>>
>>> I have literally believed that DRM KMS UAPI simply does not support
>>> blending in optical space, unless your framebuffers are in optical
>>> which no-one does, until the color management properties are added to
I think Mario Kleiner had a use-case that made use of that and introduced
FP16 format support in amdgpu.
>>> KMS planes. This never even seemed weird, because non-linear blending
>>> is so common.
>>>
>>> So I have been misunderstanding the CRTC DEGAMMA property forever. Am I
>>> the only one? Do all drivers agree today at what point does CRTC
>>> DEGAMMA apply, before blending on all planes or after blending?
>>>
>>
>> I'd like to know current userspace cases on Linux of this CRTC DEGAMMA
>> LUT.
>
> I don't know of any, but that doesn't mean anything.
>
>>> Does anyone know of any doc about that?
>>
>> From what I retrieved about the introduction of CRTC color props[1], it
>> seems the main concern at that point was getting a linear space for
>> CTM[2] and CRTC degamma property seems to have followed intel
>> requirements, but didn't find anything about the blending space.
>
> Right. I've always thought CRTC props apply after blending.
>
>> AFAIU, we have just interpreted that all CRTC color properties for DRM
>> interface are after blending[3]. Can this be seen in another way?
>
> Joshua did, and he has a logical point.
>
> I guess if we really want to know, someone would need review all
> drivers exposing these props, and even check if they changed in the
> past.
>
> FWIW, the usefulness of (RE)GAMMA (not DEGAMMA) LUT is limited by the
> fact that attempting to represent 1/2.2 power function as a uniformly
> distributed LUT is infeasible due to the approximation errors near zero.
>
IMO, CRTC should be post-blending. Blending is at the plane/crtc boundary
by design, therefore CRTC properties apply post-blending.
Though I can understand why DEGAMMA can be interpreted to be applied
pre-blending. Though, I think that's wrong for the DRM/KMS model and
should be fixed in amdgpu.
Harry
>
> Thanks,
> pq
>
>> [1] https://patchwork.freedesktop.org/series/2720/
>> [2] https://codereview.chromium.org/1182063002
>> [3] https://dri.freedesktop.org/docs/drm/_images/dcn3_cm_drm_current.svg
>>
>>>
>>> If drivers do not agree on the behaviour of a KMS property, then that
>>> property is useless for generic userspace.
>>>
>>>
>>> Thanks,
>>> pq
>>>
>>>
>>>> On Tuesday, 22 August 2023, Pekka Paalanen <pekka.paalanen at collabora.com>
>>>> wrote:
>>>>> On Thu, 10 Aug 2023 15:02:59 -0100
>>>>> Melissa Wen <mwen at 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 at amd.com>
>>>>>> Signed-off-by: Melissa Wen <mwen at 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,
>>>>>> °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.
>>>>>
>>>>>
>>>>> 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;
>>>>>
>>>>>
>>>
>
More information about the amd-gfx
mailing list