Re: [RFC PATCH v2 00/18] Add DRM CRTC 3D LUT interface

[Pekka Paalanen <ppaalanen@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 9876 bytes --]

On Mon, 9 Jan 2023 14:38:09 -0100
Melissa Wen <mwen@igalia.com> wrote:

> On 01/09, Melissa Wen wrote:
> > Hi,
> > 
> > After collecting comments in different places, here is a second version
> > of the work on adding DRM CRTC 3D LUT support to the current DRM color
> > mgmt interface. In comparison to previous proposals [1][2][3], here we
> > add 3D LUT before gamma 1D LUT, but also a shaper 1D LUT before 3D LUT,
> > that means the following DRM CRTC color correction pipeline:
> > 
> > Blend -> Degamma 1D LUT -> CTM -> Shaper 1D LUT -> 3D LUT -> Gamma 1D LUT

Hi Melissa,

that makes sense to me, for CRTCs. It would be really good to have that
as a diagram in the KMS UAPI documentation.

If someone wants to add a 3D LUT to KMS planes as well, then I'm not
sure if it should be this order or swapped. I will probably have an
opinion about that once Weston is fully HDR capable and has been tried
in the wild for a while with the HDR color operations fine-tuned based
on community feedback. IOW, not for a long time. The YUV to RGB
conversion factors in there as well.


> > 
> > and we also add a DRM CRTC LUT3D_MODE property, based on Alex Hung
> > proposal for pre-blending 3D LUT [4] (Thanks!), instead of just a
> > LUT3D_SIZE, that allows userspace to use different supported settings of
> > 3D LUT, fitting VA-API and new color API better. In this sense, I
> > adjusted the pre-blending proposal for post-blending usage.
> > 
> > Patches 1-6 targets the addition of shaper LUT and 3D LUT properties to
> > the current DRM CRTC color mgmt pipeline. Patch 6 can be considered an
> > extra/optional patch to define a default value for LUT3D_MODE, inspired
> > by what we do for the plane blend mode property (pre-multiplied).
> > 
> > Patches 7-18 targets AMD display code to enable shaper and 3D LUT usage
> > on DCN 301 (our HW case). Patches 7-9 performs code cleanups on current
> > AMD DM colors code, patch 10 updates AMD stream in case of user 3D LUT
> > changes, patch 11/12 rework AMD MPC 3D LUT resource handling by context
> > for DCN 301 (easily extendible to other DCN families). Finally, from
> > 13-18, we wire up SHAPER LUT, LUT3D and LUT3D MODE to AMD display
> > driver, exposing modes supported by HW and programming user shaper and
> > 3D LUT accordingly.
> > 
> > Our target userspace is Gamescope/SteamOS.
> > 
> > Basic IGT tests were based on [5][6] and are available here (in-progress):
> > https://gitlab.freedesktop.org/mwen/igt-gpu-tools/-/commits/crtc-lut3d-api
> > 
> > [1] https://lore.kernel.org/all/20201221015730.28333-1-laurent.pinchart+renesas@ideasonboard.com/
> > [2] https://github.com/vsyrjala/linux/commit/4d28e8ddf2a076f30f9e5bdc17cbb4656fe23e69
> > [3] https://lore.kernel.org/amd-gfx/20220619223104.667413-1-mwen@igalia.com/
> > [4] https://lore.kernel.org/dri-devel/20221004211451.1475215-1-alex.hung@amd.com/
> > [5] https://patchwork.freedesktop.org/series/90165/
> > [6] https://patchwork.freedesktop.org/series/109402/
> > [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.  
> 
> +Simon Ser, +Pekka Paalanen who might also be interested in this series.

Unfortunately I don't have the patch emails to reply to, so here's a
messy bunch of comments. I'll concentrate on the UAPI design as always.

+/*
+ * struct drm_mode_lut3d_mode - 3D LUT mode information.
+ * @lut_size: number of valid points on every dimension of 3D LUT.
+ * @lut_stride: number of points on every dimension of 3D LUT.
+ * @bit_depth: number of bits of RGB. If color_mode defines entries with higher
+ *             bit_depth the least significant bits will be truncated.
+ * @color_format: fourcc values, ex. DRM_FORMAT_XRGB16161616 or DRM_FORMAT_XBGR16161616.
+ * @flags: flags for hardware-sepcific features
+ */
+struct drm_mode_lut3d_mode {
+	__u16 lut_size;
+	__u16 lut_stride[3];
+	__u16 bit_depth;
+	__u32 color_format;
+	__u32 flags;
+};

Why is lut_stride an array of 3, but lut_size is not?

What is the color_mode the comment is referring to?

What is "number of bits of RGB"? Input precision? Output precision?
Integer or floating point?

Flags cannot be hardware specific, because it makes the whole KMS UAPI
hardware specific. That won't work. You have to have driver-agnostic
definitions for all possible flags.

Why is this the whole first patch? There is no documentation for the
UAPI on how this struct works, so I cannot review this. Explaining just
the individual fields is not enough to understand it. Is this something
the kernel fills in and is read-only to userspace? Is userspace filling
this in?


+ * “LUT3D”:
+ *	Blob property to set the 3D LUT mapping pixel data after the color
+ *	transformation matrix and before gamma 1D lut correction. The
+ *	data is interpreted as an array of &struct drm_color_lut elements.
+ *	Hardware might choose not to use the full precision of the LUT
+ *	elements.
+ *
+ *	Setting this to NULL (blob property value set to 0) means a the output
+ *	color is identical to the input color. This is generally the driver
+ *	boot-up state too. Drivers can access this blob through
+ *	&drm_crtc_state.gamma_lut.
+ *

You need to define how the 1-D array of drm_color_lut elements blob
will be interpreted as a 3-D array for the 3D LUT, and how the
dimensions match to the R, G and B channels. It's a bit like the
question about row-major or column-major storage for matrices, except
more complicated and not in those words.

+ * “LUT3D_MODE”:
+ *	Enum property to give the mode of the 3D lookup table to be set on the
+ *	LUT3D property. A mode specifies size, stride, bit depth and color
+ *	format and depends on the underlying hardware). If drivers support
+ *	multiple 3D LUT modes, they should be declared in a array of
+ *	drm_color_lut3d_mode and they will be advertised as an enum.

