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

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

[Melissa Wen]

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

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.

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

[RFC PATCH v2 01/18] drm: Add 3D LUT mode and its attributes

[Melissa Wen]

From: Alex Hung <alex.hung@amd.com>

A struct is defined for 3D LUT modes to be supported by hardware.
The elements includes lut_size, lut_stride, bit_depth, color_format
and flags.

Note: A patchset "IGT tests for pre-blending 3D LUT interfaces" for this
proposal is sent to IGT mailing list.

Signed-off-by: Alex Hung <alex.hung@amd.com>
---
 include/uapi/drm/drm_mode.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index fa953309d9ce..fddb86dad4db 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -849,6 +849,23 @@ struct drm_color_lut {
 	__u16 reserved;
 };
 
+/*
+ * 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;
+};
+
 /**
  * struct hdr_metadata_infoframe - HDR Metadata Infoframe Data.
  *
-- 
2.35.1

[RFC PATCH v2 02/18] drm/drm_color_mgmt: add shaper LUT to color mgmt properties

[Melissa Wen]

Shaper LUT is used to shape the content after blending, i.e.,
de-linearize or normalize space before applying a 3D LUT color
correction. In the next patch, we add 3D LUT property to DRM color
management after this shaper LUT and before the current gamma LUT.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 drivers/gpu/drm/drm_atomic_state_helper.c |  4 ++++
 drivers/gpu/drm/drm_atomic_uapi.c         | 10 ++++++++++
 drivers/gpu/drm/drm_color_mgmt.c          | 18 ++++++++++++++++++
 drivers/gpu/drm/drm_fb_helper.c           |  3 +++
 drivers/gpu/drm/drm_mode_config.c         | 14 ++++++++++++++
 include/drm/drm_crtc.h                    | 14 ++++++++++++--
 include/drm/drm_mode_config.h             | 12 ++++++++++++
 include/drm/drm_mode_object.h             |  2 +-
 8 files changed, 74 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
index bf31b9d92094..4bc860d6cf75 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -141,8 +141,11 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
 		drm_property_blob_get(state->degamma_lut);
 	if (state->ctm)
 		drm_property_blob_get(state->ctm);
+	if (state->shaper_lut)
+		drm_property_blob_get(state->shaper_lut);
 	if (state->gamma_lut)
 		drm_property_blob_get(state->gamma_lut);
+
 	state->mode_changed = false;
 	state->active_changed = false;
 	state->planes_changed = false;
@@ -214,6 +217,7 @@ void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state)
 	drm_property_blob_put(state->mode_blob);
 	drm_property_blob_put(state->degamma_lut);
 	drm_property_blob_put(state->ctm);
+	drm_property_blob_put(state->shaper_lut);
 	drm_property_blob_put(state->gamma_lut);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_crtc_destroy_state);
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 79730fa1dd8e..5339350c858d 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -430,6 +430,14 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
 					&replaced);
 		state->color_mgmt_changed |= replaced;
 		return ret;
+	} else if (property == config->shaper_lut_property) {
+		ret = drm_atomic_replace_property_blob_from_id(dev,
+					&state->shaper_lut,
+					val,
+					-1, sizeof(struct drm_color_lut),
+					&replaced);
+		state->color_mgmt_changed |= replaced;
+		return ret;
 	} else if (property == config->gamma_lut_property) {
 		ret = drm_atomic_replace_property_blob_from_id(dev,
 					&state->gamma_lut,
@@ -481,6 +489,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
 		*val = (state->degamma_lut) ? state->degamma_lut->base.id : 0;
 	else if (property == config->ctm_property)
 		*val = (state->ctm) ? state->ctm->base.id : 0;
+	else if (property == config->shaper_lut_property)
+		*val = (state->shaper_lut) ? state->shaper_lut->base.id : 0;
 	else if (property == config->gamma_lut_property)
 		*val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
 	else if (property == config->prop_out_fence_ptr)
diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
index d021497841b8..cf6a998b4298 100644
--- a/drivers/gpu/drm/drm_color_mgmt.c
+++ b/drivers/gpu/drm/drm_color_mgmt.c
@@ -69,6 +69,24 @@
  *	boot-up state too. Drivers can access the blob for the color conversion
  *	matrix through &drm_crtc_state.ctm.
  *
+ * “SHAPER_LUT”:
+ *	Blob property to set the shaper lut shaping pixel data after the color
+ *	transformation matrix and before applying 3D Lookup Table (3D LUT). It
+ *	can be used to delinearize content to get an effective 3D LUT mapping.
+ *	The data is interpreted as an array of &struct drm_color_lut elements.
+ *
+ *	Setting this to NULL (blob property value set to 0) means 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.
+ *
+ * “SHAPER_LUT_SIZE”:
+ *	Unsigned range property to give the size of the shaper lookup table to
+ *	be set on the SHAPER_LUT property (the size depends on the underlying
+ *	hardware). If drivers support multiple LUT sizes then they should
+ *	publish the largest size, and sub-sample smaller sized LUTs
+ *	appropriately.
+ *
  * “GAMMA_LUT”:
  *	Blob property to set the gamma lookup table (LUT) mapping pixel data
  *	after the transformation matrix to data sent to the connector. The
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 71edb80fe0fb..f8b4611cbb4a 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1129,8 +1129,11 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
 		replaced  = drm_property_replace_blob(&crtc_state->degamma_lut,
 						      NULL);
 		replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
+		replaced |= drm_property_replace_blob(&crtc_state->shaper_lut,
+						      NULL);
 		replaced |= drm_property_replace_blob(&crtc_state->gamma_lut,
 						      gamma_lut);
+
 		crtc_state->color_mgmt_changed |= replaced;
 	}
 
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index 939d621c9ad4..6468cecc7081 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -354,6 +354,20 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
 		return -ENOMEM;
 	dev->mode_config.ctm_property = prop;
 
+	prop = drm_property_create(dev,
+			DRM_MODE_PROP_BLOB,
+			"SHAPER_LUT", 0);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.shaper_lut_property = prop;
+
+	prop = drm_property_create_range(dev,
+			DRM_MODE_PROP_IMMUTABLE,
+			"SHAPER_LUT_SIZE", 0, UINT_MAX);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.shaper_lut_size_property = prop;
+
 	prop = drm_property_create(dev,
 			DRM_MODE_PROP_BLOB,
 			"GAMMA_LUT", 0);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 8e1cbc75143e..7f289e0153aa 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -152,8 +152,9 @@ struct drm_crtc_state {
 	bool zpos_changed : 1;
 	/**
 	 * @color_mgmt_changed: Color management properties have changed
-	 * (@gamma_lut, @degamma_lut or @ctm). Used by the atomic helpers and
-	 * drivers to steer the atomic commit control flow.
+	 * (@shaper_lut, @gamma_lut, @degamma_lut or @ctm). Used by
+	 * the atomic helpers and drivers to steer the atomic commit control
+	 * flow.
 	 */
 	bool color_mgmt_changed : 1;
 
@@ -279,6 +280,15 @@ struct drm_crtc_state {
 	 */
 	struct drm_property_blob *gamma_lut;
 
+	/**
+	 * @shaper_lut:
+	 *
+	 * Lookup table used to de-linearize pixel data for gamma correction.
+	 * See drm_crtc_enable_color_mgmt(). The blob (if not NULL) is an array
+	 * of &struct drm_color_lut.
+	 */
+	struct drm_property_blob *shaper_lut;
+
 	/**
 	 * @target_vblank:
 	 *
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 6b5e01295348..c174d3a7c951 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -800,6 +800,18 @@ struct drm_mode_config {
 	 * degamma LUT.
 	 */
 	struct drm_property *ctm_property;
+
+	/**
+	 * @shaper_lut_property: Optional CRTC property to set the shaper LUT used to
+	 * convert colors before 3D LUT conversion.
+	 */
+	struct drm_property *shaper_lut_property;
+	/**
+	 * @shaper_lut_size_property: Optional CRTC property for the size of the
+	 * shaper LUT as supported by the driver (read-only).
+	 */
+	struct drm_property *shaper_lut_size_property;
+
 	/**
 	 * @gamma_lut_property: Optional CRTC property to set the LUT used to
 	 * convert the colors, after the CTM matrix, to the gamma space of the
diff --git a/include/drm/drm_mode_object.h b/include/drm/drm_mode_object.h
index 912f1e415685..5e75b51936e6 100644
--- a/include/drm/drm_mode_object.h
+++ b/include/drm/drm_mode_object.h
@@ -60,7 +60,7 @@ struct drm_mode_object {
 	void (*free_cb)(struct kref *kref);
 };
 
-#define DRM_OBJECT_MAX_PROPERTY 24
+#define DRM_OBJECT_MAX_PROPERTY 26
 /**
  * struct drm_object_properties - property tracking for &drm_mode_object
  */
-- 
2.35.1

[RFC PATCH v2 03/18] drm/drm_color_mgmt: add 3D LUT props to DRM color mgmt

[Melissa Wen]

Add 3D LUT for gammar correction using a 3D lookup table. The position
in the color correction pipeline where 3D LUT is applied depends on hw
design, being after CTM or gamma. If just after CTM, a shaper lut must
be set to shape the content for a non-linear space. That details should
be handled by the driver according to its color capabilities.

--
v3:
- refactor CRTC 3D LUT API to use Alex Hung proposal for planes

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 drivers/gpu/drm/drm_atomic_state_helper.c |  3 +++
 drivers/gpu/drm/drm_atomic_uapi.c         | 14 ++++++++++++++
 drivers/gpu/drm/drm_color_mgmt.c          | 19 +++++++++++++++++++
 drivers/gpu/drm/drm_fb_helper.c           |  2 ++
 drivers/gpu/drm/drm_mode_config.c         |  7 +++++++
 include/drm/drm_crtc.h                    | 20 +++++++++++++++++++-
 include/drm/drm_mode_config.h             | 13 +++++++++++++
 include/drm/drm_mode_object.h             |  2 +-
 8 files changed, 78 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
index 4bc860d6cf75..6e70f08e2f44 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -143,6 +143,8 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
 		drm_property_blob_get(state->ctm);
 	if (state->shaper_lut)
 		drm_property_blob_get(state->shaper_lut);
+	if (state->lut3d)
+		drm_property_blob_get(state->lut3d);
 	if (state->gamma_lut)
 		drm_property_blob_get(state->gamma_lut);
 
@@ -218,6 +220,7 @@ void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state)
 	drm_property_blob_put(state->degamma_lut);
 	drm_property_blob_put(state->ctm);
 	drm_property_blob_put(state->shaper_lut);
+	drm_property_blob_put(state->lut3d);
 	drm_property_blob_put(state->gamma_lut);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_crtc_destroy_state);
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 5339350c858d..29297badf028 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -438,6 +438,16 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
 					&replaced);
 		state->color_mgmt_changed |= replaced;
 		return ret;
+	} else if (property == config->lut3d_property) {
+		ret = drm_atomic_replace_property_blob_from_id(dev,
+					&state->lut3d,
+					val,
+					-1, sizeof(struct drm_color_lut),
+					&replaced);
+		state->color_mgmt_changed |= replaced;
+		return ret;
+	} else if (property == config->lut3d_mode_property) {
+		state->lut3d_mode = val;
 	} else if (property == config->gamma_lut_property) {
 		ret = drm_atomic_replace_property_blob_from_id(dev,
 					&state->gamma_lut,
@@ -491,6 +501,10 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
 		*val = (state->ctm) ? state->ctm->base.id : 0;
 	else if (property == config->shaper_lut_property)
 		*val = (state->shaper_lut) ? state->shaper_lut->base.id : 0;
+	else if (property == config->lut3d_property)
+		*val = (state->lut3d) ? state->lut3d->base.id : 0;
+	else if (property == config->lut3d_mode_property)
+		*val = state->lut3d_mode;
 	else if (property == config->gamma_lut_property)
 		*val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
 	else if (property == config->prop_out_fence_ptr)
diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
index cf6a998b4298..f92633b3b67e 100644
--- a/drivers/gpu/drm/drm_color_mgmt.c
+++ b/drivers/gpu/drm/drm_color_mgmt.c
@@ -87,6 +87,25 @@
  *	publish the largest size, and sub-sample smaller sized LUTs
  *	appropriately.
  *
+ * “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.
+ *
+ * “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.
+ *
  * “GAMMA_LUT”:
  *	Blob property to set the gamma lookup table (LUT) mapping pixel data
  *	after the transformation matrix to data sent to the connector. The
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index f8b4611cbb4a..6130c44c79f6 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1131,6 +1131,8 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
 		replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
 		replaced |= drm_property_replace_blob(&crtc_state->shaper_lut,
 						      NULL);
+		replaced |= drm_property_replace_blob(&crtc_state->lut3d,
+						      NULL);
 		replaced |= drm_property_replace_blob(&crtc_state->gamma_lut,
 						      gamma_lut);
 
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index 6468cecc7081..5c37d84cdabf 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -368,6 +368,13 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
 		return -ENOMEM;
 	dev->mode_config.shaper_lut_size_property = prop;
 
+	prop = drm_property_create(dev,
+			DRM_MODE_PROP_BLOB,
+			"LUT3D", 0);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.lut3d_property = prop;
+
 	prop = drm_property_create(dev,
 			DRM_MODE_PROP_BLOB,
 			"GAMMA_LUT", 0);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 7f289e0153aa..805a50c36a70 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -152,7 +152,7 @@ struct drm_crtc_state {
 	bool zpos_changed : 1;
 	/**
 	 * @color_mgmt_changed: Color management properties have changed
-	 * (@shaper_lut, @gamma_lut, @degamma_lut or @ctm). Used by
+	 * (@shaper_lut, @lut3d, @gamma_lut, @degamma_lut or @ctm). Used by
 	 * the atomic helpers and drivers to steer the atomic commit control
 	 * flow.
 	 */
@@ -289,6 +289,24 @@ struct drm_crtc_state {
 	 */
 	struct drm_property_blob *shaper_lut;
 
+	/**
+	 * @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;
+
+	/**
+	 * @lut3d_mode:
+	 * This is a blob_id and exposes the platform capabilities wrt
+	 * various 3dlut. This also helps user select a 3dlut mode amongst
+	 * the supported ones.
+	 */
+	u32 lut3d_mode;
+
 	/**
 	 * @target_vblank:
 	 *
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index c174d3a7c951..6cb96f403de2 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -812,6 +812,19 @@ struct drm_mode_config {
 	 */
 	struct drm_property *shaper_lut_size_property;
 
+	/**
+	 * @lut3d_property: Optional CRTC property to set the 3D LUT used to
+	 * convert colors; before or after gamma conversion depends on hw
+	 * design. A shaper LUT can be used to delinearize content before apply
+	 * 3D LUT correction.
+	 */
+	struct drm_property *lut3d_property;
+	/**
+	 * @lut3d_mode_property: Optional CRTC property to describe 3D LUT modes
+	 * supported by the driver.
+	 */
+	struct drm_property *lut3d_mode_property;
+
 	/**
 	 * @gamma_lut_property: Optional CRTC property to set the LUT used to
 	 * convert the colors, after the CTM matrix, to the gamma space of the
diff --git a/include/drm/drm_mode_object.h b/include/drm/drm_mode_object.h
index 5e75b51936e6..d39ee21f26db 100644
--- a/include/drm/drm_mode_object.h
+++ b/include/drm/drm_mode_object.h
@@ -60,7 +60,7 @@ struct drm_mode_object {
 	void (*free_cb)(struct kref *kref);
 };
 
-#define DRM_OBJECT_MAX_PROPERTY 26
+#define DRM_OBJECT_MAX_PROPERTY 28
 /**
  * struct drm_object_properties - property tracking for &drm_mode_object
  */
-- 
2.35.1

[RFC PATCH v2 04/18] drm/drm_color_mgmt: add function to create 3D LUT modes supported

[Melissa Wen]

DRM color function to create modes for lut3d mode property from an array
of drm_color_lut3d_mode modes supported by the HW and advertise to
userspace. Userspace can get the description of a specific mode in the
enum list from its blob data.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 drivers/gpu/drm/drm_color_mgmt.c | 43 +++++++++++++++++++++++++++++++-
 include/drm/drm_color_mgmt.h     |  4 +++
 2 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
index f92633b3b67e..6ce48007cdd4 100644
--- a/drivers/gpu/drm/drm_color_mgmt.c
+++ b/drivers/gpu/drm/drm_color_mgmt.c
@@ -104,7 +104,7 @@
  *	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.
+ *	drm_mode_lut3d_mode and they will be advertised as an enum.
  *
  * “GAMMA_LUT”:
  *	Blob property to set the gamma lookup table (LUT) mapping pixel data
@@ -228,6 +228,47 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
 }
 EXPORT_SYMBOL(drm_crtc_enable_color_mgmt);
 
+int drm_crtc_create_lut3d_mode_property(struct drm_crtc *crtc,
+					const struct drm_mode_lut3d_mode modes[],
+					unsigned int num_modes)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_property_blob *blob;
+	struct drm_property *prop;
+	char *name;
+	int ret;
+
+	if (dev->mode_config.lut3d_mode_property)
+		return 0;
+
+	prop = drm_property_create(dev, DRM_MODE_PROP_ENUM, "LUT3D_MODE", num_modes);
+	if (!prop)
+		return -EINVAL;
+
+	for (int i = 0; i < num_modes; i++) {
+		blob = drm_property_create_blob(dev, sizeof(modes[i]), &modes[i]);
+		if (IS_ERR(blob))
+			return PTR_ERR(blob);
+
+		name = kasprintf(GFP_KERNEL, "lut3d_%d_%dbit",
+				 modes[i].lut_size, modes[i].bit_depth);
+		if (!name)
+			return -ENOMEM;
+
+		ret = drm_property_add_enum(prop, blob->base.id, name);
+		if (ret) {
+			drm_property_blob_put(blob);
+			kfree(name);
+			return ret;
+		}
+		kfree(name);
+	}
+	dev->mode_config.lut3d_mode_property = prop;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_crtc_create_lut3d_mode_property);
+
 /**
  * drm_mode_crtc_set_gamma_size - set the gamma table size
  * @crtc: CRTC to set the gamma table size for
diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
index 81c298488b0c..af9305925572 100644
--- a/include/drm/drm_color_mgmt.h
+++ b/include/drm/drm_color_mgmt.h
@@ -59,6 +59,10 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
 				bool has_ctm,
 				uint gamma_lut_size);
 
+int drm_crtc_create_lut3d_mode_property(struct drm_crtc *crtc,
+					const struct drm_mode_lut3d_mode modes[],
+					unsigned int num_modes);
+
 int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
 				 int gamma_size);
 
-- 
2.35.1

[RFC PATCH v2 05/18] drm/drm_color_mgmt: add function to attach 3D LUT props

[Melissa Wen]

If the driver supports user 3D LUT then it calls a drm function to
attach 3D LUT related properties according to HW caps.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 drivers/gpu/drm/drm_color_mgmt.c | 35 ++++++++++++++++++++++++++++++++
 include/drm/drm_color_mgmt.h     |  3 +++
 2 files changed, 38 insertions(+)

diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
index 6ce48007cdd4..06503f693ecd 100644
--- a/drivers/gpu/drm/drm_color_mgmt.c
+++ b/drivers/gpu/drm/drm_color_mgmt.c
@@ -269,6 +269,41 @@ int drm_crtc_create_lut3d_mode_property(struct drm_crtc *crtc,
 }
 EXPORT_SYMBOL(drm_crtc_create_lut3d_mode_property);
 
+/**
+ * drm_crtc_enable_lut3d - enable 3D LUT properties
+ * @crtc: DRM CRTC
+ * @shaper_lut_size: the size of shaper lut
+ *
+ * This function lets the driver enable the 3D LUT color correction property
+ * on a CRTC. This includes 3D LUT and also a shaper LUT, if set. The shaper
+ * LUT property is only attached if its size is not 0 and 3D LUT is set, being
+ * therefore optional.
+ */
+void drm_crtc_enable_lut3d(struct drm_crtc *crtc,
+			   uint shaper_lut_size)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_mode_config *config = &dev->mode_config;
+
+	if (!config->lut3d_mode_property)
+		return;
+
+	drm_object_attach_property(&crtc->base,
+				   config->lut3d_property, 0);
+	drm_object_attach_property(&crtc->base,
+				   config->lut3d_mode_property, 0);
+
+	if (!shaper_lut_size)
+		return;
+
+	drm_object_attach_property(&crtc->base,
+				   config->shaper_lut_property, 0);
+	drm_object_attach_property(&crtc->base,
+				   config->shaper_lut_size_property,
+				   shaper_lut_size);
+}
+EXPORT_SYMBOL(drm_crtc_enable_lut3d);
+
 /**
  * drm_mode_crtc_set_gamma_size - set the gamma table size
  * @crtc: CRTC to set the gamma table size for
diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
index af9305925572..db2026dc825e 100644
--- a/include/drm/drm_color_mgmt.h
+++ b/include/drm/drm_color_mgmt.h
@@ -63,6 +63,9 @@ int drm_crtc_create_lut3d_mode_property(struct drm_crtc *crtc,
 					const struct drm_mode_lut3d_mode modes[],
 					unsigned int num_modes);
 
+void drm_crtc_enable_lut3d(struct drm_crtc *crtc,
+			   uint shaper_lut_size);
+
 int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
 				 int gamma_size);
 
-- 
2.35.1

[RFC PATCH v2 06/18] drm/drm_color_mgmt: set first lut3d mode as default

[Melissa Wen]

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 drivers/gpu/drm/drm_color_mgmt.c | 18 ++++++++++++++++--
 include/drm/drm_color_mgmt.h     |  3 ++-
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
index 06503f693ecd..d6283f049881 100644
--- a/drivers/gpu/drm/drm_color_mgmt.c
+++ b/drivers/gpu/drm/drm_color_mgmt.c
@@ -280,18 +280,32 @@ EXPORT_SYMBOL(drm_crtc_create_lut3d_mode_property);
  * therefore optional.
  */
 void drm_crtc_enable_lut3d(struct drm_crtc *crtc,
-			   uint shaper_lut_size)
+			   uint shaper_lut_size,
+			   bool first_value_as_default)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_property_enum *prop;
+	uint init_value = 0;
 
 	if (!config->lut3d_mode_property)
 		return;
 
 	drm_object_attach_property(&crtc->base,
 				   config->lut3d_property, 0);
