On 2023-08-29 04:51, Pekka Paalanen wrote: > On Mon, 28 Aug 2023 12:56:04 -0100 > Melissa Wen <mwen@xxxxxxxxxx> wrote: > >> On 08/28, Pekka Paalanen wrote: >>> On Mon, 28 Aug 2023 09:45:44 +0100 >>> Joshua Ashton <joshua@xxxxxxxxx> 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@xxxxxxxxxxxxx> >>>> 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. >>>>> >>>>> >>>>> 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; >>>>> >>>>> >>> >
