On 2024-04-15 11:27, Melissa Wen wrote: > On 04/12, Harry Wentland wrote: >> >> >> 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. > > Hi Harry, > > Thank you for explaning the latest changes in the color pipeline. > Since MCM is fixed pre-blending in the current dcn32 driver: > > 1. I understand that the implementation for post-blending shaper/3dlut > isn't available in the current dcn32 driver, right? > Correct. > 2. Does the dcn32 hardware itself support the configuration of both pre- > and post-blending shaper/3dlut at the same time? Or, for example, dcn32 > will advertise two color pipelines: one with pre-blending 3dlut and no > post-blending, and another without pre-blending and only post-blending > 3dlut? > HW supports it. SW (driver) support is lacking. At this point I'm only targetting pre-blend MCM. We can consider postblend MCM if there's a concrete need. Color pipelines would be per plane or stream. In the case where we'd want to support both pre-blend and post-blend MCM we would have 3dlut in both the plane and the (to-be-implemented) stream color pipeline and reject an atomic commit if the configuration can't be supported. In theory one could support both pre-blending and post-blending 3dlut if one picks two pipes to drive the configuration. Either way, adding a post-blend MCM will complicate the entire thing a fair bit and for that reason I would prefer to avoid dealing with a post-blend 3dlut unless we get a specific ask for it. Harry > Melissa > >> >> 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@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 >>>>>>> >>