+
+	if (first_value_as_default) {
+		prop = list_first_entry_or_null(&config->lut3d_mode_property->enum_list,
+						typeof(*prop), head);
+		init_value = prop ? prop->value : 0;
+	}
+
 	drm_object_attach_property(&crtc->base,
-				   config->lut3d_mode_property, 0);
+				   config->lut3d_mode_property,
+				   init_value);
+
+	if (crtc->state && prop)
+		crtc->state->lut3d_mode = init_value;
 
 	if (!shaper_lut_size)
 		return;
diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
index db2026dc825e..d13e99e2e877 100644
--- a/include/drm/drm_color_mgmt.h
+++ b/include/drm/drm_color_mgmt.h
@@ -64,7 +64,8 @@ int drm_crtc_create_lut3d_mode_property(struct drm_crtc *crtc,
 					unsigned int num_modes);
 
 void drm_crtc_enable_lut3d(struct drm_crtc *crtc,
-			   uint shaper_lut_size);
+			   uint shaper_lut_size,
+			   bool first_value_as_default);
 
 int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
 				 int gamma_size);
-- 
2.35.1

[RFC PATCH v2 07/18] drm/amd/display: remove unused regamma condition

[Melissa Wen]

The function __set_output_tf is only called by
amdgpu_dm_update_crtc_color_mgmt() when using atomic regamma. In this
situation, func->tf == TRANSFER_FUNCTION_LINEAR (the original if
condition) and it never falls into tf != LINEAR (the else condition).
Therefore, remove unused condition to avoid misunderstandings here.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../amd/display/amdgpu_dm/amdgpu_dm_color.c   | 32 ++++++-------------
 1 file changed, 10 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
