[igt-dev] [RFC PATCH 0/7] IGT tests for the KMS Color Pipeline API

[igt-dev] [RFC PATCH 0/7] IGT tests for the KMS Color Pipeline API

[Harry Wentland]

This series introduces support for
* drm_colorop DRM objects
* COLOR_PIPELINE plane property

Kernel changes:
https://gitlab.freedesktop.org/hwentland/linux/-/merge_requests/5

libdrm changes:
https://gitlab.freedesktop.org/hwentland/drm/-/merge_requests/1

It also adds a new kms_colorop test case that tests the color pipeline
API. The tests are designed to be easily extensible with a "transform"
and "compare" function pointer for each test. The "transform" function
performs the transformations under test via SW routines. The "compare"
function compares the DRM/KMS result (via a writeback connector) with
the result derived via the SW "transform".

Currently there is only one test for a single sRGB EOTF. I would like
to expand this to a more complex color pipeline, using VKMS.

Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
Cc: Simon Ser <contact@emersion.fr>
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Melissa Wen <mwen@igalia.com>
Cc: Jonas Ådahl <jadahl@redhat.com>
Cc: Sebastian Wick <sebastian.wick@redhat.com>
Cc: Shashank Sharma <shashank.sharma@amd.com>
Cc: Alexander Goins <agoins@nvidia.com>
Cc: Joshua Ashton <joshua@froggi.es>
Cc: Michel Dänzer <mdaenzer@redhat.com>
Cc: Aleix Pol <aleixpol@kde.org>
Cc: Xaver Hugl <xaver.hugl@gmail.com>
Cc: Victoria Brekenfeld <victoria@system76.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Uma Shankar <uma.shankar@intel.com>
Cc: Naseer Ahmed <quic_naseer@quicinc.com>
Cc: Christopher Braga <quic_cbraga@quicinc.com>


Harry Wentland (7):
  include/drm-uapi: Add COLOROP object
  lib/igt_kms: Introduce drm_colorop object
  lib/igt_kms: Add new COLOR PIPELINE plane property
  tests/kms_properties: Add colorop properties test
  igt/color: Add SW color transform functionality
  lib/igt_fb: Add copy_fb function
  tests/kms_colorop: Add kms_colorop tests

 include/drm-uapi/drm_mode.h |   1 +
 lib/igt_color.c             | 330 ++++++++++++++++++++++++++++
 lib/igt_color.h             | 105 +++++++++
 lib/igt_fb.c                |  40 +++-
 lib/igt_fb.h                |   3 +
 lib/igt_kms.c               | 237 +++++++++++++++++++-
 lib/igt_kms.h               | 101 +++++++++
 lib/meson.build             |   1 +
 tests/kms_colorop.c         | 424 ++++++++++++++++++++++++++++++++++++
 tests/kms_colorop.h         |  79 +++++++
 tests/kms_properties.c      |  66 ++++++
 tests/meson.build           |   1 +
 12 files changed, 1383 insertions(+), 5 deletions(-)
 create mode 100644 lib/igt_color.c
 create mode 100644 lib/igt_color.h
 create mode 100644 tests/kms_colorop.c
 create mode 100644 tests/kms_colorop.h

--
2.42.0

[igt-dev] [RFC PATCH 1/7] include/drm-uapi: Add COLOROP object

[Harry Wentland]

Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
Cc: Simon Ser <contact@emersion.fr>
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Melissa Wen <mwen@igalia.com>
Cc: Jonas Ådahl <jadahl@redhat.com>
Cc: Sebastian Wick <sebastian.wick@redhat.com>
Cc: Shashank Sharma <shashank.sharma@amd.com>
Cc: Alexander Goins <agoins@nvidia.com>
Cc: Joshua Ashton <joshua@froggi.es>
Cc: Michel Dänzer <mdaenzer@redhat.com>
Cc: Aleix Pol <aleixpol@kde.org>
Cc: Xaver Hugl <xaver.hugl@gmail.com>
Cc: Victoria Brekenfeld <victoria@system76.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Uma Shankar <uma.shankar@intel.com>
Cc: Naseer Ahmed <quic_naseer@quicinc.com>
Cc: Christopher Braga <quic_cbraga@quicinc.com>
---
 include/drm-uapi/drm_mode.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/drm-uapi/drm_mode.h b/include/drm-uapi/drm_mode.h