How does that work exactly? I didn't get it. I could guess, but having
to guess on API is bad.


+	/**
+	 * @lut3d:
+	 *
+	 * 3D Lookup table for converting pixel data. Position where it takes
+	 * place depends on hw design, after @ctm or @gamma_lut. See
+	 * drm_crtc_enable_color_mgmt(). The blob (if not NULL) is an array of
+	 * &struct drm_color_lut.
+	 */
+	struct drm_property_blob *lut3d;

I do not like the wording of "depends on hw design", and it is used in
very many places here. The KMS UAPI semantics cannot vary based on
hardware. Your cover letter defines the order in the color pipeline, so
I don't understand how this here can depend on hw.

What can depend on hardware is which KMS UAPI properties are exposed,
and how you map a property to a hardware unit (which can even change
based on the exact pipeline configuration as long as the results are as
the UAPI doc defines). But this comment here is talking about the UAPI
properties, not hw elements.


I'm happy that the 3D LUT interface is being developed, but as you can
see from my questions, the UAPI documentation is practically missing. I
would have no idea how to use this as is.


Thanks!
pq

> 
> Also please let me know if I forgot to address any comments.
> 
> Melissa
> 
> > 
> > Thanks,
> > 
> > Melissa
> > 
> > Alex Hung (2):
> >   drm: Add 3D LUT mode and its attributes
> >   drm/amd/display: Define 3D LUT struct for HDR planes
> > 
> > Melissa Wen (16):
> >   drm/drm_color_mgmt: add shaper LUT to color mgmt properties
> >   drm/drm_color_mgmt: add 3D LUT props to DRM color mgmt
> >   drm/drm_color_mgmt: add function to create 3D LUT modes supported
> >   drm/drm_color_mgmt: add function to attach 3D LUT props
> >   drm/drm_color_mgmt: set first lut3d mode as default
> >   drm/amd/display: remove unused regamma condition
> >   drm/amd/display: add comments to describe DM crtc color mgmt behavior
> >   drm/amd/display: encapsulate atomic regamma operation
> >   drm/amd/display: update lut3d and shaper lut to stream
> >   drm/amd/display: handle MPC 3D LUT resources for a given context
> >   drm/amd/display: acquire/release 3D LUT resources for ctx on DCN301
> >   drm/amd/display: expand array of supported 3D LUT modes
> >   drm/amd/display: enable 3D-LUT DRM properties if supported
> >   drm/amd/display: add user 3D LUT support to the amdgpu_dm color
> >     pipeline
> >   drm/amd/display: decouple steps to reuse in shaper LUT support
> >   drm/amd/display: add user shaper LUT support to amdgpu_dm color
> >     pipeline
> > 
> >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |   6 +
> >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   3 +
> >  .../amd/display/amdgpu_dm/amdgpu_dm_color.c   | 370 ++++++++++++++++--
> >  .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c    |   2 +
> >  drivers/gpu/drm/amd/display/dc/core/dc.c      |  49 ++-
> >  drivers/gpu/drm/amd/display/dc/dc.h           |   8 +
> >  .../amd/display/dc/dcn301/dcn301_resource.c   |  47 ++-
> >  .../amd/display/modules/color/color_gamma.h   |  43 ++
> >  drivers/gpu/drm/drm_atomic_state_helper.c     |   7 +
> >  drivers/gpu/drm/drm_atomic_uapi.c             |  24 ++
> >  drivers/gpu/drm/drm_color_mgmt.c              | 127 ++++++
> >  drivers/gpu/drm/drm_fb_helper.c               |   5 +
> >  drivers/gpu/drm/drm_mode_config.c             |  21 +
> >  include/drm/drm_color_mgmt.h                  |   8 +
> >  include/drm/drm_crtc.h                        |  32 +-
> >  include/drm/drm_mode_config.h                 |  25 ++
> >  include/drm/drm_mode_object.h                 |   2 +-
> >  include/uapi/drm/drm_mode.h                   |  17 +
> >  18 files changed, 757 insertions(+), 39 deletions(-)
> > 
> > -- 
> > 2.35.1
> >   


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]