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. 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@xxxxxxxxx> >>>> >>>> Cc: Harry Wentland <harry.wentland@xxxxxxx> >>>> Cc: Xaver Hugl <xaver.hugl@xxxxxxxxx> >>>> Cc: Melissa Wen <mwen@xxxxxxxxxx> >>>> Cc: Ethan Lee <flibitijibibo@xxxxxxxxx> >>>> --- >>>> 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 >>>>
