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

[Harry Wentland]

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