[PATCH] drm/amd/display: Enable ogam_ram for dcn32+dcn35+dcn351

Fri Apr 12 21:05:01 UTC 2024

On 2024-04-12 16:22, Harry Wentland wrote:
> 
> 
> On 2024-04-12 12:26, Melissa Wen wrote:
>> On 04/12, Joshua Ashton wrote:
>>>
>>>
>>> On 4/11/24 3:26 PM, Melissa Wen wrote:
>>>> On 04/10, Joshua Ashton wrote:
>>>>> The comment here states "no OGAM in DPP since DCN1", yet that is not
>>>>> true.
>>>>>
>>>>> Testing on an RX 7900XTX (dcn32), it actually does exist in hardware and
>>>>> works fine.
>>>>> My best guess is the comment is confused with OGAM ROM for DPP, rather
>>>>> than OGAM RAM.
>>>>>
>>>>> I did not test dcn35/351 as I do not have that hardware, but I assume
>>>>> the same follows there given the seemingly erroneous comment.
>>>>> Someone at AMD should check that before merging this commit.
>>>>
>>>> hmm... I don't have any of these hw versions, but AFAIU if there is
>>>> ogam/blend lut block in dcn32, the helper implementation for programming
>>>> it properly (i.e. dpp_program_blnd_lut) is also missing here:
>>>> - https://gitlab.freedesktop.org/agd5f/linux/-/blob/amd-staging-drm-next/drivers/gpu/drm/amd/display/dc/dpp/dcn32/dcn32_dpp.c#L125
>>>> right? So, it's good if AMD people can check it too.
>>>>
>>>> Melissa
>>>
>>> Hmm, yes. But, see dcn32_set_mcm_luts, that seems to handle per-plane blend
>>> + shaper + 3D LUT state which is equivalent to what existed before?
>>
>> oh, cool! nice finding. blnd_lut is set on plane state, but programmed
>> on MPC now.
>>
>> But I see the color pipeline changed in many stages from this version:
>> - shaper + 3dlut before **or** after blending, but not before **and** after?
>> - where post-blending gamut_remap_matrix is located now in this
>>   pipeline with mpcc postblend_1dlut and shaper+3dlut with mutable
>>   position?
>>   I guess something like:
>>   - [shaper -> 3dlut -> blnd_lut] -> ctm -> regamma ??
> 
> [1dlut -> 3dlut -> 1dlut] is now a movable block, called Movable CM
> (color management), or MCM for short. It can occur before blending or
> after. Because of that it moved to MPC (multi-plane combiner).
> 
> If it's positioned for pre-blending it looks like this:
> 
> DPP -> MCM -> BLND -> Blending -> Gamut Remap -> Out Gam -> OPP -> ...
> 
> If it's positioned for post-blending it looks like this:
> 
> DPP -> BLND -> Blending -> MCM -> Gamut Remap -> Out Gam -> OPP
> 
> This is the case since DCN 3.2.
> 
> Because of that you don't have an ogam_ram in dpp anymore for DCN 3.2
> and newer.
> 
>>>
>>> Therefore, I think I am actually wrong with enabling the ogam_ram in DPP cap
>>> here, and the right solution is to change the check for exposing the
>>> property to account for these LUTs being available per-plane with mcm.
>>>
>>> (what is mcm btw...? lots of acronyms and stuff moving around in hw hehe)
>>
>> yes, shaper, 3dlut, blend_lut don't seem DPP caps anymore. MCM looks
>> like a component of MPC, so I think we need new mpc.color caps to
>> describe them properly (?)
>>
>> I also didn't find in the Linux/AMD glossary or code comment that
>> describe what MCM is...
>>
>>>
>>> What's a good way for us to check for that? Seems like the caps don't help
>>> much there. We could check for the literal function ptr I guess...?
>>>
>>> What are your thoughts, Harry and Melissa?
>>
> 
> I wonder if it makes sense to add an mcm cap to mpc_color_caps. Will need to
> chat with some people about that.
> 

I dug through the code a bit and talked with our resident expert. It looks
like all the programming for in_shaper_func, lut_3d_func, and blend_tf should
still work and assume a fixed pre-blending mcm, which is what we want.

Caps don't really reflect MCM very well yet. We could either add an mcm
flag to mpc_color_caps or simply check that DCN is 3.2 or newer:
	amdgpu_ip_version(adev, DCE_HWIP, 0) >= IP_VERSION(3, 2, 0)

Harry