index e4a2570a6058..e7e56150616b 100644
--- a/include/drm-uapi/drm_mode.h
+++ b/include/drm-uapi/drm_mode.h
@@ -626,6 +626,7 @@ struct drm_mode_connector_set_property {
 #define DRM_MODE_OBJECT_FB 0xfbfbfbfb
 #define DRM_MODE_OBJECT_BLOB 0xbbbbbbbb
 #define DRM_MODE_OBJECT_PLANE 0xeeeeeeee
+#define DRM_MODE_OBJECT_COLOROP 0xfafafafa
 #define DRM_MODE_OBJECT_ANY 0
 
 struct drm_mode_obj_get_properties {
-- 
2.42.0

[igt-dev] [RFC PATCH 2/7] lib/igt_kms: Introduce drm_colorop object

[Harry Wentland]

Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
Cc: Simon Ser <contact@emersion.fr>
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Melissa Wen <mwen@igalia.com>
Cc: Jonas Ådahl <jadahl@redhat.com>
Cc: Sebastian Wick <sebastian.wick@redhat.com>
Cc: Shashank Sharma <shashank.sharma@amd.com>
Cc: Alexander Goins <agoins@nvidia.com>
Cc: Joshua Ashton <joshua@froggi.es>
Cc: Michel Dänzer <mdaenzer@redhat.com>
Cc: Aleix Pol <aleixpol@kde.org>
Cc: Xaver Hugl <xaver.hugl@gmail.com>
Cc: Victoria Brekenfeld <victoria@system76.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Uma Shankar <uma.shankar@intel.com>
Cc: Naseer Ahmed <quic_naseer@quicinc.com>
Cc: Christopher Braga <quic_cbraga@quicinc.com>
---
 lib/igt_kms.c | 184 +++++++++++++++++++++++++++++++++++++++++++++++++-
 lib/igt_kms.h |  91 +++++++++++++++++++++++++
 2 files changed, 273 insertions(+), 2 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index c548414867c2..bb6f2b47e243 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -617,6 +617,13 @@ const char * const igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = {
 	[IGT_PLANE_SCALING_FILTER] = "SCALING_FILTER",
 };
 
+const char * const igt_colorop_prop_names[IGT_NUM_COLOROP_PROPS] = {
+	[IGT_COLOROP_TYPE] = "TYPE",
+	[IGT_COLOROP_BYPASS] = "BYPASS",
+	[IGT_COLOROP_CURVE_1D_TYPE] = "CURVE_1D_TYPE",
+	[IGT_COLOROP_NEXT] = "NEXT",
+};
+
 const char * const igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
 	[IGT_CRTC_CTM] = "CTM",
 	[IGT_CRTC_GAMMA_LUT] = "GAMMA_LUT",
@@ -682,6 +689,21 @@ igt_plane_rotations(igt_display_t *display, igt_plane_t *plane,
 	return rotations;
 }
 
+igt_colorop_t *igt_find_colorop(igt_display_t *display, uint32_t id)
+{
+	int i;
+
+	/* find corresponding igt_colorop */
+	for (i = 0; i < display->n_colorops; ++i) {
+		igt_colorop_t *colorop = &display->colorops[i];
+
+		if (colorop->drm_colorop->colorop_id == id)
+			return colorop;
+	}
+
+	return NULL;
+}
+
 /*
  * Retrieve all the properies specified in props_name and store them into
  * plane->props.
@@ -722,6 +744,40 @@ igt_fill_plane_props(igt_display_t *display, igt_plane_t *plane,
 	drmModeFreeObjectProperties(props);
 }
 
+/*
+ * Retrieve all the properies specified in props_name and store them into
+ * colorop->props.
+ */
+static void
+igt_fill_colorop_props(igt_display_t *display, igt_colorop_t *colorop,
+		       int num_props, const char * const prop_names[])
+{
+	drmModeObjectPropertiesPtr props;
+	int i, j, fd;
+
+	fd = display->drm_fd;
+
+	props = drmModeObjectGetProperties(fd, colorop->drm_colorop->colorop_id, DRM_MODE_OBJECT_COLOROP);
+	igt_assert(props);
+
+	for (i = 0; i < props->count_props; i++) {
+		drmModePropertyPtr prop =
+			drmModeGetProperty(fd, props->props[i]);
+
+		for (j = 0; j < num_props; j++) {
+			if (strcmp(prop->name, prop_names[j]) != 0)
+				continue;
+
+			colorop->props[j] = props->props[i];
+			break;
+		}
+
+		drmModeFreeProperty(prop);
+	}
+
+	drmModeFreeObjectProperties(props);
+}
+
 /*
  * Retrieve all the properies specified in props_name and store them into
  * config->atomic_props_crtc and config->atomic_props_connector.
@@ -2624,7 +2680,8 @@ void igt_display_require(igt_display_t *display, int drm_fd)
 {
 	drmModeRes *resources;
 	drmModePlaneRes *plane_resources;
-	int i;
+	drmModeColoropRes *colorop_resources;
+	int i, j;
 	bool is_intel_dev;
 
 	memset(display, 0, sizeof(igt_display_t));
@@ -2709,17 +2766,47 @@ void igt_display_require(igt_display_t *display, int drm_fd)
 
 	drmModeFreePlaneResources(plane_resources);
 
+	/* get all colorop resources */
+	colorop_resources = drmModeGetColoropResources(display->drm_fd);
+
+	if (colorop_resources) {
+		display->n_colorops = colorop_resources->count_colorops;
+		display->colorops = calloc(sizeof(igt_colorop_t), display->n_colorops);
+		igt_assert_f(display->colorops, "Failed to allocate memory for %d colorops\n", display->n_colorops);
+
+		for (i = 0; i < colorop_resources->count_colorops; ++i) {
+			igt_colorop_t *colorop = &display->colorops[i];
+			uint32_t id = colorop_resources->colorops[i];
+
+			colorop->drm_colorop = drmModeGetColorop(display->drm_fd, id);
+			igt_assert(colorop->drm_colorop);
+
+			igt_fill_colorop_props(display, colorop, IGT_NUM_COLOROP_PROPS, igt_colorop_prop_names);
+
+			for (j = 0; j < display->n_planes; ++j)
+				if (display->planes[j].drm_plane->plane_id == colorop->drm_colorop->plane_id)
+					colorop->plane = &display->planes[j];
+
+		}
+
+
+
+		drmModeFreeColoropResources(colorop_resources);
+	}
+
+
 	for_each_pipe(display, i) {
 		igt_pipe_t *pipe = &display->pipes[i];
 		igt_plane_t *plane;
 		int p = 1, crtc_mask = 0;
-		int j, type;
+		int type;
 		uint8_t last_plane = 0, n_planes = 0;
 
 		pipe->display = display;
 		pipe->plane_cursor = -1;
 		pipe->plane_primary = -1;
 		pipe->planes = NULL;
+		pipe->colorops = NULL;
 		pipe->num_primary_planes = 0;
 
 		igt_fill_pipe_props(display, pipe, IGT_NUM_CRTC_PROPS, igt_crtc_prop_names);
@@ -3001,6 +3088,15 @@ void igt_display_fini(igt_display_t *display)
 	if (is_xe_device(drm_fd))
 		xe_device_put(drm_fd);
 
+	for (i = 0; i < display->n_colorops; ++i) {
+		igt_colorop_t *colorop = &display->colorops[i];
+
+		if (colorop->drm_colorop) {
+			drmModeFreeColorop(colorop->drm_colorop);
+			colorop->drm_colorop = NULL;
+		}
+	}
+
 	for (i = 0; i < display->n_planes; ++i) {
 		igt_plane_t *plane = &display->planes[i];
 
@@ -3336,6 +3432,43 @@ igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
 	}
 }
 
+
+/*
+ * Add colorop properties
+ */
+static void
+igt_atomic_prepare_colorop_commit(igt_colorop_t *colorop, igt_pipe_t *pipe,
+	drmModeAtomicReq *req)
+{
+	igt_display_t *display = pipe->display;
+	int i;
+
+	igt_assert(colorop->drm_colorop);
+
+	LOG(display,
+	    "populating colorop data: %s.%d\n",
+	    kmstest_pipe_name(pipe->pipe),
+	    colorop->drm_colorop->colorop_id);
+
+	/* TODO handle future colorop->next pointer */
+
+	for (i = 0; i < IGT_NUM_COLOROP_PROPS; i++) {
+		if (!igt_colorop_is_prop_changed(colorop, i))
+			continue;
+
+		/* it's an error to try an unsupported feature */
+		igt_assert(colorop->props[i]);
+
+		igt_debug("colorop %s.%d: Setting property \"%s\" to 0x%"PRIx64"/%"PRIi64"\n",
+			kmstest_pipe_name(pipe->pipe), colorop->drm_colorop->colorop_id, igt_colorop_prop_names[i],
+			colorop->values[i], colorop->values[i]);
+
+		igt_assert_lt(0, drmModeAtomicAddProperty(req, colorop->drm_colorop->colorop_id,
+						  colorop->props[i],
+						  colorop->values[i]));
+	}
+}
+
 /*
  * Properties that can be changed through legacy SetProperty:
  * - Obviously not the XYWH SRC/CRTC coordinates.
@@ -3783,6 +3916,25 @@ uint64_t igt_plane_get_prop(igt_plane_t *plane, enum igt_atomic_plane_properties
 					plane->drm_plane->plane_id, plane->props[prop]);
 }
 
+/**
+ * igt_colorop_get_prop:
+ * @colorop: Target colorop.
+ * @prop: Property to check.
+ *
+ * Return current value on a colorop for a given property.
+ *
+ * Returns: The value the property is set to, if this
+ * is a blob, the blob id is returned. This can be passed
+ * to drmModeGetPropertyBlob() to get the contents of the blob.
+ */
+uint64_t igt_colorop_get_prop(igt_display_t *display, igt_colorop_t *colorop, enum igt_atomic_colorop_properties prop)
+{
+	igt_assert(igt_colorop_has_prop(colorop, prop));
+
+	return igt_mode_object_get_prop(display, DRM_MODE_OBJECT_COLOROP,
+					colorop->drm_colorop->colorop_id, colorop->props[prop]);
+}
+
 static bool igt_mode_object_get_prop_enum_value(int drm_fd, uint32_t id, const char *str, uint64_t *val)
 {
 	drmModePropertyPtr prop = drmModeGetProperty(drm_fd, id);
@@ -3825,6 +3977,34 @@ void igt_plane_set_prop_enum(igt_plane_t *plane,
 	igt_assert(igt_plane_try_prop_enum(plane, prop, val));
 }
 
+bool igt_colorop_try_prop_enum(igt_display_t *display,
+			       igt_colorop_t *colorop,
+			       enum igt_atomic_colorop_properties prop,
+			       const char *val)
+{
+	uint64_t uval;
+
+	igt_assert(colorop->props[prop]);
+
+	if (!igt_mode_object_get_prop_enum_value(display->drm_fd,
+						 colorop->props[prop], val, &uval))
+		return false;
+
+	igt_colorop_set_prop_value(colorop, prop, uval);
+	return true;
+}
+
+void igt_colorop_set_prop_enum(igt_display_t *display,
+			       igt_colorop_t *colorop,
+			       enum igt_atomic_colorop_properties prop,
+			       const char *val)
+{
+	bool result = false;
+	result = igt_colorop_try_prop_enum(display, colorop, prop, val);
+	igt_assert(result);
+}
+
+
 /**
  * igt_plane_replace_prop_blob:
  * @plane: plane to set property on.
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index 1b6988c17aeb..6cf50b3c8280 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -321,6 +321,19 @@ enum igt_atomic_plane_properties {
        IGT_NUM_PLANE_PROPS
 };
 
+enum igt_atomic_colorop_properties {
+	IGT_COLOROP_TYPE,
+	IGT_COLOROP_BYPASS,
+	IGT_COLOROP_CURVE_1D_TYPE,
+	IGT_COLOROP_NEXT,
+	IGT_NUM_COLOROP_PROPS
+};
+
+/* TODO move to libdrm header?? */
+enum drm_colorop_type {
+	DRM_COLOROP_1D_CURVE
+};
+
 /**
  * igt_plane_prop_names
  *
@@ -329,6 +342,14 @@ enum igt_atomic_plane_properties {
  */
 extern const char * const igt_plane_prop_names[];
 
+/**
+ * igt_colorop_prop_names
+ *
+ * igt_colorop_prop_names contains a list of colorop property names,
+ * as indexed by the igt_atomic_colorop_properties enum.
+ */
+extern const char * const igt_colorop_prop_names[];
+
 typedef struct igt_display igt_display_t;
 typedef struct igt_pipe igt_pipe_t;
 typedef uint32_t igt_fixed_t;			/* 16.16 fixed point */
@@ -351,6 +372,24 @@ static inline bool igt_rotation_90_or_270(igt_rotation_t rotation)
 	return rotation & (IGT_ROTATION_90 | IGT_ROTATION_270);
 }
 
+typedef struct igt_plane igt_plane_t;
+
+typedef struct igt_colorop {
+	/*< private >*/
+	igt_pipe_t *pipe;
+
+	drmModeColorop *drm_colorop;
+
+	igt_plane_t *plane;
+
+	char name[DRM_PROP_NAME_LEN];
+
+	uint64_t changed;
+	uint32_t props[IGT_NUM_COLOROP_PROPS];
+	uint64_t values[IGT_NUM_COLOROP_PROPS];
+
+} igt_colorop_t;
+
 typedef struct igt_plane {
 	/*< private >*/
 	igt_pipe_t *pipe;
@@ -404,6 +443,7 @@ struct igt_pipe {
 	int plane_cursor;
 	int plane_primary;
 	igt_plane_t *planes;
+	igt_colorop_t *colorops;
 
 	uint64_t changed;
 	uint32_t props[IGT_NUM_CRTC_PROPS];
@@ -442,9 +482,11 @@ struct igt_display {
 	int log_shift;
 	int n_pipes;
 	int n_planes;
+	int n_colorops;
 	int n_outputs;
 	igt_output_t *outputs;
 	igt_plane_t *planes;
+	igt_colorop_t *colorops;
 	igt_pipe_t *pipes;
 	bool has_cursor_plane;
 	bool is_atomic;
@@ -721,6 +763,53 @@ extern void igt_plane_set_prop_enum(igt_plane_t *plane,
 				    enum igt_atomic_plane_properties prop,
 				    const char *val);
 
+
+
+extern bool igt_plane_is_valid_colorop(igt_plane_t *plane, igt_colorop_t *colorop);
+
+/**
+ * igt_colorop_has_prop:
+ * @colorop: colorop to check.
+ * @prop: Property to check.
+ *
+ * Check whether colorop supports a given property.
+ *
+ * Returns: True if the property is supported, otherwise false.
+ */
+static inline bool
+igt_colorop_has_prop(igt_colorop_t *colorop, enum igt_atomic_colorop_properties prop)
+{
+	return colorop->props[prop];
+}
+
+uint64_t igt_colorop_get_prop(igt_display_t *display, igt_colorop_t *colorop, enum igt_atomic_colorop_properties prop);
+
+#define igt_colorop_is_prop_changed(colorop, prop) \
+	(!!((colorop)->changed & (1 << (prop))))
+
+#define igt_colorop_set_prop_changed(colorop, prop) \
+	(colorop)->changed |= 1 << (prop)
+
+#define igt_colorop_clear_prop_changed(colorop, prop) \
+	(colorop)->changed &= ~(1 << (prop))
+
+#define igt_colorop_set_prop_value(colorop, prop, value) \
+	do { \
+		colorop->values[prop] = value; \
+		igt_colorop_set_prop_changed(colorop, prop); \
+	} while (0)
+
+
+extern bool igt_colorop_try_prop_enum(igt_display_t *display,
+				      igt_colorop_t *colorop,
+				      enum igt_atomic_colorop_properties prop,
+				      const char *val);
+
+extern void igt_colorop_set_prop_enum(igt_display_t *display,
+				      igt_colorop_t *colorop,
+				      enum igt_atomic_colorop_properties prop,
+				      const char *val);
+
 extern void igt_plane_replace_prop_blob(igt_plane_t *plane,
 					enum igt_atomic_plane_properties prop,
 					const void *ptr, size_t length);
@@ -1005,4 +1094,6 @@ bool igt_check_bigjoiner_support(igt_display_t *display);
 bool igt_parse_mode_string(const char *mode_string, drmModeModeInfo *mode);
 bool i915_pipe_output_combo_valid(igt_display_t *display);
 
+igt_colorop_t *igt_find_colorop(igt_display_t *display, uint32_t id);
+
 #endif /* __IGT_KMS_H__ */
-- 
2.42.0

[igt-dev] [RFC PATCH 3/7] lib/igt_kms: Add new COLOR PIPELINE plane property

[Harry Wentland]

Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
Cc: Simon Ser <contact@emersion.fr>
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Melissa Wen <mwen@igalia.com>
Cc: Jonas Ådahl <jadahl@redhat.com>
Cc: Sebastian Wick <sebastian.wick@redhat.com>
Cc: Shashank Sharma <shashank.sharma@amd.com>
Cc: Alexander Goins <agoins@nvidia.com>
Cc: Joshua Ashton <joshua@froggi.es>
Cc: Michel Dänzer <mdaenzer@redhat.com>
Cc: Aleix Pol <aleixpol@kde.org>
Cc: Xaver Hugl <xaver.hugl@gmail.com>
Cc: Victoria Brekenfeld <victoria@system76.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Uma Shankar <uma.shankar@intel.com>
Cc: Naseer Ahmed <quic_naseer@quicinc.com>
Cc: Christopher Braga <quic_cbraga@quicinc.com>
---
 lib/igt_kms.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/igt_kms.h | 10 ++++++++++
 2 files changed, 63 insertions(+)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index bb6f2b47e243..9c8c61eac49d 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -615,6 +615,7 @@ const char * const igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = {
 	[IGT_PLANE_ZPOS] = "zpos",
 	[IGT_PLANE_FB_DAMAGE_CLIPS] = "FB_DAMAGE_CLIPS",
 	[IGT_PLANE_SCALING_FILTER] = "SCALING_FILTER",
+	[IGT_PLANE_COLOR_PIPELINE] = "COLOR_PIPELINE",
 };
 
 const char * const igt_colorop_prop_names[IGT_NUM_COLOROP_PROPS] = {
@@ -704,6 +705,27 @@ igt_colorop_t *igt_find_colorop(igt_display_t *display, uint32_t id)
 	return NULL;
 }
 
+static void
+igt_fill_plane_color_pipelines(igt_display_t *display, igt_plane_t *plane,
+			       drmModePropertyPtr prop)
+{
+	int i;
+
+	plane->num_color_pipelines = 0;
+
+	for (i = 0; i < prop->count_enums; i++) {
+		igt_colorop_t *colorop = igt_find_colorop(display, prop->enums[i].value);
+
+		if (colorop) {
+			plane->color_pipelines[plane->num_color_pipelines++] = colorop;
+			memcpy(colorop->name, prop->enums[i].name, sizeof(colorop->name));
+		}
+	}
+
+	igt_assert(plane->num_color_pipelines < IGT_NUM_PLANE_COLOR_PIPELINES);
+
+}
+
 /*
  * Retrieve all the properies specified in props_name and store them into
  * plane->props.
@@ -735,6 +757,9 @@ igt_fill_plane_props(igt_display_t *display, igt_plane_t *plane,
 		if (strcmp(prop->name, "rotation") == 0)
 			plane->rotations = igt_plane_rotations(display, plane, prop);
 
+		if (strcmp(prop->name, "COLOR_PIPELINE") == 0)
+			igt_fill_plane_color_pipelines(display, plane, prop);
+
 		drmModeFreeProperty(prop);
 	}
 
@@ -3994,6 +4019,29 @@ bool igt_colorop_try_prop_enum(igt_display_t *display,
 	return true;
 }
 
+bool igt_plane_is_valid_colorop(igt_plane_t *plane, igt_colorop_t *colorop)
+{
+	int i;
+	bool found = false;
+
+	for (i = 0; i < plane->num_color_pipelines; i++) {
+		if (plane->color_pipelines[i] == colorop) {
+			found = true;
+			break;
+		}
+	}
+
+	return found;
+}
+
+void igt_plane_set_color_pipeline(igt_plane_t *plane, igt_colorop_t *colorop)
+{
+	igt_assert(igt_plane_is_valid_colorop(plane, colorop));
+
+	plane->assigned_color_pipeline = colorop;
+	igt_plane_set_prop_enum(plane, IGT_PLANE_COLOR_PIPELINE, colorop->name);
+}
+
 void igt_colorop_set_prop_enum(igt_display_t *display,
 			       igt_colorop_t *colorop,
 			       enum igt_atomic_colorop_properties prop,
@@ -4273,6 +4321,11 @@ static int igt_atomic_commit(igt_display_t *display, uint32_t flags, void *user_
 
 			if (plane->changed)
 				igt_atomic_prepare_plane_commit(plane, pipe_obj, req);
+
+			/* TODO iterate over assigned color pipeline and prepare colorop commit */
+			if (plane->assigned_color_pipeline)
+				igt_atomic_prepare_colorop_commit(plane->assigned_color_pipeline,
+								  pipe_obj, req);
 		}
 
 	}
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index 6cf50b3c8280..df27ea4ea40a 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -318,6 +318,7 @@ enum igt_atomic_plane_properties {
        IGT_PLANE_ZPOS,
        IGT_PLANE_FB_DAMAGE_CLIPS,
        IGT_PLANE_SCALING_FILTER,
+       IGT_PLANE_COLOR_PIPELINE,
        IGT_NUM_PLANE_PROPS
 };
 
@@ -354,6 +355,8 @@ typedef struct igt_display igt_display_t;
 typedef struct igt_pipe igt_pipe_t;
 typedef uint32_t igt_fixed_t;			/* 16.16 fixed point */
 
+#define IGT_NUM_PLANE_COLOR_PIPELINES 4
+
 typedef enum {
 	/* this maps to the kernel API */
 	IGT_ROTATION_0   = 1 << 0,
@@ -423,6 +426,11 @@ typedef struct igt_plane {
 	uint64_t *modifiers;
 	uint32_t *formats;
 	int format_mod_count;
+
+	igt_colorop_t *color_pipelines[IGT_NUM_PLANE_COLOR_PIPELINES];
+	int num_color_pipelines;
+
+	igt_colorop_t *assigned_color_pipeline;
 } igt_plane_t;
 
 /*
@@ -767,6 +775,8 @@ extern void igt_plane_set_prop_enum(igt_plane_t *plane,
 
 extern bool igt_plane_is_valid_colorop(igt_plane_t *plane, igt_colorop_t *colorop);
 
+extern void igt_plane_set_color_pipeline(igt_plane_t *plane, igt_colorop_t *colorop);
+
 /**
  * igt_colorop_has_prop:
  * @colorop: colorop to check.
-- 
2.42.0

[igt-dev] [RFC PATCH 4/7] tests/kms_properties: Add colorop properties test

[Harry Wentland]

Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
Cc: Simon Ser <contact@emersion.fr>
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Melissa Wen <mwen@igalia.com>
Cc: Jonas Ådahl <jadahl@redhat.com>
Cc: Sebastian Wick <sebastian.wick@redhat.com>
Cc: Shashank Sharma <shashank.sharma@amd.com>
Cc: Alexander Goins <agoins@nvidia.com>
Cc: Joshua Ashton <joshua@froggi.es>
Cc: Michel Dänzer <mdaenzer@redhat.com>
Cc: Aleix Pol <aleixpol@kde.org>
Cc: Xaver Hugl <xaver.hugl@gmail.com>
Cc: Victoria Brekenfeld <victoria@system76.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Uma Shankar <uma.shankar@intel.com>
Cc: Naseer Ahmed <quic_naseer@quicinc.com>
Cc: Christopher Braga <quic_cbraga@quicinc.com>
---
 tests/kms_properties.c | 66 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/tests/kms_properties.c b/tests/kms_properties.c
index 2fe8dfa66bcf..60883b2503ca 100644
--- a/tests/kms_properties.c
+++ b/tests/kms_properties.c
@@ -183,6 +183,39 @@ static void test_properties(int fd, uint32_t type, uint32_t id, bool atomic)
 	}
 }
 
+static void run_colorop_property_tests(igt_display_t *display, enum pipe pipe, igt_output_t *output)
+{
+	struct igt_fb fb;
+	igt_plane_t *plane;
+	int i;
+
+	prepare_pipe(display, pipe, output, &fb);
+
+	for_each_plane_on_pipe(display, pipe, plane) {
+		igt_info("Testing colorop properties on plane %s.#%d-%s (output: %s)\n",
+			 kmstest_pipe_name(pipe), plane->index, kmstest_plane_type_name(plane->type), output->name);
+
+		for (i = 0; i < display->n_colorops; i++) {
+			igt_colorop_t *global_colorop = &display->colorops[i];
+			drmModeColorop *drm_colorop = global_colorop->drm_colorop;
+
+			igt_info("Testing colorop properties on %s.#%d.#%d-%s (output: %s)\n",
+				kmstest_pipe_name(pipe), plane->index, drm_colorop->colorop_id, kmstest_plane_type_name(plane->type), output->name);
+
+			if (drm_colorop->plane_id != plane->drm_plane->plane_id) {
+				printf("bla: colorop %d is on plane %d, not on plane %d\n",
+					drm_colorop->colorop_id,
+					drm_colorop->plane_id,
+					plane->drm_plane->plane_id);
+				continue;
+			}
+			test_properties(display->drm_fd, DRM_MODE_OBJECT_COLOROP, drm_colorop->colorop_id, true);
+		}
+	}
+
+	cleanup_pipe(display, pipe, output, &fb);
+}
+
 static void run_plane_property_tests(igt_display_t *display, enum pipe pipe, igt_output_t *output, bool atomic)
 {
 	struct igt_fb fb;
@@ -228,6 +261,35 @@ static void run_connector_property_tests(igt_display_t *display, enum pipe pipe,
 		cleanup_pipe(display, pipe, output, &fb);
 }
 
+static void colorop_properties(igt_display_t *display)
+{
+	bool found_any = false, found;
+	igt_output_t *output;
+	enum pipe pipe;
+
+	/* colorops are only available with atomic */
+	igt_skip_on(!display->is_atomic);
+
+	for_each_pipe(display, pipe) {
+		found = false;
+
+		for_each_valid_output_on_pipe(display, pipe, output) {
+			igt_display_reset(display);
+
+			igt_output_set_pipe(output, pipe);
+			if (!i915_pipe_output_combo_valid(display))
+				continue;
+
+			found_any = found = true;
+
+			run_colorop_property_tests(display, pipe, output);
+			break;
+		}
+	}
+
+	igt_skip_on(!found_any);
+}
+
 static void plane_properties(igt_display_t *display, bool atomic)
 {
 	bool found_any = false, found;
@@ -761,6 +823,10 @@ igt_main
 	igt_subtest("plane-properties-atomic")
 		plane_properties(&display, true);
 
+	igt_describe("Tests colorop properties with atomic commit");
+	igt_subtest("colorop-properties-atomic")
+		colorop_properties(&display);
+
 	igt_describe("Tests crtc properties with legacy commit");
 	igt_subtest("crtc-properties-legacy")
 		crtc_properties(&display, false);
-- 
2.42.0

[igt-dev] [RFC PATCH 5/7] igt/color: Add SW color transform functionality

[Harry Wentland]

In order to test color we want to compare a HW (KMS) transform
with a SW transform. This introduces color transform for an
sRGB EOTF but this can be extended to other transforms. Code is
borrowed from Skia.

Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
Cc: Simon Ser <contact@emersion.fr>
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Melissa Wen <mwen@igalia.com>
Cc: Jonas Ådahl <jadahl@redhat.com>
Cc: Sebastian Wick <sebastian.wick@redhat.com>
Cc: Shashank Sharma <shashank.sharma@amd.com>
Cc: Alexander Goins <agoins@nvidia.com>
Cc: Joshua Ashton <joshua@froggi.es>
Cc: Michel Dänzer <mdaenzer@redhat.com>
Cc: Aleix Pol <aleixpol@kde.org>
Cc: Xaver Hugl <xaver.hugl@gmail.com>
Cc: Victoria Brekenfeld <victoria@system76.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Uma Shankar <uma.shankar@intel.com>
Cc: Naseer Ahmed <quic_naseer@quicinc.com>
Cc: Christopher Braga <quic_cbraga@quicinc.com>
---
 lib/igt_color.c | 330 ++++++++++++++++++++++++++++++++++++++++++++++++
 lib/igt_color.h | 105 +++++++++++++++
 lib/igt_fb.c    |   6 +-
 lib/igt_fb.h    |   2 +
 lib/meson.build |   1 +
 5 files changed, 441 insertions(+), 3 deletions(-)
 create mode 100644 lib/igt_color.c
 create mode 100644 lib/igt_color.h

diff --git a/lib/igt_color.c b/lib/igt_color.c
new file mode 100644
index 000000000000..12ff9ad1f324
--- /dev/null
+++ b/lib/igt_color.c
@@ -0,0 +1,330 @@
+/*
+ * Copyright 2023 Advanced Micro Devices, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#include <errno.h>
+#include <math.h>
+
+#include "igt_color.h"
+#include "igt_core.h"
+#include "igt_x86.h"
+
+
+static float clamp(float val, float min, float max)
+{
+	return ((val < min) ? min : ((val > max) ? max : val));
+}
+
+/*
+ * Below code is taken from Skia and adapted for style and needs of
+ * IGT.
+ *
+ * https://chromium.googlesource.com/chromium/src/+/3e1a26c44c024d97dc9a4c09bbc6a2365398ca2c/ui/gfx/skia_color_space_util.cc#15
+ *
+ * To comply with the original code's license we'll include it, as well
+ * as the original copyright here:
+ *
+ * // Copyright 2015 The Chromium Authors
+ * //
+ * // Redistribution and use in source and binary forms, with or without
+ * // modification, are permitted provided that the following conditions are
+ * // met:
+ * //
+ * //    * Redistributions of source code must retain the above copyright
+ * // notice, this list of conditions and the following disclaimer.
+ * //    * Redistributions in binary form must reproduce the above
+ * // copyright notice, this list of conditions and the following disclaimer
+ * // in the documentation and/or other materials provided with the
+ * // distribution.
+ * //    * Neither the name of Google LLC nor the names of its
+ * // contributors may be used to endorse or promote products derived from
+ * // this software without specific prior written permission.
+ * //
+ * // THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * // "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * // LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * // A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * // OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * // SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * // LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * // DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * // THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+static float igt_color_tf_eval_unclamped(const struct igt_color_tf *fn, float x)
+{
+	if (x < fn->d)
+		return fn->c * x + fn->f;
+	return pow(fn->a * x + fn->b, fn->g) + fn->e;
+}
+
+static float igt_color_tf_eval(const struct igt_color_tf *fn, float x)
+{
+	float fn_at_x_unclamped = igt_color_tf_eval_unclamped(fn, x);
+	return clamp(fn_at_x_unclamped, 0.0f, 1.0f);
+}
+
+#if 0
+skcms_TransferFunction SkTransferFnInverse(const skcms_TransferFunction& fn) {
+  skcms_TransferFunction fn_inv = {0};
+  if (fn.a > 0 && fn.g > 0) {
+    double a_to_the_g = std::pow(fn.a, fn.g);
+    fn_inv.a = 1.f / a_to_the_g;
+    fn_inv.b = -fn.e / a_to_the_g;
+    fn_inv.g = 1.f / fn.g;
+  }
+  fn_inv.d = fn.c * fn.d + fn.f;
+  fn_inv.e = -fn.b / fn.a;
+  if (fn.c != 0) {
+    fn_inv.c = 1.f / fn.c;
+    fn_inv.f = -fn.f / fn.c;
+  }
+  return fn_inv;
+}
+#endif
+
+/* end of Skia-based code */
+
+static void igt_color_srgb_inv_eotf(igt_pixel_t *pixel)
+{
+}
+
+
+
+static void igt_color_srgb_eotf(igt_pixel_t *pixel)
+{
+	pixel->r = igt_color_tf_eval(&srgb_tf, pixel->r);
+	pixel->g = igt_color_tf_eval(&srgb_tf, pixel->g);
+	pixel->b = igt_color_tf_eval(&srgb_tf, pixel->b);
+}
+
+int igt_color_transform_pixels(igt_fb_t *fb, igt_pixel_transform transform)
+{
+	uint32_t *line = NULL;
+	void *map;
+	char *ptr;
+	int x, y, cpp = igt_drm_format_to_bpp(fb->drm_format) / 8;
+	uint32_t stride = igt_fb_calc_plane_stride(fb, 0);
+
+	if (fb->num_planes != 1)
+		return -EINVAL;
+
+	/* TODO expand for other formats */
+	if (fb->drm_format != DRM_FORMAT_XRGB8888)
+		return -EINVAL;
+
+	ptr = igt_fb_map_buffer(fb->fd, fb);
+	igt_assert(ptr);
+	map = ptr;
+
+	/*
+	 * Framebuffers are often uncached, which can make byte-wise accesses
+	 * very slow. We copy each line of the FB into a local buffer to speed
+	 * up the hashing.
+	 */
+	line = malloc(stride);
+	if (!line) {
+		munmap(map, fb->size);
+		return -ENOMEM;
+	}
+
+	for (y = 0; y < fb->height; y++, ptr += stride) {
+
+		/* get line from buffer */
+		igt_memcpy_from_wc(line, ptr, fb->width * cpp);
+
+		for (x = 0; x < fb->width; x++) {
+			uint32_t raw_pixel = le32_to_cpu(line[x]);
+			igt_pixel_t pixel;
+			raw_pixel &= 0x00ffffff;
+
+			/*
+			 * unpack pixel into igt_pixel_t
+			 *
+			 * only for XRGB8888 for now
+			 *
+			 * TODO add "generic" mechanism for unpacking
+			 * other FB formats
+			 */
+			pixel.r = (raw_pixel & 0x00ff0000) >> 16;
+			pixel.g = (raw_pixel & 0x0000ff00) >> 8;
+			pixel.b = (raw_pixel & 0x000000ff);
+
+			/* normalize for 8-bit */
+			pixel.r /= (0xff);
+			pixel.g /= (0xff);
+			pixel.b /= (0xff);
+
+			/* TODO use read_rgb from igt_fb? */
+
+			/* run transform on pixel */
+			transform(&pixel);
+
+			/* de-normalize back to 8-bit */
+			pixel.r *= (0xff);
+			pixel.g *= (0xff);
+			pixel.b *= (0xff);
+
+			/* re-pack pixel into FB*/
+			raw_pixel = 0x0;
+			raw_pixel |= ((uint8_t) (lround(pixel.r) & 0xff)) << 16;
+			raw_pixel |= ((uint8_t) (lround(pixel.g) & 0xff)) << 8;
+			raw_pixel |= ((uint8_t) (lround(pixel.b) & 0xff));
+			/* TODO use write_rgb from igt_fb? */
+
+			/* write back to line */
+			line[x] = cpu_to_le32(raw_pixel);
+		}
+
+		/* copy line back to fb buffer */
+		igt_memcpy_from_wc(ptr, line, fb->width * cpp);
+	}
+
+	free(line);
+	igt_fb_unmap_buffer(fb, map);
+
+	return 0;
+
+	/* for each pixel */
+
+	/* convert to float and create igt_pixel */
+
+	/* call transform */
+
+	/* convert back to fb format from igt_pixel */
+
+
+}
+
+int igt_color_transform_srgb_eotf(igt_fb_t *fb)
+{
+	return igt_color_transform_pixels(fb, &igt_color_srgb_eotf);
+}
+
+int igt_color_transform_srgb_inv_eotf(igt_fb_t *fb)
+{
+	return igt_color_transform_pixels(fb, &igt_color_srgb_inv_eotf);
+}
+
+#if 0
+#define PRINT_PIXEL_CMP
+#endif
+
+bool igt_cmp_fb_component(uint16_t comp1, uint16_t comp2, uint8_t up, uint8_t down)
+{
+	int16_t diff = comp2 - comp1;
+
+	if ((diff < -down) || (diff > up)) {
+		printf("comp1 %x comp2 %x diff %d down %d, up %d\n", comp1, comp2, diff, -down, up);
+		return false;
+	}
+
+	return true;
+}
+
+bool igt_cmp_fb_pixels(igt_fb_t *fb1, igt_fb_t *fb2, uint8_t up, uint8_t down)
+{
+	uint32_t *ptr1, *ptr2;
+	uint32_t pixel1, pixel2, i, j;
+	bool matched = true;
+
+#ifdef PRINT_PIXEL_CMP
+	printf("hwhw: %s %d\n", __func__, __LINE__);
+#endif
+
+	ptr1 = igt_fb_map_buffer(fb1->fd, fb1);
+	ptr2 = igt_fb_map_buffer(fb2->fd, fb2);
+
+	igt_assert(fb1->drm_format == fb2->drm_format);
+	igt_assert(fb1->size == fb2->size);
+
+	for (i = 0; i < fb1->size / sizeof(uint32_t); i++) {
+		uint16_t mask = 0xff;
+		uint16_t shift = 8;
+
+		if (fb1->drm_format == DRM_FORMAT_XRGB2101010) {
+			/* ignore alpha */
+			pixel1 = ptr1[i] & ~0xc0000000;
+			pixel2 = ptr2[i] & ~0xc0000000;
+
+			mask = 0x3ff;
+			shift = 10;
+
+
+		} else if (fb1->drm_format == DRM_FORMAT_XRGB8888) {
+			/* ignore alpha */
+			pixel1 = ptr1[i] & ~0xff000000;
+			pixel2 = ptr2[i] & ~0xff000000;
+
+			mask = 0xff;
+			shift = 8;
+
+		} else {
+			pixel1 = ptr1[i];
+			pixel2 = ptr2[i];
+		}
+
+#ifdef PRINT_PIXEL_CMP
+		/* print first 5 pixels */
+		if (i < 5) {
+			printf("hwhw: %s %u\n", __func__, i);
+			printf("\t\traw_in\t%u\n", ptr1[i]);
+			printf("\t\traw_out\t%u\n", ptr2[i]);
+			printf("\t\tin\t%u\n", pixel1);
+			printf("\t\tout\t%u\n", pixel2);
+		}
+#endif
+
+		for (j = 0; j < 3; j++) {
+			uint16_t comp1 = (pixel1 >> (shift*j)) & mask;
+			uint16_t comp2 = (pixel2 >> (shift*j)) & mask;
+
+			if (!igt_cmp_fb_component(comp1, comp2, up, down)) {
+				/* TODO use proper log*/
+				printf("i %d j %d shift %d mask %x comp1 %x comp2 %x, pixel1 %x pixel2 %x\n",
+				       i, j, shift, mask, comp1, comp2, pixel1, pixel2);
+				return false;
+			}
+		}
+	}
+
+	igt_fb_unmap_buffer(fb1, ptr1);
+	igt_fb_unmap_buffer(fb2, ptr2);
+
+	return matched;
+}
+
+
+void igt_dump_fb(igt_display_t *display, igt_fb_t *fb,
+		 const char *path_name, const char *file_name)
+{
+	char filepath_out[PATH_MAX];
+	cairo_surface_t *fb_surface_out;
+	cairo_status_t status;
+
+	snprintf(filepath_out, PATH_MAX, "%s/%s.png", path_name, file_name);
+	fb_surface_out = igt_get_cairo_surface(display->drm_fd, fb);
+	status = cairo_surface_write_to_png(fb_surface_out, filepath_out);
+	igt_assert_eq(status, CAIRO_STATUS_SUCCESS);
+	cairo_surface_destroy(fb_surface_out);
+}
\ No newline at end of file
diff --git a/lib/igt_color.h b/lib/igt_color.h
new file mode 100644
index 000000000000..fc136eecfd17
--- /dev/null
+++ b/lib/igt_color.h
@@ -0,0 +1,105 @@
+/*
+ * Copyright 2023 Advanced Micro Devices, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+
+#ifndef __IGT_COLOR_H__
+#define __IGT_COLOR_H__
+
+#include <limits.h>
+
+#include "igt_fb.h"
+#include "igt_kms.h"
+
+/*
+ * Below code is taken from Skia and adapted for style and needs of
+ * IGT.
+ *
+ * https://chromium.googlesource.com/chromium/src/+/3e1a26c44c024d97dc9a4c09bbc6a2365398ca2c/ui/gfx/skia_color_space_util.cc#15
+ *
+ * To comply with the original code's license we'll include it, as well
+ * as the original copyright here:
+ *
+ * // Copyright 2015 The Chromium Authors
+ * //
+ * // Redistribution and use in source and binary forms, with or without
+ * // modification, are permitted provided that the following conditions are
+ * // met:
+ * //
+ * //    * Redistributions of source code must retain the above copyright
+ * // notice, this list of conditions and the following disclaimer.
+ * //    * Redistributions in binary form must reproduce the above
+ * // copyright notice, this list of conditions and the following disclaimer
+ * // in the documentation and/or other materials provided with the
+ * // distribution.
+ * //    * Neither the name of Google LLC nor the names of its
+ * // contributors may be used to endorse or promote products derived from
+ * // this software without specific prior written permission.
+ * //
+ * // THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * // "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * // LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * // A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * // OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * // SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * // LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * // DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * // THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+/*
+ * A transfer function mapping encoded values to linear values,
+ * represented by this 7-parameter piecewise function:
+ *
+ *   linear = sign(encoded) *  (c*|encoded| + f)       , 0 <= |encoded| < d
+ *          = sign(encoded) * ((a*|encoded| + b)^g + e), d <= |encoded|
+ *
+ * (A simple gamma transfer function sets g to gamma and a to 1.)
+ */
+struct igt_color_tf {
+    float g, a,b,c,d,e,f;
+};
+
+const struct igt_color_tf srgb_tf = {2.4f, (float)(1/1.055), (float)(0.055/1.055), (float)(1/12.92), 0.04045f, 0, 0};
+
+/* end of Skia-based code */
+
+typedef struct igt_pixel {
+	float r;
+	float g;
+	float b;
+} igt_pixel_t;
+
+bool igt_cmp_fb_component(uint16_t comp1, uint16_t comp2, uint8_t up, uint8_t down);
+bool igt_cmp_fb_pixels(igt_fb_t *fb1, igt_fb_t *fb2, uint8_t up, uint8_t down);
+
+void igt_dump_fb(igt_display_t *display, igt_fb_t *fb, const char *path_name, const char *file_name);
+
+/* TODO also allow 64-bit pixels, or other weird sizes */
+typedef void (*igt_pixel_transform)(igt_pixel_t *pixel);
+
+int igt_color_transform_pixels(igt_fb_t *fb, igt_pixel_transform transform);
+
+int igt_color_transform_srgb_eotf(igt_fb_t *fb);
+int igt_color_transform_srgb_inv_eotf(igt_fb_t *fb);
+
+#endif
\ No newline at end of file
diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index 0fe5b6adf8f4..0667d3fcbcff 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -739,7 +739,7 @@ void igt_init_fb(struct igt_fb *fb, int fd, int width, int height,
 	}
 }
 
-static uint32_t calc_plane_stride(struct igt_fb *fb, int plane)
+uint32_t igt_fb_calc_plane_stride(struct igt_fb *fb, int plane)
 {
 	uint32_t min_stride = fb->plane_width[plane] *
 		(fb->plane_bpp[plane] / 8);
@@ -916,7 +916,7 @@ static uint64_t calc_fb_size(struct igt_fb *fb)
 
 		/* respect the stride requested by the caller */
 		if (!fb->strides[plane])
-			fb->strides[plane] = calc_plane_stride(fb, plane);
+			fb->strides[plane] = igt_fb_calc_plane_stride(fb, plane);
 
 		align = get_plane_alignment(fb, plane);
 		if (align)
@@ -4471,7 +4471,7 @@ int igt_fb_get_fnv1a_crc(struct igt_fb *fb, igt_crc_t *crc)
 	void *map;
 	char *ptr;
 	int x, y, cpp = igt_drm_format_to_bpp(fb->drm_format) / 8;
-	uint32_t stride = calc_plane_stride(fb, 0);
+	uint32_t stride = igt_fb_calc_plane_stride(fb, 0);
 
 	if (fb->num_planes != 1)
 		return -EINVAL;
diff --git a/lib/igt_fb.h b/lib/igt_fb.h
index 73bdfc8669d8..b753c1580884 100644
--- a/lib/igt_fb.h
+++ b/lib/igt_fb.h
@@ -170,6 +170,8 @@ int igt_dirty_fb(int fd, struct igt_fb *fb);
 void *igt_fb_map_buffer(int fd, struct igt_fb *fb);
 void igt_fb_unmap_buffer(struct igt_fb *fb, void *buffer);
 
+uint32_t igt_fb_calc_plane_stride(struct igt_fb *fb, int plane);
+
 void igt_create_bo_for_fb(int fd, int width, int height,
 			  uint32_t format, uint64_t modifier,
 			  struct igt_fb *fb);
diff --git a/lib/meson.build b/lib/meson.build
index 85f100f75510..7c9aa56d429c 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -97,6 +97,7 @@ lib_sources = [
 	'igt_edid.c',
 	'igt_eld.c',
 	'igt_infoframe.c',
+	'igt_color.c',
 	'veboxcopy_gen12.c',
 	'igt_msm.c',
 	'igt_dsc.c',
-- 
2.42.0

[igt-dev] [RFC PATCH 6/7] lib/igt_fb: Add copy_fb function

[Harry Wentland]

Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
Cc: Simon Ser <contact@emersion.fr>
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Melissa Wen <mwen@igalia.com>
Cc: Jonas Ådahl <jadahl@redhat.com>
Cc: Sebastian Wick <sebastian.wick@redhat.com>
Cc: Shashank Sharma <shashank.sharma@amd.com>
Cc: Alexander Goins <agoins@nvidia.com>
Cc: Joshua Ashton <joshua@froggi.es>
Cc: Michel Dänzer <mdaenzer@redhat.com>
Cc: Aleix Pol <aleixpol@kde.org>
Cc: Xaver Hugl <xaver.hugl@gmail.com>
Cc: Victoria Brekenfeld <victoria@system76.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Uma Shankar <uma.shankar@intel.com>
Cc: Naseer Ahmed <quic_naseer@quicinc.com>
Cc: Christopher Braga <quic_cbraga@quicinc.com>
---
 lib/igt_fb.c | 34 ++++++++++++++++++++++++++++++++++
 lib/igt_fb.h |  1 +
 2 files changed, 35 insertions(+)

diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index 0667d3fcbcff..4db28e6c53f0 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -2121,6 +2121,40 @@ unsigned int igt_create_fb(int fd, int width, int height, uint32_t format,
 					  fb, 0, 0);
 }
 
+unsigned int igt_copy_fb(int fd, struct igt_fb *src, struct igt_fb *fb)
+{
+	char *in_ptr, *out_ptr;
+	int cpp = igt_drm_format_to_bpp(src->drm_format) / 8;
+
+	int fb_id = 0;
+	igt_assert(src);
+
+	/* TODO allow multiple planes */
+	if (src->num_planes != 1)
+		return -EINVAL;
+
+	/* TODO expand for other formats */
+	if (src->drm_format != DRM_FORMAT_XRGB8888)
+		return -EINVAL;
+
+	fb_id = igt_create_fb(fd, src->width, src->height, src->drm_format,
+			    src->modifier, fb);
+
+	/* copy buffer contents */
+	/* TODO simplify :D */
+	in_ptr = igt_fb_map_buffer(fb->fd, src);
+	igt_assert(in_ptr);
+	out_ptr = igt_fb_map_buffer(fb->fd, fb);
+	igt_assert(out_ptr);
+
+	igt_memcpy_from_wc(out_ptr, in_ptr, fb->width * fb->height * cpp);
+
+	igt_fb_unmap_buffer(fb, out_ptr);
+	igt_fb_unmap_buffer(src, in_ptr);
+
+	return fb_id;
+}
+
 /**
  * igt_create_color_fb:
  * @fd: open drm file descriptor
diff --git a/lib/igt_fb.h b/lib/igt_fb.h
index b753c1580884..a738678b6a85 100644
--- a/lib/igt_fb.h
+++ b/lib/igt_fb.h
@@ -142,6 +142,7 @@ struct intel_buf *igt_fb_create_intel_buf(int fd, struct buf_ops *bops,
 					  const struct igt_fb *fb, const char *name);
 unsigned int igt_create_fb(int fd, int width, int height, uint32_t format,
 			   uint64_t modifier, struct igt_fb *fb);
+unsigned int igt_copy_fb(int fd, struct igt_fb *src, struct igt_fb *fb);
 unsigned int igt_create_color_fb(int fd, int width, int height,
 				 uint32_t format, uint64_t modifier,
 				 double r, double g, double b,
-- 
2.42.0

[igt-dev] [RFC PATCH 7/7] tests/kms_colorop: Add kms_colorop tests

[Harry Wentland]

This tests the color pipeline API, exposed via the
COLOR PIPELINE plane property and drm_colorop objects.

Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
Cc: Simon Ser <contact@emersion.fr>
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Melissa Wen <mwen@igalia.com>
Cc: Jonas Ådahl <jadahl@redhat.com>
Cc: Sebastian Wick <sebastian.wick@redhat.com>
Cc: Shashank Sharma <shashank.sharma@amd.com>
Cc: Alexander Goins <agoins@nvidia.com>
Cc: Joshua Ashton <joshua@froggi.es>
Cc: Michel Dänzer <mdaenzer@redhat.com>
Cc: Aleix Pol <aleixpol@kde.org>
Cc: Xaver Hugl <xaver.hugl@gmail.com>
Cc: Victoria Brekenfeld <victoria@system76.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Uma Shankar <uma.shankar@intel.com>
Cc: Naseer Ahmed <quic_naseer@quicinc.com>
Cc: Christopher Braga <quic_cbraga@quicinc.com>
---
 tests/kms_colorop.c | 424 ++++++++++++++++++++++++++++++++++++++++++++
 tests/kms_colorop.h |  79 +++++++++
 tests/meson.build   |   1 +
 3 files changed, 504 insertions(+)
 create mode 100644 tests/kms_colorop.c
 create mode 100644 tests/kms_colorop.h

diff --git a/tests/kms_colorop.c b/tests/kms_colorop.c
new file mode 100644
index 000000000000..48c5aaf49955
--- /dev/null
+++ b/tests/kms_colorop.c
@@ -0,0 +1,424 @@
+/*
+ * Copyright 2023 Advanced Micro Devices, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#include "igt.h"
+#include "igt_color.h"
+#include "sw_sync.h"
+
+#include "kms_colorop.h"
+
+
+/* TODO move to lib for kms_writeback and kms_colorop (and other future) use */
+static bool check_writeback_config(igt_display_t *display, igt_output_t *output,
+				    drmModeModeInfo override_mode)
+{
+	igt_fb_t input_fb, output_fb;
+	igt_plane_t *plane;
+	uint32_t writeback_format = DRM_FORMAT_XRGB8888;
+	uint64_t modifier = DRM_FORMAT_MOD_LINEAR;
+	int width, height, ret;
+
+	igt_output_override_mode(output, &override_mode);
+
+	width = override_mode.hdisplay;
+	height = override_mode.vdisplay;
+
+	ret = igt_create_fb(display->drm_fd, width, height,
+			    DRM_FORMAT_XRGB8888, modifier, &input_fb);
+	igt_assert(ret >= 0);
+
+	ret = igt_create_fb(display->drm_fd, width, height,
+			    writeback_format, modifier, &output_fb);
+	igt_assert(ret >= 0);
+
+	plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
+	igt_plane_set_fb(plane, &input_fb);
+	igt_output_set_writeback_fb(output, &output_fb);
+
+	ret = igt_display_try_commit_atomic(display, DRM_MODE_ATOMIC_TEST_ONLY |
+					    DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
+	igt_plane_set_fb(plane, NULL);
+	igt_remove_fb(display->drm_fd, &input_fb);
+	igt_remove_fb(display->drm_fd, &output_fb);
+
+	return !ret;
+}
+
+/* TODO move to lib for kms_writeback and kms_colorop (and other future) use */
+typedef struct {
+	bool builtin_mode;
+	bool custom_mode;
+	bool list_modes;
+	bool dump_check;
+	int mode_index;
+	drmModeModeInfo user_mode;
+} data_t;
+
+static data_t data;
+
+/* TODO move to lib for kms_writeback and kms_colorop (and other future) use */
+static igt_output_t *kms_writeback_get_output(igt_display_t *display)
+{
+	int i;
+	enum pipe pipe;
+
+	drmModeModeInfo override_mode = {
+		.clock = 25175,
+		.hdisplay = 640,
+		.hsync_start = 656,
+		.hsync_end = 752,
+		.htotal = 800,
+		.hskew = 0,
+		.vdisplay = 480,
+		.vsync_start = 490,
+		.vsync_end = 492,
+		.vtotal = 525,
+		.vscan = 0,
+		.vrefresh = 60,
+		.flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
+		.name = {"640x480-60"},
+	};
+
+	for (i = 0; i < display->n_outputs; i++) {
+		igt_output_t *output = &display->outputs[i];
+
+		if (output->config.connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK)
+			continue;
+
+		for_each_pipe(display, pipe) {
+			igt_output_set_pipe(output, pipe);
+
+			if (data.custom_mode)
+				override_mode = data.user_mode;
+			if (data.builtin_mode)
+				override_mode = output->config.connector->modes[data.mode_index];
+
+			if (check_writeback_config(display, output, override_mode)) {
+				igt_debug("Using connector %u:%s on pipe %d\n",
+					  output->config.connector->connector_id,
+					  output->name, pipe);
+				return output;
+			}
+		}
+
+		igt_debug("We found %u:%s, but this test will not be able to use it.\n",
+			  output->config.connector->connector_id, output->name);
+
+		/* Restore any connectors we don't use, so we don't trip on them later */
+		kmstest_force_connector(display->drm_fd, output->config.connector, FORCE_CONNECTOR_UNSPECIFIED);
+	}
+
+	return NULL;
+}
+
+/* TODO move to lib for kms_writeback and kms_colorop (and other future) use */
+static uint64_t get_writeback_fb_id(igt_output_t *output)
+{
+	return igt_output_get_prop(output, IGT_CONNECTOR_WRITEBACK_FB_ID);
+}
+
+/* TODO move to lib for kms_writeback and kms_colorop (and other future) use */
+static void detach_crtc(igt_display_t *display, igt_output_t *output)
+{
+	if (get_writeback_fb_id(output) == 0)
+		return;
+
+	igt_output_set_pipe(output, PIPE_NONE);
+	igt_display_commit2(display, COMMIT_ATOMIC);
+}
+
+static void get_and_wait_out_fence(igt_output_t *output)
+{
+	int ret;
+
+	igt_assert(output->writeback_out_fence_fd >= 0);
+
+	ret = sync_fence_wait(output->writeback_out_fence_fd, 1000);
+	igt_assert_f(ret == 0, "sync_fence_wait failed: %s\n", strerror(-ret));
+	close(output->writeback_out_fence_fd);
+	output->writeback_out_fence_fd = -1;
+}
+
+static bool can_use_colorop(igt_display_t *display, igt_colorop_t *colorop, kms_colorop_t *desired)
+{
+	switch (desired->type) {
+	case KMS_COLOROP_ENUMERATED_LUT1D:
+		if (igt_colorop_get_prop(display, colorop, IGT_COLOROP_TYPE) == DRM_COLOROP_1D_CURVE) {
+			return true;
+		}
+	case KMS_COLOROP_CUSTOM_LUT1D:
+	case KMS_COLOROP_CTM:
+	case KMS_COLOROP_LUT3D:
+	default:
+		return false;
+	}
+}
+
+/**
+ * Iterate color pipeline that begins with colorop and try to map
+ * colorops[] to it.
+ */
+static bool map_to_pipeline(igt_display_t *display,
+			    igt_colorop_t *colorop,
+			    kms_colorop_t colorops[],
+			    int num_colorops)
+{
+	igt_colorop_t *next = colorop;
+	int i = 0;
+	int prop_val = 0;
+
+	while (next) {
+		if (can_use_colorop(display, next, &colorops[i])) {
+			colorops[i].colorop = next;
+			i++;
+			if (i >= num_colorops)
+				break;
+		}
+		prop_val = igt_colorop_get_prop(display, colorop,
+						IGT_COLOROP_NEXT);
+		next = igt_find_colorop(display, prop_val);
+	}
+
+	if (i < num_colorops) {
+		/* we failed to map the pipeline */
+
+		/* clean up colorops[i].colorop mappings */
+		for (i = 0; i < num_colorops; i++)
+			colorops[i].colorop = NULL;
+
+		return false;
+	}
+
+	return true;
+}
+
+static igt_colorop_t *get_color_pipeline(igt_display_t *display,
+					 igt_plane_t *plane,
+					 kms_colorop_t colorops[],
+					 int num_colorops)
+{
+	igt_colorop_t *colorop;
+	int i;
+
+	igt_assert(num_colorops > 0);
+
+	/* go through all color pipelines */
+	for (i = 0; i < plane->num_color_pipelines; ++i) {
+		colorop = plane->color_pipelines[i];
+		if (map_to_pipeline(display, colorop, colorops, num_colorops))
+			break;
+	}
+	igt_assert(colorop);
+
+	return colorop;
+}
+
+static void set_colorop(igt_display_t *display,
+			kms_colorop_t *colorop)
+{
+	igt_colorop_set_prop_value(colorop->colorop, IGT_COLOROP_BYPASS, colorop->colorop ? 0 : 1);
+
+	/* TODO set to desired value from kms_colorop_t */
+	switch (colorop->type) {
+	case KMS_COLOROP_ENUMERATED_LUT1D:
+		switch (colorop->enumerated_lut1d_info.tf) {
+		case KMS_COLOROP_LUT1D_SRGB_EOTF:
+			igt_colorop_set_prop_enum(display, colorop->colorop, IGT_COLOROP_CURVE_1D_TYPE, "sRGB EOTF");
+			break;
+		case KMS_COLOROP_LUT1D_SRGB_INV_EOTF:
+		case KMS_COLOROP_LUT1D_PQ_EOTF:
+		case KMS_COLOROP_LUT1D_PQ_INV_EOTF:
+		default:
+			igt_fail(IGT_EXIT_FAILURE);
+		}
+		break;
+	case KMS_COLOROP_CUSTOM_LUT1D:
+	case KMS_COLOROP_CTM:
+	case KMS_COLOROP_LUT3D:
+	default:
+		igt_fail(IGT_EXIT_FAILURE);
+	}
+}
+
+static void set_color_pipeline(igt_display_t *display,
+			       igt_plane_t *plane,
+			       kms_colorop_t colorops[],
+			       int num_colorops,
+			       igt_colorop_t *colorop)
+{
+	int i;
+
+	igt_plane_set_color_pipeline(plane, colorop);
+
+	for (i = 0; i < num_colorops; i++) {
+		set_colorop(display, &colorops[i]);
+	}
+}
+
+static void set_color_pipeline_bypass(igt_plane_t *plane)
+{
+	igt_plane_set_prop_enum(plane, IGT_PLANE_COLOR_PIPELINE, "Bypass");
+}
+
+static bool compare_with_bracket(igt_fb_t *in, igt_fb_t *out)
+{
+	return igt_cmp_fb_pixels(in, out, 1, 0);
+}
+
+#define DUMP_FBS 0
+
+static void colorop_plane_test(igt_display_t *display,
+			       kms_colorop_t colorops[],
+			       int num_colorops,
+			       transform_fb transform,
+			       compare_fb_t compare)
+{
+	igt_colorop_t *colorop = NULL;
+	igt_output_t *output;
+	igt_plane_t *plane;
+	igt_fb_t input_fb;
+	igt_fb_t sw_transform_fb;
+	igt_fb_t output_fb;
+	drmModeModeInfo mode;
+	unsigned int fb_id;
+	igt_crc_t input_crc, output_crc;
+
+	output = kms_writeback_get_output(display);
+	igt_require(output);
+
+	if (output->use_override_mode)
+		memcpy(&mode, &output->override_mode, sizeof(mode));
+	else
+		memcpy(&mode, &output->config.default_mode, sizeof(mode));
+
+	/* create input fb */
+	plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
+	igt_assert(plane);
+
+	fb_id = igt_create_color_pattern_fb(display->drm_fd,
+					mode.hdisplay, mode.vdisplay,
+					DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR,
+					0.2, 0.2, 0.2, &input_fb);
+
+	igt_assert(fb_id >= 0);
+	igt_plane_set_fb(plane, &input_fb);
+#if DUMP_FBS
+	igt_dump_fb(display, &input_fb, ".", "input");
+#endif
+
+	/* create output fb */
+	fb_id = igt_create_fb(display->drm_fd, mode.hdisplay, mode.vdisplay,
+				DRM_FORMAT_XRGB8888,
+				igt_fb_mod_to_tiling(0),
+				&output_fb);
+	igt_require(fb_id > 0);
+
+
+	igt_fb_get_fnv1a_crc(&input_fb, &input_crc);
+
+	igt_require(igt_plane_has_prop(plane, IGT_PLANE_COLOR_PIPELINE));
+
+	/* reset color pipeline*/
+
+	/* TODO do we need this? might be good sanity anyways */
+	set_color_pipeline_bypass(plane);
+
+	/* Commit */
+	igt_plane_set_fb(plane, &input_fb);
+	igt_output_set_writeback_fb(output, &output_fb);
+
+	igt_display_commit_atomic(output->display,
+				DRM_MODE_ATOMIC_ALLOW_MODESET,
+				NULL);
+	get_and_wait_out_fence(output);
+
+	/* Compare input and output buffers. They should be equal here. */
+	igt_fb_get_fnv1a_crc(&output_fb, &output_crc);
+
+	igt_assert_crc_equal(&input_crc, &output_crc);
+
+	/* create sw transformed buffer */
+
+	igt_copy_fb(display->drm_fd, &input_fb, &sw_transform_fb);
+	igt_assert(igt_cmp_fb_pixels(&input_fb, &sw_transform_fb, 0, 0));
+	transform(&sw_transform_fb);
+#if DUMP_FBS
+	igt_dump_fb(display, &sw_transform_fb, ".", "sw_transform");
+#endif
+	/* discover and set COLOR PIPELINE */
+
+	/* get COLOR_PIPELINE enum */
+	colorop = get_color_pipeline(display, plane, colorops, num_colorops);
+
+	set_color_pipeline(display, plane, colorops, num_colorops, colorop);
+
+	igt_output_set_writeback_fb(output, &output_fb);
+
+	/* commit COLOR_PIPELINE */
+	igt_display_commit_atomic(display,
+				DRM_MODE_ATOMIC_ALLOW_MODESET,
+				NULL);
+	get_and_wait_out_fence(output);
+#if DUMP_FBS
+	igt_dump_fb(display, &output_fb, ".", "output");
+#endif
+
+	/* compare sw transformed and KMS transformed FBs */
+	igt_assert(compare(&sw_transform_fb, &output_fb));
+
+	/* cleanup */
+	detach_crtc(display, output);
+	igt_remove_fb(display->drm_fd, &input_fb);
+	igt_remove_fb(display->drm_fd, &output_fb);
+}
+
+
+igt_main
+{
+	igt_display_t display;
+
+
+	igt_fixture {
+		display.drm_fd = drm_open_driver_master(DRIVER_ANY);
+		igt_display_require(&display, display.drm_fd);
+
+		kmstest_set_vt_graphics_mode();
+
+		igt_display_require(&display, display.drm_fd);
+
+		igt_require(display.is_atomic);
+
+	}
+
+	igt_describe("Tests getting and setting COLOR_PIPELINE property on plane");
+	igt_subtest("colorop-plane") {
+		kms_colorop_t colorops[] = { kms_colorop_srgb_eotf };
+
+		colorop_plane_test(&display, colorops, 1, igt_color_transform_srgb_eotf, compare_with_bracket);
+	}
+
+	igt_fixture {
+
+		igt_display_fini(&display);
+	}
+}
+
diff --git a/tests/kms_colorop.h b/tests/kms_colorop.h
new file mode 100644
index 000000000000..0d0d2e83ebd2
--- /dev/null
+++ b/tests/kms_colorop.h
@@ -0,0 +1,79 @@
+/*
+ * Copyright 2023 Advanced Micro Devices, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#ifndef __KMS_COLOROP_H__
+#define __KMS_COLOROP_H__
+
+/* Test version definitions */
+typedef enum kms_colorop_type {
+	KMS_COLOROP_ENUMERATED_LUT1D,
+	KMS_COLOROP_CUSTOM_LUT1D,
+	KMS_COLOROP_CTM,
+	KMS_COLOROP_LUT3D
+} kms_colorop_type_t;
+
+typedef enum kms_colorop_lut1d_tf {
+	KMS_COLOROP_LUT1D_SRGB_EOTF,
+	KMS_COLOROP_LUT1D_SRGB_INV_EOTF,
+	KMS_COLOROP_LUT1D_PQ_EOTF,
+	KMS_COLOROP_LUT1D_PQ_INV_EOTF,
+} kms_colorop_lut1d_tf_t;
+
+typedef struct kms_colorop_enumerated_lut1d_info {
+	kms_colorop_lut1d_tf_t tf;
+} kms_colorop_enumerated_lut1d_info_t;
+
+typedef struct kms_colorop {
+	kms_colorop_type_t type;
+
+	union {
+		kms_colorop_enumerated_lut1d_info_t enumerated_lut1d_info;
+	};
+
+	const char *name;
+
+	/* Mapped colorop */
+	igt_colorop_t *colorop;
+
+} kms_colorop_t;
+
+kms_colorop_t kms_colorop_srgb_eotf = {
+	.type = KMS_COLOROP_ENUMERATED_LUT1D,
+	.enumerated_lut1d_info = {
+		.tf = KMS_COLOROP_LUT1D_SRGB_EOTF
+	},
+	.name = "srgb-eotf"
+};
+
+kms_colorop_t kms_colorop_srgb_inf_eotf = {
+	.type = KMS_COLOROP_ENUMERATED_LUT1D,
+	.enumerated_lut1d_info = {
+		.tf = KMS_COLOROP_LUT1D_SRGB_INV_EOTF
+	},
+	.name = "srgb-inv-eotf"
+};
+
+typedef bool (*compare_fb_t)(igt_fb_t *in, igt_fb_t *out);
+
+typedef int (*transform_fb)(igt_fb_t *in);
+
+#endif /* __KMS_COLOROP_H__ */
diff --git a/tests/meson.build b/tests/meson.build
index 38f080f7c26e..e24f3efc9078 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -22,6 +22,7 @@ test_progs = [
 	'kms_atomic_transition',
 	'kms_bw',
 	'kms_concurrent',
+	'kms_colorop',
 	'kms_content_protection',
 	'kms_cursor_crc',
 	'kms_cursor_edge_walk',
-- 
2.42.0

[igt-dev] ✗ Fi.CI.BUILD: failure for IGT tests for the KMS Color Pipeline API

[Patchwork]

== Series Details ==

Series: IGT tests for the KMS Color Pipeline API
URL   : https://patchwork.freedesktop.org/series/123448/
State : failure

== Summary ==

Applying: include/drm-uapi: Add COLOROP object
Applying: lib/igt_kms: Introduce drm_colorop object
Using index info to reconstruct a base tree...
M	lib/igt_kms.c
M	lib/igt_kms.h
Falling back to patching base and 3-way merge...
Auto-merging lib/igt_kms.h
Auto-merging lib/igt_kms.c
Applying: lib/igt_kms: Add new COLOR PIPELINE plane property
Using index info to reconstruct a base tree...
M	lib/igt_kms.c
M	lib/igt_kms.h
Falling back to patching base and 3-way merge...
Auto-merging lib/igt_kms.h
CONFLICT (content): Merge conflict in lib/igt_kms.h
Auto-merging lib/igt_kms.c
Patch failed at 0003 lib/igt_kms: Add new COLOR PIPELINE plane property
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Re: [igt-dev] [RFC PATCH 5/7] igt/color: Add SW color transform functionality

[Pekka Paalanen]

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

On Fri, 8 Sep 2023 11:03:13 -0400
Harry Wentland <harry.wentland@amd.com> wrote:

> In order to test color we want to compare a HW (KMS) transform
> with a SW transform. This introduces color transform for an
> sRGB EOTF but this can be extended to other transforms. Code is
> borrowed from Skia.
> 
> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
> Cc: Simon Ser <contact@emersion.fr>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Melissa Wen <mwen@igalia.com>
> Cc: Jonas Ådahl <jadahl@redhat.com>
> Cc: Sebastian Wick <sebastian.wick@redhat.com>
> Cc: Shashank Sharma <shashank.sharma@amd.com>
> Cc: Alexander Goins <agoins@nvidia.com>
> Cc: Joshua Ashton <joshua@froggi.es>
> Cc: Michel Dänzer <mdaenzer@redhat.com>
> Cc: Aleix Pol <aleixpol@kde.org>
> Cc: Xaver Hugl <xaver.hugl@gmail.com>
> Cc: Victoria Brekenfeld <victoria@system76.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Uma Shankar <uma.shankar@intel.com>
> Cc: Naseer Ahmed <quic_naseer@quicinc.com>
> Cc: Christopher Braga <quic_cbraga@quicinc.com>
> ---
>  lib/igt_color.c | 330 ++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/igt_color.h | 105 +++++++++++++++
>  lib/igt_fb.c    |   6 +-
>  lib/igt_fb.h    |   2 +
>  lib/meson.build |   1 +
>  5 files changed, 441 insertions(+), 3 deletions(-)
>  create mode 100644 lib/igt_color.c
>  create mode 100644 lib/igt_color.h
> 
> diff --git a/lib/igt_color.c b/lib/igt_color.c
> new file mode 100644
> index 000000000000..12ff9ad1f324
> --- /dev/null
> +++ b/lib/igt_color.c
> @@ -0,0 +1,330 @@
> +/*
> + * Copyright 2023 Advanced Micro Devices, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +#include <errno.h>
> +#include <math.h>
> +
> +#include "igt_color.h"
> +#include "igt_core.h"
> +#include "igt_x86.h"
> +
> +
> +static float clamp(float val, float min, float max)
> +{
> +	return ((val < min) ? min : ((val > max) ? max : val));
> +}
> +
> +/*
> + * Below code is taken from Skia and adapted for style and needs of
> + * IGT.
> + *
> + * https://chromium.googlesource.com/chromium/src/+/3e1a26c44c024d97dc9a4c09bbc6a2365398ca2c/ui/gfx/skia_color_space_util.cc#15
> + *
> + * To comply with the original code's license we'll include it, as well
> + * as the original copyright here:
> + *
> + * // Copyright 2015 The Chromium Authors
> + * //
> + * // Redistribution and use in source and binary forms, with or without
> + * // modification, are permitted provided that the following conditions are
> + * // met:
> + * //
> + * //    * Redistributions of source code must retain the above copyright
> + * // notice, this list of conditions and the following disclaimer.
> + * //    * Redistributions in binary form must reproduce the above
> + * // copyright notice, this list of conditions and the following disclaimer
> + * // in the documentation and/or other materials provided with the
> + * // distribution.
> + * //    * Neither the name of Google LLC nor the names of its
> + * // contributors may be used to endorse or promote products derived from
> + * // this software without specific prior written permission.
> + * //
> + * // THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * // "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * // LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * // A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * // OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * // SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * // LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * // DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * // THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +static float igt_color_tf_eval_unclamped(const struct igt_color_tf *fn, float x)
> +{
> +	if (x < fn->d)
> +		return fn->c * x + fn->f;
> +	return pow(fn->a * x + fn->b, fn->g) + fn->e;
> +}
> +
> +static float igt_color_tf_eval(const struct igt_color_tf *fn, float x)
> +{
> +	float fn_at_x_unclamped = igt_color_tf_eval_unclamped(fn, x);
> +	return clamp(fn_at_x_unclamped, 0.0f, 1.0f);
> +}

...

> +#if 0
> +#define PRINT_PIXEL_CMP
> +#endif
> +
> +bool igt_cmp_fb_component(uint16_t comp1, uint16_t comp2, uint8_t up, uint8_t down)
> +{
> +	int16_t diff = comp2 - comp1;
> +
> +	if ((diff < -down) || (diff > up)) {
> +		printf("comp1 %x comp2 %x diff %d down %d, up %d\n", comp1, comp2, diff, -down, up);
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +bool igt_cmp_fb_pixels(igt_fb_t *fb1, igt_fb_t *fb2, uint8_t up, uint8_t down)
> +{
> +	uint32_t *ptr1, *ptr2;
> +	uint32_t pixel1, pixel2, i, j;
> +	bool matched = true;
> +
> +#ifdef PRINT_PIXEL_CMP
> +	printf("hwhw: %s %d\n", __func__, __LINE__);
> +#endif
> +
> +	ptr1 = igt_fb_map_buffer(fb1->fd, fb1);
> +	ptr2 = igt_fb_map_buffer(fb2->fd, fb2);
> +
> +	igt_assert(fb1->drm_format == fb2->drm_format);
> +	igt_assert(fb1->size == fb2->size);
> +
> +	for (i = 0; i < fb1->size / sizeof(uint32_t); i++) {
> +		uint16_t mask = 0xff;
> +		uint16_t shift = 8;
> +
> +		if (fb1->drm_format == DRM_FORMAT_XRGB2101010) {
> +			/* ignore alpha */
> +			pixel1 = ptr1[i] & ~0xc0000000;
> +			pixel2 = ptr2[i] & ~0xc0000000;
> +
> +			mask = 0x3ff;
> +			shift = 10;
> +
> +
> +		} else if (fb1->drm_format == DRM_FORMAT_XRGB8888) {
> +			/* ignore alpha */
> +			pixel1 = ptr1[i] & ~0xff000000;
> +			pixel2 = ptr2[i] & ~0xff000000;
> +
> +			mask = 0xff;
> +			shift = 8;
> +
> +		} else {
> +			pixel1 = ptr1[i];
> +			pixel2 = ptr2[i];
> +		}
> +
> +#ifdef PRINT_PIXEL_CMP
> +		/* print first 5 pixels */
> +		if (i < 5) {
> +			printf("hwhw: %s %u\n", __func__, i);
> +			printf("\t\traw_in\t%u\n", ptr1[i]);
> +			printf("\t\traw_out\t%u\n", ptr2[i]);
> +			printf("\t\tin\t%u\n", pixel1);
> +			printf("\t\tout\t%u\n", pixel2);
> +		}
> +#endif
> +
> +		for (j = 0; j < 3; j++) {
> +			uint16_t comp1 = (pixel1 >> (shift*j)) & mask;
> +			uint16_t comp2 = (pixel2 >> (shift*j)) & mask;
> +
> +			if (!igt_cmp_fb_component(comp1, comp2, up, down)) {
> +				/* TODO use proper log*/
> +				printf("i %d j %d shift %d mask %x comp1 %x comp2 %x, pixel1 %x pixel2 %x\n",
> +				       i, j, shift, mask, comp1, comp2, pixel1, pixel2);
> +				return false;
> +			}
> +		}
> +	}
> +
> +	igt_fb_unmap_buffer(fb1, ptr1);
> +	igt_fb_unmap_buffer(fb2, ptr2);
> +
> +	return matched;
> +}

Maybe you'd like to borrow some facilities from Weston for comparing
images?

Statistics collecting:
https://gitlab.freedesktop.org/wayland/weston/-/blob/71616edc4dfce1cf9deafee36009dd9c37a9ba8d/tests/color_util.c#L504-530
https://gitlab.freedesktop.org/wayland/weston/-/blob/71616edc4dfce1cf9deafee36009dd9c37a9ba8d/tests/color_util.h#L159-178

Example usage of the above:
https://gitlab.freedesktop.org/wayland/weston/-/blob/71616edc4dfce1cf9deafee36009dd9c37a9ba8d/tests/color-icc-output-test.c#L726-764



> +
> +
> +void igt_dump_fb(igt_display_t *display, igt_fb_t *fb,
> +		 const char *path_name, const char *file_name)
> +{
> +	char filepath_out[PATH_MAX];
> +	cairo_surface_t *fb_surface_out;
> +	cairo_status_t status;
> +
> +	snprintf(filepath_out, PATH_MAX, "%s/%s.png", path_name, file_name);
> +	fb_surface_out = igt_get_cairo_surface(display->drm_fd, fb);
> +	status = cairo_surface_write_to_png(fb_surface_out, filepath_out);
> +	igt_assert_eq(status, CAIRO_STATUS_SUCCESS);
> +	cairo_surface_destroy(fb_surface_out);
> +}
> \ No newline at end of file
> diff --git a/lib/igt_color.h b/lib/igt_color.h
> new file mode 100644
> index 000000000000..fc136eecfd17
> --- /dev/null
> +++ b/lib/igt_color.h
> @@ -0,0 +1,105 @@
> +/*
> + * Copyright 2023 Advanced Micro Devices, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +
> +#ifndef __IGT_COLOR_H__
> +#define __IGT_COLOR_H__
> +
> +#include <limits.h>
> +
> +#include "igt_fb.h"
> +#include "igt_kms.h"
> +
> +/*
> + * Below code is taken from Skia and adapted for style and needs of
> + * IGT.
> + *
> + * https://chromium.googlesource.com/chromium/src/+/3e1a26c44c024d97dc9a4c09bbc6a2365398ca2c/ui/gfx/skia_color_space_util.cc#15
> + *
> + * To comply with the original code's license we'll include it, as well
> + * as the original copyright here:
> + *
> + * // Copyright 2015 The Chromium Authors
> + * //
> + * // Redistribution and use in source and binary forms, with or without
> + * // modification, are permitted provided that the following conditions are
> + * // met:
> + * //
> + * //    * Redistributions of source code must retain the above copyright
> + * // notice, this list of conditions and the following disclaimer.
> + * //    * Redistributions in binary form must reproduce the above
> + * // copyright notice, this list of conditions and the following disclaimer
> + * // in the documentation and/or other materials provided with the
> + * // distribution.
> + * //    * Neither the name of Google LLC nor the names of its
> + * // contributors may be used to endorse or promote products derived from
> + * // this software without specific prior written permission.
> + * //
> + * // THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * // "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * // LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * // A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * // OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * // SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * // LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * // DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * // THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +/*
> + * A transfer function mapping encoded values to linear values,
> + * represented by this 7-parameter piecewise function:
> + *
> + *   linear = sign(encoded) *  (c*|encoded| + f)       , 0 <= |encoded| < d
> + *          = sign(encoded) * ((a*|encoded| + b)^g + e), d <= |encoded|

The code you have does not actually do the extended form (use of sign
and absolute value), but clamps the result to [0.0, 1.0].


> + *
> + * (A simple gamma transfer function sets g to gamma and a to 1.)
> + */
> +struct igt_color_tf {
> +    float g, a,b,c,d,e,f;
> +};
> +
> +const struct igt_color_tf srgb_tf = {2.4f, (float)(1/1.055), (float)(0.055/1.055), (float)(1/12.92), 0.04045f, 0, 0};
> +
> +/* end of Skia-based code */
> +
> +typedef struct igt_pixel {
> +	float r;
> +	float g;
> +	float b;
> +} igt_pixel_t;

Thanks,
pq

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

Re: [igt-dev] [RFC PATCH 5/7] igt/color: Add SW color transform functionality

[Harry Wentland]

On 2023-09-15 10:52, Pekka Paalanen wrote:
> On Fri, 8 Sep 2023 11:03:13 -0400
> Harry Wentland <harry.wentland@amd.com> wrote:
> 
>> In order to test color we want to compare a HW (KMS) transform
>> with a SW transform. This introduces color transform for an
>> sRGB EOTF but this can be extended to other transforms. Code is
>> borrowed from Skia.
>>
>> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
>> Cc: Simon Ser <contact@emersion.fr>
>> Cc: Harry Wentland <harry.wentland@amd.com>
>> Cc: Melissa Wen <mwen@igalia.com>
>> Cc: Jonas Ådahl <jadahl@redhat.com>
>> Cc: Sebastian Wick <sebastian.wick@redhat.com>
>> Cc: Shashank Sharma <shashank.sharma@amd.com>
>> Cc: Alexander Goins <agoins@nvidia.com>
>> Cc: Joshua Ashton <joshua@froggi.es>
>> Cc: Michel Dänzer <mdaenzer@redhat.com>
>> Cc: Aleix Pol <aleixpol@kde.org>
>> Cc: Xaver Hugl <xaver.hugl@gmail.com>
>> Cc: Victoria Brekenfeld <victoria@system76.com>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Cc: Uma Shankar <uma.shankar@intel.com>
>> Cc: Naseer Ahmed <quic_naseer@quicinc.com>
>> Cc: Christopher Braga <quic_cbraga@quicinc.com>
>> ---
>>  lib/igt_color.c | 330 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  lib/igt_color.h | 105 +++++++++++++++
>>  lib/igt_fb.c    |   6 +-
>>  lib/igt_fb.h    |   2 +
>>  lib/meson.build |   1 +
>>  5 files changed, 441 insertions(+), 3 deletions(-)
>>  create mode 100644 lib/igt_color.c
>>  create mode 100644 lib/igt_color.h
>>
>> diff --git a/lib/igt_color.c b/lib/igt_color.c
>> new file mode 100644
>> index 000000000000..12ff9ad1f324
>> --- /dev/null
>> +++ b/lib/igt_color.c
>> @@ -0,0 +1,330 @@
>> +/*
>> + * Copyright 2023 Advanced Micro Devices, Inc.
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>> + * OTHER DEALINGS IN THE SOFTWARE.
>> + */
>> +
>> +#include <errno.h>
>> +#include <math.h>
>> +
>> +#include "igt_color.h"
>> +#include "igt_core.h"
>> +#include "igt_x86.h"
>> +
>> +
>> +static float clamp(float val, float min, float max)
>> +{
>> +	return ((val < min) ? min : ((val > max) ? max : val));
>> +}
>> +
>> +/*
>> + * Below code is taken from Skia and adapted for style and needs of
>> + * IGT.
>> + *
>> + * https://chromium.googlesource.com/chromium/src/+/3e1a26c44c024d97dc9a4c09bbc6a2365398ca2c/ui/gfx/skia_color_space_util.cc#15
>> + *
>> + * To comply with the original code's license we'll include it, as well
>> + * as the original copyright here:
>> + *
>> + * // Copyright 2015 The Chromium Authors
>> + * //
>> + * // Redistribution and use in source and binary forms, with or without
>> + * // modification, are permitted provided that the following conditions are
>> + * // met:
>> + * //
>> + * //    * Redistributions of source code must retain the above copyright
>> + * // notice, this list of conditions and the following disclaimer.
>> + * //    * Redistributions in binary form must reproduce the above
>> + * // copyright notice, this list of conditions and the following disclaimer
>> + * // in the documentation and/or other materials provided with the
>> + * // distribution.
>> + * //    * Neither the name of Google LLC nor the names of its
>> + * // contributors may be used to endorse or promote products derived from
>> + * // this software without specific prior written permission.
>> + * //
>> + * // THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
>> + * // "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
>> + * // LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
>> + * // A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
>> + * // OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
>> + * // SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
>> + * // LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
>> + * // DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
>> + * // THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
>> + * // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
>> + * // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>> + */
>> +
>> +static float igt_color_tf_eval_unclamped(const struct igt_color_tf *fn, float x)
>> +{
>> +	if (x < fn->d)
>> +		return fn->c * x + fn->f;
>> +	return pow(fn->a * x + fn->b, fn->g) + fn->e;
>> +}
>> +
>> +static float igt_color_tf_eval(const struct igt_color_tf *fn, float x)
>> +{
>> +	float fn_at_x_unclamped = igt_color_tf_eval_unclamped(fn, x);
>> +	return clamp(fn_at_x_unclamped, 0.0f, 1.0f);
>> +}
> 
> ...
> 
>> +#if 0
>> +#define PRINT_PIXEL_CMP
>> +#endif
>> +
>> +bool igt_cmp_fb_component(uint16_t comp1, uint16_t comp2, uint8_t up, uint8_t down)
>> +{
>> +	int16_t diff = comp2 - comp1;
>> +
>> +	if ((diff < -down) || (diff > up)) {
>> +		printf("comp1 %x comp2 %x diff %d down %d, up %d\n", comp1, comp2, diff, -down, up);
>> +		return false;
>> +	}
>> +
>> +	return true;
>> +}
>> +
>> +bool igt_cmp_fb_pixels(igt_fb_t *fb1, igt_fb_t *fb2, uint8_t up, uint8_t down)
>> +{
>> +	uint32_t *ptr1, *ptr2;
>> +	uint32_t pixel1, pixel2, i, j;
>> +	bool matched = true;
>> +
>> +#ifdef PRINT_PIXEL_CMP
>> +	printf("hwhw: %s %d\n", __func__, __LINE__);
>> +#endif
>> +
>> +	ptr1 = igt_fb_map_buffer(fb1->fd, fb1);
>> +	ptr2 = igt_fb_map_buffer(fb2->fd, fb2);
>> +
>> +	igt_assert(fb1->drm_format == fb2->drm_format);
>> +	igt_assert(fb1->size == fb2->size);
>> +
>> +	for (i = 0; i < fb1->size / sizeof(uint32_t); i++) {
>> +		uint16_t mask = 0xff;
>> +		uint16_t shift = 8;
>> +
>> +		if (fb1->drm_format == DRM_FORMAT_XRGB2101010) {
>> +			/* ignore alpha */
>> +			pixel1 = ptr1[i] & ~0xc0000000;
>> +			pixel2 = ptr2[i] & ~0xc0000000;
>> +
>> +			mask = 0x3ff;
>> +			shift = 10;
>> +
>> +
>> +		} else if (fb1->drm_format == DRM_FORMAT_XRGB8888) {
>> +			/* ignore alpha */
>> +			pixel1 = ptr1[i] & ~0xff000000;
>> +			pixel2 = ptr2[i] & ~0xff000000;
>> +
>> +			mask = 0xff;
>> +			shift = 8;
>> +
>> +		} else {
>> +			pixel1 = ptr1[i];
>> +			pixel2 = ptr2[i];
>> +		}
>> +
>> +#ifdef PRINT_PIXEL_CMP
>> +		/* print first 5 pixels */
>> +		if (i < 5) {
>> +			printf("hwhw: %s %u\n", __func__, i);
>> +			printf("\t\traw_in\t%u\n", ptr1[i]);
>> +			printf("\t\traw_out\t%u\n", ptr2[i]);
>> +			printf("\t\tin\t%u\n", pixel1);
>> +			printf("\t\tout\t%u\n", pixel2);
>> +		}
>> +#endif
>> +
>> +		for (j = 0; j < 3; j++) {
>> +			uint16_t comp1 = (pixel1 >> (shift*j)) & mask;
>> +			uint16_t comp2 = (pixel2 >> (shift*j)) & mask;
>> +
>> +			if (!igt_cmp_fb_component(comp1, comp2, up, down)) {
>> +				/* TODO use proper log*/
>> +				printf("i %d j %d shift %d mask %x comp1 %x comp2 %x, pixel1 %x pixel2 %x\n",
>> +				       i, j, shift, mask, comp1, comp2, pixel1, pixel2);
>> +				return false;
>> +			}
>> +		}
>> +	}
>> +
>> +	igt_fb_unmap_buffer(fb1, ptr1);
>> +	igt_fb_unmap_buffer(fb2, ptr2);
>> +
>> +	return matched;
>> +}
> 
> Maybe you'd like to borrow some facilities from Weston for comparing
> images?
> 
> Statistics collecting:
> https://gitlab.freedesktop.org/wayland/weston/-/blob/71616edc4dfce1cf9deafee36009dd9c37a9ba8d/tests/color_util.c#L504-530
> https://gitlab.freedesktop.org/wayland/weston/-/blob/71616edc4dfce1cf9deafee36009dd9c37a9ba8d/tests/color_util.h#L159-178
> 
> Example usage of the above:
> https://gitlab.freedesktop.org/wayland/weston/-/blob/71616edc4dfce1cf9deafee36009dd9c37a9ba8d/tests/color-icc-output-test.c#L726-764
> 

Interesting. Thanks for sharing. That looks quite useful.

> 
> 
>> +
>> +
>> +void igt_dump_fb(igt_display_t *display, igt_fb_t *fb,
>> +		 const char *path_name, const char *file_name)
>> +{
>> +	char filepath_out[PATH_MAX];
>> +	cairo_surface_t *fb_surface_out;
>> +	cairo_status_t status;
>> +
>> +	snprintf(filepath_out, PATH_MAX, "%s/%s.png", path_name, file_name);
>> +	fb_surface_out = igt_get_cairo_surface(display->drm_fd, fb);
>> +	status = cairo_surface_write_to_png(fb_surface_out, filepath_out);
>> +	igt_assert_eq(status, CAIRO_STATUS_SUCCESS);
>> +	cairo_surface_destroy(fb_surface_out);
>> +}
>> \ No newline at end of file
>> diff --git a/lib/igt_color.h b/lib/igt_color.h
>> new file mode 100644
>> index 000000000000..fc136eecfd17
>> --- /dev/null
>> +++ b/lib/igt_color.h
>> @@ -0,0 +1,105 @@
>> +/*
>> + * Copyright 2023 Advanced Micro Devices, Inc.
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>> + * OTHER DEALINGS IN THE SOFTWARE.
>> + */
>> +
>> +
>> +#ifndef __IGT_COLOR_H__
>> +#define __IGT_COLOR_H__
>> +
>> +#include <limits.h>
>> +
>> +#include "igt_fb.h"
>> +#include "igt_kms.h"
>> +
>> +/*
>> + * Below code is taken from Skia and adapted for style and needs of
>> + * IGT.
>> + *
>> + * https://chromium.googlesource.com/chromium/src/+/3e1a26c44c024d97dc9a4c09bbc6a2365398ca2c/ui/gfx/skia_color_space_util.cc#15
>> + *
>> + * To comply with the original code's license we'll include it, as well
>> + * as the original copyright here:
>> + *
>> + * // Copyright 2015 The Chromium Authors
>> + * //
>> + * // Redistribution and use in source and binary forms, with or without
>> + * // modification, are permitted provided that the following conditions are
>> + * // met:
>> + * //
>> + * //    * Redistributions of source code must retain the above copyright
>> + * // notice, this list of conditions and the following disclaimer.
>> + * //    * Redistributions in binary form must reproduce the above
>> + * // copyright notice, this list of conditions and the following disclaimer
>> + * // in the documentation and/or other materials provided with the
>> + * // distribution.
>> + * //    * Neither the name of Google LLC nor the names of its
>> + * // contributors may be used to endorse or promote products derived from
>> + * // this software without specific prior written permission.
>> + * //
>> + * // THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
>> + * // "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
>> + * // LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
>> + * // A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
>> + * // OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
>> + * // SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
>> + * // LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
>> + * // DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
>> + * // THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
>> + * // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
>> + * // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>> + */
>> +/*
>> + * A transfer function mapping encoded values to linear values,
>> + * represented by this 7-parameter piecewise function:
>> + *
>> + *   linear = sign(encoded) *  (c*|encoded| + f)       , 0 <= |encoded| < d
>> + *          = sign(encoded) * ((a*|encoded| + b)^g + e), d <= |encoded|
> 
> The code you have does not actually do the extended form (use of sign
> and absolute value), but clamps the result to [0.0, 1.0].
> 

Yes, the intention (and I didn't write this but copied it) is to be able to use
this to describe most (all?) standard transfer functions. See
https://github.com/google/skia/blob/main/include/core/SkColorSpace.h

Harry

> 
>> + *
>> + * (A simple gamma transfer function sets g to gamma and a to 1.)
>> + */
>> +struct igt_color_tf {
>> +    float g, a,b,c,d,e,f;
>> +};
>> +
>> +const struct igt_color_tf srgb_tf = {2.4f, (float)(1/1.055), (float)(0.055/1.055), (float)(1/12.92), 0.04045f, 0, 0};
>> +
>> +/* end of Skia-based code */
>> +
>> +typedef struct igt_pixel {
>> +	float r;
>> +	float g;
>> +	float b;
>> +} igt_pixel_t;
> 
> Thanks,
> pq

Re: [igt-dev] [RFC PATCH 5/7] igt/color: Add SW color transform functionality

[Pekka Paalanen]

On Fri, 15 Sep 2023 15:50:52 -0400
Harry Wentland <harry.wentland@amd.com> wrote:

> On 2023-09-15 10:52, Pekka Paalanen wrote:
> > On Fri, 8 Sep 2023 11:03:13 -0400
> > Harry Wentland <harry.wentland@amd.com> wrote:
> >   
> >> In order to test color we want to compare a HW (KMS) transform
> >> with a SW transform. This introduces color transform for an
> >> sRGB EOTF but this can be extended to other transforms. Code is
> >> borrowed from Skia.
> >>
> >> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
> >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> >> Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
> >> Cc: Simon Ser <contact@emersion.fr>
> >> Cc: Harry Wentland <harry.wentland@amd.com>
> >> Cc: Melissa Wen <mwen@igalia.com>
> >> Cc: Jonas Ådahl <jadahl@redhat.com>
> >> Cc: Sebastian Wick <sebastian.wick@redhat.com>
> >> Cc: Shashank Sharma <shashank.sharma@amd.com>
> >> Cc: Alexander Goins <agoins@nvidia.com>
> >> Cc: Joshua Ashton <joshua@froggi.es>
> >> Cc: Michel Dänzer <mdaenzer@redhat.com>
> >> Cc: Aleix Pol <aleixpol@kde.org>
> >> Cc: Xaver Hugl <xaver.hugl@gmail.com>
> >> Cc: Victoria Brekenfeld <victoria@system76.com>
> >> Cc: Daniel Vetter <daniel@ffwll.ch>
> >> Cc: Uma Shankar <uma.shankar@intel.com>
> >> Cc: Naseer Ahmed <quic_naseer@quicinc.com>
> >> Cc: Christopher Braga <quic_cbraga@quicinc.com>
> >> ---
> >>  lib/igt_color.c | 330 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>  lib/igt_color.h | 105 +++++++++++++++
> >>  lib/igt_fb.c    |   6 +-
> >>  lib/igt_fb.h    |   2 +
> >>  lib/meson.build |   1 +
> >>  5 files changed, 441 insertions(+), 3 deletions(-)
> >>  create mode 100644 lib/igt_color.c
> >>  create mode 100644 lib/igt_color.h

...

> >> +/*
> >> + * A transfer function mapping encoded values to linear values,
> >> + * represented by this 7-parameter piecewise function:
> >> + *
> >> + *   linear = sign(encoded) *  (c*|encoded| + f)       , 0 <= |encoded| < d
> >> + *          = sign(encoded) * ((a*|encoded| + b)^g + e), d <= |encoded|  
> > 
> > The code you have does not actually do the extended form (use of sign
> > and absolute value), but clamps the result to [0.0, 1.0].
> >   
> 
> Yes, the intention (and I didn't write this but copied it) is to be able to use
> this to describe most (all?) standard transfer functions. See
> https://github.com/google/skia/blob/main/include/core/SkColorSpace.h

My complaint is about doc and implementation disagreeing.

xvYCC and integer-encoded scRGB would need an extended range if anyone
wanted to test those, if they even fit here. I also do not understand
how it can be possible to express PQ EOTF or HLG OETF with this
parameterisation. Maybe it's an approximation on a very limited domain?


Thanks,
pq

> >> + *
> >> + * (A simple gamma transfer function sets g to gamma and a to 1.)
> >> + */
> >> +struct igt_color_tf {
> >> +    float g, a,b,c,d,e,f;
> >> +};
> >> +
> >> +const struct igt_color_tf srgb_tf = {2.4f, (float)(1/1.055), (float)(0.055/1.055), (float)(1/12.92), 0.04045f, 0, 0};
> >> +
> >> +/* end of Skia-based code */
> >> +
> >> +typedef struct igt_pixel {
> >> +	float r;
> >> +	float g;
> >> +	float b;
> >> +} igt_pixel_t;  
> > 
> > Thanks,
> > pq  
> 

Re: [igt-dev] [RFC PATCH 5/7] igt/color: Add SW color transform functionality

[Kamil Konieczny]

Hi Harry,

On 2023-09-08 at 11:03:13 -0400, Harry Wentland wrote:
> In order to test color we want to compare a HW (KMS) transform
> with a SW transform. This introduces color transform for an
> sRGB EOTF but this can be extended to other transforms. Code is
> borrowed from Skia.

From what you wrote below it looks like you borrowed only few
functions from Skia. Am I missing something?

> 
> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
> Cc: Simon Ser <contact@emersion.fr>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Melissa Wen <mwen@igalia.com>
> Cc: Jonas Ådahl <jadahl@redhat.com>
> Cc: Sebastian Wick <sebastian.wick@redhat.com>
> Cc: Shashank Sharma <shashank.sharma@amd.com>
> Cc: Alexander Goins <agoins@nvidia.com>
> Cc: Joshua Ashton <joshua@froggi.es>
> Cc: Michel Dänzer <mdaenzer@redhat.com>
> Cc: Aleix Pol <aleixpol@kde.org>
> Cc: Xaver Hugl <xaver.hugl@gmail.com>
> Cc: Victoria Brekenfeld <victoria@system76.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Uma Shankar <uma.shankar@intel.com>
> Cc: Naseer Ahmed <quic_naseer@quicinc.com>
> Cc: Christopher Braga <quic_cbraga@quicinc.com>
> ---
>  lib/igt_color.c | 330 ++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/igt_color.h | 105 +++++++++++++++
>  lib/igt_fb.c    |   6 +-
>  lib/igt_fb.h    |   2 +
>  lib/meson.build |   1 +
>  5 files changed, 441 insertions(+), 3 deletions(-)
>  create mode 100644 lib/igt_color.c
>  create mode 100644 lib/igt_color.h
> 
> diff --git a/lib/igt_color.c b/lib/igt_color.c
> new file mode 100644
> index 000000000000..12ff9ad1f324
> --- /dev/null
> +++ b/lib/igt_color.c
> @@ -0,0 +1,330 @@
> +/*
> + * Copyright 2023 Advanced Micro Devices, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a

Use SPDX instead, look at other igt-gpu-tools sources for
guide how to use it in source.c and header.h

> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +#include <errno.h>
> +#include <math.h>
> +
> +#include "igt_color.h"
> +#include "igt_core.h"
> +#include "igt_x86.h"
> +
> +
> +static float clamp(float val, float min, float max)
> +{
> +	return ((val < min) ? min : ((val > max) ? max : val));
> +}
> +
> +/*
> + * Below code is taken from Skia and adapted for style and needs of
> + * IGT.
> + *
> + * https://chromium.googlesource.com/chromium/src/+/3e1a26c44c024d97dc9a4c09bbc6a2365398ca2c/ui/gfx/skia_color_space_util.cc#15
> + *
> + * To comply with the original code's license we'll include it, as well
> + * as the original copyright here:
> + *
> + * // Copyright 2015 The Chromium Authors
> + * //

imho this is strange, I am against this, either add Chromium
to copyright header at begin of file or delete this. What
exactly licence is this? GNU GPL? MIT?

+cc Pteri

Regards,
Kamil

> + * // Redistribution and use in source and binary forms, with or without
> + * // modification, are permitted provided that the following conditions are
> + * // met:
> + * //
> + * //    * Redistributions of source code must retain the above copyright
> + * // notice, this list of conditions and the following disclaimer.
> + * //    * Redistributions in binary form must reproduce the above
> + * // copyright notice, this list of conditions and the following disclaimer
> + * // in the documentation and/or other materials provided with the
> + * // distribution.
> + * //    * Neither the name of Google LLC nor the names of its
> + * // contributors may be used to endorse or promote products derived from
> + * // this software without specific prior written permission.
> + * //
> + * // THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * // "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * // LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * // A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * // OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * // SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * // LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * // DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * // THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +static float igt_color_tf_eval_unclamped(const struct igt_color_tf *fn, float x)
> +{
> +	if (x < fn->d)
> +		return fn->c * x + fn->f;
> +	return pow(fn->a * x + fn->b, fn->g) + fn->e;
> +}
> +
> +static float igt_color_tf_eval(const struct igt_color_tf *fn, float x)
> +{
> +	float fn_at_x_unclamped = igt_color_tf_eval_unclamped(fn, x);
> +	return clamp(fn_at_x_unclamped, 0.0f, 1.0f);
> +}
> +
> +#if 0
> +skcms_TransferFunction SkTransferFnInverse(const skcms_TransferFunction& fn) {
> +  skcms_TransferFunction fn_inv = {0};
> +  if (fn.a > 0 && fn.g > 0) {
> +    double a_to_the_g = std::pow(fn.a, fn.g);
> +    fn_inv.a = 1.f / a_to_the_g;
> +    fn_inv.b = -fn.e / a_to_the_g;
> +    fn_inv.g = 1.f / fn.g;
> +  }
> +  fn_inv.d = fn.c * fn.d + fn.f;
> +  fn_inv.e = -fn.b / fn.a;
> +  if (fn.c != 0) {
> +    fn_inv.c = 1.f / fn.c;
> +    fn_inv.f = -fn.f / fn.c;
> +  }
> +  return fn_inv;
> +}
> +#endif
> +
> +/* end of Skia-based code */
> +
> +static void igt_color_srgb_inv_eotf(igt_pixel_t *pixel)
> +{
> +}
> +
> +
> +
> +static void igt_color_srgb_eotf(igt_pixel_t *pixel)
> +{
> +	pixel->r = igt_color_tf_eval(&srgb_tf, pixel->r);
> +	pixel->g = igt_color_tf_eval(&srgb_tf, pixel->g);
> +	pixel->b = igt_color_tf_eval(&srgb_tf, pixel->b);
> +}
> +
> +int igt_color_transform_pixels(igt_fb_t *fb, igt_pixel_transform transform)
> +{
> +	uint32_t *line = NULL;
> +	void *map;
> +	char *ptr;
> +	int x, y, cpp = igt_drm_format_to_bpp(fb->drm_format) / 8;
> +	uint32_t stride = igt_fb_calc_plane_stride(fb, 0);
> +
> +	if (fb->num_planes != 1)
> +		return -EINVAL;
> +
> +	/* TODO expand for other formats */
> +	if (fb->drm_format != DRM_FORMAT_XRGB8888)
> +		return -EINVAL;
> +
> +	ptr = igt_fb_map_buffer(fb->fd, fb);
> +	igt_assert(ptr);
> +	map = ptr;
> +
> +	/*
> +	 * Framebuffers are often uncached, which can make byte-wise accesses
> +	 * very slow. We copy each line of the FB into a local buffer to speed
> +	 * up the hashing.
> +	 */
> +	line = malloc(stride);
> +	if (!line) {
> +		munmap(map, fb->size);
> +		return -ENOMEM;
> +	}
> +
> +	for (y = 0; y < fb->height; y++, ptr += stride) {
> +
> +		/* get line from buffer */
> +		igt_memcpy_from_wc(line, ptr, fb->width * cpp);
> +
> +		for (x = 0; x < fb->width; x++) {
> +			uint32_t raw_pixel = le32_to_cpu(line[x]);
> +			igt_pixel_t pixel;
> +			raw_pixel &= 0x00ffffff;
> +
> +			/*
> +			 * unpack pixel into igt_pixel_t
> +			 *
> +			 * only for XRGB8888 for now
> +			 *
> +			 * TODO add "generic" mechanism for unpacking
> +			 * other FB formats
> +			 */
> +			pixel.r = (raw_pixel & 0x00ff0000) >> 16;
> +			pixel.g = (raw_pixel & 0x0000ff00) >> 8;
> +			pixel.b = (raw_pixel & 0x000000ff);
> +
> +			/* normalize for 8-bit */
> +			pixel.r /= (0xff);
> +			pixel.g /= (0xff);
> +			pixel.b /= (0xff);
> +
> +			/* TODO use read_rgb from igt_fb? */
> +
> +			/* run transform on pixel */
> +			transform(&pixel);
> +
> +			/* de-normalize back to 8-bit */
> +			pixel.r *= (0xff);
> +			pixel.g *= (0xff);
> +			pixel.b *= (0xff);
> +
> +			/* re-pack pixel into FB*/
> +			raw_pixel = 0x0;
> +			raw_pixel |= ((uint8_t) (lround(pixel.r) & 0xff)) << 16;
> +			raw_pixel |= ((uint8_t) (lround(pixel.g) & 0xff)) << 8;
> +			raw_pixel |= ((uint8_t) (lround(pixel.b) & 0xff));
> +			/* TODO use write_rgb from igt_fb? */
> +
> +			/* write back to line */
> +			line[x] = cpu_to_le32(raw_pixel);
> +		}
> +
> +		/* copy line back to fb buffer */
> +		igt_memcpy_from_wc(ptr, line, fb->width * cpp);
> +	}
> +
> +	free(line);
> +	igt_fb_unmap_buffer(fb, map);
> +
> +	return 0;
> +
> +	/* for each pixel */
> +
> +	/* convert to float and create igt_pixel */
> +
> +	/* call transform */
> +
> +	/* convert back to fb format from igt_pixel */
> +
> +
> +}
> +
> +int igt_color_transform_srgb_eotf(igt_fb_t *fb)
> +{
> +	return igt_color_transform_pixels(fb, &igt_color_srgb_eotf);
> +}
> +
> +int igt_color_transform_srgb_inv_eotf(igt_fb_t *fb)
> +{
> +	return igt_color_transform_pixels(fb, &igt_color_srgb_inv_eotf);
> +}
> +
> +#if 0
> +#define PRINT_PIXEL_CMP
> +#endif
> +
> +bool igt_cmp_fb_component(uint16_t comp1, uint16_t comp2, uint8_t up, uint8_t down)
> +{
> +	int16_t diff = comp2 - comp1;
> +
> +	if ((diff < -down) || (diff > up)) {
> +		printf("comp1 %x comp2 %x diff %d down %d, up %d\n", comp1, comp2, diff, -down, up);
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +bool igt_cmp_fb_pixels(igt_fb_t *fb1, igt_fb_t *fb2, uint8_t up, uint8_t down)
> +{
> +	uint32_t *ptr1, *ptr2;
> +	uint32_t pixel1, pixel2, i, j;
> +	bool matched = true;
> +
> +#ifdef PRINT_PIXEL_CMP
> +	printf("hwhw: %s %d\n", __func__, __LINE__);
> +#endif
> +
> +	ptr1 = igt_fb_map_buffer(fb1->fd, fb1);
> +	ptr2 = igt_fb_map_buffer(fb2->fd, fb2);
> +
> +	igt_assert(fb1->drm_format == fb2->drm_format);
> +	igt_assert(fb1->size == fb2->size);
> +
> +	for (i = 0; i < fb1->size / sizeof(uint32_t); i++) {
> +		uint16_t mask = 0xff;
> +		uint16_t shift = 8;
> +
> +		if (fb1->drm_format == DRM_FORMAT_XRGB2101010) {
> +			/* ignore alpha */
> +			pixel1 = ptr1[i] & ~0xc0000000;
> +			pixel2 = ptr2[i] & ~0xc0000000;
> +
> +			mask = 0x3ff;
> +			shift = 10;
> +
> +
> +		} else if (fb1->drm_format == DRM_FORMAT_XRGB8888) {
> +			/* ignore alpha */
> +			pixel1 = ptr1[i] & ~0xff000000;
> +			pixel2 = ptr2[i] & ~0xff000000;
> +
> +			mask = 0xff;
> +			shift = 8;
> +
> +		} else {
> +			pixel1 = ptr1[i];
> +			pixel2 = ptr2[i];
> +		}
> +
> +#ifdef PRINT_PIXEL_CMP
> +		/* print first 5 pixels */
> +		if (i < 5) {
> +			printf("hwhw: %s %u\n", __func__, i);
> +			printf("\t\traw_in\t%u\n", ptr1[i]);
> +			printf("\t\traw_out\t%u\n", ptr2[i]);
> +			printf("\t\tin\t%u\n", pixel1);
> +			printf("\t\tout\t%u\n", pixel2);
> +		}
> +#endif
> +
> +		for (j = 0; j < 3; j++) {
> +			uint16_t comp1 = (pixel1 >> (shift*j)) & mask;
> +			uint16_t comp2 = (pixel2 >> (shift*j)) & mask;
> +
> +			if (!igt_cmp_fb_component(comp1, comp2, up, down)) {
> +				/* TODO use proper log*/
> +				printf("i %d j %d shift %d mask %x comp1 %x comp2 %x, pixel1 %x pixel2 %x\n",
> +				       i, j, shift, mask, comp1, comp2, pixel1, pixel2);
> +				return false;
> +			}
> +		}
> +	}
> +
> +	igt_fb_unmap_buffer(fb1, ptr1);
> +	igt_fb_unmap_buffer(fb2, ptr2);
> +
> +	return matched;
> +}
> +
> +
> +void igt_dump_fb(igt_display_t *display, igt_fb_t *fb,
> +		 const char *path_name, const char *file_name)
> +{
> +	char filepath_out[PATH_MAX];
> +	cairo_surface_t *fb_surface_out;
> +	cairo_status_t status;
> +
> +	snprintf(filepath_out, PATH_MAX, "%s/%s.png", path_name, file_name);
> +	fb_surface_out = igt_get_cairo_surface(display->drm_fd, fb);
> +	status = cairo_surface_write_to_png(fb_surface_out, filepath_out);
> +	igt_assert_eq(status, CAIRO_STATUS_SUCCESS);
> +	cairo_surface_destroy(fb_surface_out);
> +}
> \ No newline at end of file
> diff --git a/lib/igt_color.h b/lib/igt_color.h
> new file mode 100644
> index 000000000000..fc136eecfd17
> --- /dev/null
> +++ b/lib/igt_color.h
> @@ -0,0 +1,105 @@
> +/*
> + * Copyright 2023 Advanced Micro Devices, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +
> +#ifndef __IGT_COLOR_H__
> +#define __IGT_COLOR_H__
> +
> +#include <limits.h>
> +
> +#include "igt_fb.h"
> +#include "igt_kms.h"
> +
> +/*
> + * Below code is taken from Skia and adapted for style and needs of
> + * IGT.
> + *
> + * https://chromium.googlesource.com/chromium/src/+/3e1a26c44c024d97dc9a4c09bbc6a2365398ca2c/ui/gfx/skia_color_space_util.cc#15
> + *
> + * To comply with the original code's license we'll include it, as well
> + * as the original copyright here:
> + *
> + * // Copyright 2015 The Chromium Authors
> + * //
> + * // Redistribution and use in source and binary forms, with or without
> + * // modification, are permitted provided that the following conditions are
> + * // met:
> + * //
> + * //    * Redistributions of source code must retain the above copyright
> + * // notice, this list of conditions and the following disclaimer.
> + * //    * Redistributions in binary form must reproduce the above
> + * // copyright notice, this list of conditions and the following disclaimer
> + * // in the documentation and/or other materials provided with the
> + * // distribution.
> + * //    * Neither the name of Google LLC nor the names of its
> + * // contributors may be used to endorse or promote products derived from
> + * // this software without specific prior written permission.
> + * //
> + * // THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * // "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * // LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * // A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * // OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * // SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * // LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * // DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * // THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +/*
> + * A transfer function mapping encoded values to linear values,
> + * represented by this 7-parameter piecewise function:
> + *
> + *   linear = sign(encoded) *  (c*|encoded| + f)       , 0 <= |encoded| < d
> + *          = sign(encoded) * ((a*|encoded| + b)^g + e), d <= |encoded|
> + *
> + * (A simple gamma transfer function sets g to gamma and a to 1.)
> + */
> +struct igt_color_tf {
> +    float g, a,b,c,d,e,f;
> +};
> +
> +const struct igt_color_tf srgb_tf = {2.4f, (float)(1/1.055), (float)(0.055/1.055), (float)(1/12.92), 0.04045f, 0, 0};
> +
> +/* end of Skia-based code */
> +
> +typedef struct igt_pixel {
> +	float r;
> +	float g;
> +	float b;
> +} igt_pixel_t;
> +
> +bool igt_cmp_fb_component(uint16_t comp1, uint16_t comp2, uint8_t up, uint8_t down);
> +bool igt_cmp_fb_pixels(igt_fb_t *fb1, igt_fb_t *fb2, uint8_t up, uint8_t down);
> +
> +void igt_dump_fb(igt_display_t *display, igt_fb_t *fb, const char *path_name, const char *file_name);
> +
> +/* TODO also allow 64-bit pixels, or other weird sizes */
> +typedef void (*igt_pixel_transform)(igt_pixel_t *pixel);
> +
> +int igt_color_transform_pixels(igt_fb_t *fb, igt_pixel_transform transform);
> +
> +int igt_color_transform_srgb_eotf(igt_fb_t *fb);
> +int igt_color_transform_srgb_inv_eotf(igt_fb_t *fb);
> +
> +#endif
> \ No newline at end of file
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> index 0fe5b6adf8f4..0667d3fcbcff 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -739,7 +739,7 @@ void igt_init_fb(struct igt_fb *fb, int fd, int width, int height,
>  	}
>  }
>  
> -static uint32_t calc_plane_stride(struct igt_fb *fb, int plane)
> +uint32_t igt_fb_calc_plane_stride(struct igt_fb *fb, int plane)
>  {
>  	uint32_t min_stride = fb->plane_width[plane] *
>  		(fb->plane_bpp[plane] / 8);
> @@ -916,7 +916,7 @@ static uint64_t calc_fb_size(struct igt_fb *fb)
>  
>  		/* respect the stride requested by the caller */
>  		if (!fb->strides[plane])
> -			fb->strides[plane] = calc_plane_stride(fb, plane);
> +			fb->strides[plane] = igt_fb_calc_plane_stride(fb, plane);
>  
>  		align = get_plane_alignment(fb, plane);
>  		if (align)
> @@ -4471,7 +4471,7 @@ int igt_fb_get_fnv1a_crc(struct igt_fb *fb, igt_crc_t *crc)
>  	void *map;
>  	char *ptr;
>  	int x, y, cpp = igt_drm_format_to_bpp(fb->drm_format) / 8;
> -	uint32_t stride = calc_plane_stride(fb, 0);
> +	uint32_t stride = igt_fb_calc_plane_stride(fb, 0);
>  
>  	if (fb->num_planes != 1)
>  		return -EINVAL;
> diff --git a/lib/igt_fb.h b/lib/igt_fb.h
> index 73bdfc8669d8..b753c1580884 100644
> --- a/lib/igt_fb.h
> +++ b/lib/igt_fb.h
> @@ -170,6 +170,8 @@ int igt_dirty_fb(int fd, struct igt_fb *fb);
>  void *igt_fb_map_buffer(int fd, struct igt_fb *fb);
>  void igt_fb_unmap_buffer(struct igt_fb *fb, void *buffer);
>  
> +uint32_t igt_fb_calc_plane_stride(struct igt_fb *fb, int plane);
> +
>  void igt_create_bo_for_fb(int fd, int width, int height,
>  			  uint32_t format, uint64_t modifier,
>  			  struct igt_fb *fb);
> diff --git a/lib/meson.build b/lib/meson.build
> index 85f100f75510..7c9aa56d429c 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -97,6 +97,7 @@ lib_sources = [
>  	'igt_edid.c',
>  	'igt_eld.c',
>  	'igt_infoframe.c',
> +	'igt_color.c',
>  	'veboxcopy_gen12.c',
>  	'igt_msm.c',
>  	'igt_dsc.c',
> -- 
> 2.42.0
> 

Re: [igt-dev] [RFC PATCH 1/7] include/drm-uapi: Add COLOROP object

[Kamil Konieczny]

Hi Harry,

On 2023-09-08 at 11:03:09 -0400, Harry Wentland wrote:
> Signed-off-by: Harry Wentland <harry.wentland@amd.com>

Put here description from cover letter or at least links
to drm-uapi commits (prefereable lore.kernel.org) with subjects
cited (as links can break in future).

Regards,
Kamil

> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
> Cc: Simon Ser <contact@emersion.fr>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Melissa Wen <mwen@igalia.com>
> Cc: Jonas Ådahl <jadahl@redhat.com>
> Cc: Sebastian Wick <sebastian.wick@redhat.com>
> Cc: Shashank Sharma <shashank.sharma@amd.com>
> Cc: Alexander Goins <agoins@nvidia.com>
> Cc: Joshua Ashton <joshua@froggi.es>
> Cc: Michel Dänzer <mdaenzer@redhat.com>
> Cc: Aleix Pol <aleixpol@kde.org>
> Cc: Xaver Hugl <xaver.hugl@gmail.com>
> Cc: Victoria Brekenfeld <victoria@system76.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Uma Shankar <uma.shankar@intel.com>
> Cc: Naseer Ahmed <quic_naseer@quicinc.com>
> Cc: Christopher Braga <quic_cbraga@quicinc.com>
> ---
>  include/drm-uapi/drm_mode.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/drm-uapi/drm_mode.h b/include/drm-uapi/drm_mode.h
> index e4a2570a6058..e7e56150616b 100644
> --- a/include/drm-uapi/drm_mode.h
> +++ b/include/drm-uapi/drm_mode.h
> @@ -626,6 +626,7 @@ struct drm_mode_connector_set_property {
>  #define DRM_MODE_OBJECT_FB 0xfbfbfbfb
>  #define DRM_MODE_OBJECT_BLOB 0xbbbbbbbb
>  #define DRM_MODE_OBJECT_PLANE 0xeeeeeeee
> +#define DRM_MODE_OBJECT_COLOROP 0xfafafafa
>  #define DRM_MODE_OBJECT_ANY 0
>  
>  struct drm_mode_obj_get_properties {
> -- 
> 2.42.0
> 

Re: [igt-dev] [RFC PATCH 2/7] lib/igt_kms: Introduce drm_colorop object

[Kamil Konieczny]

Hi Harry,

On 2023-09-08 at 11:03:10 -0400, Harry Wentland wrote:
> Signed-off-by: Harry Wentland <harry.wentland@amd.com>

Please describe here what you added to lib.

> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
> Cc: Simon Ser <contact@emersion.fr>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Melissa Wen <mwen@igalia.com>
> Cc: Jonas Ådahl <jadahl@redhat.com>
> Cc: Sebastian Wick <sebastian.wick@redhat.com>
> Cc: Shashank Sharma <shashank.sharma@amd.com>
> Cc: Alexander Goins <agoins@nvidia.com>
> Cc: Joshua Ashton <joshua@froggi.es>
> Cc: Michel Dänzer <mdaenzer@redhat.com>
> Cc: Aleix Pol <aleixpol@kde.org>
> Cc: Xaver Hugl <xaver.hugl@gmail.com>
> Cc: Victoria Brekenfeld <victoria@system76.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Uma Shankar <uma.shankar@intel.com>
> Cc: Naseer Ahmed <quic_naseer@quicinc.com>
> Cc: Christopher Braga <quic_cbraga@quicinc.com>

Do you need to repeat this exetensive list of Cc?
imho it should be enough to add this to cover letter
and to drm-uapi patch 1/7.

> ---
>  lib/igt_kms.c | 184 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  lib/igt_kms.h |  91 +++++++++++++++++++++++++
>  2 files changed, 273 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index c548414867c2..bb6f2b47e243 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -617,6 +617,13 @@ const char * const igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = {
>  	[IGT_PLANE_SCALING_FILTER] = "SCALING_FILTER",
>  };
>  
> +const char * const igt_colorop_prop_names[IGT_NUM_COLOROP_PROPS] = {
> +	[IGT_COLOROP_TYPE] = "TYPE",
> +	[IGT_COLOROP_BYPASS] = "BYPASS",
> +	[IGT_COLOROP_CURVE_1D_TYPE] = "CURVE_1D_TYPE",
> +	[IGT_COLOROP_NEXT] = "NEXT",
> +};
> +
>  const char * const igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
>  	[IGT_CRTC_CTM] = "CTM",
>  	[IGT_CRTC_GAMMA_LUT] = "GAMMA_LUT",
> @@ -682,6 +689,21 @@ igt_plane_rotations(igt_display_t *display, igt_plane_t *plane,
>  	return rotations;
>  }
>  

Describe each public function added to lib.

> +igt_colorop_t *igt_find_colorop(igt_display_t *display, uint32_t id)
> +{
> +	int i;
> +
> +	/* find corresponding igt_colorop */
> +	for (i = 0; i < display->n_colorops; ++i) {
> +		igt_colorop_t *colorop = &display->colorops[i];
> +
> +		if (colorop->drm_colorop->colorop_id == id)
> +			return colorop;
> +	}
> +
> +	return NULL;
> +}
> +
>  /*
>   * Retrieve all the properies specified in props_name and store them into
>   * plane->props.
> @@ -722,6 +744,40 @@ igt_fill_plane_props(igt_display_t *display, igt_plane_t *plane,
>  	drmModeFreeObjectProperties(props);
>  }
>  
> +/*
> + * Retrieve all the properies specified in props_name and store them into
> + * colorop->props.
> + */
> +static void
> +igt_fill_colorop_props(igt_display_t *display, igt_colorop_t *colorop,
> +		       int num_props, const char * const prop_names[])
> +{
> +	drmModeObjectPropertiesPtr props;
> +	int i, j, fd;
> +
> +	fd = display->drm_fd;
> +
> +	props = drmModeObjectGetProperties(fd, colorop->drm_colorop->colorop_id, DRM_MODE_OBJECT_COLOROP);
> +	igt_assert(props);
> +
> +	for (i = 0; i < props->count_props; i++) {
> +		drmModePropertyPtr prop =
> +			drmModeGetProperty(fd, props->props[i]);
> +
> +		for (j = 0; j < num_props; j++) {
> +			if (strcmp(prop->name, prop_names[j]) != 0)
> +				continue;
> +
> +			colorop->props[j] = props->props[i];
> +			break;
> +		}
> +
> +		drmModeFreeProperty(prop);
> +	}
> +
> +	drmModeFreeObjectProperties(props);
> +}
> +
>  /*
>   * Retrieve all the properies specified in props_name and store them into
>   * config->atomic_props_crtc and config->atomic_props_connector.
> @@ -2624,7 +2680,8 @@ void igt_display_require(igt_display_t *display, int drm_fd)
>  {
>  	drmModeRes *resources;
>  	drmModePlaneRes *plane_resources;
> -	int i;
> +	drmModeColoropRes *colorop_resources;
> +	int i, j;
>  	bool is_intel_dev;
>  
>  	memset(display, 0, sizeof(igt_display_t));
> @@ -2709,17 +2766,47 @@ void igt_display_require(igt_display_t *display, int drm_fd)
>  
>  	drmModeFreePlaneResources(plane_resources);
>  
> +	/* get all colorop resources */
> +	colorop_resources = drmModeGetColoropResources(display->drm_fd);
> +
> +	if (colorop_resources) {
> +		display->n_colorops = colorop_resources->count_colorops;
> +		display->colorops = calloc(sizeof(igt_colorop_t), display->n_colorops);
> +		igt_assert_f(display->colorops, "Failed to allocate memory for %d colorops\n", display->n_colorops);
> +
> +		for (i = 0; i < colorop_resources->count_colorops; ++i) {
> +			igt_colorop_t *colorop = &display->colorops[i];
> +			uint32_t id = colorop_resources->colorops[i];
> +
> +			colorop->drm_colorop = drmModeGetColorop(display->drm_fd, id);
> +			igt_assert(colorop->drm_colorop);
> +
> +			igt_fill_colorop_props(display, colorop, IGT_NUM_COLOROP_PROPS, igt_colorop_prop_names);
> +
> +			for (j = 0; j < display->n_planes; ++j)
> +				if (display->planes[j].drm_plane->plane_id == colorop->drm_colorop->plane_id)
> +					colorop->plane = &display->planes[j];
> +
> +		}
> +
> +
> +
> +		drmModeFreeColoropResources(colorop_resources);
> +	}
> +
> +

Remove one newline.

>  	for_each_pipe(display, i) {
>  		igt_pipe_t *pipe = &display->pipes[i];
>  		igt_plane_t *plane;
>  		int p = 1, crtc_mask = 0;
> -		int j, type;
----------- ^
Why dropped 'j'?

> +		int type;
>  		uint8_t last_plane = 0, n_planes = 0;
>  
>  		pipe->display = display;
>  		pipe->plane_cursor = -1;
>  		pipe->plane_primary = -1;
>  		pipe->planes = NULL;
> +		pipe->colorops = NULL;
>  		pipe->num_primary_planes = 0;
>  
>  		igt_fill_pipe_props(display, pipe, IGT_NUM_CRTC_PROPS, igt_crtc_prop_names);
> @@ -3001,6 +3088,15 @@ void igt_display_fini(igt_display_t *display)
>  	if (is_xe_device(drm_fd))
>  		xe_device_put(drm_fd);
>  
> +	for (i = 0; i < display->n_colorops; ++i) {
> +		igt_colorop_t *colorop = &display->colorops[i];
> +
> +		if (colorop->drm_colorop) {
> +			drmModeFreeColorop(colorop->drm_colorop);
> +			colorop->drm_colorop = NULL;
> +		}
> +	}
> +
>  	for (i = 0; i < display->n_planes; ++i) {
>  		igt_plane_t *plane = &display->planes[i];
>  
> @@ -3336,6 +3432,43 @@ igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
>  	}
>  }
>  
> +

Remove newline.

> +/*
> + * Add colorop properties
> + */
> +static void
> +igt_atomic_prepare_colorop_commit(igt_colorop_t *colorop, igt_pipe_t *pipe,
> +	drmModeAtomicReq *req)
> +{
> +	igt_display_t *display = pipe->display;
> +	int i;
> +
> +	igt_assert(colorop->drm_colorop);
> +
> +	LOG(display,
> +	    "populating colorop data: %s.%d\n",
> +	    kmstest_pipe_name(pipe->pipe),
> +	    colorop->drm_colorop->colorop_id);
> +
> +	/* TODO handle future colorop->next pointer */
> +
> +	for (i = 0; i < IGT_NUM_COLOROP_PROPS; i++) {
> +		if (!igt_colorop_is_prop_changed(colorop, i))
> +			continue;
> +
> +		/* it's an error to try an unsupported feature */
> +		igt_assert(colorop->props[i]);
> +
> +		igt_debug("colorop %s.%d: Setting property \"%s\" to 0x%"PRIx64"/%"PRIi64"\n",
> +			kmstest_pipe_name(pipe->pipe), colorop->drm_colorop->colorop_id, igt_colorop_prop_names[i],
> +			colorop->values[i], colorop->values[i]);
> +
> +		igt_assert_lt(0, drmModeAtomicAddProperty(req, colorop->drm_colorop->colorop_id,
> +						  colorop->props[i],
> +						  colorop->values[i]));
> +	}
> +}
> +
>  /*
>   * Properties that can be changed through legacy SetProperty:
>   * - Obviously not the XYWH SRC/CRTC coordinates.
> @@ -3783,6 +3916,25 @@ uint64_t igt_plane_get_prop(igt_plane_t *plane, enum igt_atomic_plane_properties
>  					plane->drm_plane->plane_id, plane->props[prop]);
>  }
>  
> +/**
> + * igt_colorop_get_prop:
> + * @colorop: Target colorop.
> + * @prop: Property to check.
> + *
> + * Return current value on a colorop for a given property.
> + *
> + * Returns: The value the property is set to, if this
> + * is a blob, the blob id is returned. This can be passed
> + * to drmModeGetPropertyBlob() to get the contents of the blob.
> + */
> +uint64_t igt_colorop_get_prop(igt_display_t *display, igt_colorop_t *colorop, enum igt_atomic_colorop_properties prop)
> +{
> +	igt_assert(igt_colorop_has_prop(colorop, prop));
> +
> +	return igt_mode_object_get_prop(display, DRM_MODE_OBJECT_COLOROP,
> +					colorop->drm_colorop->colorop_id, colorop->props[prop]);
> +}
> +
>  static bool igt_mode_object_get_prop_enum_value(int drm_fd, uint32_t id, const char *str, uint64_t *val)
>  {
>  	drmModePropertyPtr prop = drmModeGetProperty(drm_fd, id);
> @@ -3825,6 +3977,34 @@ void igt_plane_set_prop_enum(igt_plane_t *plane,
>  	igt_assert(igt_plane_try_prop_enum(plane, prop, val));
>  }
>  

Add description to public function.

> +bool igt_colorop_try_prop_enum(igt_display_t *display,
> +			       igt_colorop_t *colorop,
> +			       enum igt_atomic_colorop_properties prop,
> +			       const char *val)
> +{
> +	uint64_t uval;
> +
> +	igt_assert(colorop->props[prop]);
> +
> +	if (!igt_mode_object_get_prop_enum_value(display->drm_fd,
> +						 colorop->props[prop], val, &uval))
> +		return false;
> +
> +	igt_colorop_set_prop_value(colorop, prop, uval);
> +	return true;
> +}
> +

Same here.

> +void igt_colorop_set_prop_enum(igt_display_t *display,
> +			       igt_colorop_t *colorop,
> +			       enum igt_atomic_colorop_properties prop,
> +			       const char *val)
> +{
> +	bool result = false;
> +	result = igt_colorop_try_prop_enum(display, colorop, prop, val);
> +	igt_assert(result);
> +}
> +
> +

Remove newline.

>  /**
>   * igt_plane_replace_prop_blob:
>   * @plane: plane to set property on.
> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> index 1b6988c17aeb..6cf50b3c8280 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -321,6 +321,19 @@ enum igt_atomic_plane_properties {
>         IGT_NUM_PLANE_PROPS
>  };
>  
> +enum igt_atomic_colorop_properties {
> +	IGT_COLOROP_TYPE,
> +	IGT_COLOROP_BYPASS,
> +	IGT_COLOROP_CURVE_1D_TYPE,
> +	IGT_COLOROP_NEXT,
> +	IGT_NUM_COLOROP_PROPS
> +};
> +
> +/* TODO move to libdrm header?? */
> +enum drm_colorop_type {
> +	DRM_COLOROP_1D_CURVE
> +};
> +
>  /**
>   * igt_plane_prop_names
>   *
> @@ -329,6 +342,14 @@ enum igt_atomic_plane_properties {
>   */
>  extern const char * const igt_plane_prop_names[];
>  
> +/**
> + * igt_colorop_prop_names
> + *
> + * igt_colorop_prop_names contains a list of colorop property names,
> + * as indexed by the igt_atomic_colorop_properties enum.
> + */
> +extern const char * const igt_colorop_prop_names[];
> +
>  typedef struct igt_display igt_display_t;
>  typedef struct igt_pipe igt_pipe_t;
>  typedef uint32_t igt_fixed_t;			/* 16.16 fixed point */
> @@ -351,6 +372,24 @@ static inline bool igt_rotation_90_or_270(igt_rotation_t rotation)
>  	return rotation & (IGT_ROTATION_90 | IGT_ROTATION_270);
>  }
>  
> +typedef struct igt_plane igt_plane_t;
----------------------------^
It should not be needed, it is already in header igt_kms.h

> +
> +typedef struct igt_colorop {
> +	/*< private >*/
> +	igt_pipe_t *pipe;
> +
> +	drmModeColorop *drm_colorop;
> +
> +	igt_plane_t *plane;
> +
> +	char name[DRM_PROP_NAME_LEN];
> +
> +	uint64_t changed;
> +	uint32_t props[IGT_NUM_COLOROP_PROPS];
> +	uint64_t values[IGT_NUM_COLOROP_PROPS];
> +
> +} igt_colorop_t;
> +
>  typedef struct igt_plane {
>  	/*< private >*/
>  	igt_pipe_t *pipe;
> @@ -404,6 +443,7 @@ struct igt_pipe {
>  	int plane_cursor;
>  	int plane_primary;
>  	igt_plane_t *planes;
> +	igt_colorop_t *colorops;
>  
>  	uint64_t changed;
>  	uint32_t props[IGT_NUM_CRTC_PROPS];
> @@ -442,9 +482,11 @@ struct igt_display {
>  	int log_shift;
>  	int n_pipes;
>  	int n_planes;
> +	int n_colorops;
>  	int n_outputs;
>  	igt_output_t *outputs;
>  	igt_plane_t *planes;
> +	igt_colorop_t *colorops;
>  	igt_pipe_t *pipes;
>  	bool has_cursor_plane;
>  	bool is_atomic;
> @@ -721,6 +763,53 @@ extern void igt_plane_set_prop_enum(igt_plane_t *plane,
>  				    enum igt_atomic_plane_properties prop,
>  				    const char *val);
>  
> +
> +

Remove newlines.

Regards,
Kamil

> +extern bool igt_plane_is_valid_colorop(igt_plane_t *plane, igt_colorop_t *colorop);
> +
> +/**
> + * igt_colorop_has_prop:
> + * @colorop: colorop to check.
> + * @prop: Property to check.
> + *
> + * Check whether colorop supports a given property.
> + *
> + * Returns: True if the property is supported, otherwise false.
> + */
> +static inline bool
> +igt_colorop_has_prop(igt_colorop_t *colorop, enum igt_atomic_colorop_properties prop)
> +{
> +	return colorop->props[prop];
> +}
> +
> +uint64_t igt_colorop_get_prop(igt_display_t *display, igt_colorop_t *colorop, enum igt_atomic_colorop_properties prop);
> +
> +#define igt_colorop_is_prop_changed(colorop, prop) \
> +	(!!((colorop)->changed & (1 << (prop))))
> +
> +#define igt_colorop_set_prop_changed(colorop, prop) \
> +	(colorop)->changed |= 1 << (prop)
> +
> +#define igt_colorop_clear_prop_changed(colorop, prop) \
> +	(colorop)->changed &= ~(1 << (prop))
> +
> +#define igt_colorop_set_prop_value(colorop, prop, value) \
> +	do { \
> +		colorop->values[prop] = value; \
> +		igt_colorop_set_prop_changed(colorop, prop); \
> +	} while (0)
> +
> +
> +extern bool igt_colorop_try_prop_enum(igt_display_t *display,
> +				      igt_colorop_t *colorop,
> +				      enum igt_atomic_colorop_properties prop,
> +				      const char *val);
> +
> +extern void igt_colorop_set_prop_enum(igt_display_t *display,
> +				      igt_colorop_t *colorop,
> +				      enum igt_atomic_colorop_properties prop,
> +				      const char *val);
> +
>  extern void igt_plane_replace_prop_blob(igt_plane_t *plane,
>  					enum igt_atomic_plane_properties prop,
>  					const void *ptr, size_t length);
> @@ -1005,4 +1094,6 @@ bool igt_check_bigjoiner_support(igt_display_t *display);
>  bool igt_parse_mode_string(const char *mode_string, drmModeModeInfo *mode);
>  bool i915_pipe_output_combo_valid(igt_display_t *display);
>  
> +igt_colorop_t *igt_find_colorop(igt_display_t *display, uint32_t id);
> +
>  #endif /* __IGT_KMS_H__ */
> -- 
> 2.42.0
> 

Re: [igt-dev] [RFC PATCH 2/7] lib/igt_kms: Introduce drm_colorop object

[Harry Wentland]

On 2023-09-18 08:48, Kamil Konieczny wrote:
> Hi Harry,
> 
> On 2023-09-08 at 11:03:10 -0400, Harry Wentland wrote:
>> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
> 
> Please describe here what you added to lib.
> 
>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
>> Cc: Simon Ser <contact@emersion.fr>
>> Cc: Harry Wentland <harry.wentland@amd.com>
>> Cc: Melissa Wen <mwen@igalia.com>
>> Cc: Jonas Ådahl <jadahl@redhat.com>
>> Cc: Sebastian Wick <sebastian.wick@redhat.com>
>> Cc: Shashank Sharma <shashank.sharma@amd.com>
>> Cc: Alexander Goins <agoins@nvidia.com>
>> Cc: Joshua Ashton <joshua@froggi.es>
>> Cc: Michel Dänzer <mdaenzer@redhat.com>
>> Cc: Aleix Pol <aleixpol@kde.org>
>> Cc: Xaver Hugl <xaver.hugl@gmail.com>
>> Cc: Victoria Brekenfeld <victoria@system76.com>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Cc: Uma Shankar <uma.shankar@intel.com>
>> Cc: Naseer Ahmed <quic_naseer@quicinc.com>
>> Cc: Christopher Braga <quic_cbraga@quicinc.com>
> 
> Do you need to repeat this exetensive list of Cc?
> imho it should be enough to add this to cover letter
> and to drm-uapi patch 1/7.
> 

I don't need to but I don't know what people prefer? This whole set
only makes sense as a set in its entirety. My own experience is that
I get confused if I'm CC'd in some patches and don't see the others
because they get auto-filtered to my IGT folder. Maybe that's a
me-problem, but I was hoping to avoid similar confusion for readers
of this series.

An alternative to email is a gitlab MR that avoids this entirely.

>> ---
>>  lib/igt_kms.c | 184 +++++++++++++++++++++++++++++++++++++++++++++++++-
>>  lib/igt_kms.h |  91 +++++++++++++++++++++++++
>>  2 files changed, 273 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>> index c548414867c2..bb6f2b47e243 100644
>> --- a/lib/igt_kms.c
>> +++ b/lib/igt_kms.c
>> @@ -617,6 +617,13 @@ const char * const igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = {
>>  	[IGT_PLANE_SCALING_FILTER] = "SCALING_FILTER",
>>  };
>>  
>> +const char * const igt_colorop_prop_names[IGT_NUM_COLOROP_PROPS] = {
>> +	[IGT_COLOROP_TYPE] = "TYPE",
>> +	[IGT_COLOROP_BYPASS] = "BYPASS",
>> +	[IGT_COLOROP_CURVE_1D_TYPE] = "CURVE_1D_TYPE",
>> +	[IGT_COLOROP_NEXT] = "NEXT",
>> +};
>> +
>>  const char * const igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
>>  	[IGT_CRTC_CTM] = "CTM",
>>  	[IGT_CRTC_GAMMA_LUT] = "GAMMA_LUT",
>> @@ -682,6 +689,21 @@ igt_plane_rotations(igt_display_t *display, igt_plane_t *plane,
>>  	return rotations;
>>  }
>>  
> 
> Describe each public function added to lib.
> 
>> +igt_colorop_t *igt_find_colorop(igt_display_t *display, uint32_t id)
>> +{
>> +	int i;
>> +
>> +	/* find corresponding igt_colorop */
>> +	for (i = 0; i < display->n_colorops; ++i) {
>> +		igt_colorop_t *colorop = &display->colorops[i];
>> +
>> +		if (colorop->drm_colorop->colorop_id == id)
>> +			return colorop;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>>  /*
>>   * Retrieve all the properies specified in props_name and store them into
>>   * plane->props.
>> @@ -722,6 +744,40 @@ igt_fill_plane_props(igt_display_t *display, igt_plane_t *plane,
>>  	drmModeFreeObjectProperties(props);
>>  }
>>  
>> +/*
>> + * Retrieve all the properies specified in props_name and store them into
>> + * colorop->props.
>> + */
>> +static void
>> +igt_fill_colorop_props(igt_display_t *display, igt_colorop_t *colorop,
>> +		       int num_props, const char * const prop_names[])
>> +{
>> +	drmModeObjectPropertiesPtr props;
>> +	int i, j, fd;
>> +
>> +	fd = display->drm_fd;
>> +
>> +	props = drmModeObjectGetProperties(fd, colorop->drm_colorop->colorop_id, DRM_MODE_OBJECT_COLOROP);
>> +	igt_assert(props);
>> +
>> +	for (i = 0; i < props->count_props; i++) {
>> +		drmModePropertyPtr prop =
>> +			drmModeGetProperty(fd, props->props[i]);
>> +
>> +		for (j = 0; j < num_props; j++) {
>> +			if (strcmp(prop->name, prop_names[j]) != 0)
>> +				continue;
>> +
>> +			colorop->props[j] = props->props[i];
>> +			break;
>> +		}
>> +
>> +		drmModeFreeProperty(prop);
>> +	}
>> +
>> +	drmModeFreeObjectProperties(props);
>> +}
>> +
>>  /*
>>   * Retrieve all the properies specified in props_name and store them into
>>   * config->atomic_props_crtc and config->atomic_props_connector.
>> @@ -2624,7 +2680,8 @@ void igt_display_require(igt_display_t *display, int drm_fd)
>>  {
>>  	drmModeRes *resources;
>>  	drmModePlaneRes *plane_resources;
>> -	int i;
>> +	drmModeColoropRes *colorop_resources;
>> +	int i, j;
>>  	bool is_intel_dev;
>>  
>>  	memset(display, 0, sizeof(igt_display_t));
>> @@ -2709,17 +2766,47 @@ void igt_display_require(igt_display_t *display, int drm_fd)
>>  
>>  	drmModeFreePlaneResources(plane_resources);
>>  
>> +	/* get all colorop resources */
>> +	colorop_resources = drmModeGetColoropResources(display->drm_fd);
>> +
>> +	if (colorop_resources) {
>> +		display->n_colorops = colorop_resources->count_colorops;
>> +		display->colorops = calloc(sizeof(igt_colorop_t), display->n_colorops);
>> +		igt_assert_f(display->colorops, "Failed to allocate memory for %d colorops\n", display->n_colorops);
>> +
>> +		for (i = 0; i < colorop_resources->count_colorops; ++i) {
>> +			igt_colorop_t *colorop = &display->colorops[i];
>> +			uint32_t id = colorop_resources->colorops[i];
>> +
>> +			colorop->drm_colorop = drmModeGetColorop(display->drm_fd, id);
>> +			igt_assert(colorop->drm_colorop);
>> +
>> +			igt_fill_colorop_props(display, colorop, IGT_NUM_COLOROP_PROPS, igt_colorop_prop_names);
>> +
>> +			for (j = 0; j < display->n_planes; ++j)
>> +				if (display->planes[j].drm_plane->plane_id == colorop->drm_colorop->plane_id)
>> +					colorop->plane = &display->planes[j];
>> +
>> +		}
>> +
>> +
>> +
>> +		drmModeFreeColoropResources(colorop_resources);
>> +	}
>> +
>> +
> 
> Remove one newline.
> 
>>  	for_each_pipe(display, i) {
>>  		igt_pipe_t *pipe = &display->pipes[i];
>>  		igt_plane_t *plane;
>>  		int p = 1, crtc_mask = 0;
>> -		int j, type;
> ----------- ^
> Why dropped 'j'?
> 

This moved outside since I needed it in another loop. But that code
has been changed in the next version of this patch, so this isn't
needed anymore.

>> +		int type;
>>  		uint8_t last_plane = 0, n_planes = 0;
>>  
>>  		pipe->display = display;
>>  		pipe->plane_cursor = -1;
>>  		pipe->plane_primary = -1;
>>  		pipe->planes = NULL;
>> +		pipe->colorops = NULL;
>>  		pipe->num_primary_planes = 0;
>>  
>>  		igt_fill_pipe_props(display, pipe, IGT_NUM_CRTC_PROPS, igt_crtc_prop_names);
>> @@ -3001,6 +3088,15 @@ void igt_display_fini(igt_display_t *display)
>>  	if (is_xe_device(drm_fd))
>>  		xe_device_put(drm_fd);
>>  
>> +	for (i = 0; i < display->n_colorops; ++i) {
>> +		igt_colorop_t *colorop = &display->colorops[i];
>> +
>> +		if (colorop->drm_colorop) {
>> +			drmModeFreeColorop(colorop->drm_colorop);
>> +			colorop->drm_colorop = NULL;
>> +		}
>> +	}
>> +
>>  	for (i = 0; i < display->n_planes; ++i) {
>>  		igt_plane_t *plane = &display->planes[i];
>>  
>> @@ -3336,6 +3432,43 @@ igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
>>  	}
>>  }
>>  
>> +
> 
> Remove newline.
> 
>> +/*
>> + * Add colorop properties
>> + */
>> +static void
>> +igt_atomic_prepare_colorop_commit(igt_colorop_t *colorop, igt_pipe_t *pipe,
>> +	drmModeAtomicReq *req)
>> +{
>> +	igt_display_t *display = pipe->display;
>> +	int i;
>> +
>> +	igt_assert(colorop->drm_colorop);
>> +
>> +	LOG(display,
>> +	    "populating colorop data: %s.%d\n",
>> +	    kmstest_pipe_name(pipe->pipe),
>> +	    colorop->drm_colorop->colorop_id);
>> +
>> +	/* TODO handle future colorop->next pointer */
>> +
>> +	for (i = 0; i < IGT_NUM_COLOROP_PROPS; i++) {
>> +		if (!igt_colorop_is_prop_changed(colorop, i))
>> +			continue;
>> +
>> +		/* it's an error to try an unsupported feature */
>> +		igt_assert(colorop->props[i]);
>> +
>> +		igt_debug("colorop %s.%d: Setting property \"%s\" to 0x%"PRIx64"/%"PRIi64"\n",
>> +			kmstest_pipe_name(pipe->pipe), colorop->drm_colorop->colorop_id, igt_colorop_prop_names[i],
>> +			colorop->values[i], colorop->values[i]);
>> +
>> +		igt_assert_lt(0, drmModeAtomicAddProperty(req, colorop->drm_colorop->colorop_id,
>> +						  colorop->props[i],
>> +						  colorop->values[i]));
>> +	}
>> +}
>> +
>>  /*
>>   * Properties that can be changed through legacy SetProperty:
>>   * - Obviously not the XYWH SRC/CRTC coordinates.
>> @@ -3783,6 +3916,25 @@ uint64_t igt_plane_get_prop(igt_plane_t *plane, enum igt_atomic_plane_properties
>>  					plane->drm_plane->plane_id, plane->props[prop]);
>>  }
>>  
>> +/**
>> + * igt_colorop_get_prop:
>> + * @colorop: Target colorop.
>> + * @prop: Property to check.
>> + *
>> + * Return current value on a colorop for a given property.
>> + *
>> + * Returns: The value the property is set to, if this
>> + * is a blob, the blob id is returned. This can be passed
>> + * to drmModeGetPropertyBlob() to get the contents of the blob.
>> + */
>> +uint64_t igt_colorop_get_prop(igt_display_t *display, igt_colorop_t *colorop, enum igt_atomic_colorop_properties prop)
>> +{
>> +	igt_assert(igt_colorop_has_prop(colorop, prop));
>> +
>> +	return igt_mode_object_get_prop(display, DRM_MODE_OBJECT_COLOROP,
>> +					colorop->drm_colorop->colorop_id, colorop->props[prop]);
>> +}
>> +
>>  static bool igt_mode_object_get_prop_enum_value(int drm_fd, uint32_t id, const char *str, uint64_t *val)
>>  {
>>  	drmModePropertyPtr prop = drmModeGetProperty(drm_fd, id);
>> @@ -3825,6 +3977,34 @@ void igt_plane_set_prop_enum(igt_plane_t *plane,
>>  	igt_assert(igt_plane_try_prop_enum(plane, prop, val));
>>  }
>>  
> 
> Add description to public function.
> 
>> +bool igt_colorop_try_prop_enum(igt_display_t *display,
>> +			       igt_colorop_t *colorop,
>> +			       enum igt_atomic_colorop_properties prop,
>> +			       const char *val)
>> +{
>> +	uint64_t uval;
>> +
>> +	igt_assert(colorop->props[prop]);
>> +
>> +	if (!igt_mode_object_get_prop_enum_value(display->drm_fd,
>> +						 colorop->props[prop], val, &uval))
>> +		return false;
>> +
>> +	igt_colorop_set_prop_value(colorop, prop, uval);
>> +	return true;
>> +}
>> +
> 
> Same here.
> 
>> +void igt_colorop_set_prop_enum(igt_display_t *display,
>> +			       igt_colorop_t *colorop,
>> +			       enum igt_atomic_colorop_properties prop,
>> +			       const char *val)
>> +{
>> +	bool result = false;
>> +	result = igt_colorop_try_prop_enum(display, colorop, prop, val);
>> +	igt_assert(result);
>> +}
>> +
>> +
> 
> Remove newline.
> 
>>  /**
>>   * igt_plane_replace_prop_blob:
>>   * @plane: plane to set property on.
>> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
>> index 1b6988c17aeb..6cf50b3c8280 100644
>> --- a/lib/igt_kms.h
>> +++ b/lib/igt_kms.h
>> @@ -321,6 +321,19 @@ enum igt_atomic_plane_properties {
>>         IGT_NUM_PLANE_PROPS
>>  };
>>  
>> +enum igt_atomic_colorop_properties {
>> +	IGT_COLOROP_TYPE,
>> +	IGT_COLOROP_BYPASS,
>> +	IGT_COLOROP_CURVE_1D_TYPE,
>> +	IGT_COLOROP_NEXT,
>> +	IGT_NUM_COLOROP_PROPS
>> +};
>> +
>> +/* TODO move to libdrm header?? */
>> +enum drm_colorop_type {
>> +	DRM_COLOROP_1D_CURVE
>> +};
>> +
>>  /**
>>   * igt_plane_prop_names
>>   *
>> @@ -329,6 +342,14 @@ enum igt_atomic_plane_properties {
>>   */
>>  extern const char * const igt_plane_prop_names[];
>>  
>> +/**
>> + * igt_colorop_prop_names
>> + *
>> + * igt_colorop_prop_names contains a list of colorop property names,
>> + * as indexed by the igt_atomic_colorop_properties enum.
>> + */
>> +extern const char * const igt_colorop_prop_names[];
>> +
>>  typedef struct igt_display igt_display_t;
>>  typedef struct igt_pipe igt_pipe_t;
>>  typedef uint32_t igt_fixed_t;			/* 16.16 fixed point */
>> @@ -351,6 +372,24 @@ static inline bool igt_rotation_90_or_270(igt_rotation_t rotation)
>>  	return rotation & (IGT_ROTATION_90 | IGT_ROTATION_270);
>>  }
>>  
>> +typedef struct igt_plane igt_plane_t;
> ----------------------------^
> It should not be needed, it is already in header igt_kms.h
> 

igt_plane_t is needed in igt_colorop definition, which in turn is
needed in igt_plane definition. Hence we need the forward declaration.

>> +
>> +typedef struct igt_colorop {
>> +	/*< private >*/
>> +	igt_pipe_t *pipe;
>> +
>> +	drmModeColorop *drm_colorop;
>> +
>> +	igt_plane_t *plane;
>> +
>> +	char name[DRM_PROP_NAME_LEN];
>> +
>> +	uint64_t changed;
>> +	uint32_t props[IGT_NUM_COLOROP_PROPS];
>> +	uint64_t values[IGT_NUM_COLOROP_PROPS];
>> +
>> +} igt_colorop_t;
>> +
>>  typedef struct igt_plane {
>>  	/*< private >*/
>>  	igt_pipe_t *pipe;
>> @@ -404,6 +443,7 @@ struct igt_pipe {
>>  	int plane_cursor;
>>  	int plane_primary;
>>  	igt_plane_t *planes;
>> +	igt_colorop_t *colorops;
>>  
>>  	uint64_t changed;
>>  	uint32_t props[IGT_NUM_CRTC_PROPS];
>> @@ -442,9 +482,11 @@ struct igt_display {
>>  	int log_shift;
>>  	int n_pipes;
>>  	int n_planes;
>> +	int n_colorops;
>>  	int n_outputs;
>>  	igt_output_t *outputs;
>>  	igt_plane_t *planes;
>> +	igt_colorop_t *colorops;
>>  	igt_pipe_t *pipes;
>>  	bool has_cursor_plane;
>>  	bool is_atomic;
>> @@ -721,6 +763,53 @@ extern void igt_plane_set_prop_enum(igt_plane_t *plane,
>>  				    enum igt_atomic_plane_properties prop,
>>  				    const char *val);
>>  
>> +
>> +
> 
> Remove newlines.
> 

Thanks for your review.

Harry

> Regards,
> Kamil
> 
>> +extern bool igt_plane_is_valid_colorop(igt_plane_t *plane, igt_colorop_t *colorop);
>> +
>> +/**
>> + * igt_colorop_has_prop:
>> + * @colorop: colorop to check.
>> + * @prop: Property to check.
>> + *
>> + * Check whether colorop supports a given property.
>> + *
>> + * Returns: True if the property is supported, otherwise false.
>> + */
>> +static inline bool
>> +igt_colorop_has_prop(igt_colorop_t *colorop, enum igt_atomic_colorop_properties prop)
>> +{
>> +	return colorop->props[prop];
>> +}
>> +
>> +uint64_t igt_colorop_get_prop(igt_display_t *display, igt_colorop_t *colorop, enum igt_atomic_colorop_properties prop);
>> +
>> +#define igt_colorop_is_prop_changed(colorop, prop) \
>> +	(!!((colorop)->changed & (1 << (prop))))
>> +
>> +#define igt_colorop_set_prop_changed(colorop, prop) \
>> +	(colorop)->changed |= 1 << (prop)
>> +
>> +#define igt_colorop_clear_prop_changed(colorop, prop) \
>> +	(colorop)->changed &= ~(1 << (prop))
>> +
>> +#define igt_colorop_set_prop_value(colorop, prop, value) \
>> +	do { \
>> +		colorop->values[prop] = value; \
>> +		igt_colorop_set_prop_changed(colorop, prop); \
>> +	} while (0)
>> +
>> +
>> +extern bool igt_colorop_try_prop_enum(igt_display_t *display,
>> +				      igt_colorop_t *colorop,
>> +				      enum igt_atomic_colorop_properties prop,
>> +				      const char *val);
>> +
>> +extern void igt_colorop_set_prop_enum(igt_display_t *display,
>> +				      igt_colorop_t *colorop,
>> +				      enum igt_atomic_colorop_properties prop,
>> +				      const char *val);
>> +
>>  extern void igt_plane_replace_prop_blob(igt_plane_t *plane,
>>  					enum igt_atomic_plane_properties prop,
>>  					const void *ptr, size_t length);
>> @@ -1005,4 +1094,6 @@ bool igt_check_bigjoiner_support(igt_display_t *display);
>>  bool igt_parse_mode_string(const char *mode_string, drmModeModeInfo *mode);
>>  bool i915_pipe_output_combo_valid(igt_display_t *display);
>>  
>> +igt_colorop_t *igt_find_colorop(igt_display_t *display, uint32_t id);
>> +
>>  #endif /* __IGT_KMS_H__ */
>> -- 
>> 2.42.0
>>

Re: [igt-dev] [RFC PATCH 1/7] include/drm-uapi: Add COLOROP object

[Harry Wentland]

On 2023-09-18 05:24, Kamil Konieczny wrote:
> Hi Harry,
> 
> On 2023-09-08 at 11:03:09 -0400, Harry Wentland wrote:
>> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
> 
> Put here description from cover letter or at least links
> to drm-uapi commits (prefereable lore.kernel.org) with subjects
> cited (as links can break in future).
> 

I agree this needs a bit of a description and will be remedied
in the next version, but for the bigger picture shouldn't this
be in the cover letter? If I copy-paste the cover letter here
then why not in every other patch of the series?

(Email patch series still confuse me -_-)

Harry

> Regards,
> Kamil
> 
>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
>> Cc: Simon Ser <contact@emersion.fr>
>> Cc: Harry Wentland <harry.wentland@amd.com>
>> Cc: Melissa Wen <mwen@igalia.com>
>> Cc: Jonas Ådahl <jadahl@redhat.com>
>> Cc: Sebastian Wick <sebastian.wick@redhat.com>
>> Cc: Shashank Sharma <shashank.sharma@amd.com>
>> Cc: Alexander Goins <agoins@nvidia.com>
>> Cc: Joshua Ashton <joshua@froggi.es>
>> Cc: Michel Dänzer <mdaenzer@redhat.com>
>> Cc: Aleix Pol <aleixpol@kde.org>
>> Cc: Xaver Hugl <xaver.hugl@gmail.com>
>> Cc: Victoria Brekenfeld <victoria@system76.com>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Cc: Uma Shankar <uma.shankar@intel.com>
>> Cc: Naseer Ahmed <quic_naseer@quicinc.com>
>> Cc: Christopher Braga <quic_cbraga@quicinc.com>
>> ---
>>  include/drm-uapi/drm_mode.h | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/include/drm-uapi/drm_mode.h b/include/drm-uapi/drm_mode.h
>> index e4a2570a6058..e7e56150616b 100644
>> --- a/include/drm-uapi/drm_mode.h
>> +++ b/include/drm-uapi/drm_mode.h
>> @@ -626,6 +626,7 @@ struct drm_mode_connector_set_property {
>>  #define DRM_MODE_OBJECT_FB 0xfbfbfbfb
>>  #define DRM_MODE_OBJECT_BLOB 0xbbbbbbbb
>>  #define DRM_MODE_OBJECT_PLANE 0xeeeeeeee
>> +#define DRM_MODE_OBJECT_COLOROP 0xfafafafa
>>  #define DRM_MODE_OBJECT_ANY 0
>>  
>>  struct drm_mode_obj_get_properties {
>> -- 
>> 2.42.0
>>

Re: [igt-dev] [RFC PATCH 5/7] igt/color: Add SW color transform functionality

[Harry Wentland]

On 2023-09-18 05:21, Kamil Konieczny wrote:
> Hi Harry,
> 
> On 2023-09-08 at 11:03:13 -0400, Harry Wentland wrote:
>> In order to test color we want to compare a HW (KMS) transform
>> with a SW transform. This introduces color transform for an
>> sRGB EOTF but this can be extended to other transforms. Code is
>> borrowed from Skia.
> 
> From what you wrote below it looks like you borrowed only few
> functions from Skia. Am I missing something?
> 

Correct, I'm only pulling in what I need.

>>
>> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
>> Cc: Simon Ser <contact@emersion.fr>
>> Cc: Harry Wentland <harry.wentland@amd.com>
>> Cc: Melissa Wen <mwen@igalia.com>
>> Cc: Jonas Ådahl <jadahl@redhat.com>
>> Cc: Sebastian Wick <sebastian.wick@redhat.com>
>> Cc: Shashank Sharma <shashank.sharma@amd.com>
>> Cc: Alexander Goins <agoins@nvidia.com>
>> Cc: Joshua Ashton <joshua@froggi.es>
>> Cc: Michel Dänzer <mdaenzer@redhat.com>
>> Cc: Aleix Pol <aleixpol@kde.org>
>> Cc: Xaver Hugl <xaver.hugl@gmail.com>
>> Cc: Victoria Brekenfeld <victoria@system76.com>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Cc: Uma Shankar <uma.shankar@intel.com>
>> Cc: Naseer Ahmed <quic_naseer@quicinc.com>
>> Cc: Christopher Braga <quic_cbraga@quicinc.com>
>> ---
>>  lib/igt_color.c | 330 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  lib/igt_color.h | 105 +++++++++++++++
>>  lib/igt_fb.c    |   6 +-
>>  lib/igt_fb.h    |   2 +
>>  lib/meson.build |   1 +
>>  5 files changed, 441 insertions(+), 3 deletions(-)
>>  create mode 100644 lib/igt_color.c
>>  create mode 100644 lib/igt_color.h
>>
>> diff --git a/lib/igt_color.c b/lib/igt_color.c
>> new file mode 100644
>> index 000000000000..12ff9ad1f324
>> --- /dev/null
>> +++ b/lib/igt_color.c
>> @@ -0,0 +1,330 @@
>> +/*
>> + * Copyright 2023 Advanced Micro Devices, Inc.
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
> 
> Use SPDX instead, look at other igt-gpu-tools sources for
> guide how to use it in source.c and header.h
> 
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>> + * OTHER DEALINGS IN THE SOFTWARE.
>> + */
>> +
>> +#include <errno.h>
>> +#include <math.h>
>> +
>> +#include "igt_color.h"
>> +#include "igt_core.h"
>> +#include "igt_x86.h"
>> +
>> +
>> +static float clamp(float val, float min, float max)
>> +{
>> +	return ((val < min) ? min : ((val > max) ? max : val));
>> +}
>> +
>> +/*
>> + * Below code is taken from Skia and adapted for style and needs of
>> + * IGT.
>> + *
>> + * https://chromium.googlesource.com/chromium/src/+/3e1a26c44c024d97dc9a4c09bbc6a2365398ca2c/ui/gfx/skia_color_space_util.cc#15
>> + *
>> + * To comply with the original code's license we'll include it, as well
>> + * as the original copyright here:
>> + *
>> + * // Copyright 2015 The Chromium Authors
>> + * //
> 
> imho this is strange, I am against this, either add Chromium
> to copyright header at begin of file or delete this. What
> exactly licence is this? GNU GPL? MIT?
> 

BSD. From what I can tell it's compatible with MIT and all I need to do
is add Google's copyright at the top, so I'll do that for the next version.

Harry

> +cc Pteri
> 
> Regards,
> Kamil
> 
>> + * // Redistribution and use in source and binary forms, with or without
>> + * // modification, are permitted provided that the following conditions are
>> + * // met:
>> + * //
>> + * //    * Redistributions of source code must retain the above copyright
>> + * // notice, this list of conditions and the following disclaimer.
>> + * //    * Redistributions in binary form must reproduce the above
>> + * // copyright notice, this list of conditions and the following disclaimer
>> + * // in the documentation and/or other materials provided with the
>> + * // distribution.
>> + * //    * Neither the name of Google LLC nor the names of its
>> + * // contributors may be used to endorse or promote products derived from
>> + * // this software without specific prior written permission.
>> + * //
>> + * // THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
>> + * // "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
>> + * // LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
>> + * // A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
>> + * // OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
>> + * // SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
>> + * // LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
>> + * // DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
>> + * // THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
>> + * // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
>> + * // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>> + */
>> +
>> +static float igt_color_tf_eval_unclamped(const struct igt_color_tf *fn, float x)
>> +{
>> +	if (x < fn->d)
>> +		return fn->c * x + fn->f;
>> +	return pow(fn->a * x + fn->b, fn->g) + fn->e;
>> +}
>> +
>> +static float igt_color_tf_eval(const struct igt_color_tf *fn, float x)
>> +{
>> +	float fn_at_x_unclamped = igt_color_tf_eval_unclamped(fn, x);
>> +	return clamp(fn_at_x_unclamped, 0.0f, 1.0f);
>> +}
>> +
>> +#if 0
>> +skcms_TransferFunction SkTransferFnInverse(const skcms_TransferFunction& fn) {
>> +  skcms_TransferFunction fn_inv = {0};
>> +  if (fn.a > 0 && fn.g > 0) {
>> +    double a_to_the_g = std::pow(fn.a, fn.g);
>> +    fn_inv.a = 1.f / a_to_the_g;
>> +    fn_inv.b = -fn.e / a_to_the_g;
>> +    fn_inv.g = 1.f / fn.g;
>> +  }
>> +  fn_inv.d = fn.c * fn.d + fn.f;
>> +  fn_inv.e = -fn.b / fn.a;
>> +  if (fn.c != 0) {
>> +    fn_inv.c = 1.f / fn.c;
>> +    fn_inv.f = -fn.f / fn.c;
>> +  }
>> +  return fn_inv;
>> +}
>> +#endif
>> +
>> +/* end of Skia-based code */
>> +
>> +static void igt_color_srgb_inv_eotf(igt_pixel_t *pixel)
>> +{
>> +}
>> +
>> +
>> +
>> +static void igt_color_srgb_eotf(igt_pixel_t *pixel)
>> +{
>> +	pixel->r = igt_color_tf_eval(&srgb_tf, pixel->r);
>> +	pixel->g = igt_color_tf_eval(&srgb_tf, pixel->g);
>> +	pixel->b = igt_color_tf_eval(&srgb_tf, pixel->b);
>> +}
>> +
>> +int igt_color_transform_pixels(igt_fb_t *fb, igt_pixel_transform transform)
>> +{
>> +	uint32_t *line = NULL;
>> +	void *map;
>> +	char *ptr;
>> +	int x, y, cpp = igt_drm_format_to_bpp(fb->drm_format) / 8;
>> +	uint32_t stride = igt_fb_calc_plane_stride(fb, 0);
>> +
>> +	if (fb->num_planes != 1)
>> +		return -EINVAL;
>> +
>> +	/* TODO expand for other formats */
>> +	if (fb->drm_format != DRM_FORMAT_XRGB8888)
>> +		return -EINVAL;
>> +
>> +	ptr = igt_fb_map_buffer(fb->fd, fb);
>> +	igt_assert(ptr);
>> +	map = ptr;
>> +
>> +	/*
>> +	 * Framebuffers are often uncached, which can make byte-wise accesses
>> +	 * very slow. We copy each line of the FB into a local buffer to speed
>> +	 * up the hashing.
>> +	 */
>> +	line = malloc(stride);
>> +	if (!line) {
>> +		munmap(map, fb->size);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	for (y = 0; y < fb->height; y++, ptr += stride) {
>> +
>> +		/* get line from buffer */
>> +		igt_memcpy_from_wc(line, ptr, fb->width * cpp);
>> +
>> +		for (x = 0; x < fb->width; x++) {
>> +			uint32_t raw_pixel = le32_to_cpu(line[x]);
>> +			igt_pixel_t pixel;
>> +			raw_pixel &= 0x00ffffff;
>> +
>> +			/*
>> +			 * unpack pixel into igt_pixel_t
>> +			 *
>> +			 * only for XRGB8888 for now
>> +			 *
>> +			 * TODO add "generic" mechanism for unpacking
>> +			 * other FB formats
>> +			 */
>> +			pixel.r = (raw_pixel & 0x00ff0000) >> 16;
>> +			pixel.g = (raw_pixel & 0x0000ff00) >> 8;
>> +			pixel.b = (raw_pixel & 0x000000ff);
>> +
>> +			/* normalize for 8-bit */
>> +			pixel.r /= (0xff);
>> +			pixel.g /= (0xff);
>> +			pixel.b /= (0xff);
>> +
>> +			/* TODO use read_rgb from igt_fb? */
>> +
>> +			/* run transform on pixel */
>> +			transform(&pixel);
>> +
>> +			/* de-normalize back to 8-bit */
>> +			pixel.r *= (0xff);
>> +			pixel.g *= (0xff);
>> +			pixel.b *= (0xff);
>> +
>> +			/* re-pack pixel into FB*/
>> +			raw_pixel = 0x0;
>> +			raw_pixel |= ((uint8_t) (lround(pixel.r) & 0xff)) << 16;
>> +			raw_pixel |= ((uint8_t) (lround(pixel.g) & 0xff)) << 8;
>> +			raw_pixel |= ((uint8_t) (lround(pixel.b) & 0xff));
>> +			/* TODO use write_rgb from igt_fb? */
>> +
>> +			/* write back to line */
>> +			line[x] = cpu_to_le32(raw_pixel);
>> +		}
>> +
>> +		/* copy line back to fb buffer */
>> +		igt_memcpy_from_wc(ptr, line, fb->width * cpp);
>> +	}
>> +
>> +	free(line);
>> +	igt_fb_unmap_buffer(fb, map);
>> +
>> +	return 0;
>> +
>> +	/* for each pixel */
>> +
>> +	/* convert to float and create igt_pixel */
>> +
>> +	/* call transform */
>> +
>> +	/* convert back to fb format from igt_pixel */
>> +
>> +
>> +}
>> +
>> +int igt_color_transform_srgb_eotf(igt_fb_t *fb)
>> +{
>> +	return igt_color_transform_pixels(fb, &igt_color_srgb_eotf);
>> +}
>> +
>> +int igt_color_transform_srgb_inv_eotf(igt_fb_t *fb)
>> +{
>> +	return igt_color_transform_pixels(fb, &igt_color_srgb_inv_eotf);
>> +}
>> +
>> +#if 0
>> +#define PRINT_PIXEL_CMP
>> +#endif
>> +
>> +bool igt_cmp_fb_component(uint16_t comp1, uint16_t comp2, uint8_t up, uint8_t down)
>> +{
>> +	int16_t diff = comp2 - comp1;
>> +
>> +	if ((diff < -down) || (diff > up)) {
>> +		printf("comp1 %x comp2 %x diff %d down %d, up %d\n", comp1, comp2, diff, -down, up);
>> +		return false;
>> +	}
>> +
>> +	return true;
>> +}
>> +
>> +bool igt_cmp_fb_pixels(igt_fb_t *fb1, igt_fb_t *fb2, uint8_t up, uint8_t down)
>> +{
>> +	uint32_t *ptr1, *ptr2;
>> +	uint32_t pixel1, pixel2, i, j;
>> +	bool matched = true;
>> +
>> +#ifdef PRINT_PIXEL_CMP
>> +	printf("hwhw: %s %d\n", __func__, __LINE__);
>> +#endif
>> +
>> +	ptr1 = igt_fb_map_buffer(fb1->fd, fb1);
>> +	ptr2 = igt_fb_map_buffer(fb2->fd, fb2);
>> +
>> +	igt_assert(fb1->drm_format == fb2->drm_format);
>> +	igt_assert(fb1->size == fb2->size);
>> +
>> +	for (i = 0; i < fb1->size / sizeof(uint32_t); i++) {
>> +		uint16_t mask = 0xff;
>> +		uint16_t shift = 8;
>> +
>> +		if (fb1->drm_format == DRM_FORMAT_XRGB2101010) {
>> +			/* ignore alpha */
>> +			pixel1 = ptr1[i] & ~0xc0000000;
>> +			pixel2 = ptr2[i] & ~0xc0000000;
>> +
>> +			mask = 0x3ff;
>> +			shift = 10;
>> +
>> +
>> +		} else if (fb1->drm_format == DRM_FORMAT_XRGB8888) {
>> +			/* ignore alpha */
>> +			pixel1 = ptr1[i] & ~0xff000000;
>> +			pixel2 = ptr2[i] & ~0xff000000;
>> +
>> +			mask = 0xff;
>> +			shift = 8;
>> +
>> +		} else {
>> +			pixel1 = ptr1[i];
>> +			pixel2 = ptr2[i];
>> +		}
>> +
>> +#ifdef PRINT_PIXEL_CMP
>> +		/* print first 5 pixels */
>> +		if (i < 5) {
>> +			printf("hwhw: %s %u\n", __func__, i);
>> +			printf("\t\traw_in\t%u\n", ptr1[i]);
>> +			printf("\t\traw_out\t%u\n", ptr2[i]);
>> +			printf("\t\tin\t%u\n", pixel1);
>> +			printf("\t\tout\t%u\n", pixel2);
>> +		}
>> +#endif
>> +
>> +		for (j = 0; j < 3; j++) {
>> +			uint16_t comp1 = (pixel1 >> (shift*j)) & mask;
>> +			uint16_t comp2 = (pixel2 >> (shift*j)) & mask;
>> +
>> +			if (!igt_cmp_fb_component(comp1, comp2, up, down)) {
>> +				/* TODO use proper log*/
>> +				printf("i %d j %d shift %d mask %x comp1 %x comp2 %x, pixel1 %x pixel2 %x\n",
>> +				       i, j, shift, mask, comp1, comp2, pixel1, pixel2);
>> +				return false;
>> +			}
>> +		}
>> +	}
>> +
>> +	igt_fb_unmap_buffer(fb1, ptr1);
>> +	igt_fb_unmap_buffer(fb2, ptr2);
>> +
>> +	return matched;
>> +}
>> +
>> +
>> +void igt_dump_fb(igt_display_t *display, igt_fb_t *fb,
>> +		 const char *path_name, const char *file_name)
>> +{
>> +	char filepath_out[PATH_MAX];
>> +	cairo_surface_t *fb_surface_out;
>> +	cairo_status_t status;
>> +
>> +	snprintf(filepath_out, PATH_MAX, "%s/%s.png", path_name, file_name);
>> +	fb_surface_out = igt_get_cairo_surface(display->drm_fd, fb);
>> +	status = cairo_surface_write_to_png(fb_surface_out, filepath_out);
>> +	igt_assert_eq(status, CAIRO_STATUS_SUCCESS);
>> +	cairo_surface_destroy(fb_surface_out);
>> +}
>> \ No newline at end of file
>> diff --git a/lib/igt_color.h b/lib/igt_color.h
>> new file mode 100644
>> index 000000000000..fc136eecfd17
>> --- /dev/null
>> +++ b/lib/igt_color.h
>> @@ -0,0 +1,105 @@
>> +/*
>> + * Copyright 2023 Advanced Micro Devices, Inc.
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>> + * OTHER DEALINGS IN THE SOFTWARE.
>> + */
>> +
>> +
>> +#ifndef __IGT_COLOR_H__
>> +#define __IGT_COLOR_H__
>> +
>> +#include <limits.h>
>> +
>> +#include "igt_fb.h"
>> +#include "igt_kms.h"
>> +
>> +/*
>> + * Below code is taken from Skia and adapted for style and needs of
>> + * IGT.
>> + *
>> + * https://chromium.googlesource.com/chromium/src/+/3e1a26c44c024d97dc9a4c09bbc6a2365398ca2c/ui/gfx/skia_color_space_util.cc#15
>> + *
>> + * To comply with the original code's license we'll include it, as well
>> + * as the original copyright here:
>> + *
>> + * // Copyright 2015 The Chromium Authors
>> + * //
>> + * // Redistribution and use in source and binary forms, with or without
>> + * // modification, are permitted provided that the following conditions are
>> + * // met:
>> + * //
>> + * //    * Redistributions of source code must retain the above copyright
>> + * // notice, this list of conditions and the following disclaimer.
>> + * //    * Redistributions in binary form must reproduce the above
>> + * // copyright notice, this list of conditions and the following disclaimer
>> + * // in the documentation and/or other materials provided with the
>> + * // distribution.
>> + * //    * Neither the name of Google LLC nor the names of its
>> + * // contributors may be used to endorse or promote products derived from
>> + * // this software without specific prior written permission.
>> + * //
>> + * // THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
>> + * // "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
>> + * // LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
>> + * // A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
>> + * // OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
>> + * // SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
>> + * // LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
>> + * // DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
>> + * // THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
>> + * // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
>> + * // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>> + */
>> +/*
>> + * A transfer function mapping encoded values to linear values,
>> + * represented by this 7-parameter piecewise function:
>> + *
>> + *   linear = sign(encoded) *  (c*|encoded| + f)       , 0 <= |encoded| < d
>> + *          = sign(encoded) * ((a*|encoded| + b)^g + e), d <= |encoded|
>> + *
>> + * (A simple gamma transfer function sets g to gamma and a to 1.)
>> + */
>> +struct igt_color_tf {
>> +    float g, a,b,c,d,e,f;
>> +};
>> +
>> +const struct igt_color_tf srgb_tf = {2.4f, (float)(1/1.055), (float)(0.055/1.055), (float)(1/12.92), 0.04045f, 0, 0};
>> +
>> +/* end of Skia-based code */
>> +
>> +typedef struct igt_pixel {
>> +	float r;
>> +	float g;
>> +	float b;
>> +} igt_pixel_t;
>> +
>> +bool igt_cmp_fb_component(uint16_t comp1, uint16_t comp2, uint8_t up, uint8_t down);
>> +bool igt_cmp_fb_pixels(igt_fb_t *fb1, igt_fb_t *fb2, uint8_t up, uint8_t down);
>> +
>> +void igt_dump_fb(igt_display_t *display, igt_fb_t *fb, const char *path_name, const char *file_name);
>> +
>> +/* TODO also allow 64-bit pixels, or other weird sizes */
>> +typedef void (*igt_pixel_transform)(igt_pixel_t *pixel);
>> +
>> +int igt_color_transform_pixels(igt_fb_t *fb, igt_pixel_transform transform);
>> +
>> +int igt_color_transform_srgb_eotf(igt_fb_t *fb);
>> +int igt_color_transform_srgb_inv_eotf(igt_fb_t *fb);
>> +
>> +#endif
>> \ No newline at end of file
>> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
>> index 0fe5b6adf8f4..0667d3fcbcff 100644
>> --- a/lib/igt_fb.c
>> +++ b/lib/igt_fb.c
>> @@ -739,7 +739,7 @@ void igt_init_fb(struct igt_fb *fb, int fd, int width, int height,
>>  	}
>>  }
>>  
>> -static uint32_t calc_plane_stride(struct igt_fb *fb, int plane)
>> +uint32_t igt_fb_calc_plane_stride(struct igt_fb *fb, int plane)
>>  {
>>  	uint32_t min_stride = fb->plane_width[plane] *
>>  		(fb->plane_bpp[plane] / 8);
>> @@ -916,7 +916,7 @@ static uint64_t calc_fb_size(struct igt_fb *fb)
>>  
>>  		/* respect the stride requested by the caller */
>>  		if (!fb->strides[plane])
>> -			fb->strides[plane] = calc_plane_stride(fb, plane);
>> +			fb->strides[plane] = igt_fb_calc_plane_stride(fb, plane);
>>  
>>  		align = get_plane_alignment(fb, plane);
>>  		if (align)
>> @@ -4471,7 +4471,7 @@ int igt_fb_get_fnv1a_crc(struct igt_fb *fb, igt_crc_t *crc)
>>  	void *map;
>>  	char *ptr;
>>  	int x, y, cpp = igt_drm_format_to_bpp(fb->drm_format) / 8;
>> -	uint32_t stride = calc_plane_stride(fb, 0);
>> +	uint32_t stride = igt_fb_calc_plane_stride(fb, 0);
>>  
>>  	if (fb->num_planes != 1)
>>  		return -EINVAL;
>> diff --git a/lib/igt_fb.h b/lib/igt_fb.h
>> index 73bdfc8669d8..b753c1580884 100644
>> --- a/lib/igt_fb.h
>> +++ b/lib/igt_fb.h
>> @@ -170,6 +170,8 @@ int igt_dirty_fb(int fd, struct igt_fb *fb);
>>  void *igt_fb_map_buffer(int fd, struct igt_fb *fb);
>>  void igt_fb_unmap_buffer(struct igt_fb *fb, void *buffer);
>>  
>> +uint32_t igt_fb_calc_plane_stride(struct igt_fb *fb, int plane);
>> +
>>  void igt_create_bo_for_fb(int fd, int width, int height,
>>  			  uint32_t format, uint64_t modifier,
>>  			  struct igt_fb *fb);
>> diff --git a/lib/meson.build b/lib/meson.build
>> index 85f100f75510..7c9aa56d429c 100644
>> --- a/lib/meson.build
>> +++ b/lib/meson.build
>> @@ -97,6 +97,7 @@ lib_sources = [
>>  	'igt_edid.c',
>>  	'igt_eld.c',
>>  	'igt_infoframe.c',
>> +	'igt_color.c',
>>  	'veboxcopy_gen12.c',
>>  	'igt_msm.c',
>>  	'igt_dsc.c',
>> -- 
>> 2.42.0
>>

Re: [igt-dev] [RFC PATCH 5/7] igt/color: Add SW color transform functionality

[Harry Wentland]

On 2023-09-18 04:02, Pekka Paalanen wrote:
> On Fri, 15 Sep 2023 15:50:52 -0400
> Harry Wentland <harry.wentland@amd.com> wrote:
> 
>> On 2023-09-15 10:52, Pekka Paalanen wrote:
>>> On Fri, 8 Sep 2023 11:03:13 -0400
>>> Harry Wentland <harry.wentland@amd.com> wrote:
>>>   
>>>> In order to test color we want to compare a HW (KMS) transform
>>>> with a SW transform. This introduces color transform for an
>>>> sRGB EOTF but this can be extended to other transforms. Code is
>>>> borrowed from Skia.
>>>>
>>>> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
>>>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>>>> Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
>>>> Cc: Simon Ser <contact@emersion.fr>
>>>> Cc: Harry Wentland <harry.wentland@amd.com>
>>>> Cc: Melissa Wen <mwen@igalia.com>
>>>> Cc: Jonas Ådahl <jadahl@redhat.com>
>>>> Cc: Sebastian Wick <sebastian.wick@redhat.com>
>>>> Cc: Shashank Sharma <shashank.sharma@amd.com>
>>>> Cc: Alexander Goins <agoins@nvidia.com>
>>>> Cc: Joshua Ashton <joshua@froggi.es>
>>>> Cc: Michel Dänzer <mdaenzer@redhat.com>
>>>> Cc: Aleix Pol <aleixpol@kde.org>
>>>> Cc: Xaver Hugl <xaver.hugl@gmail.com>
>>>> Cc: Victoria Brekenfeld <victoria@system76.com>
>>>> Cc: Daniel Vetter <daniel@ffwll.ch>
>>>> Cc: Uma Shankar <uma.shankar@intel.com>
>>>> Cc: Naseer Ahmed <quic_naseer@quicinc.com>
>>>> Cc: Christopher Braga <quic_cbraga@quicinc.com>
>>>> ---
>>>>  lib/igt_color.c | 330 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  lib/igt_color.h | 105 +++++++++++++++
>>>>  lib/igt_fb.c    |   6 +-
>>>>  lib/igt_fb.h    |   2 +
>>>>  lib/meson.build |   1 +
>>>>  5 files changed, 441 insertions(+), 3 deletions(-)
>>>>  create mode 100644 lib/igt_color.c
>>>>  create mode 100644 lib/igt_color.h
> 
> ...
> 
>>>> +/*
>>>> + * A transfer function mapping encoded values to linear values,
>>>> + * represented by this 7-parameter piecewise function:
>>>> + *
>>>> + *   linear = sign(encoded) *  (c*|encoded| + f)       , 0 <= |encoded| < d
>>>> + *          = sign(encoded) * ((a*|encoded| + b)^g + e), d <= |encoded|  
>>>
>>> The code you have does not actually do the extended form (use of sign
>>> and absolute value), but clamps the result to [0.0, 1.0].
>>>   
>>
>> Yes, the intention (and I didn't write this but copied it) is to be able to use
>> this to describe most (all?) standard transfer functions. See
>> https://github.com/google/skia/blob/main/include/core/SkColorSpace.h
> 
> My complaint is about doc and implementation disagreeing.
> 
> xvYCC and integer-encoded scRGB would need an extended range if anyone
> wanted to test those, if they even fit here. I also do not understand
> how it can be possible to express PQ EOTF or HLG OETF with this
> parameterisation. Maybe it's an approximation on a very limited domain?
> 

I have a feeling Skia maps everything to [0.0, 1.0] and agree that this
wouldn't work in the cases you mention (PQ would work with an additional
scaling of the values to 125.0).

What's here seems to work fine for sRGB but I'll have to hook this all up
to amdgpu and test both sRGB and PQ at the very least.

Harry

> 
> Thanks,
> pq
> 
>>>> + *
>>>> + * (A simple gamma transfer function sets g to gamma and a to 1.)
>>>> + */
>>>> +struct igt_color_tf {
>>>> +    float g, a,b,c,d,e,f;
>>>> +};
>>>> +
>>>> +const struct igt_color_tf srgb_tf = {2.4f, (float)(1/1.055), (float)(0.055/1.055), (float)(1/12.92), 0.04045f, 0, 0};
>>>> +
>>>> +/* end of Skia-based code */
>>>> +
>>>> +typedef struct igt_pixel {
>>>> +	float r;
>>>> +	float g;
>>>> +	float b;
>>>> +} igt_pixel_t;  
>>>
>>> Thanks,
>>> pq  
>>
>