Re: [RFC PATCH 0/5] DRM CRTC 3D LUT interface for AMD DCN

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux