On 2022-06-19 18:30, Melissa Wen wrote: > Hi, > > I've been working on a proposal to add 3D LUT interface to DRM CRTC > color mgmt, that means new **after-blending** properties for color > correction. As I'm targeting AMD display drivers, I need to map these > new properties to AMD DC interface and I have some doubts about the 3D > LUT programming on DCN blocks. > > First of all, this patchset is a working in progress and further > discussions about the DRM interface should be done. I've examined > previous proposal to add 3D LUT[1][2] and I understand that the main > difference between them is regarding the property position in the DRM > color management pipeline (between CTM and Gamma 1D or after Gamma 1D). > On the other hand, AMD DC always considers a shaper (1D) LUT before a 3D > LUT, used to delinearize and shape the content. These LUTs are then > positioned between DRM CTM and Gamma (1D). > > By comparing the AMD design with the other proposals, I see that it's > possible to cover all of them by adding and combining shaper (1D) LUT > and 3D LUT as new color mgmt properties. Moreover, it'll not limit the > exposure of AMD color correction caps to the userspace. Therefore, my > proposal is to add these two new properties in the DRM color mgmt > pipeline as follows: > > +------------+ > | | > | Degamma | > +-----+------+ > | > +-----v------+ > | | > | CTM | > +-----+------+ > | > ++-----v------++ > || || > || Shaper LUT || > ++-----+------++ > | > ++-----v------++ > || || > || 3D LUT || > ++-----+------++ > | > +-----v------+ > | | > | Gamma (1D) | > +------------+ > As Ville already mentioned on patch 4, the increasing complexity of the color pipeline and the arguments about the placement of the 3D LUT means that we will probably need a definition of a particular HW's color pipeline. Something like this proposal from Sebastian: https://gitlab.freedesktop.org/pq/color-and-hdr/-/issues/11 > However, many doubts arose when I was mapping these two new properties > to DC interface. This is why I decided to share an not-yet-completed > implementation to get some feedback and explanation. > > This RFC patchset is divided in three scopes of change. The first two > patches document the AMD DM color correction mapping. Some comments were > rewritten as kernel doc entries. I also summarize all information > provided in previous discussions[3] and also redid those diagrams to > svg. All doc should be reviewed and some struct members lack > explanation. I can add them to documentation if you can provide a > description. Some examples that lack description so far: > * in amdgpu_display_manager: dmub_outbox_params, dmub_aux_transfer_done, delayed_hpd_wq; > * in dpp_color_caps: dgam_ram, dgam_rom_for_yuv; > * in mpc_color_caps: ogam_ram. > > The next two patches expand DRM color mgmt interface to add shaper LUT > and 3D LUT. Finally, the last patch focuses on mapping DRM properties to > DC interface and these are my doubts so far: > > - To configure a shaper LUT, I related it to the current configuration > of gamma 1D. For dc_transfer_func, I should set tf according to the > input space, that means, LINEAR for shaper LUT after blending, right? > When 3D LUT is set, the input space for gamma 1D will no longer be > linear, so how to define the tf? should I set tf as sRGB, BT709 or > what? > We don't know the input space. It's nowhere defined in the KMS API. It entirely depends on how a compositor renders the framebuffer (or transforms it using some future KMS plane API). DC interfaces are designed for a system where the driver is aware of the input color space and linearity/non-linearity. This means that you'll often need to dig through the API down to the HW programming bits to understand what it actually does. A leaky abstraction, essentially. Because KMS drivers don't know the linearity/non-linearity at any point int the pipeline we need an API where the KMS client provides the appropriate shaper LUT. In the case of any current KMS client that will always be non-colormanaged and is assumed to be sRGB. If your framebuffer is non-linear (sRGB) and you're not linearizing it using the CRTC Degamma you'll already have non-linear values and won't need to program the shaper LUT (i.e. use it in bypass or linear). If your framebuffer is linear and you're not de-linearizing it with the CRTC Degamma LUT you'll have linear values and need to program the inverse EOTF for sRGB in your shaper (or degamma) LUT. > - I see the 3dlut values being mapped to struct tetrahedral_17 as four > arrays lut0-4. From that I am considering tetrahedral interpolation. > Is there any other interpolation option? Also, as the total size of the > four arrays is the same of the 3D LUT size, I'm mapping DRM color lut > values in ascending order, starting by filling lut0 to lut4. Is it right > or is there another way to distribute these values across lut0-4 arrays? > We seem to do this on other platforms (illustrating for red only) for (lut_i = 0, i = 0; i < lut_size-4; lut_i++, i += 4) { lut0[lut_i].red = rgb[i].red; lut1[lut_i].red = rgb[i + 1].red; lut2[lut_i].red = rgb[i + 2].red; lut3[lut_i].red = rgb[i + 3].red; } lut0[lut_i].red = rgb[i].red; And yes, it uses tetrahedral interpolation. > - I also see tetrahedral 9x9x9, that means DC supports 9x9x9 3D LUT too? > If so, does it depend on hw caps or it is defined by the user? Also, I > see 10 and 12 bits color channel precision, does it depend on hw caps or > it is also defined by the userspace? Any ideas on how to expose it? > This is user-configurable, HW supports both 9^3 and 17^3 and both 10 and 12-bit variants. > - Why func_shaper and lut3d_func are defined as constant on > dc_stream_state, while the other color properties are not? How should > I change them from the proposed DRM properties? should I set 3D LUT in a > different struct of the DC interface or a different DC pipeline stage? > It's a pointer to constant struct. If you want to change it you should allocate a new func_shaper or lut3d_func struct. See https://www.internalpointers.com/post/constant-pointers-vs-pointer-constants-c-and-c > - In mpc3_program_3dlut(), why we are setting is_12bits_color_channel in > get3dlut_config(), since right after that we are changing its values > with this line `is_12bits_color_channel = params->use_12bits`? > We're reading the HW default to be used unless someone sets use_12bits but then we're always setting it based on use_12bits anyways. We wouldn't need the former but it's code that's never hurt anyone. :) > - In mpc3_set3dlut_ram10(), there is a suspicious comment for a shift > operation: `should we shift red 22bit and green 12? ask Nvenko` So, is > this operation stable/working as expected? > You can safely ignore this, at least as long as your LUT programming works. If it doesn't this comment is an indication of what you can try. Harry > Thanks in advance for clarifications, > > Melissa > > [1] https://lore.kernel.org/all/20201221015730.28333-1-laurent.pinchart+renesas@xxxxxxxxxxxxxxxx/ > [2] https://github.com/vsyrjala/linux/commit/4d28e8ddf2a076f30f9e5bdc17cbb4656fe23e69 > [3] https://lore.kernel.org/amd-gfx/20220505220744.3sex7ka2ha2vcguv@xxxxxxxxxxxxxxx/ > > Melissa Wen (5): > Documentation/amdgpu_dm: Add DM color correction documentation > Documentation/amdgpu/display: add DC color caps info > 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: mapping new DRM 3D LUT properties to AMD hw blocks > > .../amdgpu/display/dcn2_cm_drm_current.svg | 1370 +++++++++++++++ > .../amdgpu/display/dcn3_cm_drm_current.svg | 1528 +++++++++++++++++ > .../gpu/amdgpu/display/display-manager.rst | 44 + > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +- > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 5 +- > .../amd/display/amdgpu_dm/amdgpu_dm_color.c | 265 ++- > drivers/gpu/drm/amd/display/dc/dc.h | 53 +- > drivers/gpu/drm/amd/display/dc/dc_stream.h | 4 +- > 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 + > 16 files changed, 3411 insertions(+), 62 deletions(-) > create mode 100644 Documentation/gpu/amdgpu/display/dcn2_cm_drm_current.svg > create mode 100644 Documentation/gpu/amdgpu/display/dcn3_cm_drm_current.svg >
