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

Fri Apr 12 16:26:03 UTC 2024

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 ??
> 
> 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?

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
> > > 