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. > 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 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? Does anyone know of any doc about that? 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; > > > >