> Harry
> 
>> yeah, AFAIK color caps values are manually set and may contain
>> misleading information. I'm unsure that using function ptr would solve
>> the issue of having undocumented caps introduced to the color pipeline,
>> such as MCM, but your suggestion seems more reliable.
>>
>> Anyway, the timing where the color pipeline was merged didn't help, but
>> if new caps and functions are not documented and the DM handles are not
>> updated accordingly, we will have the same issue in the future. 
>> For example, I see two new ptrs were introduced and implemented here:
>> - https://gitlab.freedesktop.org/agd5f/linux/-/commit/a820190204aef
>> - https://gitlab.freedesktop.org/agd5f/linux/-/commit/90f33674a0756
>> and we would need to update the DM color mgmt anyway to check these
>> new/unknown functions.
>>
>> Seems okay if we check program_1dlut instead of ogam_ram caps, but what
>> should we do for dpp/mpc shaper+3d lut in set_mcm_luts? I mean, should
>> we enable plane or CRTC shaper+3dlut on DM? x_X
>>
>> Anyway, thanks for all these findings!
>>
>> I would like to hear more from AMD too.
>>
>> Melissa
>>
>>>
>>> - Joshie 🐸✨
>>>
>>>>
>>>>>
>>>>> Signed-off-by: Joshua Ashton <joshua at froggi.es>
>>>>>
>>>>> Cc: Harry Wentland <harry.wentland at amd.com>
>>>>> Cc: Xaver Hugl <xaver.hugl at gmail.com>
>>>>> Cc: Melissa Wen <mwen at igalia.com>
>>>>> Cc: Ethan Lee <flibitijibibo at gmail.com>
>>>>> ---
>>>>>   drivers/gpu/drm/amd/display/dc/resource/dcn32/dcn32_resource.c  | 2 +-
>>>>>   drivers/gpu/drm/amd/display/dc/resource/dcn35/dcn35_resource.c  | 2 +-
>>>>>   .../gpu/drm/amd/display/dc/resource/dcn351/dcn351_resource.c    | 2 +-
>>>>>   3 files changed, 3 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/display/dc/resource/dcn32/dcn32_resource.c b/drivers/gpu/drm/amd/display/dc/resource/dcn32/dcn32_resource.c
>>>>> index 9aa39bd25be9..94f5d2b5aadf 100644
>>>>> --- a/drivers/gpu/drm/amd/display/dc/resource/dcn32/dcn32_resource.c
>>>>> +++ b/drivers/gpu/drm/amd/display/dc/resource/dcn32/dcn32_resource.c
>>>>> @@ -2182,7 +2182,7 @@ static bool dcn32_resource_construct(
>>>>>   	dc->caps.color.dpp.dgam_rom_for_yuv = 0;
>>>>>   	dc->caps.color.dpp.hw_3d_lut = 1;
>>>>> -	dc->caps.color.dpp.ogam_ram = 0;  // no OGAM in DPP since DCN1
>>>>> +	dc->caps.color.dpp.ogam_ram = 1;
>>>>>   	// no OGAM ROM on DCN2 and later ASICs
>>>>>   	dc->caps.color.dpp.ogam_rom_caps.srgb = 0;
>>>>>   	dc->caps.color.dpp.ogam_rom_caps.bt2020 = 0;
>>>>> diff --git a/drivers/gpu/drm/amd/display/dc/resource/dcn35/dcn35_resource.c b/drivers/gpu/drm/amd/display/dc/resource/dcn35/dcn35_resource.c
>>>>> index 25ac450944e7..708d63cc3f7f 100644
>>>>> --- a/drivers/gpu/drm/amd/display/dc/resource/dcn35/dcn35_resource.c
>>>>> +++ b/drivers/gpu/drm/amd/display/dc/resource/dcn35/dcn35_resource.c
>>>>> @@ -1861,7 +1861,7 @@ static bool dcn35_resource_construct(
>>>>>   	dc->caps.color.dpp.dgam_rom_for_yuv = 0;
>>>>>   	dc->caps.color.dpp.hw_3d_lut = 1;
>>>>> -	dc->caps.color.dpp.ogam_ram = 0;  // no OGAM in DPP since DCN1
>>>>> +	dc->caps.color.dpp.ogam_ram = 1;
>>>>>   	// no OGAM ROM on DCN301
>>>>>   	dc->caps.color.dpp.ogam_rom_caps.srgb = 0;
>>>>>   	dc->caps.color.dpp.ogam_rom_caps.bt2020 = 0;
>>>>> diff --git a/drivers/gpu/drm/amd/display/dc/resource/dcn351/dcn351_resource.c b/drivers/gpu/drm/amd/display/dc/resource/dcn351/dcn351_resource.c
>>>>> index 8a57adb27264..053e8ec6d1ef 100644
>>>>> --- a/drivers/gpu/drm/amd/display/dc/resource/dcn351/dcn351_resource.c
>>>>> +++ b/drivers/gpu/drm/amd/display/dc/resource/dcn351/dcn351_resource.c
>>>>> @@ -1841,7 +1841,7 @@ static bool dcn351_resource_construct(
>>>>>   	dc->caps.color.dpp.dgam_rom_for_yuv = 0;
>>>>>   	dc->caps.color.dpp.hw_3d_lut = 1;
>>>>> -	dc->caps.color.dpp.ogam_ram = 0;  // no OGAM in DPP since DCN1
>>>>> +	dc->caps.color.dpp.ogam_ram = 1;
>>>>>   	// no OGAM ROM on DCN301
>>>>>   	dc->caps.color.dpp.ogam_rom_caps.srgb = 0;
>>>>>   	dc->caps.color.dpp.ogam_rom_caps.bt2020 = 0;
>>>>> -- 
>>>>> 2.44.0
>>>>>