On 2022-09-06 12:46, Melissa Wen wrote: > Hi, > > From all feedback at [3DLUT_RFC] and an extensive AMD driver > examination, here I am back with a first attempt to wire up a user 3D > LUT (post-blending) to DC interface via DM CRTC color mgmt :) > > I'm following some specific approaches to handle user shaper LUT and > user 3D LUT that I would like to validate if the path taken is correct. > > I used a modified version [igt_tests] of Ville's IGT 3D LUT test to > verify that the shaper and 3D LUT programming is happening. However, I > still have doubts about hw behavior and DC MPC's current implementation > for 3D LUT. > > Despite some initial patches for code cleanup and DRM interface, my > focus here is the inclusion of a user 3D LUT in the Display Manager, > which is done in the last five patches of this series: > > - drm/amd/display: enable DRM shaper and 3D LUT properties > - drm/amd/display: update lut3d and shaper lut to stream > - drm/amd/display: add user shaper LUT support to amdgpu_dm color pipeline > - drm/amd/display: add user 3D LUT support to the amdgpu_dm color pipeline > - drm/amd/display: encapsulate atomic regamma operation > > Things to take into account: > > - 3D LUT (and shaper LUT) is only available in the atomic pipeline (I > didn't work on any implicit conversions that are done in the legacy > path) > > - Therefore, I'm not doing any implicit conversions for shaper LUT > considering the input space, which means: it's set or not. When there > is no shaper LUT, it's set to BYPASS, but unfortunately, it seems that > the BYPASS mode for shaper LUT is not supported in the current DC > dcn30_set_mpc_shaper_3dlut(), since it returns false when > mpc3_program_shaper returns false (no params). Is the combination of a > user 3D LUT with a bypassed shaper LUT accepted by the hw? > AFAIK this should be supported. The current DC code is centered on use-cases for other OSes but I'm not sure this is the key issue here, though it might explain why things won't always look as expected. mpc3_program_shaper will program the shaper LUT to bypass when params are NULL. It will return false so dcn30_set_output_transfer_func knows no shaper LUT is set. The reason behind this is that shaper LUT and output gamma tend to work in tandem. If you have linear content incoming and want linear content on the output side of the 3DLUT you'll need to program both shaper LUT and output gamma LUTs with EOTF^-1 and EOTF respectively. (Btw, please watch my HDR presentation at XDC or look through the slides, as I'll show why the current DRM LUTs can't represent EOTF^-1 functions.) In your case I don't expect you're operating on linear content. Your content will already be in sRGB, so shaper LUT and output gamma should really be both in bypass mode. You might need to debug dcn30_set_output_transfer_func to make sure it's doing what you're expecting (and add new logic if needed). > - I also see in dcn30_set_mpc_shaper_3dlut() that some bits need to be > set in lut3d_func to have the 3D LUT programmed on the MPC block. In > this sense, I used the dc_acquire_release_mpc_3dlut() function to get > the lut3d_func from the resource pool, but I'm not sure if the timing to > acquire and release the lut3d_func from the resource pool is correct > (and if I can really use it directly or I should make a copy). > There are a limited number of MPC 3DLUT blocks (shaper + 3DLUT) available. Acquiring and releasing them in DM is the right way to go about this. Unfortunately dc_acquire_release_mpc_3dlut can fail. For this reason we should run it in atomic_check. Unfortunately this function operates on the current state, which makes it unsuitable for atomic_check. We need a new function that does what dc_acquire_release_mpc_3dlut does but takes a dm_state->context as parameter and operates on that instead. Once we have that you can call that in dm_update_crtc_state to acquire or release the 3DLUT on a CRTC as needed. > - Still, on this topic, I use for lut3d the same bit.out_tf to update > the stream in the commit_tail because it triggers > .set_output_transfer_func that is in charge of setting both OGAM and 3D > LUT on MPC. There is a chance I got it wrong here, so I appreciate any > input on this topic. > This looks correct to me. > - Finally, in set_output_transfer_func, AFAIU, even if a user OGAM is > set, it won't be programmed if the shaper LUT and 3D LUT programming > are successful. However, if shaper/3DLUT programming fails, OGAM can be > considered. Should DM only accept DRM regamma if no DRM 3D LUT is > passed, or allowing the programming of both is still desirable? > I don't know why we skip the OGAM programming when shaper + 3DLUT are set. This doesn't make sense to me. As described above, the thing with the shaper LUT is that you might need to sandwich the 3DLUT with two 1DLUTs, one being the shaper, the other the OGAM. Obviously it depends on your intended transformations on which of these you want to program but in general I would expect we still need the ability to set OGAM when the 3DLUT is programmed. > Regarding the other patches: > > - drm/drm_color_mgmt: add 3D LUT to color mgmt properties > - drm/drm_color_mgmt: add shaper LUT to color mgmt properties > > Here, an initial DRM 3D LUT interface is exposed to enable the entire > kernel path and is only available for the atomic pipeline. So far, it > only includes the LUT data and its size, but improvements on this > interface may also add stride and bit depth [VA_API]. > Additionally, I'm aware of the current work on exposing a color pipeline > API [KMS_pipe_API]. > Alex Hung will send an RFC on a pre-blending 3DLUT. Similar to your patchset it's not fully where we need it to be but he has a good definition of what I think we'll need to describe a 3DLUT accurately. His proposal is missing the shaper LUT, though, and I really value how your patchset is figuring out how to combine the shaper LUT with the 3DLUT. Thanks for your work on this and I hope we can chat more at XDC next week. Harry > I'm including some minor changes in this series that I made to better > understand the current DM color mgmt behavior: > > - drm/amd/display: add comments to describe DM crtc color mgmt behavior > - drm/amd/display: remove unused regamma condition > > ...but, there are other code cleanup patches that I'm not including in > this series to avoid unnecessary noise. > > You can check the entire work here: > https://gitlab.freedesktop.org/mwen/linux-amd/-/commits/drm_lut3d > > [igt_tests] IGT exploratory tests here: > https://gitlab.freedesktop.org/mwen/igt-gpu-tools/-/commits/kms_color_3dlut > > [3DLUT_RFC] https://lore.kernel.org/amd-gfx/20220619223104.667413-1-mwen@xxxxxxxxxx/ > [VA_API] http://intel.github.io/libva/structVAProcFilterParameterBuffer3DLUT.html > [KMS_pipe_API] https://gitlab.freedesktop.org/pq/color-and-hdr/-/issues/11 > > Let me know your thoughts. > > Thanks in advance, > > Melissa > > Melissa Wen (9): > drm/amd/display: remove unused regamma condition > drm/amd/display: add comments to describe DM crtc color mgmt behavior > drm/drm_color_mgmt: add shaper LUT to color mgmt properties > drm/drm_color_mgmt: add 3D LUT to color mgmt properties > drm/amd/display: encapsulate atomic regamma operation > drm/amd/display: add user 3D LUT support to the amdgpu_dm color > pipeline > drm/amd/display: add user shaper LUT support to amdgpu_dm color > pipeline > drm/amd/display: update lut3d and shaper lut to stream > drm/amd/display: enable DRM shaper and 3D LUT properties > > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 6 + > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 5 + > .../amd/display/amdgpu_dm/amdgpu_dm_color.c | 297 ++++++++++++++++-- > .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c | 6 + > drivers/gpu/drm/amd/display/dc/core/dc.c | 10 +- > .../gpu/drm/amd/display/dc/core/dc_stream.c | 13 + > drivers/gpu/drm/drm_atomic_state_helper.c | 7 + > drivers/gpu/drm/drm_atomic_uapi.c | 20 ++ > drivers/gpu/drm/drm_color_mgmt.c | 89 +++++- > drivers/gpu/drm/drm_fb_helper.c | 5 + > drivers/gpu/drm/drm_mode_config.c | 28 ++ > include/drm/drm_color_mgmt.h | 4 + > include/drm/drm_crtc.h | 24 +- > include/drm/drm_mode_config.h | 25 ++ > 14 files changed, 495 insertions(+), 44 deletions(-) >
