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

Mon Aug 28 10:23:55 UTC 2023

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.

> 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 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,
> >>                                                &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.
> >
> >
> > 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;  
> >
> >  