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

[Pekka Paalanen <pekka.paalanen@collabora.com>]

[-- Attachment #1: Type: text/plain, Size: 10759 bytes --]

On Mon, 28 Aug 2023 12:56:04 -0100
Melissa Wen <mwen@igalia.com> wrote:

> On 08/28, Pekka Paalanen wrote:
> > On Mon, 28 Aug 2023 09:45:44 +0100
> > Joshua Ashton <joshua@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
> > 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.


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@collabora.com>
> > > wrote:  
> > > > On Thu, 10 Aug 2023 15:02:59 -0100
> > > > Melissa Wen <mwen@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@amd.com>
> > > >> Signed-off-by: Melissa Wen <mwen@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;    
> > > >
> > > >    
> >   


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]