index a4cb23d059bd..10a29d131424 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
@@ -255,14 +255,13 @@ static int __set_legacy_tf(struct dc_transfer_func *func,
  * @func: transfer function
  * @lut: lookup table that defines the color space
  * @lut_size: size of respective lut
- * @has_rom: if ROM can be used for hardcoded curve
  *
  * Returns:
  * 0 in case of success. -ENOMEM if fails.
  */
 static int __set_output_tf(struct dc_transfer_func *func,
-			   const struct drm_color_lut *lut, uint32_t lut_size,
-			   bool has_rom)
+			   const struct drm_color_lut *lut,
+			   uint32_t lut_size)
 {
 	struct dc_gamma *gamma = NULL;
 	struct calculate_buffer cal_buffer = {0};
@@ -279,24 +278,13 @@ static int __set_output_tf(struct dc_transfer_func *func,
 	gamma->num_entries = lut_size;
 	__drm_lut_to_dc_gamma(lut, gamma, false);
 
-	if (func->tf == TRANSFER_FUNCTION_LINEAR) {
-		/*
-		 * Color module doesn't like calculating regamma params
-		 * on top of a linear input. But degamma params can be used
-		 * instead to simulate this.
-		 */
-		gamma->type = GAMMA_CUSTOM;
-		res = mod_color_calculate_degamma_params(NULL, func,
-							gamma, true);
-	} else {
-		/*
-		 * Assume sRGB. The actual mapping will depend on whether the
-		 * input was legacy or not.
-		 */
-		gamma->type = GAMMA_CS_TFM_1D;
-		res = mod_color_calculate_regamma_params(func, gamma, false,
-							 has_rom, NULL, &cal_buffer);
-	}
+	/*
+	 * Color module doesn't like calculating regamma params
+	 * on top of a linear input. But degamma params can be used
+	 * instead to simulate this.
+	 */
+	gamma->type = GAMMA_CUSTOM;
+	res = mod_color_calculate_degamma_params(NULL, func, gamma, true);
 
 	dc_gamma_release(&gamma);
 
@@ -450,7 +438,7 @@ int amdgpu_dm_update_crtc_color_mgmt(struct dm_crtc_state *crtc)
 		stream->out_transfer_func->tf = TRANSFER_FUNCTION_LINEAR;
 
 		r = __set_output_tf(stream->out_transfer_func, regamma_lut,
-				    regamma_size, has_rom);
+				    regamma_size);
 		if (r)
 			return r;
 	} else {
-- 
2.35.1

[RFC PATCH v2 08/18] drm/amd/display: add comments to describe DM crtc color mgmt behavior

[Melissa Wen]

Describe some expected behavior of the AMD DM color mgmt programming.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
index 10a29d131424..b54ef1392895 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
@@ -428,12 +428,23 @@ int amdgpu_dm_update_crtc_color_mgmt(struct dm_crtc_state *crtc)
 		stream->out_transfer_func->type = TF_TYPE_DISTRIBUTED_POINTS;
 		stream->out_transfer_func->tf = TRANSFER_FUNCTION_SRGB;
 
+		/* Note: even if we pass has_rom as parameter here, we never
+		 * actually use ROM because the color module only takes the ROM
+		 * path if transfer_func->type == PREDEFINED.
+		 *
+		 * See more in mod_color_calculate_regamma_params()
+		 */
 		r = __set_legacy_tf(stream->out_transfer_func, regamma_lut,
 				    regamma_size, has_rom);
 		if (r)
 			return r;
 	} else if (has_regamma) {
-		/* If atomic regamma, CRTC RGM goes into RGM LUT. */
+		/* CRTC RGM goes into RGM LUT.
+		 *
+		 * Note: here there is no implicit sRGB regamma. We are using
+		 * degamma calculation from color module to calculate the curve
+		 * from a linear base.
+		 */
 		stream->out_transfer_func->type = TF_TYPE_DISTRIBUTED_POINTS;
 		stream->out_transfer_func->tf = TRANSFER_FUNCTION_LINEAR;
 
-- 
2.35.1

[RFC PATCH v2 09/18] drm/amd/display: encapsulate atomic regamma operation

[Melissa Wen]

We are introducing DRM 3D LUT property to DM color pipeline in the next
patch, but so far, only for atomic interface. By checking
.set_output_transfer_func in DC drivers with MPC 3D LUT support, we can
verify that regamma is only programmed when 3D LUT programming fails. As
a groundwork to introduce 3D LUT programming and better understand each
step, detach atomic regamma programming from the crtc colocr updating
code.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../amd/display/amdgpu_dm/amdgpu_dm_color.c   | 52 ++++++++++++-------
 1 file changed, 33 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
index b54ef1392895..54d95745f0f0 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
@@ -291,6 +291,36 @@ static int __set_output_tf(struct dc_transfer_func *func,
 	return res ? 0 : -ENOMEM;
 }
 
+static int amdgpu_dm_set_atomic_regamma(struct dc_stream_state *stream,
+					const struct drm_color_lut *regamma_lut,
+					uint32_t regamma_size)
+{
+	int ret = 0;
+
+	if (regamma_size) {
+		/* CRTC RGM goes into RGM LUT.
+		 *
+		 * Note: here there is no implicit sRGB regamma. We are using
+		 * degamma calculation from color module to calculate the curve
+		 * from a linear base.
+		 */
+		stream->out_transfer_func->type = TF_TYPE_DISTRIBUTED_POINTS;
+		stream->out_transfer_func->tf = TRANSFER_FUNCTION_LINEAR;
+
+		ret = __set_output_tf(stream->out_transfer_func, regamma_lut,
+				      regamma_size);
+	} else {
+		/*
+		 * No CRTC RGM means we can just put the block into bypass
+		 * since we don't have any plane level adjustments using it.
+		 */
+		stream->out_transfer_func->type = TF_TYPE_BYPASS;
+		stream->out_transfer_func->tf = TRANSFER_FUNCTION_LINEAR;
+	}
+
+	return ret;
+}
+
 /**
  * __set_input_tf - calculates the input transfer function based on expected
  * input space.
@@ -438,27 +468,11 @@ int amdgpu_dm_update_crtc_color_mgmt(struct dm_crtc_state *crtc)
 				    regamma_size, has_rom);
 		if (r)
 			return r;
-	} else if (has_regamma) {
-		/* CRTC RGM goes into RGM LUT.
-		 *
-		 * Note: here there is no implicit sRGB regamma. We are using
-		 * degamma calculation from color module to calculate the curve
-		 * from a linear base.
-		 */
-		stream->out_transfer_func->type = TF_TYPE_DISTRIBUTED_POINTS;
-		stream->out_transfer_func->tf = TRANSFER_FUNCTION_LINEAR;
-
-		r = __set_output_tf(stream->out_transfer_func, regamma_lut,
-				    regamma_size);
+	} else {
+		regamma_size = has_regamma ? regamma_size : 0;
+		r = amdgpu_dm_set_atomic_regamma(stream, regamma_lut, regamma_size);
 		if (r)
 			return r;
-	} else {
-		/*
-		 * No CRTC RGM means we can just put the block into bypass
-		 * since we don't have any plane level adjustments using it.
-		 */
-		stream->out_transfer_func->type = TF_TYPE_BYPASS;
-		stream->out_transfer_func->tf = TRANSFER_FUNCTION_LINEAR;
 	}
 
 	/*
-- 
2.35.1

[RFC PATCH v2 10/18] drm/amd/display: update lut3d and shaper lut to stream

[Melissa Wen]

It follows the same path of out_transfer_func for stream updates, since
shaper LUT and 3D LUT is programmed in funcs.set_output_transfer_func()
and this function is called in the atomic commit_tail when
update_flags.bits.out_tf is set.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 drivers/gpu/drm/amd/display/dc/core/dc.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c
index 2c18c8527079..88f1130c3b83 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
@@ -2562,7 +2562,7 @@ static enum surface_update_type check_update_surfaces_for_stream(
 			stream_update->integer_scaling_update)
 			su_flags->bits.scaling = 1;
 
-		if (stream_update->out_transfer_func)
+		if (stream_update->out_transfer_func || stream_update->lut3d_func)
 			su_flags->bits.out_tf = 1;
 
 		if (stream_update->abm_level)
@@ -2911,6 +2911,14 @@ static void copy_stream_update_to_stream(struct dc *dc,
 		       sizeof(struct dc_transfer_func_distributed_points));
 	}
 
+	if (update->func_shaper &&
+	    stream->func_shaper != update->func_shaper)
+		stream->func_shaper = update->func_shaper;
+
+	if (update->lut3d_func &&
+	    stream->lut3d_func != update->lut3d_func)
+		stream->lut3d_func = update->lut3d_func;
+
 	if (update->hdr_static_metadata)
 		stream->hdr_static_metadata = *update->hdr_static_metadata;
 
-- 
2.35.1

[RFC PATCH v2 11/18] drm/amd/display: handle MPC 3D LUT resources for a given context

[Melissa Wen]

In the original dc_acquire_release_mpc_3dlut(), only current ctx is
considered, which doesn't fit the steps for atomic checking new ctx.
Therefore, create a function to handle 3D LUT resource for a given
context, so that we can check resources availability in atomic_check
time and handle failures properly.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 drivers/gpu/drm/amd/display/dc/core/dc.c | 39 ++++++++++++++++++++++++
 drivers/gpu/drm/amd/display/dc/dc.h      |  8 +++++
 2 files changed, 47 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c
index 88f1130c3b83..76270d21286a 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
@@ -2085,6 +2085,45 @@ bool dc_acquire_release_mpc_3dlut(
 	return ret;
 }
 
+bool
+dc_acquire_release_mpc_3dlut_for_ctx(struct dc *dc,
+				     bool acquire,
+				     struct dc_state *state,
+				     struct dc_stream_state *stream,
+				     struct dc_3dlut **lut,
+				     struct dc_transfer_func **shaper)
+{
+	int pipe_idx;
+	bool ret = false;
+	bool found_pipe_idx = false;
+	const struct resource_pool *pool = dc->res_pool;
+	struct resource_context *res_ctx = &state->res_ctx;
+	int mpcc_id = 0;
+
+	if (pool && res_ctx) {
+		if (acquire) {
+			/*find pipe idx for the given stream*/
+			for (pipe_idx = 0; pipe_idx < pool->pipe_count; pipe_idx++) {
+				if (res_ctx->pipe_ctx[pipe_idx].stream == stream) {
+					found_pipe_idx = true;
+					mpcc_id = res_ctx->pipe_ctx[pipe_idx].plane_res.hubp->inst;
+					break;
+				}
+			}
+		} else
+			found_pipe_idx = true;/*for release pipe_idx is not required*/
+
+		if (found_pipe_idx) {
+			if (acquire && pool->funcs->acquire_post_bldn_3dlut)
+				ret = pool->funcs->acquire_post_bldn_3dlut(res_ctx, pool, mpcc_id, lut, shaper);
+			else if (!acquire && pool->funcs->release_post_bldn_3dlut)
+				ret = pool->funcs->release_post_bldn_3dlut(res_ctx, pool, lut, shaper);
+		}
+	}
+	return ret;
+}
+
+
 static bool is_flip_pending_in_pipes(struct dc *dc, struct dc_state *context)
 {
 	int i;
diff --git a/drivers/gpu/drm/amd/display/dc/dc.h b/drivers/gpu/drm/amd/display/dc/dc.h
index 72963617553e..a5abf7f308c3 100644
--- a/drivers/gpu/drm/amd/display/dc/dc.h
+++ b/drivers/gpu/drm/amd/display/dc/dc.h
@@ -1345,6 +1345,14 @@ bool dc_acquire_release_mpc_3dlut(
 		struct dc_3dlut **lut,
 		struct dc_transfer_func **shaper);
 
+bool
+dc_acquire_release_mpc_3dlut_for_ctx(struct dc *dc,
+				     bool acquire,
+				     struct dc_state *state,
+				     struct dc_stream_state *stream,
+				     struct dc_3dlut **lut,
+				     struct dc_transfer_func **shaper);
+
 void dc_resource_state_copy_construct(
 		const struct dc_state *src_ctx,
 		struct dc_state *dst_ctx);
-- 
2.35.1

[RFC PATCH v2 12/18] drm/amd/display: acquire/release 3D LUT resources for ctx on DCN301

[Melissa Wen]

Acquire and release 3D LUT and shaper LUT every time we create/remove a
new ctx and add/remove stream to/from it. 3D LUT acquire/release can
fail and therefore we should check its availability during atomic check
considering the new context created not the current one.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../amd/display/dc/dcn301/dcn301_resource.c   | 47 ++++++++++++++++++-
 1 file changed, 45 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/dcn301/dcn301_resource.c b/drivers/gpu/drm/amd/display/dc/dcn301/dcn301_resource.c
index ee62ae3eb98f..5bae0972bd5e 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn301/dcn301_resource.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn301/dcn301_resource.c
@@ -1260,6 +1260,49 @@ static struct display_stream_compressor *dcn301_dsc_create(
 	return &dsc->base;
 }
 
+static enum dc_status
+dcn301_add_stream_to_ctx(struct dc *dc,
+			 struct dc_state *new_ctx,
+			 struct dc_stream_state *dc_stream)
+{
+	enum dc_status result = DC_ERROR_UNEXPECTED;
+	struct dc_3dlut *lut3d_func_new = NULL;
+	struct dc_transfer_func *func_shaper_new = NULL;
+
+	result = dcn20_add_stream_to_ctx(dc, new_ctx, dc_stream);
+	if (result != DC_OK)
+		return result;
+
+	if (!dc_acquire_release_mpc_3dlut_for_ctx(dc, true, new_ctx, dc_stream,
+						  &lut3d_func_new, &func_shaper_new))
+		return DC_ERROR_UNEXPECTED;
+
+	dc_stream->lut3d_func = lut3d_func_new;
+	dc_stream->func_shaper = func_shaper_new;
+
+	return DC_OK;
+}
+
+static enum dc_status
+dcn301_remove_stream_from_ctx(struct dc *dc,
+			      struct dc_state *new_ctx,
+			      struct dc_stream_state *dc_stream)
+{
+	struct dc_3dlut *lut3d_func;
+	struct dc_transfer_func *func_shaper;
+
+	lut3d_func = (struct dc_3dlut *)dc_stream->lut3d_func;
+	func_shaper = (struct dc_transfer_func *)dc_stream->func_shaper;
+
+	if (!dc_acquire_release_mpc_3dlut_for_ctx(dc, false, new_ctx, dc_stream,
+						  &lut3d_func, &func_shaper))
+		return DC_ERROR_UNEXPECTED;
+
+	dc_stream->lut3d_func = lut3d_func;
+	dc_stream->func_shaper = func_shaper;
+
+	return dcn20_remove_stream_from_ctx(dc, new_ctx, dc_stream);
+}
 
 static void dcn301_destroy_resource_pool(struct resource_pool **pool)
 {
@@ -1406,9 +1449,9 @@ static struct resource_funcs dcn301_res_pool_funcs = {
 	.update_soc_for_wm_a = dcn30_update_soc_for_wm_a,
 	.populate_dml_pipes = dcn30_populate_dml_pipes_from_context,
 	.acquire_idle_pipe_for_layer = dcn20_acquire_idle_pipe_for_layer,
-	.add_stream_to_ctx = dcn30_add_stream_to_ctx,
+	.add_stream_to_ctx = dcn301_add_stream_to_ctx,
 	.add_dsc_to_stream_resource = dcn20_add_dsc_to_stream_resource,
-	.remove_stream_from_ctx = dcn20_remove_stream_from_ctx,
+	.remove_stream_from_ctx = dcn301_remove_stream_from_ctx,
 	.populate_dml_writeback_from_context = dcn30_populate_dml_writeback_from_context,
 	.set_mcif_arb_params = dcn30_set_mcif_arb_params,
 	.find_first_free_match_stream_enc_for_link = dcn10_find_first_free_match_stream_enc_for_link,
-- 
2.35.1

[RFC PATCH v2 13/18] drm/amd/display: Define 3D LUT struct for HDR planes

[Melissa Wen]

From: Alex Hung <alex.hung@amd.com>

Add a 3D LUT mode supported by amdgpu driver.

Note: A patchset "IGT tests for pre-blending 3D LUT interfaces" for this
proposal is sent to IGT mailing list.

Signed-off-by: Alex Hung <alex.hung@amd.com>
---
 .../gpu/drm/amd/display/modules/color/color_gamma.h  | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/modules/color/color_gamma.h b/drivers/gpu/drm/amd/display/modules/color/color_gamma.h
index 2893abf48208..8e159b1eb9c6 100644
--- a/drivers/gpu/drm/amd/display/modules/color/color_gamma.h
+++ b/drivers/gpu/drm/amd/display/modules/color/color_gamma.h
@@ -27,6 +27,7 @@
 #define COLOR_MOD_COLOR_GAMMA_H_
 
 #include "color_table.h"
+#include <drm/drm_fourcc.h>
 
 struct dc_transfer_func;
 struct dc_gamma;
@@ -35,6 +36,17 @@ struct dc_rgb_fixed;
 struct dc_color_caps;
 enum dc_transfer_func_predefined;
 
+/*
+ * 3D LUT mode for 17x17x17 LUT and 12 bits of color depth
+ */
+static const struct drm_mode_lut3d_mode lut3d_mode_17_12bit = {
+	.lut_size = 17,
+	.lut_stride = {17, 17, 18},
+	.bit_depth = 12,
+	.color_format = DRM_FORMAT_XRGB16161616,
+	.flags = 0,
+};
+
 /* For SetRegamma ADL interface support
  * Must match escape type
  */
-- 
2.35.1

[RFC PATCH v2 14/18] drm/amd/display: expand array of supported 3D LUT modes

[Melissa Wen]

AMD MPC block support 3D LUTs of dimensions 17 and 9, and also bit
depth 12 and 10, therefore, advertise them to the userspace.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../amd/display/modules/color/color_gamma.h   | 31 +++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/modules/color/color_gamma.h b/drivers/gpu/drm/amd/display/modules/color/color_gamma.h
index 8e159b1eb9c6..69b9a1aa6dd4 100644
--- a/drivers/gpu/drm/amd/display/modules/color/color_gamma.h
+++ b/drivers/gpu/drm/amd/display/modules/color/color_gamma.h
@@ -47,6 +47,37 @@ static const struct drm_mode_lut3d_mode lut3d_mode_17_12bit = {
 	.flags = 0,
 };
 
+static const struct drm_mode_lut3d_mode amdgpu_lut3d_modes[] = {
+	{
+		.lut_size = 17,
+		.lut_stride = {17, 17, 18},
+		.bit_depth = 12,
+		.color_format = DRM_FORMAT_XRGB16161616,
+		.flags = 0,
+	},
+	{
+		.lut_size = 17,
+		.lut_stride = {17, 17, 18},
+		.bit_depth = 10,
+		.color_format = DRM_FORMAT_XRGB16161616,
+		.flags = 0,
+	},
+	{
+		.lut_size = 9,
+		.lut_stride = {9, 9, 10},
+		.bit_depth = 12,
+		.color_format = DRM_FORMAT_XRGB16161616,
+		.flags = 0,
+	},
+	{
+		.lut_size = 9,
+		.lut_stride = {9, 9, 10},
+		.bit_depth = 10,
+		.color_format = DRM_FORMAT_XRGB16161616,
+		.flags = 0,
+	},
+};
+
 /* For SetRegamma ADL interface support
  * Must match escape type
  */
-- 
2.35.1

[RFC PATCH v2 15/18] drm/amd/display: enable 3D-LUT DRM properties if supported

[Melissa Wen]

Enable DRM crtc properties related to 3D LUT resources (shaper LUT, 3D
LUT and 3D LUT modes) if it's supported by DCN HW, that means DCN
families 3.0+

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  1 +
 .../amd/display/amdgpu_dm/amdgpu_dm_color.c   | 25 +++++++++++++++++++
 .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c    |  2 ++
 3 files changed, 28 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index abbbb3813c1e..6f04719d0c1f 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -786,6 +786,7 @@ void amdgpu_dm_trigger_timing_sync(struct drm_device *dev);
 
 void amdgpu_dm_init_color_mod(void);
 int amdgpu_dm_verify_lut_sizes(const struct drm_crtc_state *crtc_state);
+void amdgpu_dm_enable_lut3d_prop(struct amdgpu_display_manager *dm, struct drm_crtc *crtc);
 int amdgpu_dm_update_crtc_color_mgmt(struct dm_crtc_state *crtc);
 int amdgpu_dm_update_plane_color_mgmt(struct dm_crtc_state *crtc,
 				      struct dc_plane_state *dc_plane_state);
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
index 54d95745f0f0..c547957acd73 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
@@ -388,6 +388,31 @@ int amdgpu_dm_verify_lut_sizes(const struct drm_crtc_state *crtc_state)
 	return 0;
 }
 
+static bool has_mpc_lut3d_caps(struct amdgpu_display_manager *dm)
+{
+	return dm->dc->caps.color.mpc.num_3dluts ? true : false;
+}
+
+/**
+ * amdgpu_dm_enable_lut3d_prop - enable 3D LUT DRM props if HW supports
+ * @crtc: DRM crtc
+ * @dm: amdgpu display manager
+ */
+void amdgpu_dm_enable_lut3d_prop(struct amdgpu_display_manager *dm, struct drm_crtc *crtc)
+{
+	int res;
+
+	if (!has_mpc_lut3d_caps(dm))
+		return;
+
+	res = drm_crtc_create_lut3d_mode_property(crtc, amdgpu_lut3d_modes,
+						  ARRAY_SIZE(amdgpu_lut3d_modes));
+	if (res)
+		drm_dbg(crtc->dev, "CRTC init: Failed to create LUT 3D mode properties\n");
+
+	drm_crtc_enable_lut3d(crtc, MAX_COLOR_LUT_ENTRIES, true);
+}
+
 /**
  * amdgpu_dm_update_crtc_color_mgmt: Maps DRM color management to DC stream.
  * @crtc: amdgpu_dm crtc state
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
index 22125daf9dcf..96494f72a6f4 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
@@ -461,6 +461,8 @@ int amdgpu_dm_crtc_init(struct amdgpu_display_manager *dm,
 	drm_crtc_enable_color_mgmt(&acrtc->base, is_dcn ? MAX_COLOR_LUT_ENTRIES : 0,
 				   true, MAX_COLOR_LUT_ENTRIES);
 
+	amdgpu_dm_enable_lut3d_prop(dm, &acrtc->base);
+
 	drm_mode_crtc_set_gamma_size(&acrtc->base, MAX_COLOR_LEGACY_LUT_ENTRIES);
 
 	return 0;
-- 
2.35.1

[RFC PATCH v2 16/18] drm/amd/display: add user 3D LUT support to the amdgpu_dm color pipeline

[Melissa Wen]

Map DRM 3D LUT in the atomic color mgmt pipeline to DC. 3D LUT works
better in a non-linear color space, therefore using a degamma to
linearize the input space may produce unexpected results. The next patch
introduces shaper LUT support that can be used to delinearize the color
space before applying 3D LUT conversion.

Note that there is no implicit sRGB degamma/regamma in the current
implementation for DRM atomic color mgmt. Atomic degamma/regamma 1D LUT
is applied considering a linear base.

For reference, see IGT test amdgpu/amd_color and commit
cf020d49b3c4 ("drm/amd/display: Rework CRTC color management")

dc_acquire_release_mpc_3dlut initializes the bits required to program
3DLUT in DC MPC hw block, that is applied by set_output_transfer_func().
I still need to double check the timing to acquire and release shaper
and 3D LUTs from the resource pool.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |   6 +
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   2 +
 .../amd/display/amdgpu_dm/amdgpu_dm_color.c   | 189 ++++++++++++++++++
 3 files changed, 197 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index b8638f0508b0..7aa41dd2143b 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -9704,6 +9704,12 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
 			goto fail;
 		}
 
+		ret = amdgpu_dm_verify_lut3d_size(adev, new_crtc_state);
+		if (ret) {
+			DRM_DEBUG_DRIVER("amdgpu_dm_verify_lut_sizes() failed\n");
+			goto fail;
+		}
+
 		if (!new_crtc_state->enable)
 			continue;
 
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index 6f04719d0c1f..59ab1b8f7b05 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -786,6 +786,8 @@ void amdgpu_dm_trigger_timing_sync(struct drm_device *dev);
 
 void amdgpu_dm_init_color_mod(void);
 int amdgpu_dm_verify_lut_sizes(const struct drm_crtc_state *crtc_state);
+int amdgpu_dm_verify_lut3d_size(struct amdgpu_device *adev,
+				const struct drm_crtc_state *crtc_state);
 void amdgpu_dm_enable_lut3d_prop(struct amdgpu_display_manager *dm, struct drm_crtc *crtc);
 int amdgpu_dm_update_crtc_color_mgmt(struct dm_crtc_state *crtc);
 int amdgpu_dm_update_plane_color_mgmt(struct dm_crtc_state *crtc,
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
index c547957acd73..0fb1244c8aef 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
@@ -413,6 +413,175 @@ void amdgpu_dm_enable_lut3d_prop(struct amdgpu_display_manager *dm, struct drm_c
 	drm_crtc_enable_lut3d(crtc, MAX_COLOR_LUT_ENTRIES, true);
 }
 
+static void __to_dc_lut3d_color(struct dc_rgb *rgb,
+				const struct drm_color_lut lut,
+				int bit_precision)
+{
+	rgb->red = drm_color_lut_extract(lut.red, bit_precision);
+	rgb->green = drm_color_lut_extract(lut.green, bit_precision);
+	rgb->blue  = drm_color_lut_extract(lut.blue, bit_precision);
+}
+
+static void __drm_3dlut_to_dc_3dlut(const struct drm_color_lut *lut,
+				    uint32_t lut3d_size,
+				    struct tetrahedral_params *params,
+				    bool use_tetrahedral_9,
+				    int bit_depth)
+{
+	struct dc_rgb *lut0;
+	struct dc_rgb *lut1;
+	struct dc_rgb *lut2;
+	struct dc_rgb *lut3;
+	int lut_i, i;
+
+
+	if (use_tetrahedral_9) {
+		lut0 = params->tetrahedral_9.lut0;
+		lut1 = params->tetrahedral_9.lut1;
+		lut2 = params->tetrahedral_9.lut2;
+		lut3 = params->tetrahedral_9.lut3;
+	} else {
+		lut0 = params->tetrahedral_17.lut0;
+		lut1 = params->tetrahedral_17.lut1;
+		lut2 = params->tetrahedral_17.lut2;
+		lut3 = params->tetrahedral_17.lut3;
+	}
+
+	for (lut_i = 0, i = 0; i < lut3d_size - 4; lut_i++, i += 4) {
+		/* We should consider the 3dlut RGB values are distributed
+		 * along four arrays lut0-3 where the first sizes 1229 and the
+		 * other 1228. The bit depth supported for 3dlut channel is
+		 * 12-bit, but DC also supports 10-bit.
+		 *
+		 * TODO: improve color pipeline API to enable the userspace set
+		 * bit depth and 3D LUT size/stride, as specified by VA-API.
+		 */
+		__to_dc_lut3d_color(&lut0[lut_i], lut[i], bit_depth);
+		__to_dc_lut3d_color(&lut1[lut_i], lut[i + 1], bit_depth);
+		__to_dc_lut3d_color(&lut2[lut_i], lut[i + 2], bit_depth);
+		__to_dc_lut3d_color(&lut3[lut_i], lut[i + 3], bit_depth);
+	}
+	/* lut0 has 1229 points (lut_size/4 + 1) */
+	__to_dc_lut3d_color(&lut0[lut_i], lut[i], bit_depth);
+}
+
+/* amdgpu_dm_atomic_lut3d - set DRM 3D LUT to DC stream
+ * @stream: DC stream state to set shaper LUT and 3D LUT
+ * @drm_lut3d: DRM CRTC (user) 3D LUT
+ * @drm_lut3d_size: size of 3D LUT
+ * @lut3d: DC 3D LUT
+ *
+ * Map DRM CRTC 3D LUT to DC 3D LUT and all necessary bits to program it
+ * on DCN MPC accordingly.
+ */
+static void amdgpu_dm_atomic_lut3d(struct dc_stream_state *stream,
+				   const struct drm_color_lut *drm_lut,
+				   uint32_t drm_lut3d_size,
+				   const struct drm_mode_lut3d_mode *mode,
+				   struct dc_3dlut *lut)
+{
+	int size = mode->lut_size * mode->lut_size * mode->lut_size;
+
+	ASSERT(lut && drm_lut3d_size == size);
+
+	/* Stride and bit depth is not programmable by API so far. Therefore,
+	 * only supports 17x17x17 3D LUT with 12-bit.
+	 */
+	lut->lut_3d.use_tetrahedral_9 = mode->lut_size == 9;
+	lut->lut_3d.use_12bits = mode->bit_depth == 12;
+	lut->state.bits.initialized = 1;
+
+	__drm_3dlut_to_dc_3dlut(drm_lut, size, &lut->lut_3d,
+				lut->lut_3d.use_tetrahedral_9, mode->bit_depth);
+
+	stream->lut3d_func = lut;
+}
+
+/* amdgpu_dm_atomic_shaper_lut3d - set DRM CRTC shaper LUT and 3D LUT to DC
+ * interface
+ * @dc: Display Core control structure
+ * @stream: DC stream state to set shaper LUT and 3D LUT
+ * @drm_lut3d: DRM CRTC (user) 3D LUT
+ * @drm_lut3d_size: size of 3D LUT
+ *
+ * Returns:
+ * 0 on success.
+ */
+static int amdgpu_dm_atomic_shaper_lut3d(struct dc *dc,
+					 struct dc_stream_state *stream,
+					 const struct drm_color_lut *drm_lut3d,
+					 uint32_t drm_lut3d_size,
+					 const struct drm_mode_lut3d_mode *mode)
+{
+	struct dc_3dlut *lut3d_func_new;
+	struct dc_transfer_func *func_shaper_new;
+
+	lut3d_func_new = (struct dc_3dlut *) stream->lut3d_func;
+	func_shaper_new = (struct dc_transfer_func *) stream->func_shaper;
+
+	/* We don't get DRM shaper LUT yet. We assume the input color space is
+	 * already delinearized, so we don't need a shaper LUT and we can just
+	 * BYPASS.
+	 */
+	func_shaper_new->type = TF_TYPE_BYPASS;
+	func_shaper_new->tf = TRANSFER_FUNCTION_LINEAR;
+	stream->func_shaper = func_shaper_new;
+
+	amdgpu_dm_atomic_lut3d(stream, drm_lut3d, drm_lut3d_size,
+			       mode, lut3d_func_new);
+
+	return 0;
+}
+
+static const struct drm_mode_lut3d_mode *
+get_lut3d_mode(struct amdgpu_device *adev,
+	       const struct drm_crtc_state *crtc_state)
+{
+	struct drm_property_blob *blob;
+
+	if (!has_mpc_lut3d_caps(&adev->dm))
+		return NULL;
+
+	blob =  drm_property_lookup_blob(crtc_state->state->dev,
+					 crtc_state->lut3d_mode);
+
+	return blob ? (const struct drm_mode_lut3d_mode *)blob->data : NULL;
+}
+
+/**
+ * amdgpu_dm_verify_lut3d_size - verifies if 3D LUT is supported and if DRM 3D
+ * LUT matches the hw supported size
+ * @adev: amdgpu device
+ * @crtc_state: the DRM CRTC state
+ *
+ * Verifies if post-blending (MPC) 3D LUT is supported by the HW (DCN 3.0 or
+ * newer) and if the DRM 3D LUT matches the supported size.
+ *
+ * Returns:
+ * 0 on success. -EINVAL if lut size are invalid.
+ */
+int amdgpu_dm_verify_lut3d_size(struct amdgpu_device *adev,
+				const struct drm_crtc_state *crtc_state)
+{
+	const struct drm_color_lut *lut3d = NULL;
+	const struct drm_mode_lut3d_mode *mode;
+	uint32_t exp_size, size;
+
+	mode = get_lut3d_mode(adev, crtc_state);
+	exp_size = mode ? mode->lut_size * mode->lut_size * mode->lut_size : 0;
+
+	lut3d = __extract_blob_lut(crtc_state->lut3d, &size);
+
+	if (lut3d && size != exp_size) {
+		DRM_DEBUG_DRIVER("Invalid Gamma 3D LUT size. Should be %u but got %u.\n",
+				 exp_size, size);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+
 /**
  * amdgpu_dm_update_crtc_color_mgmt: Maps DRM color management to DC stream.
  * @crtc: amdgpu_dm crtc state
@@ -442,8 +611,11 @@ int amdgpu_dm_update_crtc_color_mgmt(struct dm_crtc_state *crtc)
 	bool has_rom = adev->asic_type <= CHIP_RAVEN;
 	struct drm_color_ctm *ctm = NULL;
 	const struct drm_color_lut *degamma_lut, *regamma_lut;
+	const struct drm_color_lut *lut3d;
 	uint32_t degamma_size, regamma_size;
+	uint32_t lut3d_size;
 	bool has_regamma, has_degamma;
+	bool has_lut3d;
 	bool is_legacy;
 	int r;
 
@@ -451,12 +623,19 @@ int amdgpu_dm_update_crtc_color_mgmt(struct dm_crtc_state *crtc)
 	if (r)
 		return r;
 
+	r =  amdgpu_dm_verify_lut3d_size(adev, &crtc->base);
+	if (r)
+		return r;
+
 	degamma_lut = __extract_blob_lut(crtc->base.degamma_lut, &degamma_size);
+	lut3d = __extract_blob_lut(crtc->base.lut3d, &lut3d_size);
 	regamma_lut = __extract_blob_lut(crtc->base.gamma_lut, &regamma_size);
 
 	has_degamma =
 		degamma_lut && !__is_lut_linear(degamma_lut, degamma_size);
 
+	has_lut3d = lut3d != NULL;
+
 	has_regamma =
 		regamma_lut && !__is_lut_linear(regamma_lut, regamma_size);
 
@@ -494,6 +673,16 @@ int amdgpu_dm_update_crtc_color_mgmt(struct dm_crtc_state *crtc)
 		if (r)
 			return r;
 	} else {
+		if (has_lut3d) {
+			r = amdgpu_dm_atomic_shaper_lut3d(adev->dm.dc, stream, lut3d, lut3d_size,
+							  get_lut3d_mode(adev, &crtc->base));
+			if (r)
+				return r;
+		}
+		/* Note: OGAM is disabled if 3D LUT is successfully programmed.
+		 * See params and set_output_gamma in
+		 * dcn30_set_output_transfer_func()
+		 */
 		regamma_size = has_regamma ? regamma_size : 0;
 		r = amdgpu_dm_set_atomic_regamma(stream, regamma_lut, regamma_size);
 		if (r)
-- 
2.35.1

[RFC PATCH v2 17/18] drm/amd/display: decouple steps to reuse in shaper LUT support

[Melissa Wen]

Decouple steps of shaper LUT setup and LUT size validation according to
HW caps as a preparation for shaper LUT support.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../amd/display/amdgpu_dm/amdgpu_dm_color.c   | 22 ++++++++++++-------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
index 0fb1244c8aef..8a930f9bce60 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
@@ -497,6 +497,19 @@ static void amdgpu_dm_atomic_lut3d(struct dc_stream_state *stream,
 	stream->lut3d_func = lut;
 }
 
+static int amdgpu_dm_atomic_shaper_lut(struct dc_stream_state *stream,
+				       struct dc_transfer_func *func_shaper_new)
+{
+	/* We don't get DRM shaper LUT yet. We assume the input color space is already
+	 * delinearized, so we don't need a shaper LUT and we can just BYPASS
+	 */
+	func_shaper_new->type = TF_TYPE_BYPASS;
+	func_shaper_new->tf = TRANSFER_FUNCTION_LINEAR;
+	stream->func_shaper = func_shaper_new;
+
+	return 0;
+}
+
 /* amdgpu_dm_atomic_shaper_lut3d - set DRM CRTC shaper LUT and 3D LUT to DC
  * interface
  * @dc: Display Core control structure
@@ -519,18 +532,11 @@ static int amdgpu_dm_atomic_shaper_lut3d(struct dc *dc,
 	lut3d_func_new = (struct dc_3dlut *) stream->lut3d_func;
 	func_shaper_new = (struct dc_transfer_func *) stream->func_shaper;
 
-	/* We don't get DRM shaper LUT yet. We assume the input color space is
-	 * already delinearized, so we don't need a shaper LUT and we can just
-	 * BYPASS.
-	 */
-	func_shaper_new->type = TF_TYPE_BYPASS;
-	func_shaper_new->tf = TRANSFER_FUNCTION_LINEAR;
-	stream->func_shaper = func_shaper_new;
 
 	amdgpu_dm_atomic_lut3d(stream, drm_lut3d, drm_lut3d_size,
 			       mode, lut3d_func_new);
 
-	return 0;
+	return amdgpu_dm_atomic_shaper_lut(stream, func_shaper_new);
 }
 
 static const struct drm_mode_lut3d_mode *
-- 
2.35.1

[RFC PATCH v2 18/18] drm/amd/display: add user shaper LUT support to amdgpu_dm color pipeline

[Melissa Wen]

Now, we can use shaper LUT to delinearize and/or normalize the color
space for a more efficient 3D LUT support (so far, only for DRM atomic
color mgmt). If a degamma 1D LUT is passed to linearize the color space,
a custom shaper 1D LUT can be used before applying 3D LUT.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../amd/display/amdgpu_dm/amdgpu_dm_color.c   | 95 ++++++++++++++++---
 1 file changed, 83 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
index 8a930f9bce60..81b20ac9ff19 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
@@ -497,14 +497,62 @@ static void amdgpu_dm_atomic_lut3d(struct dc_stream_state *stream,
 	stream->lut3d_func = lut;
 }
 
+static int __set_func_shaper(struct dc_transfer_func *shaper_func,
+			     const struct drm_color_lut *lut, uint32_t lut_size)
+{
+	struct dc_gamma *gamma = NULL;
+	struct calculate_buffer cal_buffer = {0};
+	bool res;
+
+	ASSERT(lut && lut_size == MAX_COLOR_LUT_ENTRIES);
+
+	cal_buffer.buffer_index = -1;
+
+	gamma = dc_create_gamma();
+	if (!gamma)
+		return -ENOMEM;
+
+	gamma->num_entries = lut_size;
+	__drm_lut_to_dc_gamma(lut, gamma, false);
+
+	/*
+	 * Color module doesn't like calculating gamma params
+	 * on top of a linear input. But degamma params can be used
+	 * instead to simulate this.
+	 */
+	gamma->type = GAMMA_CUSTOM;
+	res = mod_color_calculate_degamma_params(NULL, shaper_func, gamma, true);
+
+	dc_gamma_release(&gamma);
+
+	return res ? 0 : -ENOMEM;
+}
+
 static int amdgpu_dm_atomic_shaper_lut(struct dc_stream_state *stream,
+				       const struct drm_color_lut *shaper_lut,
+				       uint32_t shaper_size,
 				       struct dc_transfer_func *func_shaper_new)
 {
-	/* We don't get DRM shaper LUT yet. We assume the input color space is already
+	/* If no DRM shaper LUT, we assume the input color space is already
 	 * delinearized, so we don't need a shaper LUT and we can just BYPASS
 	 */
-	func_shaper_new->type = TF_TYPE_BYPASS;
-	func_shaper_new->tf = TRANSFER_FUNCTION_LINEAR;
+	if (!shaper_size) {
+		func_shaper_new->type = TF_TYPE_BYPASS;
+		func_shaper_new->tf = TRANSFER_FUNCTION_LINEAR;
+	} else {
+		int r;
+
+		/* If DRM shaper LUT is set, we assume a linear color space
+		 * (linearized by DRM degamma 1D LUT or not)
+		 */
+		func_shaper_new->type = TF_TYPE_DISTRIBUTED_POINTS;
+		func_shaper_new->tf = TRANSFER_FUNCTION_LINEAR;
+
+		r = __set_func_shaper(func_shaper_new, shaper_lut, shaper_size);
+		if (r)
+			return r;
+	}
+
 	stream->func_shaper = func_shaper_new;
 
 	return 0;
@@ -514,6 +562,8 @@ static int amdgpu_dm_atomic_shaper_lut(struct dc_stream_state *stream,
  * interface
  * @dc: Display Core control structure
  * @stream: DC stream state to set shaper LUT and 3D LUT
+ * @drm_shaper_lut: DRM CRTC (user) shaper LUT
+ * @drm_shaper_size: size of shaper LUT
  * @drm_lut3d: DRM CRTC (user) 3D LUT
  * @drm_lut3d_size: size of 3D LUT
  *
@@ -522,6 +572,8 @@ static int amdgpu_dm_atomic_shaper_lut(struct dc_stream_state *stream,
  */
 static int amdgpu_dm_atomic_shaper_lut3d(struct dc *dc,
 					 struct dc_stream_state *stream,
+					 const struct drm_color_lut *drm_shaper_lut,
+					 uint32_t drm_shaper_size,
 					 const struct drm_color_lut *drm_lut3d,
 					 uint32_t drm_lut3d_size,
 					 const struct drm_mode_lut3d_mode *mode)
@@ -532,11 +584,11 @@ static int amdgpu_dm_atomic_shaper_lut3d(struct dc *dc,
 	lut3d_func_new = (struct dc_3dlut *) stream->lut3d_func;
 	func_shaper_new = (struct dc_transfer_func *) stream->func_shaper;
 
-
 	amdgpu_dm_atomic_lut3d(stream, drm_lut3d, drm_lut3d_size,
 			       mode, lut3d_func_new);
 
-	return amdgpu_dm_atomic_shaper_lut(stream, func_shaper_new);
+	return amdgpu_dm_atomic_shaper_lut(stream, drm_shaper_lut,
+					   drm_shaper_size, func_shaper_new);
 }
 
 static const struct drm_mode_lut3d_mode *
@@ -569,13 +621,23 @@ get_lut3d_mode(struct amdgpu_device *adev,
 int amdgpu_dm_verify_lut3d_size(struct amdgpu_device *adev,
 				const struct drm_crtc_state *crtc_state)
 {
-	const struct drm_color_lut *lut3d = NULL;
+	const struct drm_color_lut *shaper = NULL, *lut3d = NULL;
 	const struct drm_mode_lut3d_mode *mode;
 	uint32_t exp_size, size;
 
+	/* shaper LUT is only available if 3D LUT color caps*/
+	exp_size = has_mpc_lut3d_caps(&adev->dm) ? MAX_COLOR_LUT_ENTRIES : 0;
+	shaper = __extract_blob_lut(crtc_state->shaper_lut, &size);
+
+	if (shaper && size != exp_size) {
+		DRM_DEBUG_DRIVER(
+			"Invalid Shaper LUT size. Should be %u but got %u.\n",
+			exp_size, size);
+		return -EINVAL;
+	}
+
 	mode = get_lut3d_mode(adev, crtc_state);
 	exp_size = mode ? mode->lut_size * mode->lut_size * mode->lut_size : 0;
-
 	lut3d = __extract_blob_lut(crtc_state->lut3d, &size);
 
 	if (lut3d && size != exp_size) {
@@ -617,11 +679,11 @@ int amdgpu_dm_update_crtc_color_mgmt(struct dm_crtc_state *crtc)
 	bool has_rom = adev->asic_type <= CHIP_RAVEN;
 	struct drm_color_ctm *ctm = NULL;
 	const struct drm_color_lut *degamma_lut, *regamma_lut;
-	const struct drm_color_lut *lut3d;
+	const struct drm_color_lut *shaper_lut, *lut3d;
 	uint32_t degamma_size, regamma_size;
-	uint32_t lut3d_size;
+	uint32_t lut3d_size, shaper_size;
 	bool has_regamma, has_degamma;
-	bool has_lut3d;
+	bool has_lut3d, has_shaper_lut;
 	bool is_legacy;
 	int r;
 
@@ -634,12 +696,14 @@ int amdgpu_dm_update_crtc_color_mgmt(struct dm_crtc_state *crtc)
 		return r;
 
 	degamma_lut = __extract_blob_lut(crtc->base.degamma_lut, &degamma_size);
+	shaper_lut = __extract_blob_lut(crtc->base.shaper_lut, &shaper_size);
 	lut3d = __extract_blob_lut(crtc->base.lut3d, &lut3d_size);
 	regamma_lut = __extract_blob_lut(crtc->base.gamma_lut, &regamma_size);
 
 	has_degamma =
 		degamma_lut && !__is_lut_linear(degamma_lut, degamma_size);
 
+	has_shaper_lut = shaper_lut != NULL;
 	has_lut3d = lut3d != NULL;
 
 	has_regamma =
@@ -680,10 +744,17 @@ int amdgpu_dm_update_crtc_color_mgmt(struct dm_crtc_state *crtc)
 			return r;
 	} else {
 		if (has_lut3d) {
-			r = amdgpu_dm_atomic_shaper_lut3d(adev->dm.dc, stream, lut3d, lut3d_size,
+			/* enable 3D LUT only for DRM atomic color mgmt */
+			shaper_size = has_shaper_lut ? shaper_size : 0;
+
+			r = amdgpu_dm_atomic_shaper_lut3d(adev->dm.dc, stream,
+							  shaper_lut, shaper_size,
+							  lut3d, lut3d_size,
 							  get_lut3d_mode(adev, &crtc->base));
-			if (r)
+			if (r) {
+				DRM_DEBUG_DRIVER("Failed to set shaper and 3D LUT\n");
 				return r;
+			}
 		}
 		/* Note: OGAM is disabled if 3D LUT is successfully programmed.
 		 * See params and set_output_gamma in
-- 
2.35.1

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

[Melissa Wen]

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

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
> 
> 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.

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: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Pekka Paalanen]

[-- 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 --]

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

[Melissa Wen]

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

On 01/31, Pekka Paalanen wrote:
> 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.
> 

Hi Pekka,

Thanks for your feedbacks and your time reviewing this proposal.

> 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.
> 
I see, this is also the reason I reuse here Alex Hung's proposal for
pre-blending API. I'll work on better documentation.

> 
> > > 
> > > 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.

Sorry, the patchset is here: https://lore.kernel.org/dri-devel/20230109143846.1966301-1-mwen@igalia.com/
In the next version, I won't forget cc'ing you at first.
> 
> +/*
> + * 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?

It cames from VA-API:
https://intel.github.io/libva/structVAProcFilterParameterBuffer3DLUT.html#a682756be15d09327ba725b74a863cbcc

In short, the reason is that lut_size is the valid points and is
the same for every dimensions, but lut_stride may vary.
> 
> What is the color_mode the comment is referring to?

It refers to FB color_mode/bpp. I'm not using it in post-blending 3D LUT
implementation (should I?), it cames from pre-blending use case.  Maybe
the main issue here is if reusing the pre-blending 3D LUT mode struct is
a good approach or better create a specific for post-blending.

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

It's the bit depth of the 3D LUT values, the same for every channels. In
the AMD case, it's supports 10-bit and 12-bit, for example.
> 
> 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?

I see. I'll work on explaining/documenting it better.
> 
> 
> + * “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.

ack
> 
> + * “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.

The driver advertises all supported modes (each combination of values)
in a array as a enum, userspace can check all accepted modes and set the
one that fits the user 3D LUT settings. I think it's possible to get the
idea from this IGT test:
https://gitlab.freedesktop.org/mwen/igt-gpu-tools/-/commit/8771f444c3dcd126d7590d5a9b1b0db9706bbf6e#ed5dbc960ac210e3fbacd2361fe0270709767aaa_205_205
> 
> 
> +	/**
> +	 * @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.
> 

You are right! My initial idea was to explain that it's possible for
other vendors color pipeline to fit this pipeline internally, if they
need a 1D LUT before the 3D LUT, but not the 1D LUT in the end.

> 
> 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.

Thank you again for your valuable comments. I'll address your comments
in a next version by better explaining all these points.

Melissa

> 
> 
> 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: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Pekka Paalanen]

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

On Thu, 9 Feb 2023 13:27:02 -0100
Melissa Wen <mwen@igalia.com> wrote:

> On 01/31, Pekka Paalanen wrote:
> > 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.
> >   
> 
> Hi Pekka,
> 
> Thanks for your feedbacks and your time reviewing this proposal.

No problem, and sorry it took so long!

I'm just finishing the catch-up with everything that happened during
winter holidays.

> > 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.
> >   
> I see, this is also the reason I reuse here Alex Hung's proposal for
> pre-blending API. I'll work on better documentation.
> 
> >   
> > > > 
> > > > 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.  
> 
> Sorry, the patchset is here: https://lore.kernel.org/dri-devel/20230109143846.1966301-1-mwen@igalia.com/
> In the next version, I won't forget cc'ing you at first.
> > 
> > +/*
> > + * 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?  
> 
> It cames from VA-API:
> https://intel.github.io/libva/structVAProcFilterParameterBuffer3DLUT.html#a682756be15d09327ba725b74a863cbcc
> 
> In short, the reason is that lut_size is the valid points and is
> the same for every dimensions, but lut_stride may vary.

Ok, so lut_size is what I would have guessed it to be. Just needs to be
doc'd - pretty much all my questions are lack of docs.

I see that lut_stride is supposed to be in points, and not bytes. That
is surprising. Are you sure it's good?

> > 
> > What is the color_mode the comment is referring to?  
> 
> It refers to FB color_mode/bpp. I'm not using it in post-blending 3D LUT
> implementation (should I?), it cames from pre-blending use case.  Maybe
> the main issue here is if reusing the pre-blending 3D LUT mode struct is
> a good approach or better create a specific for post-blending.

Sorry, I have no idea what FB color_mode is. I do not recall any such
thing in the KMS uAPI.


> > 
> > What is "number of bits of RGB"? Input precision? Output precision?
> > Integer or floating point?  
> 
> It's the bit depth of the 3D LUT values, the same for every channels. In
> the AMD case, it's supports 10-bit and 12-bit, for example.

Ok. So e.g. r5g6b5 is not a possible 3D LUT element type on any
hardware ever?

What exactly is the truncation the comment refers to?

It sounds like if input has higher precision than the LUT elements,
then "truncation" occurs. I can kind of see that, but I also think it
is a false characterisation. The LUT input precision affects the
precision of LUT indexing and the precision of interpolation between
the LUT elements. I would not expect those two precisions to be
truncated to the LUT element precision (but they could be truncated to
something else hardware specific). Instead, I do expect the
interpolation result to be truncated to the LUT output precision, which
probably is the same as the LUT element precision, but not necessarily.

Maybe the comment about truncation should simply be removed? The result
is obvious if we know the LUT input, element, and output precision, and
what exactly happens with the indexing and interpolation is probably
good enough to be left hardware-specific if it is difficult to describe
in generic terms across different hardware.

> > 
> > 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?  
> 
> I see. I'll work on explaining/documenting it better.
> > 
> > 
> > + * “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.  
> 
> ack
> > 
> > + * “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.  
> 
> The driver advertises all supported modes (each combination of values)
> in a array as a enum, userspace can check all accepted modes and set the
> one that fits the user 3D LUT settings. I think it's possible to get the
> idea from this IGT test:
> https://gitlab.freedesktop.org/mwen/igt-gpu-tools/-/commit/8771f444c3dcd126d7590d5a9b1b0db9706bbf6e#ed5dbc960ac210e3fbacd2361fe0270709767aaa_205_205
> > 

You lost me at "an array as an enum".

I understand there is a blob containing an array of struct
drm_mode_lut3d_mode. What I don't understand is that you say LUT3D_MODE
is an enum property. Where does the blob come from, then? What property
provides the blob?

Am I correct in guessing that the values of LUT3D_MODE enum property
are indices into the array in the blob, and that userspace will set it?
That sounds good to me, if it's the integer value of the enum. But enum
values also need string names, because that is how the values are
usually recognized, so what name strings will be used?

In that code example, I cannot see any connection between the array of
drm_mode_lut3d_mode entries, the 'name' string, and the actual data
generated for the LUT. They must all connect somehow, but it just
doesn't seem to be in the code. It is just hardcoding mode_lut3d[0],
and the data generator never sees even that - it gets the lut size, but
none of the other parameters. I cannot see that working across multiple
drivers.

Why is it setting a linear shaper LUT explicitly instead of unsetting
it? I mean this preparation:

shaper_linear = generate_table(data->shaper_lut_size, 1.0);

"Linear" actually means "identity", right? That's what people usually
write when they mean identity.

Surely it's not actually a constant curve? That would make the whole
test useless for proving the 3D LUT works.


> > 
> > +	/**
> > +	 * @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.
> >   
> 
> You are right! My initial idea was to explain that it's possible for
> other vendors color pipeline to fit this pipeline internally, if they
> need a 1D LUT before the 3D LUT, but not the 1D LUT in the end.
> 
> > 
> > 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.  
> 
> Thank you again for your valuable comments. I'll address your comments
> in a next version by better explaining all these points.

Thank you for working on this!
pq

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

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

[Harry Wentland]

On 2/10/23 04:28, Pekka Paalanen wrote:
> On Thu, 9 Feb 2023 13:27:02 -0100
> Melissa Wen <mwen@igalia.com> wrote:
> 
>> On 01/31, Pekka Paalanen wrote:
>>> 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.
>>>   
>>
>> Hi Pekka,
>>
>> Thanks for your feedbacks and your time reviewing this proposal.
> 
> No problem, and sorry it took so long!
> 
> I'm just finishing the catch-up with everything that happened during
> winter holidays.
> 
>>> 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.
>>>   
>> I see, this is also the reason I reuse here Alex Hung's proposal for
>> pre-blending API. I'll work on better documentation.
>>
>>>   
>>>>>
>>>>> 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.  
>>
>> Sorry, the patchset is here: https://lore.kernel.org/dri-devel/20230109143846.1966301-1-mwen@igalia.com/
>> In the next version, I won't forget cc'ing you at first.
>>>
>>> +/*
>>> + * 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?  
>>
>> It cames from VA-API:
>> https://intel.github.io/libva/structVAProcFilterParameterBuffer3DLUT.html#a682756be15d09327ba725b74a863cbcc
>>
>> In short, the reason is that lut_size is the valid points and is
>> the same for every dimensions, but lut_stride may vary.
> 
> Ok, so lut_size is what I would have guessed it to be. Just needs to be
> doc'd - pretty much all my questions are lack of docs.
> 
> I see that lut_stride is supposed to be in points, and not bytes. That
> is surprising. Are you sure it's good?
> 
>>>
>>> What is the color_mode the comment is referring to?  
>>
>> It refers to FB color_mode/bpp. I'm not using it in post-blending 3D LUT
>> implementation (should I?), it cames from pre-blending use case.  Maybe
>> the main issue here is if reusing the pre-blending 3D LUT mode struct is
>> a good approach or better create a specific for post-blending.
> 
> Sorry, I have no idea what FB color_mode is. I do not recall any such
> thing in the KMS uAPI.
> 
> 
>>>
>>> What is "number of bits of RGB"? Input precision? Output precision?
>>> Integer or floating point?  
>>
>> It's the bit depth of the 3D LUT values, the same for every channels. In
>> the AMD case, it's supports 10-bit and 12-bit, for example.
> 
> Ok. So e.g. r5g6b5 is not a possible 3D LUT element type on any
> hardware ever?
> 

I haven't had a chance to go through all patches yet but if this is
modeled after Alex Hung's work this should be covered by color_format.
The idea is that color_format takes a FOURCC value and defines the
format of the entries in the 3DLUT blob.

The bit_depth describes the actual bit depth that the HW supports.
E.g., color_format could be DRM_FORMAT_XRGB16161616 but HW might only
support 12-bit precision. In that case the least significant bits get
truncated.

One could define the bit_depth per color, but I'm not sure that'll be
necessary.

> What exactly is the truncation the comment refers to?
> 
> It sounds like if input has higher precision than the LUT elements,
> then "truncation" occurs. I can kind of see that, but I also think it
> is a false characterisation. The LUT input precision affects the
> precision of LUT indexing and the precision of interpolation between
> the LUT elements. I would not expect those two precisions to be
> truncated to the LUT element precision (but they could be truncated to
> something else hardware specific). Instead, I do expect the
> interpolation result to be truncated to the LUT output precision, which
> probably is the same as the LUT element precision, but not necessarily.
> 
> Maybe the comment about truncation should simply be removed? The result
> is obvious if we know the LUT input, element, and output precision, and
> what exactly happens with the indexing and interpolation is probably
> good enough to be left hardware-specific if it is difficult to describe
> in generic terms across different hardware.
> 

Maybe it makes sense to just drop the bit_depth field.

Harry

>>>
>>> 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?  
>>
>> I see. I'll work on explaining/documenting it better.
>>>
>>>
>>> + * “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.  
>>
>> ack
>>>
>>> + * “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.  
>>
>> The driver advertises all supported modes (each combination of values)
>> in a array as a enum, userspace can check all accepted modes and set the
>> one that fits the user 3D LUT settings. I think it's possible to get the
>> idea from this IGT test:
>> https://gitlab.freedesktop.org/mwen/igt-gpu-tools/-/commit/8771f444c3dcd126d7590d5a9b1b0db9706bbf6e#ed5dbc960ac210e3fbacd2361fe0270709767aaa_205_205
>>>
> 
> You lost me at "an array as an enum".
> 
> I understand there is a blob containing an array of struct
> drm_mode_lut3d_mode. What I don't understand is that you say LUT3D_MODE
> is an enum property. Where does the blob come from, then? What property
> provides the blob?
> 
> Am I correct in guessing that the values of LUT3D_MODE enum property
> are indices into the array in the blob, and that userspace will set it?
> That sounds good to me, if it's the integer value of the enum. But enum
> values also need string names, because that is how the values are
> usually recognized, so what name strings will be used?
> 
> In that code example, I cannot see any connection between the array of
> drm_mode_lut3d_mode entries, the 'name' string, and the actual data
> generated for the LUT. They must all connect somehow, but it just
> doesn't seem to be in the code. It is just hardcoding mode_lut3d[0],
> and the data generator never sees even that - it gets the lut size, but
> none of the other parameters. I cannot see that working across multiple
> drivers.
> 
> Why is it setting a linear shaper LUT explicitly instead of unsetting
> it? I mean this preparation:
> 
> shaper_linear = generate_table(data->shaper_lut_size, 1.0);
> 
> "Linear" actually means "identity", right? That's what people usually
> write when they mean identity.
> 
> Surely it's not actually a constant curve? That would make the whole
> test useless for proving the 3D LUT works.
> 
> 
>>>
>>> +	/**
>>> +	 * @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.
>>>   
>>
>> You are right! My initial idea was to explain that it's possible for
>> other vendors color pipeline to fit this pipeline internally, if they
>> need a 1D LUT before the 3D LUT, but not the 1D LUT in the end.
>>
>>>
>>> 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.  
>>
>> Thank you again for your valuable comments. I'll address your comments
>> in a next version by better explaining all these points.
> 
> Thank you for working on this!
> pq

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

[Pekka Paalanen]

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

On Fri, 10 Feb 2023 14:47:50 -0500
Harry Wentland <harry.wentland@amd.com> wrote:

> On 2/10/23 04:28, Pekka Paalanen wrote:
> > On Thu, 9 Feb 2023 13:27:02 -0100
> > Melissa Wen <mwen@igalia.com> wrote:
> >   
> >> On 01/31, Pekka Paalanen wrote:  
> >>> 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    

...

> >>> +/*
> >>> + * 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;
> >>> +};

...

> >>> What is "number of bits of RGB"? Input precision? Output precision?
> >>> Integer or floating point?    
> >>
> >> It's the bit depth of the 3D LUT values, the same for every channels. In
> >> the AMD case, it's supports 10-bit and 12-bit, for example.  
> > 
> > Ok. So e.g. r5g6b5 is not a possible 3D LUT element type on any
> > hardware ever?
> >   
> 
> I haven't had a chance to go through all patches yet but if this is
> modeled after Alex Hung's work this should be covered by color_format.
> The idea is that color_format takes a FOURCC value and defines the
> format of the entries in the 3DLUT blob.
> 
> The bit_depth describes the actual bit depth that the HW supports.
> E.g., color_format could be DRM_FORMAT_XRGB16161616 but HW might only
> support 12-bit precision. In that case the least significant bits get
> truncated.
> 
> One could define the bit_depth per color, but I'm not sure that'll be
> necessary.

Exactly. I just have no idea how sure we should be about that.

> > What exactly is the truncation the comment refers to?
> > 
> > It sounds like if input has higher precision than the LUT elements,
> > then "truncation" occurs. I can kind of see that, but I also think it
> > is a false characterisation. The LUT input precision affects the
> > precision of LUT indexing and the precision of interpolation between
> > the LUT elements. I would not expect those two precisions to be
> > truncated to the LUT element precision (but they could be truncated to
> > something else hardware specific). Instead, I do expect the
> > interpolation result to be truncated to the LUT output precision, which
> > probably is the same as the LUT element precision, but not necessarily.
> > 
> > Maybe the comment about truncation should simply be removed? The result
> > is obvious if we know the LUT input, element, and output precision, and
> > what exactly happens with the indexing and interpolation is probably
> > good enough to be left hardware-specific if it is difficult to describe
> > in generic terms across different hardware.
> >   
> 
> Maybe it makes sense to just drop the bit_depth field.

Well, it's really interesting information for userspace, but maybe it
should have a more holistic design. Precision is a factor, when
userspace considers whether it can use KMS hardware for a conversion or
not. Unfortunately, none of the existing KMS color pipeline elements
have any information on precision IIRC, so there is more to be fixed.

The interesting thing is the minimum guaranteed precision of each
element and the connections between them. It might be different for
pass-through vs. not. Another interesting thing is the usable value
range.

This is probably a complex problem, so there should be no need to solve
it before a 3D LUT interface can land, given old elements already have
the issue.


Thanks,
pq

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

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

[Ville Syrjälä]

On Mon, Feb 13, 2023 at 11:01:31AM +0200, Pekka Paalanen wrote:
> On Fri, 10 Feb 2023 14:47:50 -0500
> Harry Wentland <harry.wentland@amd.com> wrote:
> 
> > On 2/10/23 04:28, Pekka Paalanen wrote:
> > > On Thu, 9 Feb 2023 13:27:02 -0100
> > > Melissa Wen <mwen@igalia.com> wrote:
> > >   
> > >> On 01/31, Pekka Paalanen wrote:  
> > >>> 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    
> 
> ...
> 
> > >>> +/*
> > >>> + * 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;
> > >>> +};
> 
> ...
> 
> > >>> What is "number of bits of RGB"? Input precision? Output precision?
> > >>> Integer or floating point?    
> > >>
> > >> It's the bit depth of the 3D LUT values, the same for every channels. In
> > >> the AMD case, it's supports 10-bit and 12-bit, for example.  
> > > 
> > > Ok. So e.g. r5g6b5 is not a possible 3D LUT element type on any
> > > hardware ever?
> > >   
> > 
> > I haven't had a chance to go through all patches yet but if this is
> > modeled after Alex Hung's work this should be covered by color_format.
> > The idea is that color_format takes a FOURCC value and defines the
> > format of the entries in the 3DLUT blob.
> > 
> > The bit_depth describes the actual bit depth that the HW supports.
> > E.g., color_format could be DRM_FORMAT_XRGB16161616 but HW might only
> > support 12-bit precision. In that case the least significant bits get
> > truncated.
> > 
> > One could define the bit_depth per color, but I'm not sure that'll be
> > necessary.
> 
> Exactly. I just have no idea how sure we should be about that.
> 
> > > What exactly is the truncation the comment refers to?
> > > 
> > > It sounds like if input has higher precision than the LUT elements,
> > > then "truncation" occurs. I can kind of see that, but I also think it
> > > is a false characterisation. The LUT input precision affects the
> > > precision of LUT indexing and the precision of interpolation between
> > > the LUT elements. I would not expect those two precisions to be
> > > truncated to the LUT element precision (but they could be truncated to
> > > something else hardware specific). Instead, I do expect the
> > > interpolation result to be truncated to the LUT output precision, which
> > > probably is the same as the LUT element precision, but not necessarily.
> > > 
> > > Maybe the comment about truncation should simply be removed? The result
> > > is obvious if we know the LUT input, element, and output precision, and
> > > what exactly happens with the indexing and interpolation is probably
> > > good enough to be left hardware-specific if it is difficult to describe
> > > in generic terms across different hardware.
> > >   
> > 
> > Maybe it makes sense to just drop the bit_depth field.
> 
> Well, it's really interesting information for userspace, but maybe it
> should have a more holistic design. Precision is a factor, when
> userspace considers whether it can use KMS hardware for a conversion or
> not. Unfortunately, none of the existing KMS color pipeline elements
> have any information on precision IIRC, so there is more to be fixed.
> 
> The interesting thing is the minimum guaranteed precision of each
> element and the connections between them. It might be different for
> pass-through vs. not. Another interesting thing is the usable value
> range.
> 
> This is probably a complex problem, so there should be no need to solve
> it before a 3D LUT interface can land, given old elements already have
> the issue.

Yeah, I think all the precision stuff is all better handled by
eg. the proposed GAMMA_MODE property or something similar.
It's going to be needed for 1D LUTs as well. 1D LUTs would
also need it to expose diffrent LUT sizes with different
precision tradeoffs.

As for the 3D LUT blob, I don't think the blob needs any 
strides/etc. either. I had none of that for my i915 version:
https://github.com/vsyrjala/linux/commits/3dlut
Just the LUT entries + blob size is sufficient. At least
for cube shaped LUTs. Dunno if anyone would have a need
for something else?

The two things the are absolutely needed:
- Position of the LUT in the pipeline. We've already
  seen at least two variants of this IIRC, so we'll
  likely need to define a unique property for each tap
  point.
- The order the elements are stored in the blob. I didn't
  check if all the already known hw (amdgpu, i915, rcar-du,
  were there also others?) would agree on this or not.
  If yes, maybe just follow the hw order for all those,
  and if not, then I guess flip a few coins.

-- 
Ville Syrjälä
Intel

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

[Melissa Wen]

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

On 02/10, Pekka Paalanen wrote:
> On Thu, 9 Feb 2023 13:27:02 -0100
> Melissa Wen <mwen@igalia.com> wrote:
> 
> > On 01/31, Pekka Paalanen wrote:
> > > 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.
> > >   
> > 
> > Hi Pekka,
> > 
> > Thanks for your feedbacks and your time reviewing this proposal.
> 
> No problem, and sorry it took so long!
> 
> I'm just finishing the catch-up with everything that happened during
> winter holidays.
> 
> > > 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.
> > >   
> > I see, this is also the reason I reuse here Alex Hung's proposal for
> > pre-blending API. I'll work on better documentation.
> > 
> > >   
> > > > > 
> > > > > 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.  
> > 
> > Sorry, the patchset is here: https://lore.kernel.org/dri-devel/20230109143846.1966301-1-mwen@igalia.com/
> > In the next version, I won't forget cc'ing you at first.
> > > 
> > > +/*
> > > + * 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?  
> > 
> > It cames from VA-API:
> > https://intel.github.io/libva/structVAProcFilterParameterBuffer3DLUT.html#a682756be15d09327ba725b74a863cbcc
> > 
> > In short, the reason is that lut_size is the valid points and is
> > the same for every dimensions, but lut_stride may vary.
> 
> Ok, so lut_size is what I would have guessed it to be. Just needs to be
> doc'd - pretty much all my questions are lack of docs.
> 
> I see that lut_stride is supposed to be in points, and not bytes. That
> is surprising. Are you sure it's good?
> 
> > > 
> > > What is the color_mode the comment is referring to?  
> > 
> > It refers to FB color_mode/bpp. I'm not using it in post-blending 3D LUT
> > implementation (should I?), it cames from pre-blending use case.  Maybe
> > the main issue here is if reusing the pre-blending 3D LUT mode struct is
> > a good approach or better create a specific for post-blending.
> 
> Sorry, I have no idea what FB color_mode is. I do not recall any such
> thing in the KMS uAPI.
> 
> 
> > > 
> > > What is "number of bits of RGB"? Input precision? Output precision?
> > > Integer or floating point?  
> > 
> > It's the bit depth of the 3D LUT values, the same for every channels. In
> > the AMD case, it's supports 10-bit and 12-bit, for example.
> 
> Ok. So e.g. r5g6b5 is not a possible 3D LUT element type on any
> hardware ever?
> 
> What exactly is the truncation the comment refers to?
> 
> It sounds like if input has higher precision than the LUT elements,
> then "truncation" occurs. I can kind of see that, but I also think it
> is a false characterisation. The LUT input precision affects the
> precision of LUT indexing and the precision of interpolation between
> the LUT elements. I would not expect those two precisions to be
> truncated to the LUT element precision (but they could be truncated to
> something else hardware specific). Instead, I do expect the
> interpolation result to be truncated to the LUT output precision, which
> probably is the same as the LUT element precision, but not necessarily.
> 
> Maybe the comment about truncation should simply be removed? The result
> is obvious if we know the LUT input, element, and output precision, and
> what exactly happens with the indexing and interpolation is probably
> good enough to be left hardware-specific if it is difficult to describe
> in generic terms across different hardware.
> 
> > > 
> > > 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?  
> > 
> > I see. I'll work on explaining/documenting it better.
> > > 
> > > 
> > > + * “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.  
> > 
> > ack
> > > 
> > > + * “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.  
> > 
> > The driver advertises all supported modes (each combination of values)
> > in a array as a enum, userspace can check all accepted modes and set the
> > one that fits the user 3D LUT settings. I think it's possible to get the
> > idea from this IGT test:
> > https://gitlab.freedesktop.org/mwen/igt-gpu-tools/-/commit/8771f444c3dcd126d7590d5a9b1b0db9706bbf6e#ed5dbc960ac210e3fbacd2361fe0270709767aaa_205_205
> > > 
> 
> You lost me at "an array as an enum".
> 
> I understand there is a blob containing an array of struct
> drm_mode_lut3d_mode. What I don't understand is that you say LUT3D_MODE
> is an enum property. Where does the blob come from, then? What property
> provides the blob?
> 
> Am I correct in guessing that the values of LUT3D_MODE enum property
> are indices into the array in the blob, and that userspace will set it?
> That sounds good to me, if it's the integer value of the enum. But enum
> values also need string names, because that is how the values are
> usually recognized, so what name strings will be used?

So, in this proposal, LUT3D_MODE is a list of indexes for a blob that describe a supported
3D LUT mode i.e. a `struct drm_mode_lut3d_mode` with size, stride, bit depth, etc. Strings here follow this pattern
`lut3d_{size}_{bit_depth}bit` [1]. When enabling 3D LUT support, the
driver should pass an array of `struct drm_mode_lut3d_mode` as supported
modes, with at least one element.

I think this drm_info output helps to understand the idea. Currently, we
have 5 DRM CRTC color mgmt properties. The drm_info output for them:

│           ├───"DEGAMMA_LUT": blob = 0
│           ├───"DEGAMMA_LUT_SIZE" (immutable): range [0, UINT32_MAX] = 4096
│           ├───"CTM": blob = 0
│           ├───"GAMMA_LUT": blob = 0
│           ├───"GAMMA_LUT_SIZE" (immutable): range [0, UINT32_MAX] = 4096

The idea is to add 4 new properties.

- The 3D LUT blob (LUT3D) and the indexes of supported modes
  (LUT3D_MODE), as follows (example with AMD supported modes):

│           ├───"LUT3D": blob = 0
│           ├───"LUT3D_MODE": enum {lut3d_17_12bit, lut3d_17_10bit, lut3d_9_12bit, lut3d_9_10bit} = lut3d_17_12bit

- And shaper (1D) LUT - following what we already have for degamma/gamma LUT:

│           ├───"SHAPER_LUT": blob = 0
│           └───"SHAPER_LUT_SIZE" (immutable): range [0, UINT32_MAX] = 4096


Each LUT3D_MODE element works as a index to get the blob for the
corresponding `struct drm_mode_lut3d_mode` ([2] may works as an
example). I've also included a patch to set the first element of the
list of supported mode as a predefined value [3], but maybe it's not
necessary (or even error prone).

[1] https://lore.kernel.org/dri-devel/20230109143846.1966301-5-mwen@igalia.com/
[2] https://gitlab.freedesktop.org/mwen/igt-gpu-tools/-/commit/a7b11bab9d573e38e26982b5cc5cf62e9dca6a75#ed5dbc960ac210e3fbacd2361fe0270709767aaa_886_886
[3] https://lore.kernel.org/dri-devel/20230109143846.1966301-7-mwen@igalia.com/
> 
> In that code example, I cannot see any connection between the array of
> drm_mode_lut3d_mode entries, the 'name' string, and the actual data
> generated for the LUT. They must all connect somehow, but it just
> doesn't seem to be in the code. It is just hardcoding mode_lut3d[0],
> and the data generator never sees even that - it gets the lut size, but
> none of the other parameters. I cannot see that working across multiple
> drivers.
> 
> Why is it setting a linear shaper LUT explicitly instead of unsetting
> it? I mean this preparation:
> 
> shaper_linear = generate_table(data->shaper_lut_size, 1.0);
> 
> "Linear" actually means "identity", right? That's what people usually
> write when they mean identity.
> 
> Surely it's not actually a constant curve? That would make the whole
> test useless for proving the 3D LUT works.

So, the idea of the test is only set a 3D LUT, without any shaper LUT
and see if the 3D LUT maps to max color values. However, from my tests
on AMD HW with 3D LUT support, I was not able to program a 3D LUT
without a shaper LUT, that means a shaper LUT in BYPASS mode. By
comparing CRC results I could see that if I don't set any shaper LUT,
the driver sets shaper LUT in BYPASS mode but the 3D LUT is just
ignored, so the combo shaper + 3D LUT seems a HW requirement. To
overcome this requirement and keep the expected behavior, the test sets
a linear shaper LUT to acts as a BYPASS LUT to overcome this
requirement. Am I missing something?

It's a really simple test case, but I don't see how this approach
invalidates the test results. To generalize the test for other devices,
I can restrict shaper LUT usage for AMD.

Melissa

> 
> 
> > > 
> > > +	/**
> > > +	 * @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.
> > >   
> > 
> > You are right! My initial idea was to explain that it's possible for
> > other vendors color pipeline to fit this pipeline internally, if they
> > need a 1D LUT before the 3D LUT, but not the 1D LUT in the end.
> > 
> > > 
> > > 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.  
> > 
> > Thank you again for your valuable comments. I'll address your comments
> > in a next version by better explaining all these points.
> 
> Thank you for working on this!
> pq



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

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

[Melissa Wen]

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

On 02/13, Ville Syrjälä wrote:
> On Mon, Feb 13, 2023 at 11:01:31AM +0200, Pekka Paalanen wrote:
> > On Fri, 10 Feb 2023 14:47:50 -0500
> > Harry Wentland <harry.wentland@amd.com> wrote:
> > 
> > > On 2/10/23 04:28, Pekka Paalanen wrote:
> > > > On Thu, 9 Feb 2023 13:27:02 -0100
> > > > Melissa Wen <mwen@igalia.com> wrote:
> > > >   
> > > >> On 01/31, Pekka Paalanen wrote:  
> > > >>> 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    
> > 
> > ...
> > 
> > > >>> +/*
> > > >>> + * 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;
> > > >>> +};
> > 
> > ...
> > 
> > > >>> What is "number of bits of RGB"? Input precision? Output precision?
> > > >>> Integer or floating point?    
> > > >>
> > > >> It's the bit depth of the 3D LUT values, the same for every channels. In
> > > >> the AMD case, it's supports 10-bit and 12-bit, for example.  
> > > > 
> > > > Ok. So e.g. r5g6b5 is not a possible 3D LUT element type on any
> > > > hardware ever?
> > > >   
> > > 
> > > I haven't had a chance to go through all patches yet but if this is
> > > modeled after Alex Hung's work this should be covered by color_format.
> > > The idea is that color_format takes a FOURCC value and defines the
> > > format of the entries in the 3DLUT blob.
> > > 
> > > The bit_depth describes the actual bit depth that the HW supports.
> > > E.g., color_format could be DRM_FORMAT_XRGB16161616 but HW might only
> > > support 12-bit precision. In that case the least significant bits get
> > > truncated.
> > > 
> > > One could define the bit_depth per color, but I'm not sure that'll be
> > > necessary.
> > 
> > Exactly. I just have no idea how sure we should be about that.
> > 
> > > > What exactly is the truncation the comment refers to?
> > > > 
> > > > It sounds like if input has higher precision than the LUT elements,
> > > > then "truncation" occurs. I can kind of see that, but I also think it
> > > > is a false characterisation. The LUT input precision affects the
> > > > precision of LUT indexing and the precision of interpolation between
> > > > the LUT elements. I would not expect those two precisions to be
> > > > truncated to the LUT element precision (but they could be truncated to
> > > > something else hardware specific). Instead, I do expect the
> > > > interpolation result to be truncated to the LUT output precision, which
> > > > probably is the same as the LUT element precision, but not necessarily.
> > > > 
> > > > Maybe the comment about truncation should simply be removed? The result
> > > > is obvious if we know the LUT input, element, and output precision, and
> > > > what exactly happens with the indexing and interpolation is probably
> > > > good enough to be left hardware-specific if it is difficult to describe
> > > > in generic terms across different hardware.
> > > >   
> > > 
> > > Maybe it makes sense to just drop the bit_depth field.
> > 
> > Well, it's really interesting information for userspace, but maybe it
> > should have a more holistic design. Precision is a factor, when
> > userspace considers whether it can use KMS hardware for a conversion or
> > not. Unfortunately, none of the existing KMS color pipeline elements
> > have any information on precision IIRC, so there is more to be fixed.
> > 
> > The interesting thing is the minimum guaranteed precision of each
> > element and the connections between them. It might be different for
> > pass-through vs. not. Another interesting thing is the usable value
> > range.
> > 
> > This is probably a complex problem, so there should be no need to solve
> > it before a 3D LUT interface can land, given old elements already have
> > the issue.
> 
> Yeah, I think all the precision stuff is all better handled by
> eg. the proposed GAMMA_MODE property or something similar.
> It's going to be needed for 1D LUTs as well. 1D LUTs would
> also need it to expose diffrent LUT sizes with different
> precision tradeoffs.
> 
> As for the 3D LUT blob, I don't think the blob needs any 
> strides/etc. either. I had none of that for my i915 version:
> https://github.com/vsyrjala/linux/commits/3dlut
> Just the LUT entries + blob size is sufficient. At least
> for cube shaped LUTs. Dunno if anyone would have a need
> for something else?

I only use lut_size and bit_depth for programming a CRTC 3D LUT in this
proposal, so far GAMMA_MODE also would fit. But don't know for
pre-blending 3D LUT.

> 
> The two things the are absolutely needed:
> - Position of the LUT in the pipeline. We've already
>   seen at least two variants of this IIRC, so we'll
>   likely need to define a unique property for each tap
>   point.

IIRC, I'd say three, since in rcar-du the 3D LUT is before the gamma
LUT, but there isn't a shaper 1D LUT before 3D LUT.  I'd like to know
how the gamma LUT pre-3D LUT acts on intel pipeline. If it's, in the
end, somehow similar to a shaper LUT.

I mean, if we don't name the LUTs after CTM, we could fit something
similar in terms of dimensions, as:

-> CTM -> 1D LUT -> 3D LUT -> 1D LUT

> - The order the elements are stored in the blob. I didn't
>   check if all the already known hw (amdgpu, i915, rcar-du,
>   were there also others?) would agree on this or not.
>   If yes, maybe just follow the hw order for all those,
>   and if not, then I guess flip a few coins.
> 
> -- 
> Ville Syrjälä
> Intel

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

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

[Pekka Paalanen]

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

On Mon, 13 Feb 2023 18:26:55 -0100
Melissa Wen <mwen@igalia.com> wrote:

> On 02/10, Pekka Paalanen wrote:
> > On Thu, 9 Feb 2023 13:27:02 -0100
> > Melissa Wen <mwen@igalia.com> wrote:
> >   
> > > On 01/31, Pekka Paalanen wrote:  
> > > > 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.
> > > >     
> > > 
> > > Hi Pekka,
> > > 
> > > Thanks for your feedbacks and your time reviewing this proposal.  
> > 
> > No problem, and sorry it took so long!
> > 
> > I'm just finishing the catch-up with everything that happened during
> > winter holidays.
> >   
> > > > 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.
> > > >     
> > > I see, this is also the reason I reuse here Alex Hung's proposal for
> > > pre-blending API. I'll work on better documentation.
> > >   
> > > >     
> > > > > > 
> > > > > > 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.    
> > > 
> > > Sorry, the patchset is here: https://lore.kernel.org/dri-devel/20230109143846.1966301-1-mwen@igalia.com/
> > > In the next version, I won't forget cc'ing you at first.  
> > > > 
> > > > +/*
> > > > + * 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?    
> > > 
> > > It cames from VA-API:
> > > https://intel.github.io/libva/structVAProcFilterParameterBuffer3DLUT.html#a682756be15d09327ba725b74a863cbcc
> > > 
> > > In short, the reason is that lut_size is the valid points and is
> > > the same for every dimensions, but lut_stride may vary.  
> > 
> > Ok, so lut_size is what I would have guessed it to be. Just needs to be
> > doc'd - pretty much all my questions are lack of docs.
> > 
> > I see that lut_stride is supposed to be in points, and not bytes. That
> > is surprising. Are you sure it's good?
> >   
> > > > 
> > > > What is the color_mode the comment is referring to?    
> > > 
> > > It refers to FB color_mode/bpp. I'm not using it in post-blending 3D LUT
> > > implementation (should I?), it cames from pre-blending use case.  Maybe
> > > the main issue here is if reusing the pre-blending 3D LUT mode struct is
> > > a good approach or better create a specific for post-blending.  
> > 
> > Sorry, I have no idea what FB color_mode is. I do not recall any such
> > thing in the KMS uAPI.
> > 
> >   
> > > > 
> > > > What is "number of bits of RGB"? Input precision? Output precision?
> > > > Integer or floating point?    
> > > 
> > > It's the bit depth of the 3D LUT values, the same for every channels. In
> > > the AMD case, it's supports 10-bit and 12-bit, for example.  
> > 
> > Ok. So e.g. r5g6b5 is not a possible 3D LUT element type on any
> > hardware ever?
> > 
> > What exactly is the truncation the comment refers to?
> > 
> > It sounds like if input has higher precision than the LUT elements,
> > then "truncation" occurs. I can kind of see that, but I also think it
> > is a false characterisation. The LUT input precision affects the
> > precision of LUT indexing and the precision of interpolation between
> > the LUT elements. I would not expect those two precisions to be
> > truncated to the LUT element precision (but they could be truncated to
> > something else hardware specific). Instead, I do expect the
> > interpolation result to be truncated to the LUT output precision, which
> > probably is the same as the LUT element precision, but not necessarily.
> > 
> > Maybe the comment about truncation should simply be removed? The result
> > is obvious if we know the LUT input, element, and output precision, and
> > what exactly happens with the indexing and interpolation is probably
> > good enough to be left hardware-specific if it is difficult to describe
> > in generic terms across different hardware.
> >   
> > > > 
> > > > 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?    
> > > 
> > > I see. I'll work on explaining/documenting it better.  
> > > > 
> > > > 
> > > > + * “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.    
> > > 
> > > ack  
> > > > 
> > > > + * “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.    
> > > 
> > > The driver advertises all supported modes (each combination of values)
> > > in a array as a enum, userspace can check all accepted modes and set the
> > > one that fits the user 3D LUT settings. I think it's possible to get the
> > > idea from this IGT test:
> > > https://gitlab.freedesktop.org/mwen/igt-gpu-tools/-/commit/8771f444c3dcd126d7590d5a9b1b0db9706bbf6e#ed5dbc960ac210e3fbacd2361fe0270709767aaa_205_205  
> > > >   
> > 
> > You lost me at "an array as an enum".
> > 
> > I understand there is a blob containing an array of struct
> > drm_mode_lut3d_mode. What I don't understand is that you say LUT3D_MODE
> > is an enum property. Where does the blob come from, then? What property
> > provides the blob?
> > 
> > Am I correct in guessing that the values of LUT3D_MODE enum property
> > are indices into the array in the blob, and that userspace will set it?
> > That sounds good to me, if it's the integer value of the enum. But enum
> > values also need string names, because that is how the values are
> > usually recognized, so what name strings will be used?  
> 
> So, in this proposal, LUT3D_MODE is a list of indexes for a blob that describe a supported
> 3D LUT mode

Hi Melissa,

are you sure? I believe you are looking at and explaining some kernel
internal APIs, and not the uAPI which is the important part. Internal
APIs can always be changed later, uAPI cannot.

After I had sent that email, I might have understood how it was
supposed to work instead: there is no array of struct
drm_mode_lut3d_mode.

Instead, LUT3D_MODE is a KMS property of type enum. Each integer value
of the enum is also a blob id. Each blob named by those ids is a single
struct drm_mode_lut3d_mode that userspace needs to retrieve individually
to understand what that specific enum value means.

> i.e. a `struct drm_mode_lut3d_mode` with size, stride, bit depth, etc. Strings here follow this pattern
> `lut3d_{size}_{bit_depth}bit` [1]. When enabling 3D LUT support, the
> driver should pass an array of `struct drm_mode_lut3d_mode` as supported
> modes, with at least one element.

To be clear, I do not care about kernel internal interfaces at all. I
only care about the uAPI. Therefore talking about kernel internal API
will only confuse me and every other userspace developer. Evidently it
has even confused some IGT developers, as I point out some IGT code
problems below.

If the struct drm_mode_lut3d_mode is the authoritative definition of
what each enum value means, then I think the string names should not
attempt to convey any meaning nor information. The strings should be
just "blob id 77" etc. That makes it clear what the value is.

Otherwise userspace will be tempted to use the string names only (as is
the usual uAPI design) to find the right enum value, and not look into
the struct drm_mode_lut3d_mode at all. Therefore, if you don't encode
absolutely everything of struct drm_mode_lut3d_mode into the string
name, you should encode nothing in the name. You might also have two
different descriptions ending up with the same string name under the
same enum property instance, and that must not happen.

> I think this drm_info output helps to understand the idea. Currently, we
> have 5 DRM CRTC color mgmt properties. The drm_info output for them:
> 
> │           ├───"DEGAMMA_LUT": blob = 0
> │           ├───"DEGAMMA_LUT_SIZE" (immutable): range [0, UINT32_MAX] = 4096
> │           ├───"CTM": blob = 0
> │           ├───"GAMMA_LUT": blob = 0
> │           ├───"GAMMA_LUT_SIZE" (immutable): range [0, UINT32_MAX] = 4096
> 
> The idea is to add 4 new properties.
> 
> - The 3D LUT blob (LUT3D) and the indexes of supported modes
>   (LUT3D_MODE), as follows (example with AMD supported modes):
> 
> │           ├───"LUT3D": blob = 0
> │           ├───"LUT3D_MODE": enum {lut3d_17_12bit, lut3d_17_10bit, lut3d_9_12bit, lut3d_9_10bit} = lut3d_17_12bit

Ok, but this does not show the difference between an index into an
array of struct drm_mode_lut3d_mode, and each enum value being a blob
id containing a single struct drm_mode_lut3d_mode.

> 
> - And shaper (1D) LUT - following what we already have for degamma/gamma LUT:
> 
> │           ├───"SHAPER_LUT": blob = 0
> │           └───"SHAPER_LUT_SIZE" (immutable): range [0, UINT32_MAX] = 4096
> 
> 
> Each LUT3D_MODE element works as a index to get the blob for the
> corresponding `struct drm_mode_lut3d_mode` ([2] may works as an
> example). I've also included a patch to set the first element of the
> list of supported mode as a predefined value [3], but maybe it's not
> necessary (or even error prone).

Are you sure about that terminology in the uAPI?

A blob id is not an index. If you do have an index, then which KMS
property is going to tell userspace what is the blob id that contains
the whole array? I haven't seen any KMS property for that yet.

> [1] https://lore.kernel.org/dri-devel/20230109143846.1966301-5-mwen@igalia.com/

Actually, in that patch, I see a loop that creates a new blob from each
element in a kernel-private array of struct drm_mode_lut3d_mode.

> [2] https://gitlab.freedesktop.org/mwen/igt-gpu-tools/-/commit/a7b11bab9d573e38e26982b5cc5cf62e9dca6a75#ed5dbc960ac210e3fbacd2361fe0270709767aaa_886_886

That is userspace code, which does:

	mode_3dlut = (struct drm_mode_3dlut_mode *) blob->data;
	count = blob->length / sizeof(mode_3dlut[0]);
	igt_assert(count);

	for (i = 0; i < count; i++) {

That code is semantically wrong. The blob data is never an array. It is
always a single struct drm_mode_lut3d_mode. This code is misleading the
reader really hard.


> [3] https://lore.kernel.org/dri-devel/20230109143846.1966301-7-mwen@igalia.com/

Well, it's an enum property. A property always has some value. An enum
must always enumerate all possible values. Therefore, you cannot ever
avoid setting some value as the default.

If you set zero as a value, then zero must also be enumerated.

I mentioned before that the drm_mode_lut3d_mode values should be named
"blob id %u". The zero value is not a blob id, it is a true constant
instead, so its name can be e.g. "pass-through" if that is what it
means. Or maybe that doesn't make sense, because setting the LUT3D
property to zero means "pass-through", in which case zero is not a
valid value for LUT3D_MODE ever, not even in the kernel (but sounds
like the DRM core does not guard against the driver setting enum values
that do not exist).

Note, that value zero literally means "no data". It is not the index to
the first struct drm_mode_lut3d_mode element. So the userspace code of
[2] is actually broken with respect to this: it will attempt to fetch a
blob id 0, and crash an assert.

If it does not crash, then I think the enum description is lying to
userspace: zero is a valid value that is not listed as existing.

Or is there some uAPI specification somewhere saying that any enum
property can be set to zero as well, regardless of what values are
listed as possible? I would hope not.

> > 
> > In that code example, I cannot see any connection between the array of
> > drm_mode_lut3d_mode entries, the 'name' string, and the actual data
> > generated for the LUT. They must all connect somehow, but it just
> > doesn't seem to be in the code. It is just hardcoding mode_lut3d[0],
> > and the data generator never sees even that - it gets the lut size, but
> > none of the other parameters. I cannot see that working across multiple
> > drivers.
> > 
> > Why is it setting a linear shaper LUT explicitly instead of unsetting
> > it? I mean this preparation:
> > 
> > shaper_linear = generate_table(data->shaper_lut_size, 1.0);
> > 
> > "Linear" actually means "identity", right? That's what people usually
> > write when they mean identity.
> > 
> > Surely it's not actually a constant curve? That would make the whole
> > test useless for proving the 3D LUT works.  
> 
> So, the idea of the test is only set a 3D LUT, without any shaper LUT
> and see if the 3D LUT maps to max color values. However, from my tests
> on AMD HW with 3D LUT support, I was not able to program a 3D LUT
> without a shaper LUT, that means a shaper LUT in BYPASS mode. By
> comparing CRC results I could see that if I don't set any shaper LUT,
> the driver sets shaper LUT in BYPASS mode but the 3D LUT is just
> ignored, so the combo shaper + 3D LUT seems a HW requirement. To
> overcome this requirement and keep the expected behavior, the test sets
> a linear shaper LUT to acts as a BYPASS LUT to overcome this
> requirement. Am I missing something?

Sorry, no, I just had no idea what generate_table(..., 1.0) does. I
confused it with generate_table_max() which I have seen used before.

However, this exposes a kernel bug: the driver requires both the shaper
and the 3D LUT. If userspace sets the shaper to pass-through, you say
the 3D LUT gets ignored. That's a bug: the if the driver cannot make 3D
LUT work without a shaper, then it must invent an identity shaper when
userspace sets it to pass-through. Or fail the atomic commit.

Userspace could as well set the 3D LUT to pass-through and the shaper
to something else, and expect it to work. If the atomic test commit
does not fail, it must do what userspace prescribes.

You just cannot expose arbitrary hardware limitations to userspace like
that and silently malfunction if they are violated. At the very least
fail the atomic commit if you don't want to make it work in the driver.


Thanks,
pq

> 
> It's a really simple test case, but I don't see how this approach
> invalidates the test results. To generalize the test for other devices,
> I can restrict shaper LUT usage for AMD.
> 
> Melissa
> 
> > 
> >   
> > > > 
> > > > +	/**
> > > > +	 * @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.
> > > >     
> > > 
> > > You are right! My initial idea was to explain that it's possible for
> > > other vendors color pipeline to fit this pipeline internally, if they
> > > need a 1D LUT before the 3D LUT, but not the 1D LUT in the end.
> > >   
> > > > 
> > > > 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.    
> > > 
> > > Thank you again for your valuable comments. I'll address your comments
> > > in a next version by better explaining all these points.  
> > 
> > Thank you for working on this!
> > pq  
> 
> 


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

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

[Pekka Paalanen]

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

On Mon, 13 Feb 2023 18:45:40 -0100
Melissa Wen <mwen@igalia.com> wrote:

> On 02/13, Ville Syrjälä wrote:
> > On Mon, Feb 13, 2023 at 11:01:31AM +0200, Pekka Paalanen wrote:  
> > > On Fri, 10 Feb 2023 14:47:50 -0500
> > > Harry Wentland <harry.wentland@amd.com> wrote:
> > >   
> > > > On 2/10/23 04:28, Pekka Paalanen wrote:  
> > > > > On Thu, 9 Feb 2023 13:27:02 -0100
> > > > > Melissa Wen <mwen@igalia.com> wrote:
> > > > >     
> > > > >> On 01/31, Pekka Paalanen wrote:    
> > > > >>> 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      
> > > 
> > > ...
> > >   
> > > > >>> +/*
> > > > >>> + * 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;
> > > > >>> +};  
> > > 
> > > ...
> > >   
> > > > >>> What is "number of bits of RGB"? Input precision? Output precision?
> > > > >>> Integer or floating point?      
> > > > >>
> > > > >> It's the bit depth of the 3D LUT values, the same for every channels. In
> > > > >> the AMD case, it's supports 10-bit and 12-bit, for example.    
> > > > > 
> > > > > Ok. So e.g. r5g6b5 is not a possible 3D LUT element type on any
> > > > > hardware ever?
> > > > >     
> > > > 
> > > > I haven't had a chance to go through all patches yet but if this is
> > > > modeled after Alex Hung's work this should be covered by color_format.
> > > > The idea is that color_format takes a FOURCC value and defines the
> > > > format of the entries in the 3DLUT blob.
> > > > 
> > > > The bit_depth describes the actual bit depth that the HW supports.
> > > > E.g., color_format could be DRM_FORMAT_XRGB16161616 but HW might only
> > > > support 12-bit precision. In that case the least significant bits get
> > > > truncated.
> > > > 
> > > > One could define the bit_depth per color, but I'm not sure that'll be
> > > > necessary.  
> > > 
> > > Exactly. I just have no idea how sure we should be about that.
> > >   
> > > > > What exactly is the truncation the comment refers to?
> > > > > 
> > > > > It sounds like if input has higher precision than the LUT elements,
> > > > > then "truncation" occurs. I can kind of see that, but I also think it
> > > > > is a false characterisation. The LUT input precision affects the
> > > > > precision of LUT indexing and the precision of interpolation between
> > > > > the LUT elements. I would not expect those two precisions to be
> > > > > truncated to the LUT element precision (but they could be truncated to
> > > > > something else hardware specific). Instead, I do expect the
> > > > > interpolation result to be truncated to the LUT output precision, which
> > > > > probably is the same as the LUT element precision, but not necessarily.
> > > > > 
> > > > > Maybe the comment about truncation should simply be removed? The result
> > > > > is obvious if we know the LUT input, element, and output precision, and
> > > > > what exactly happens with the indexing and interpolation is probably
> > > > > good enough to be left hardware-specific if it is difficult to describe
> > > > > in generic terms across different hardware.
> > > > >     
> > > > 
> > > > Maybe it makes sense to just drop the bit_depth field.  
> > > 
> > > Well, it's really interesting information for userspace, but maybe it
> > > should have a more holistic design. Precision is a factor, when
> > > userspace considers whether it can use KMS hardware for a conversion or
> > > not. Unfortunately, none of the existing KMS color pipeline elements
> > > have any information on precision IIRC, so there is more to be fixed.
> > > 
> > > The interesting thing is the minimum guaranteed precision of each
> > > element and the connections between them. It might be different for
> > > pass-through vs. not. Another interesting thing is the usable value
> > > range.
> > > 
> > > This is probably a complex problem, so there should be no need to solve
> > > it before a 3D LUT interface can land, given old elements already have
> > > the issue.  
> > 
> > Yeah, I think all the precision stuff is all better handled by
> > eg. the proposed GAMMA_MODE property or something similar.
> > It's going to be needed for 1D LUTs as well. 1D LUTs would
> > also need it to expose diffrent LUT sizes with different
> > precision tradeoffs.
> > 
> > As for the 3D LUT blob, I don't think the blob needs any 
> > strides/etc. either. I had none of that for my i915 version:
> > https://github.com/vsyrjala/linux/commits/3dlut
> > Just the LUT entries + blob size is sufficient. At least
> > for cube shaped LUTs. Dunno if anyone would have a need
> > for something else?  
> 
> I only use lut_size and bit_depth for programming a CRTC 3D LUT in this
> proposal, so far GAMMA_MODE also would fit. But don't know for
> pre-blending 3D LUT.

You mean in the driver, or in IGT? Surely every field of struct
drm_mode_lut3d_mode needs to be used and tested, for values that
are possible for the hardware?

If the struct has strides, then the driver must set them and userspace
must use them, naturally. Userspace like IGT cannot go and assume
something without even looking at the struct fields.

This is why I was surprised to see the IGT code ignore most of the
struct drm_mode_lut3d_mode fields.

> > 
> > The two things the are absolutely needed:
> > - Position of the LUT in the pipeline. We've already
> >   seen at least two variants of this IIRC, so we'll
> >   likely need to define a unique property for each tap
> >   point.  
> 
> IIRC, I'd say three, since in rcar-du the 3D LUT is before the gamma
> LUT, but there isn't a shaper 1D LUT before 3D LUT.  I'd like to know
> how the gamma LUT pre-3D LUT acts on intel pipeline. If it's, in the
> end, somehow similar to a shaper LUT.
> 
> I mean, if we don't name the LUTs after CTM, we could fit something
> similar in terms of dimensions, as:
> 
> -> CTM -> 1D LUT -> 3D LUT -> 1D LUT  

Yes, to me the LUT names have always been irrelevant. The names are
only a way to identify an element's position in the abstract KMS color
pipeline defined by all defined KMS properties.


Thanks,
pq

> > - The order the elements are stored in the blob. I didn't
> >   check if all the already known hw (amdgpu, i915, rcar-du,
> >   were there also others?) would agree on this or not.
> >   If yes, maybe just follow the hw order for all those,
> >   and if not, then I guess flip a few coins.
> > 
> > -- 
> > Ville Syrjälä
> > Intel  


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

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

[Sharma, Shashank]

[AMD Official Use Only - General]

+ Uday, for awareness. 

Regards
Shashank
-----Original Message-----
From: Pekka Paalanen <ppaalanen@gmail.com>
Sent: 14 February 2023 10:28
To: Melissa Wen <mwen@igalia.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>; dri-devel@lists.freedesktop.org; airlied@gmail.com; laurent.pinchart+renesas@ideasonboard.com; Sharma, Shashank <Shashank.Sharma@amd.com>; Siqueira, Rodrigo <Rodrigo.Siqueira@amd.com>; amd-gfx@lists.freedesktop.org; Hung, Alex <Alex.Hung@amd.com>; Wentland, Harry <Harry.Wentland@amd.com>; tzimmermann@suse.de; Li, Sun peng (Leo) <Sunpeng.Li@amd.com>; maarten.lankhorst@linux.intel.com; mripard@kernel.org; seanpaul@chromium.org; daniel@ffwll.ch; Lakha, Bhawanpreet <Bhawanpreet.Lakha@amd.com>; Kim, Sung joon <Sungjoon.Kim@amd.com>; contact@emersion.fr; Pan, Xinhui <Xinhui.Pan@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; kernel-dev@igalia.com; Deucher, Alexander <Alexander.Deucher@amd.com>; Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Joshua Ashton <joshua@froggi.es>
Subject: Re: [RFC PATCH v2 00/18] Add DRM CRTC 3D LUT interface

On Mon, 13 Feb 2023 18:45:40 -0100
Melissa Wen <mwen@igalia.com> wrote:

> On 02/13, Ville Syrjälä wrote:
> > On Mon, Feb 13, 2023 at 11:01:31AM +0200, Pekka Paalanen wrote:  
> > > On Fri, 10 Feb 2023 14:47:50 -0500 Harry Wentland 
> > > <harry.wentland@amd.com> wrote:
> > >   
> > > > On 2/10/23 04:28, Pekka Paalanen wrote:  
> > > > > On Thu, 9 Feb 2023 13:27:02 -0100 Melissa Wen 
> > > > > <mwen@igalia.com> wrote:
> > > > >     
> > > > >> On 01/31, Pekka Paalanen wrote:    
> > > > >>> 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      
> > > 
> > > ...
> > >   
> > > > >>> +/*
> > > > >>> + * 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;
> > > > >>> +};
> > > 
> > > ...
> > >   
> > > > >>> What is "number of bits of RGB"? Input precision? Output precision?
> > > > >>> Integer or floating point?      
> > > > >>
> > > > >> It's the bit depth of the 3D LUT values, the same for every channels. In
> > > > >> the AMD case, it's supports 10-bit and 12-bit, for example.    
> > > > > 
> > > > > Ok. So e.g. r5g6b5 is not a possible 3D LUT element type on 
> > > > > any hardware ever?
> > > > >     
> > > > 
> > > > I haven't had a chance to go through all patches yet but if this 
> > > > is modeled after Alex Hung's work this should be covered by color_format.
> > > > The idea is that color_format takes a FOURCC value and defines 
> > > > the format of the entries in the 3DLUT blob.
> > > > 
> > > > The bit_depth describes the actual bit depth that the HW supports.
> > > > E.g., color_format could be DRM_FORMAT_XRGB16161616 but HW might 
> > > > only support 12-bit precision. In that case the least 
> > > > significant bits get truncated.
> > > > 
> > > > One could define the bit_depth per color, but I'm not sure 
> > > > that'll be necessary.
> > > 
> > > Exactly. I just have no idea how sure we should be about that.
> > >   
> > > > > What exactly is the truncation the comment refers to?
> > > > > 
> > > > > It sounds like if input has higher precision than the LUT 
> > > > > elements, then "truncation" occurs. I can kind of see that, 
> > > > > but I also think it is a false characterisation. The LUT input 
> > > > > precision affects the precision of LUT indexing and the 
> > > > > precision of interpolation between the LUT elements. I would 
> > > > > not expect those two precisions to be truncated to the LUT 
> > > > > element precision (but they could be truncated to something 
> > > > > else hardware specific). Instead, I do expect the 
> > > > > interpolation result to be truncated to the LUT output precision, which probably is the same as the LUT element precision, but not necessarily.
> > > > > 
> > > > > Maybe the comment about truncation should simply be removed? 
> > > > > The result is obvious if we know the LUT input, element, and 
> > > > > output precision, and what exactly happens with the indexing 
> > > > > and interpolation is probably good enough to be left 
> > > > > hardware-specific if it is difficult to describe in generic terms across different hardware.
> > > > >     
> > > > 
> > > > Maybe it makes sense to just drop the bit_depth field.  
> > > 
> > > Well, it's really interesting information for userspace, but maybe 
> > > it should have a more holistic design. Precision is a factor, when 
> > > userspace considers whether it can use KMS hardware for a 
> > > conversion or not. Unfortunately, none of the existing KMS color 
> > > pipeline elements have any information on precision IIRC, so there is more to be fixed.
> > > 
> > > The interesting thing is the minimum guaranteed precision of each 
> > > element and the connections between them. It might be different 
> > > for pass-through vs. not. Another interesting thing is the usable 
> > > value range.
> > > 
> > > This is probably a complex problem, so there should be no need to 
> > > solve it before a 3D LUT interface can land, given old elements 
> > > already have the issue.
> > 
> > Yeah, I think all the precision stuff is all better handled by eg. 
> > the proposed GAMMA_MODE property or something similar.
> > It's going to be needed for 1D LUTs as well. 1D LUTs would also need 
> > it to expose diffrent LUT sizes with different precision tradeoffs.
> > 
> > As for the 3D LUT blob, I don't think the blob needs any 
> > strides/etc. either. I had none of that for my i915 version:
> > https://github.com/vsyrjala/linux/commits/3dlut
> > Just the LUT entries + blob size is sufficient. At least for cube 
> > shaped LUTs. Dunno if anyone would have a need for something else?
> 
> I only use lut_size and bit_depth for programming a CRTC 3D LUT in 
> this proposal, so far GAMMA_MODE also would fit. But don't know for 
> pre-blending 3D LUT.

You mean in the driver, or in IGT? Surely every field of struct drm_mode_lut3d_mode needs to be used and tested, for values that are possible for the hardware?

If the struct has strides, then the driver must set them and userspace must use them, naturally. Userspace like IGT cannot go and assume something without even looking at the struct fields.

This is why I was surprised to see the IGT code ignore most of the struct drm_mode_lut3d_mode fields.

> > 
> > The two things the are absolutely needed:
> > - Position of the LUT in the pipeline. We've already
> >   seen at least two variants of this IIRC, so we'll
> >   likely need to define a unique property for each tap
> >   point.  
> 
> IIRC, I'd say three, since in rcar-du the 3D LUT is before the gamma 
> LUT, but there isn't a shaper 1D LUT before 3D LUT.  I'd like to know 
> how the gamma LUT pre-3D LUT acts on intel pipeline. If it's, in the 
> end, somehow similar to a shaper LUT.
> 
> I mean, if we don't name the LUTs after CTM, we could fit something 
> similar in terms of dimensions, as:
> 
> -> CTM -> 1D LUT -> 3D LUT -> 1D LUT

Yes, to me the LUT names have always been irrelevant. The names are only a way to identify an element's position in the abstract KMS color pipeline defined by all defined KMS properties.


Thanks,
pq

> > - The order the elements are stored in the blob. I didn't
> >   check if all the already known hw (amdgpu, i915, rcar-du,
> >   were there also others?) would agree on this or not.
> >   If yes, maybe just follow the hw order for all those,
> >   and if not, then I guess flip a few coins.
> > 
> > --
> > Ville Syrjälä
> > Intel

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

[Pekka Paalanen]

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

On Tue, 14 Feb 2023 11:19:47 +0200
Pekka Paalanen <ppaalanen@gmail.com> wrote:

> On Mon, 13 Feb 2023 18:26:55 -0100
> Melissa Wen <mwen@igalia.com> wrote:
> 
> > On 02/10, Pekka Paalanen wrote:  
> > > On Thu, 9 Feb 2023 13:27:02 -0100
> > > Melissa Wen <mwen@igalia.com> wrote:
> > >     
> > > > On 01/31, Pekka Paalanen wrote:    
> > > > > 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.
> > > > >       
> > > > 
> > > > Hi Pekka,
> > > > 
> > > > Thanks for your feedbacks and your time reviewing this proposal.    
> > > 
> > > No problem, and sorry it took so long!
> > > 
> > > I'm just finishing the catch-up with everything that happened during
> > > winter holidays.
> > >     
> > > > > 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.
> > > > >       
> > > > I see, this is also the reason I reuse here Alex Hung's proposal for
> > > > pre-blending API. I'll work on better documentation.
> > > >     
> > > > >       
> > > > > > > 
> > > > > > > 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.      
> > > > 
> > > > Sorry, the patchset is here: https://lore.kernel.org/dri-devel/20230109143846.1966301-1-mwen@igalia.com/
> > > > In the next version, I won't forget cc'ing you at first.    
> > > > > 
> > > > > +/*
> > > > > + * 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;
> > > > > +};

Btw. there is an odd number of u16 members before a u32 member. That
means there is invisible padding in this struct, I believe. I suppose
that's not different between 32-bit and 64-bit architectures, since
there are no 64-bit members, but it's still a hole. I assume the kernel
makes sure the hole cannot contain uninitialized values which would
leak kernel data to userspace...

For more things you might want to check, I think Daniel's botching up
ioctls article is appropriate. This is not an ioctl struct, but it's...
well, essentially it is. Just read-only.

https://www.kernel.org/doc/Documentation/ioctl/botching-up-ioctls.rst

...

> > > > > + * “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.      
> > > > 
> > > > The driver advertises all supported modes (each combination of values)
> > > > in a array as a enum, userspace can check all accepted modes and set the
> > > > one that fits the user 3D LUT settings. I think it's possible to get the
> > > > idea from this IGT test:
> > > > https://gitlab.freedesktop.org/mwen/igt-gpu-tools/-/commit/8771f444c3dcd126d7590d5a9b1b0db9706bbf6e#ed5dbc960ac210e3fbacd2361fe0270709767aaa_205_205    
> > > > >     
> > > 
> > > You lost me at "an array as an enum".
> > > 
> > > I understand there is a blob containing an array of struct
> > > drm_mode_lut3d_mode. What I don't understand is that you say LUT3D_MODE
> > > is an enum property. Where does the blob come from, then? What property
> > > provides the blob?
> > > 
> > > Am I correct in guessing that the values of LUT3D_MODE enum property
> > > are indices into the array in the blob, and that userspace will set it?
> > > That sounds good to me, if it's the integer value of the enum. But enum
> > > values also need string names, because that is how the values are
> > > usually recognized, so what name strings will be used?    
> > 
> > So, in this proposal, LUT3D_MODE is a list of indexes for a blob that describe a supported
> > 3D LUT mode  
> 
> Hi Melissa,
> 
> are you sure? I believe you are looking at and explaining some kernel
> internal APIs, and not the uAPI which is the important part. Internal
> APIs can always be changed later, uAPI cannot.
> 
> After I had sent that email, I might have understood how it was
> supposed to work instead: there is no array of struct
> drm_mode_lut3d_mode.
> 
> Instead, LUT3D_MODE is a KMS property of type enum. Each integer value
> of the enum is also a blob id. Each blob named by those ids is a single
> struct drm_mode_lut3d_mode that userspace needs to retrieve individually
> to understand what that specific enum value means.
> 
> > i.e. a `struct drm_mode_lut3d_mode` with size, stride, bit depth, etc. Strings here follow this pattern
> > `lut3d_{size}_{bit_depth}bit` [1]. When enabling 3D LUT support, the
> > driver should pass an array of `struct drm_mode_lut3d_mode` as supported
> > modes, with at least one element.  
> 
> To be clear, I do not care about kernel internal interfaces at all. I
> only care about the uAPI. Therefore talking about kernel internal API
> will only confuse me and every other userspace developer. Evidently it
> has even confused some IGT developers, as I point out some IGT code
> problems below.
> 
> If the struct drm_mode_lut3d_mode is the authoritative definition of
> what each enum value means, then I think the string names should not
> attempt to convey any meaning nor information. The strings should be
> just "blob id 77" etc. That makes it clear what the value is.

One more thing came to mind: extendability.

If it turns out that struct drm_mode_lut3d_mode is not good enough for
some future hardware, what could we do? Do you have a plan for that?

A) Thinking again, I would suggest to make the enum value string names
of the format "drm_mode_lut3d_mode %u". That way the name identifies the
blob binary structure and the blob id makes it unique. If we hope that
userspace checks the name, the kernel could later add a different
structure when necessary. Even if userspace fails in that, it would
still be an informative name.

B) Otherwise, or in addition, the structure needs to be designed to be
extendable like ioctl structs. A flags field maybe? But again we trust
that userspace checks the flags before trying to interpret any further.

C) Yet another option is a new DRM client CAP, which userspace can set
to tell the kernel that userspace does understand and expect that some
new kind of structure might be present. But this needs also A or B to
identify the struct.

D) Invent a whole new KMS property for the next generation 3D LUTs. In
that case the new and old properties probably need to be mutually
exclusive.


> Otherwise userspace will be tempted to use the string names only (as is
> the usual uAPI design) to find the right enum value, and not look into
> the struct drm_mode_lut3d_mode at all. Therefore, if you don't encode
> absolutely everything of struct drm_mode_lut3d_mode into the string
> name, you should encode nothing in the name. You might also have two
> different descriptions ending up with the same string name under the
> same enum property instance, and that must not happen.


Thanks,
pq

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

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

[Jacopo Mondi]

Hello

   I'm completing the support for 3D LUT on R-Car DU peripheral
and I have used this series as a base.

I'm wondering, since quite some time has passed without any update if
this series is still a thing and it makes any sense for me to try to
bring it forward.

I'm asking as I've noticed:
"[PATCH 00/36] drm/amd/display: add AMD driver-specific properties for color mgmt"

which seems to supersede this proposal with driver-specific
properties.

I asked Melissa privately but I wasn't able to get an hold of her, so
if anyone has any clue feel free to reply :)

Thanks
  j

On Mon, Jan 09, 2023 at 01:38:28PM -0100, 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
>
> 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.
>
> 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
>

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

[Pekka Paalanen]

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

On Tue, 13 Jun 2023 17:43:55 +0200
Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:

> Hello
> 
>    I'm completing the support for 3D LUT on R-Car DU peripheral
> and I have used this series as a base.
> 
> I'm wondering, since quite some time has passed without any update if
> this series is still a thing and it makes any sense for me to try to
> bring it forward.
> 
> I'm asking as I've noticed:
> "[PATCH 00/36] drm/amd/display: add AMD driver-specific properties for color mgmt"
> 
> which seems to supersede this proposal with driver-specific
> properties.
> 
> I asked Melissa privately but I wasn't able to get an hold of her, so
> if anyone has any clue feel free to reply :)

Hi,

since no-one else replied, I'll point you to the thread starting at
https://lists.freedesktop.org/archives/dri-devel/2023-May/403173.html
and continuing to June. That is the plan of getting a common UAPI for
these things.


Thanks,
pq


> 
> Thanks
>   j
> 
> On Mon, Jan 09, 2023 at 01:38:28PM -0100, 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
> >
> > 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.
> >
> > 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 --]

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

[Jacopo Mondi]

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

Hi Pekka
    thanks for the reply

On Thu, Jun 15, 2023 at 10:14:05AM +0300, Pekka Paalanen wrote:
> On Tue, 13 Jun 2023 17:43:55 +0200
> Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
>
> > Hello
> >
> >    I'm completing the support for 3D LUT on R-Car DU peripheral
> > and I have used this series as a base.
> >
> > I'm wondering, since quite some time has passed without any update if
> > this series is still a thing and it makes any sense for me to try to
> > bring it forward.
> >
> > I'm asking as I've noticed:
> > "[PATCH 00/36] drm/amd/display: add AMD driver-specific properties for color mgmt"
> >
> > which seems to supersede this proposal with driver-specific
> > properties.
> >
> > I asked Melissa privately but I wasn't able to get an hold of her, so
> > if anyone has any clue feel free to reply :)
>
> Hi,
>
> since no-one else replied, I'll point you to the thread starting at
> https://lists.freedesktop.org/archives/dri-devel/2023-May/403173.html

Yes, Melissa pointed me to that series privately yesterday.

However, and here I might be missing something, per-plane properties do
not apply well to the HW pipeline I'm looking at.

The R-Car DU has a 1D LUT and a 3D LUT at the CRTC level (I guess
'post blending' is the right term here) ?  A per-plane property
doesn't seem to match how the HW work, but please feel free to correct
me as this is all rather new to me and I might be overlooking
something.

My plan at the moment would have been to base my work on Melissa's RFC
and re-send to prop discussions, unless it is certainly a dead-end and
I have missed how to properly use per-plane properties on our HW.

Thank you!

> and continuing to June. That is the plan of getting a common UAPI for
> these things.
>
>
> Thanks,
> pq
>
>
> >
> > Thanks
> >   j
> >
> > On Mon, Jan 09, 2023 at 01:38:28PM -0100, 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
> > >
> > > 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.
> > >
> > > 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: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Pekka Paalanen]

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

On Thu, 15 Jun 2023 10:07:35 +0200
Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:

> Hi Pekka
>     thanks for the reply
> 
> On Thu, Jun 15, 2023 at 10:14:05AM +0300, Pekka Paalanen wrote:
> > On Tue, 13 Jun 2023 17:43:55 +0200
> > Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
> >  
> > > Hello
> > >
> > >    I'm completing the support for 3D LUT on R-Car DU peripheral
> > > and I have used this series as a base.
> > >
> > > I'm wondering, since quite some time has passed without any update if
> > > this series is still a thing and it makes any sense for me to try to
> > > bring it forward.
> > >
> > > I'm asking as I've noticed:
> > > "[PATCH 00/36] drm/amd/display: add AMD driver-specific properties for color mgmt"
> > >
> > > which seems to supersede this proposal with driver-specific
> > > properties.
> > >
> > > I asked Melissa privately but I wasn't able to get an hold of her, so
> > > if anyone has any clue feel free to reply :)  
> >
> > Hi,
> >
> > since no-one else replied, I'll point you to the thread starting at
> > https://lists.freedesktop.org/archives/dri-devel/2023-May/403173.html  
> 
> Yes, Melissa pointed me to that series privately yesterday.
> 
> However, and here I might be missing something, per-plane properties do
> not apply well to the HW pipeline I'm looking at.
> 
> The R-Car DU has a 1D LUT and a 3D LUT at the CRTC level (I guess
> 'post blending' is the right term here) ?  A per-plane property
> doesn't seem to match how the HW work, but please feel free to correct
> me as this is all rather new to me and I might be overlooking
> something.

Post-blending, correct. The long term plan is to replicate the same
idea to post-blending as it is planned for pre-blending.

> 
> My plan at the moment would have been to base my work on Melissa's RFC
> and re-send to prop discussions, unless it is certainly a dead-end and
> I have missed how to properly use per-plane properties on our HW.

I'm not a kernel dev nor a maintainer, so I can't comment on what would
be acceptable in the mean time before the new pipeline design is
implemented. The long term plan is to supersede all existing color
transformation related properties with pipelines.


Thanks,
pq

> 
> Thank you!
> 
> > and continuing to June. That is the plan of getting a common UAPI for
> > these things.
> >
> >
> > Thanks,
> > pq
> >
> >  
> > >
> > > Thanks
> > >   j
> > >
> > > On Mon, Jan 09, 2023 at 01:38:28PM -0100, 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
> > > >
> > > > 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.
> > > >
> > > > 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 --]