On 06/28, Harry Wentland wrote: > > > 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. Hi Harry, So, I need to fix many points to program 3D LUT on AMD correctly... Thanks for explaining all questions, from this, I'm working on a new version. Best Regards, Melissa > > 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 > > >
Attachment:
signature.asc
Description: PGP signature
