Wayland Governance Meeting - Oct 30 / Nov 01

2023-10-19 Thread Carlos Garnacho
Hi,

I would like to propose a meeting about the color management protocol
[1] after next week (it's late to schedule for next, plus there's
people still likely at XDC). After talking with GIMP maintainers, I
think this topic might welcome some higher bandwidth place to discuss.
The doodle is at [2] to poll for the best day/time, the final date
will be announced by Wednesday 25th.

The meeting will be held at GNOME servers [3], anyone is free to join
to listen if they want to, anonymously or not.

The notes for this meeting, along with the ones from the previous meetings can
be found on the wayland-protocols wiki page [4].

Cheers,
  Carlos

[1] https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/14
[2] https://nuudel.digitalcourage.de/QYCvHEj5KR4HADfD
[3] https://meet.gnome.org/car-oot-bcf-kt4
[4] https://gitlab.freedesktop.org/wayland/wayland-protocols/-/wikis/meetings


[RFC PATCH v2 17/17] drm/vkms: Add kunit tests for linear and sRGB LUTs

2023-10-19 Thread Harry Wentland
Signed-off-by: Harry Wentland 
Cc: Ville Syrjala 
Cc: Pekka Paalanen 
Cc: Simon Ser 
Cc: Harry Wentland 
Cc: Melissa Wen 
Cc: Jonas Ådahl 
Cc: Sebastian Wick 
Cc: Shashank Sharma 
Cc: Alexander Goins 
Cc: Joshua Ashton 
Cc: Michel Dänzer 
Cc: Aleix Pol 
Cc: Xaver Hugl 
Cc: Victoria Brekenfeld 
Cc: Sima 
Cc: Uma Shankar 
Cc: Naseer Ahmed 
Cc: Christopher Braga 
Cc: Abhinav Kumar 
Cc: Arthur Grillo 
Cc: Hector Martin 
Cc: Liviu Dudau 
Cc: Sasha McIntosh 
---
 drivers/gpu/drm/vkms/tests/vkms_color_tests.c | 38 ++-
 drivers/gpu/drm/vkms/vkms_composer.c  | 13 +--
 drivers/gpu/drm/vkms/vkms_composer.h  | 14 +++
 3 files changed, 52 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/vkms/tests/vkms_color_tests.c 
b/drivers/gpu/drm/vkms/tests/vkms_color_tests.c
index 843b2e1d607e..14decb5d1b64 100644
--- a/drivers/gpu/drm/vkms/tests/vkms_color_tests.c
+++ b/drivers/gpu/drm/vkms/tests/vkms_color_tests.c
@@ -5,6 +5,7 @@
 #include 
 
 #include "../vkms_composer.h"
+#include "../vkms_luts.h"
 
 #define TEST_LUT_SIZE 16
 
@@ -33,7 +34,6 @@ const struct vkms_color_lut test_linear_lut = {
.channel_value2index_ratio = 0xf000fll
 };
 
-
 static void vkms_color_test_get_lut_index(struct kunit *test)
 {
int i;
@@ -42,6 +42,19 @@ static void vkms_color_test_get_lut_index(struct kunit *test)
 
for (i = 0; i < TEST_LUT_SIZE; i++)
KUNIT_EXPECT_EQ(test, 
drm_fixp2int_ceil(get_lut_index(&test_linear_lut, test_linear_array[i].red)), 
i);
+
+   KUNIT_EXPECT_EQ(test, drm_fixp2int(get_lut_index(&srgb_eotf, 0x0)), 
0x0);
+   KUNIT_EXPECT_EQ(test, drm_fixp2int_ceil(get_lut_index(&srgb_eotf, 
0x0)), 0x0);
+   KUNIT_EXPECT_EQ(test, drm_fixp2int_ceil(get_lut_index(&srgb_eotf, 
0x101)), 0x1);
+   KUNIT_EXPECT_EQ(test, drm_fixp2int_ceil(get_lut_index(&srgb_eotf, 
0x202)), 0x2);
+
+   KUNIT_EXPECT_EQ(test, drm_fixp2int(get_lut_index(&srgb_inv_eotf, 0x0)), 
0x0);
+   KUNIT_EXPECT_EQ(test, drm_fixp2int_ceil(get_lut_index(&srgb_inv_eotf, 
0x0)), 0x0);
+   KUNIT_EXPECT_EQ(test, drm_fixp2int_ceil(get_lut_index(&srgb_inv_eotf, 
0x101)), 0x1);
+   KUNIT_EXPECT_EQ(test, drm_fixp2int_ceil(get_lut_index(&srgb_inv_eotf, 
0x202)), 0x2);
+
+   KUNIT_EXPECT_EQ(test, drm_fixp2int_ceil(get_lut_index(&srgb_eotf, 
0xfefe)), 0xfe);
+   KUNIT_EXPECT_EQ(test, drm_fixp2int_ceil(get_lut_index(&srgb_eotf, 
0x)), 0xff);
 }
 
 static void vkms_color_test_lerp(struct kunit *test)
@@ -49,9 +62,32 @@ static void vkms_color_test_lerp(struct kunit *test)
KUNIT_EXPECT_EQ(test, lerp_u16(0x0, 0x10, 0x8000), 0x8);
 }
 
+static void vkms_color_test_linear(struct kunit *test)
+{
+   for (int i = 0; i < LUT_SIZE; i++) {
+   int linear = apply_lut_to_channel_value(&linear_eotf, i * 
0x101, LUT_RED);
+   KUNIT_EXPECT_EQ(test, DIV_ROUND_CLOSEST(linear, 0x101), i);
+   }
+}
+
+static void vkms_color_srgb_inv_srgb(struct kunit *test)
+{
+   u16 srgb, final;
+
+   for (int i = 0; i < LUT_SIZE; i++) {
+   srgb = apply_lut_to_channel_value(&srgb_eotf, i * 0x101, 
LUT_RED);
+   final = apply_lut_to_channel_value(&srgb_inv_eotf, srgb, 
LUT_RED);
+
+   KUNIT_EXPECT_GE(test, final / 0x101, i-1);
+   KUNIT_EXPECT_LE(test, final / 0x101, i+1);
+   }
+}
+
 static struct kunit_case vkms_color_test_cases[] = {
KUNIT_CASE(vkms_color_test_get_lut_index),
KUNIT_CASE(vkms_color_test_lerp),
+   KUNIT_CASE(vkms_color_test_linear),
+   KUNIT_CASE(vkms_color_srgb_inv_srgb),
{}
 };
 
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c 
b/drivers/gpu/drm/vkms/vkms_composer.c
index 73b7d5e94021..24c984f2876f 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -110,18 +110,7 @@ s64 get_lut_index(const struct vkms_color_lut *lut, u16 
channel_value)
return drm_fixp_mul(color_channel_fp, lut->channel_value2index_ratio);
 }
 
-/*
- * This enum is related to the positions of the variables inside
- * `struct drm_color_lut`, so the order of both needs to be the same.
- */
-enum lut_channel {
-   LUT_RED = 0,
-   LUT_GREEN,
-   LUT_BLUE,
-   LUT_RESERVED
-};
-
-static u16 apply_lut_to_channel_value(const struct vkms_color_lut *lut, u16 
channel_value,
+u16 apply_lut_to_channel_value(const struct vkms_color_lut *lut, u16 
channel_value,
  enum lut_channel channel)
 {
s64 lut_index = get_lut_index(lut, channel_value);
diff --git a/drivers/gpu/drm/vkms/vkms_composer.h 
b/drivers/gpu/drm/vkms/vkms_composer.h
index 11c5de9cc961..d92497c555eb 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.h
+++ b/drivers/gpu/drm/vkms/vkms_composer.h
@@ -8,4 +8,18 @@
 s64 get_lut_index(const struct vkms_color_lut *lut, u16 channel_value);
 u16 lerp_u16(u16 a, u16 b, s64 t);
 
+/*
+ * This enum is related to the positions of the variables inside
+ * `struct drm_colo

[RFC PATCH v2 15/17] drm/colorop: Add NEXT to colorop state print

2023-10-19 Thread Harry Wentland
Signed-off-by: Harry Wentland 
Cc: Ville Syrjala 
Cc: Pekka Paalanen 
Cc: Simon Ser 
Cc: Harry Wentland 
Cc: Melissa Wen 
Cc: Jonas Ådahl 
Cc: Sebastian Wick 
Cc: Shashank Sharma 
Cc: Alexander Goins 
Cc: Joshua Ashton 
Cc: Michel Dänzer 
Cc: Aleix Pol 
Cc: Xaver Hugl 
Cc: Victoria Brekenfeld 
Cc: Sima 
Cc: Uma Shankar 
Cc: Naseer Ahmed 
Cc: Christopher Braga 
Cc: Abhinav Kumar 
Cc: Arthur Grillo 
Cc: Hector Martin 
Cc: Liviu Dudau 
Cc: Sasha McIntosh 
---
 drivers/gpu/drm/drm_atomic.c  |  1 +
 drivers/gpu/drm/drm_colorop.c | 42 +++
 include/drm/drm_colorop.h |  2 ++
 3 files changed, 45 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 781bd3aa1849..cfe9199a15d2 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -803,6 +803,7 @@ static void drm_atomic_colorop_print_state(struct 
drm_printer *p,
drm_printf(p, "\ttype=%s\n", drm_get_colorop_type_name(colorop->type));
drm_printf(p, "\tbypass=%u\n", state->bypass);
drm_printf(p, "\tcurve_1d_type=%s\n", 
drm_get_colorop_curve_1d_type_name(state->curve_1d_type));
+   drm_printf(p, "\tnext=%d\n", drm_colorop_get_next_property(colorop));
 }
 
 static void drm_atomic_plane_print_state(struct drm_printer *p,
diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
index 1afd5fbe8776..ff6f938cc28c 100644
--- a/drivers/gpu/drm/drm_colorop.c
+++ b/drivers/gpu/drm/drm_colorop.c
@@ -340,3 +340,45 @@ void drm_colorop_set_next_property(struct drm_colorop 
*colorop, struct drm_color
  next->base.id);
 }
 EXPORT_SYMBOL(drm_colorop_set_next_property);
+
+/**
+ * drm_colorop_set_next_property - gets the next colorop ID
+ * @colorop: drm colorop
+ *
+ * Returns:
+ * The DRM object ID of the next colorop
+ */
+uint32_t drm_colorop_get_next_property(struct drm_colorop *colorop)
+{
+   uint64_t next_id = 0;
+
+   if (!colorop->next_property)
+   return 0;
+
+   drm_object_property_get_value(&colorop->base,
+ colorop->next_property,
+ &next_id);
+
+   return (uint32_t) next_id;
+}
+EXPORT_SYMBOL(drm_colorop_get_next_property);
+
+
+/**
+ * drm_colorop_set_next_property - gets the next colorop ID
+ * @colorop: drm colorop
+ *
+ * Returns:
+ * The DRM object ID of the next colorop
+ */
+struct drm_colorop *drm_colorop_get_next(struct drm_colorop *colorop)
+{
+   uint64_t next_id = drm_colorop_get_next_property(colorop);
+
+   if (!next_id)
+   return NULL;
+
+   return drm_colorop_find(colorop->dev, NULL, next_id);
+
+}
+EXPORT_SYMBOL(drm_colorop_get_next);
\ No newline at end of file
diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h
index 622a671d2458..2ba506a0ea4d 100644
--- a/include/drm/drm_colorop.h
+++ b/include/drm/drm_colorop.h
@@ -228,6 +228,8 @@ const char *drm_get_colorop_type_name(enum drm_colorop_type 
type);
 const char *drm_get_colorop_curve_1d_type_name(enum drm_colorop_curve_1d_type 
type);
 
 void drm_colorop_set_next_property(struct drm_colorop *colorop, struct 
drm_colorop *next);
+uint32_t drm_colorop_get_next_property(struct drm_colorop *colorop);
+struct drm_colorop *drm_colorop_get_next(struct drm_colorop *colorop);
 
 
 #endif /* __DRM_COLOROP_H__ */
-- 
2.42.0



[RFC PATCH v2 13/17] drm/colorop: Add new IOCTLs to retrieve drm_colorop objects

2023-10-19 Thread Harry Wentland
Since we created a new DRM object we need new IOCTLs (and
new libdrm functions) to retrieve those objects.

TODO: Can we make these IOCTLs and libdrm functions generic
to allow for new DRM objects in the future without the need
for new IOCTLs and libdrm functions?

Signed-off-by: Harry Wentland 
Cc: Ville Syrjala 
Cc: Pekka Paalanen 
Cc: Simon Ser 
Cc: Harry Wentland 
Cc: Melissa Wen 
Cc: Jonas Ådahl 
Cc: Sebastian Wick 
Cc: Shashank Sharma 
Cc: Alexander Goins 
Cc: Joshua Ashton 
Cc: Michel Dänzer 
Cc: Aleix Pol 
Cc: Xaver Hugl 
Cc: Victoria Brekenfeld 
Cc: Sima 
Cc: Uma Shankar 
Cc: Naseer Ahmed 
Cc: Christopher Braga 
Cc: Abhinav Kumar 
Cc: Arthur Grillo 
Cc: Hector Martin 
Cc: Liviu Dudau 
Cc: Sasha McIntosh 
---
 drivers/gpu/drm/drm_colorop.c   | 51 +
 drivers/gpu/drm/drm_crtc_internal.h |  4 +++
 drivers/gpu/drm/drm_ioctl.c |  5 +++
 include/uapi/drm/drm_mode.h | 21 
 4 files changed, 81 insertions(+)

diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
index bc1250718baf..1afd5fbe8776 100644
--- a/drivers/gpu/drm/drm_colorop.c
+++ b/drivers/gpu/drm/drm_colorop.c
@@ -32,6 +32,57 @@
 
 /* TODO big colorop doc, including properties, etc. */
 
+/* IOCTLs */
+
+int drm_mode_getcolorop_res(struct drm_device *dev, void *data,
+   struct drm_file *file_priv)
+{
+   struct drm_mode_get_colorop_res *colorop_resp = data;
+   struct drm_colorop *colorop;
+   uint32_t __user *colorop_ptr;
+   int count = 0;
+
+   if (!drm_core_check_feature(dev, DRIVER_MODESET))
+   return -EOPNOTSUPP;
+
+   colorop_ptr = u64_to_user_ptr(colorop_resp->colorop_id_ptr);
+
+   /*
+* This ioctl is called twice, once to determine how much space is
+* needed, and the 2nd time to fill it.
+*/
+   drm_for_each_colorop(colorop, dev) {
+   if (drm_lease_held(file_priv, colorop->base.id)) {
+   if (count < colorop_resp->count_colorops &&
+   put_user(colorop->base.id, colorop_ptr + count))
+   return -EFAULT;
+   count++;
+   }
+   }
+   colorop_resp->count_colorops = count;
+
+   return 0;
+}
+
+int drm_mode_getcolorop(struct drm_device *dev, void *data,
+   struct drm_file *file_priv)
+{
+   struct drm_mode_get_colorop *colorop_resp = data;
+   struct drm_colorop *colorop;
+
+   if (!drm_core_check_feature(dev, DRIVER_MODESET))
+   return -EOPNOTSUPP;
+
+   colorop = drm_colorop_find(dev, file_priv, colorop_resp->colorop_id);
+   if (!colorop)
+   return -ENOENT;
+
+   colorop_resp->colorop_id = colorop->base.id;
+   colorop_resp->plane_id = colorop->plane ? colorop->plane->base.id : 0;
+
+   return 0;
+}
+
 static const struct drm_prop_enum_list drm_colorop_type_enum_list[] = {
{ DRM_COLOROP_1D_CURVE, "1D Curve" },
 };
diff --git a/drivers/gpu/drm/drm_crtc_internal.h 
b/drivers/gpu/drm/drm_crtc_internal.h
index 8556c3b3ff88..252cd7e607e3 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -278,6 +278,10 @@ int drm_mode_getplane(struct drm_device *dev,
  void *data, struct drm_file *file_priv);
 int drm_mode_setplane(struct drm_device *dev,
  void *data, struct drm_file *file_priv);
+int drm_mode_getcolorop_res(struct drm_device *dev, void *data,
+   struct drm_file *file_priv);
+int drm_mode_getcolorop(struct drm_device *dev, void *data,
+   struct drm_file *file_priv);
 int drm_mode_cursor_ioctl(struct drm_device *dev,
  void *data, struct drm_file *file_priv);
 int drm_mode_cursor2_ioctl(struct drm_device *dev,
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 77590b0f38fa..8a4b7d8d8a0b 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -717,6 +717,11 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
DRM_IOCTL_DEF(DRM_IOCTL_MODE_LIST_LESSEES, drm_mode_list_lessees_ioctl, 
DRM_MASTER),
DRM_IOCTL_DEF(DRM_IOCTL_MODE_GET_LEASE, drm_mode_get_lease_ioctl, 
DRM_MASTER),
DRM_IOCTL_DEF(DRM_IOCTL_MODE_REVOKE_LEASE, drm_mode_revoke_lease_ioctl, 
DRM_MASTER),
+
+   DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETCOLOROPRESOURCES, 
drm_mode_getcolorop_res, 0),
+   /* TODO do we need GETCOLOROP? */
+   DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETCOLOROP, drm_mode_getcolorop, 0),
+
 };
 
 #define DRM_CORE_IOCTL_COUNT   ARRAY_SIZE(drm_ioctls)
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 009a800676ac..5c71eb011181 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -357,6 +357,27 @@ struct drm_mode_get_plane {
__u64 format_type_ptr;
 };
 
+struct drm_mode_get_colorop_res {
+   __u64 colorop

[RFC PATCH v2 16/17] drm/vkms: Add enumerated 1D curve colorop

2023-10-19 Thread Harry Wentland
This patch introduces a VKMS color pipeline that includes two
drm_colorops for named transfer functions. For now the only ones
supported are sRGB EOTF, sRGB Inverse EOTF, and a Linear TF.
We will expand this in the future but I don't want to do so
without accompanying IGT tests.

We introduce a new vkms_luts.c file that hard-codes sRGB EOTF,
sRGB Inverse EOTF, and a linear EOTF LUT. These have been
generated with 256 entries each as IGT is currently testing
only 8 bpc surfaces. We will likely need higher precision
but I'm reluctant to make that change without clear indication
that we need it. We'll revisit and, if necessary, regenerate
the LUTs when we have IGT tests for higher precision buffers.

v2:
 - Add commit description
 - Fix sRGB EOTF LUT definition
 - Add linear and sRGB inverse EOTF LUTs

Signed-off-by: Harry Wentland 
Cc: Ville Syrjala 
Cc: Pekka Paalanen 
Cc: Simon Ser 
Cc: Harry Wentland 
Cc: Melissa Wen 
Cc: Jonas Ådahl 
Cc: Sebastian Wick 
Cc: Shashank Sharma 
Cc: Alexander Goins 
Cc: Joshua Ashton 
Cc: Michel Dänzer 
Cc: Aleix Pol 
Cc: Xaver Hugl 
Cc: Victoria Brekenfeld 
Cc: Sima 
Cc: Uma Shankar 
Cc: Naseer Ahmed 
Cc: Christopher Braga 
Cc: Abhinav Kumar 
Cc: Arthur Grillo 
Cc: Hector Martin 
Cc: Liviu Dudau 
Cc: Sasha McIntosh 
---
 drivers/gpu/drm/vkms/Makefile|   4 +-
 drivers/gpu/drm/vkms/vkms_colorop.c  |  85 +++
 drivers/gpu/drm/vkms/vkms_composer.c |  46 ++
 drivers/gpu/drm/vkms/vkms_drv.h  |   4 +
 drivers/gpu/drm/vkms/vkms_luts.c | 802 +++
 drivers/gpu/drm/vkms/vkms_luts.h |  12 +
 drivers/gpu/drm/vkms/vkms_plane.c|   2 +
 7 files changed, 954 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/vkms/vkms_colorop.c
 create mode 100644 drivers/gpu/drm/vkms/vkms_luts.c
 create mode 100644 drivers/gpu/drm/vkms/vkms_luts.h

diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
index d3440f228f46..eb208f3e6780 100644
--- a/drivers/gpu/drm/vkms/Makefile
+++ b/drivers/gpu/drm/vkms/Makefile
@@ -6,7 +6,9 @@ vkms-y := \
vkms_formats.o \
vkms_crtc.o \
vkms_composer.o \
-   vkms_writeback.o
+   vkms_writeback.o \
+   vkms_colorop.o \
+   vkms_luts.o
 
 obj-$(CONFIG_DRM_VKMS) += vkms.o
 
diff --git a/drivers/gpu/drm/vkms/vkms_colorop.c 
b/drivers/gpu/drm/vkms/vkms_colorop.c
new file mode 100644
index ..9a26b9fdc4a2
--- /dev/null
+++ b/drivers/gpu/drm/vkms/vkms_colorop.c
@@ -0,0 +1,85 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define MAX_COLOR_PIPELINES 5
+
+const int vkms_initialize_tf_pipeline(struct drm_plane *plane, struct 
drm_prop_enum_list *list)
+{
+
+   struct drm_colorop *op, *prev_op;
+   struct drm_device *dev = plane->dev;
+   int ret;
+
+   /* 1st op: 1d curve */
+   op = kzalloc(sizeof(struct drm_colorop), GFP_KERNEL);
+   if (!op) {
+   DRM_ERROR("KMS: Failed to allocate colorop\n");
+   return -ENOMEM;
+   }
+
+   ret = drm_colorop_init(dev, op, plane, DRM_COLOROP_1D_CURVE);
+   if (ret)
+   return ret;
+
+   list->type = op->base.id;
+   list->name = kasprintf(GFP_KERNEL, "Color Pipeline %d", op->base.id);
+
+   prev_op = op;
+
+   /* 2nd op: 1d curve */
+   op = kzalloc(sizeof(struct drm_colorop), GFP_KERNEL);
+   if (!op) {
+   DRM_ERROR("KMS: Failed to allocate colorop\n");
+   return -ENOMEM;
+   }
+
+   ret = drm_colorop_init(dev, op, plane, DRM_COLOROP_1D_CURVE);
+   if (ret)
+   return ret;
+
+   drm_colorop_set_next_property(prev_op, op);
+
+   return 0;
+}
+
+int vkms_initialize_colorops(struct drm_plane *plane)
+{
+   struct drm_device *dev = plane->dev;
+   struct drm_property *prop;
+   struct drm_prop_enum_list pipelines[MAX_COLOR_PIPELINES];
+   int len = 0;
+   int ret;
+
+   /* Add "Bypass" (i.e. NULL) pipeline */
+   pipelines[len].type = 0;
+   pipelines[len].name = "Bypass";
+   len++;
+
+   /* Add pipeline consisting of transfer functions */
+   ret = vkms_initialize_tf_pipeline(plane, &(pipelines[len]));
+   if (ret)
+   return ret;
+   len++;
+
+   /* Create COLOR_PIPELINE property and attach */
+   prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
+   "COLOR_PIPELINE",
+   pipelines, len);
+   if (!prop)
+   return -ENOMEM;
+
+   plane->color_pipeline_property = prop;
+
+   drm_object_attach_property(&plane->base, prop, 0);
+
+   /* TODO do we even need this? */
+   if (plane->state)
+   plane->state->color_pipeline = NULL;
+
+   return 0;
+}
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c 
b/drivers/gpu/drm/vkms/vkms_composer.c
index cf1dff162920..73b7d5e94021 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
++

[RFC PATCH v2 14/17] drm/plane: Add COLOR PIPELINE property

2023-10-19 Thread Harry Wentland
We're adding a new enum COLOR PIPELINE property. This
property will have entries for each COLOR PIPELINE by
referencing the DRM object ID of the first drm_colorop
of the pipeline. 0 disables the entire COLOR PIPELINE.

Userspace can use this to discover the available color
pipelines, as well as set the desired one. The color
pipelines are programmed via properties on the actual
drm_colorop objects.

Signed-off-by: Harry Wentland 
Cc: Ville Syrjala 
Cc: Pekka Paalanen 
Cc: Simon Ser 
Cc: Harry Wentland 
Cc: Melissa Wen 
Cc: Jonas Ådahl 
Cc: Sebastian Wick 
Cc: Shashank Sharma 
Cc: Alexander Goins 
Cc: Joshua Ashton 
Cc: Michel Dänzer 
Cc: Aleix Pol 
Cc: Xaver Hugl 
Cc: Victoria Brekenfeld 
Cc: Sima 
Cc: Uma Shankar 
Cc: Naseer Ahmed 
Cc: Christopher Braga 
Cc: Abhinav Kumar 
Cc: Arthur Grillo 
Cc: Hector Martin 
Cc: Liviu Dudau 
Cc: Sasha McIntosh 
---
 drivers/gpu/drm/drm_atomic.c  | 46 +++
 drivers/gpu/drm/drm_atomic_state_helper.c |  5 +++
 drivers/gpu/drm/drm_atomic_uapi.c | 44 ++
 include/drm/drm_atomic_uapi.h |  2 +
 include/drm/drm_plane.h   |  8 
 5 files changed, 105 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 15bd18c9e2be..781bd3aa1849 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1472,6 +1472,52 @@ drm_atomic_add_affected_planes(struct drm_atomic_state 
*state,
 }
 EXPORT_SYMBOL(drm_atomic_add_affected_planes);
 
+/**
+ * drm_atomic_add_affected_colorops - add colorops for plane
+ * @state: atomic state
+ * @plane: DRM plane
+ *
+ * This function walks the current configuration and adds all colorops
+ * currently used by @plane to the atomic configuration @state. This is useful
+ * when an atomic commit also needs to check all currently enabled colorop on
+ * @plane, e.g. when changing the mode. It's also useful when re-enabling a 
plane
+ * to avoid special code to force-enable all colorops.
+ *
+ * Since acquiring a colorop state will always also acquire the w/w mutex of 
the
+ * current plane for that colorop (if there is any) adding all the colorop 
states for
+ * a plane will not reduce parallelism of atomic updates.
+ *
+ * Returns:
+ * 0 on success or can fail with -EDEADLK or -ENOMEM. When the error is EDEADLK
+ * then the w/w mutex code has detected a deadlock and the entire atomic
+ * sequence must be restarted. All other errors are fatal.
+ */
+int
+drm_atomic_add_affected_colorops(struct drm_atomic_state *state,
+struct drm_plane *plane)
+{
+   struct drm_colorop *colorop;
+   struct drm_colorop_state *colorop_state;
+
+   WARN_ON(!drm_atomic_get_new_plane_state(state, plane));
+
+   drm_dbg_atomic(plane->dev,
+  "Adding all current colorops for [plane:%d:%s] to %p\n",
+  plane->base.id, plane->name, state);
+
+   drm_for_each_colorop(colorop, plane->dev) {
+   if (colorop->plane != plane)
+   continue;
+
+   colorop_state = drm_atomic_get_colorop_state(state, colorop);
+   if (IS_ERR(colorop_state))
+   return PTR_ERR(colorop_state);
+   }
+
+   return 0;
+}
+EXPORT_SYMBOL(drm_atomic_add_affected_colorops);
+
 /**
  * drm_atomic_check_only - check whether a given config would work
  * @state: atomic configuration to check
diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c 
b/drivers/gpu/drm/drm_atomic_state_helper.c
index 784e63d70a42..3c5f2c8e33d0 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -267,6 +267,11 @@ void __drm_atomic_helper_plane_state_reset(struct 
drm_plane_state *plane_state,
plane_state->color_range = val;
}
 
+   if (plane->color_pipeline_property) {
+   /* default is always NULL, i.e., bypass */
+   plane_state->color_pipeline = NULL;
+   }
+
if (plane->zpos_property) {
if (!drm_object_property_get_default_value(&plane->base,
   plane->zpos_property,
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c 
b/drivers/gpu/drm/drm_atomic_uapi.c
index a8f7a8a6639a..c6629fdaa114 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -256,6 +256,38 @@ drm_atomic_set_fb_for_plane(struct drm_plane_state 
*plane_state,
 }
 EXPORT_SYMBOL(drm_atomic_set_fb_for_plane);
 
+
+/**
+ * drm_atomic_set_colorop_for_plane - set colorop for plane
+ * @plane_state: atomic state object for the plane
+ * @colorop: colorop to use for the plane
+ *
+ * Changing the assigned framebuffer for a plane requires us to grab a 
reference
+ * to the new fb and drop the reference to the old fb, if there is one. This
+ * function takes care of all these details besides updating the pointer in the
+ * state object itself.
+ */

[RFC PATCH v2 10/17] drm/colorop: Add BYPASS property

2023-10-19 Thread Harry Wentland
We want to be able to bypass each colorop at all times.
Introduce a new BYPASS boolean property for this.

Signed-off-by: Harry Wentland 
Cc: Ville Syrjala 
Cc: Pekka Paalanen 
Cc: Simon Ser 
Cc: Harry Wentland 
Cc: Melissa Wen 
Cc: Jonas Ådahl 
Cc: Sebastian Wick 
Cc: Shashank Sharma 
Cc: Alexander Goins 
Cc: Joshua Ashton 
Cc: Michel Dänzer 
Cc: Aleix Pol 
Cc: Xaver Hugl 
Cc: Victoria Brekenfeld 
Cc: Sima 
Cc: Uma Shankar 
Cc: Naseer Ahmed 
Cc: Christopher Braga 
Cc: Abhinav Kumar 
Cc: Arthur Grillo 
Cc: Hector Martin 
Cc: Liviu Dudau 
Cc: Sasha McIntosh 
---
 drivers/gpu/drm/drm_atomic_uapi.c |  6 +-
 drivers/gpu/drm/drm_colorop.c | 15 +++
 include/drm/drm_colorop.h | 20 
 3 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c 
b/drivers/gpu/drm/drm_atomic_uapi.c
index 52b9b48e5757..a8f7a8a6639a 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -670,7 +670,9 @@ static int drm_atomic_colorop_set_property(struct 
drm_colorop *colorop,
struct drm_colorop_state *state, struct drm_file *file_priv,
struct drm_property *property, uint64_t val)
 {
-   if (property == colorop->curve_1d_type_property) {
+   if (property == colorop->bypass_property) {
+   state->bypass = val;
+   } else if (property == colorop->curve_1d_type_property) {
state->curve_1d_type = val;
} else {
drm_dbg_atomic(colorop->dev,
@@ -690,6 +692,8 @@ drm_atomic_colorop_get_property(struct drm_colorop *colorop,
 {
if (property == colorop->type_property) {
*val = colorop->type;
+   } else if (property == colorop->bypass_property) {
+   *val = state->bypass;
} else if (property == colorop->curve_1d_type_property) {
*val = state->curve_1d_type;
} else {
diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
index 8d8f9461950f..ff6331fe5d5e 100644
--- a/drivers/gpu/drm/drm_colorop.c
+++ b/drivers/gpu/drm/drm_colorop.c
@@ -78,6 +78,18 @@ int drm_colorop_init(struct drm_device *dev, struct 
drm_colorop *colorop,
   colorop->type_property,
   colorop->type);
 
+   /* bypass */
+   /* TODO can we reuse the mode_config->active_prop? */
+   prop = drm_property_create_bool(dev, DRM_MODE_PROP_ATOMIC,
+   "BYPASS");
+   if (!prop)
+   return -ENOMEM;
+
+   colorop->bypass_property = prop;
+   drm_object_attach_property(&colorop->base,
+  colorop->bypass_property,
+  1);
+
/* curve_1d_type */
/* TODO move to mode_config? */
prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
@@ -100,6 +112,8 @@ void __drm_atomic_helper_colorop_duplicate_state(struct 
drm_colorop *colorop,
 struct drm_colorop_state 
*state)
 {
memcpy(state, colorop->state, sizeof(*state));
+
+   state->bypass = true;
 }
 
 struct drm_colorop_state *
@@ -164,6 +178,7 @@ void __drm_colorop_state_reset(struct drm_colorop_state 
*colorop_state,
   struct drm_colorop *colorop)
 {
colorop_state->colorop = colorop;
+   colorop_state->bypass = true;
 }
 EXPORT_SYMBOL(__drm_colorop_state_reset);
 
diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h
index 7701b61ff7e9..69636f6752a0 100644
--- a/include/drm/drm_colorop.h
+++ b/include/drm/drm_colorop.h
@@ -48,6 +48,14 @@ struct drm_colorop_state {
 
/* colorop properties */
 
+   /**
+* @bypass:
+*
+* True if colorop shall be bypassed. False if colorop is
+* enabled.
+*/
+   bool bypass;
+
/**
 * @curve_1d_type:
 *
@@ -135,6 +143,18 @@ struct drm_colorop {
 */
struct drm_property *type_property;
 
+   /**
+* @bypass_property:
+*
+* Boolean property to control enablement of the color
+* operation. Setting bypass to "true" shall always be supported
+* in order to allow compositors to quickly fall back to
+* alternate methods of color processing. This is important
+* since setting color operations can fail due to unique
+* HW constraints.
+*/
+   struct drm_property *bypass_property;
+
/**
 * @curve_1d_type:
 *
-- 
2.42.0



[RFC PATCH v2 12/17] drm/colorop: Add atomic state print for drm_colorop

2023-10-19 Thread Harry Wentland
Signed-off-by: Harry Wentland 
Cc: Ville Syrjala 
Cc: Pekka Paalanen 
Cc: Simon Ser 
Cc: Harry Wentland 
Cc: Melissa Wen 
Cc: Jonas Ådahl 
Cc: Sebastian Wick 
Cc: Shashank Sharma 
Cc: Alexander Goins 
Cc: Joshua Ashton 
Cc: Michel Dänzer 
Cc: Aleix Pol 
Cc: Xaver Hugl 
Cc: Victoria Brekenfeld 
Cc: Sima 
Cc: Uma Shankar 
Cc: Naseer Ahmed 
Cc: Christopher Braga 
Cc: Abhinav Kumar 
Cc: Arthur Grillo 
Cc: Hector Martin 
Cc: Liviu Dudau 
Cc: Sasha McIntosh 
---
 drivers/gpu/drm/drm_atomic.c | 29 +
 include/drm/drm_colorop.h|  5 +
 2 files changed, 34 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 524bec520287..15bd18c9e2be 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -792,6 +792,19 @@ static int drm_atomic_plane_check(const struct 
drm_plane_state *old_plane_state,
return 0;
 }
 
+
+
+static void drm_atomic_colorop_print_state(struct drm_printer *p,
+   const struct drm_colorop_state *state)
+{
+   struct drm_colorop *colorop = state->colorop;
+
+   drm_printf(p, "colorop[%u]:\n", colorop->base.id);
+   drm_printf(p, "\ttype=%s\n", drm_get_colorop_type_name(colorop->type));
+   drm_printf(p, "\tbypass=%u\n", state->bypass);
+   drm_printf(p, "\tcurve_1d_type=%s\n", 
drm_get_colorop_curve_1d_type_name(state->curve_1d_type));
+}
+
 static void drm_atomic_plane_print_state(struct drm_printer *p,
const struct drm_plane_state *state)
 {
@@ -812,6 +825,13 @@ static void drm_atomic_plane_print_state(struct 
drm_printer *p,
   drm_get_color_encoding_name(state->color_encoding));
drm_printf(p, "\tcolor-range=%s\n",
   drm_get_color_range_name(state->color_range));
+#if 0
+   drm_printf(p, "\tcolor-pipeline=%s\n",
+  drm_get_color_pipeline_name(state->color_pipeline));
+#else
+   drm_printf(p, "\tcolor-pipeline=%d\n",
+  state->color_pipeline ? state->color_pipeline->base.id : 0);
+#endif
 
if (plane->funcs->atomic_print_state)
plane->funcs->atomic_print_state(p, state);
@@ -1848,6 +1868,7 @@ static void __drm_state_dump(struct drm_device *dev, 
struct drm_printer *p,
 bool take_locks)
 {
struct drm_mode_config *config = &dev->mode_config;
+   struct drm_colorop *colorop;
struct drm_plane *plane;
struct drm_crtc *crtc;
struct drm_connector *connector;
@@ -1856,6 +1877,14 @@ static void __drm_state_dump(struct drm_device *dev, 
struct drm_printer *p,
if (!drm_drv_uses_atomic_modeset(dev))
return;
 
+   list_for_each_entry(colorop, &config->colorop_list, head) {
+   if (take_locks)
+   drm_modeset_lock(&colorop->plane->mutex, NULL);
+   drm_atomic_colorop_print_state(p, colorop->state);
+   if (take_locks)
+   drm_modeset_unlock(&colorop->plane->mutex);
+   }
+
list_for_each_entry(plane, &config->plane_list, head) {
if (take_locks)
drm_modeset_lock(&plane->mutex, NULL);
diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h
index 1ddd0e65fe36..622a671d2458 100644
--- a/include/drm/drm_colorop.h
+++ b/include/drm/drm_colorop.h
@@ -222,6 +222,11 @@ static inline unsigned int drm_colorop_index(const struct 
drm_colorop *colorop)
 #define drm_for_each_colorop(colorop, dev) \
list_for_each_entry(colorop, &(dev)->mode_config.colorop_list, head)
 
+const char *drm_get_color_pipeline_name(struct drm_colorop *colorop);
+
+const char *drm_get_colorop_type_name(enum drm_colorop_type type);
+const char *drm_get_colorop_curve_1d_type_name(enum drm_colorop_curve_1d_type 
type);
+
 void drm_colorop_set_next_property(struct drm_colorop *colorop, struct 
drm_colorop *next);
 
 
-- 
2.42.0



[RFC PATCH v2 11/17] drm/colorop: Add NEXT property

2023-10-19 Thread Harry Wentland
We'll construct color pipelines out of drm_colorop by
chaining them via the NEXT pointer. NEXT will point to
the next drm_colorop in the pipeline, or by 0 if we're
at the end of the pipeline.

Signed-off-by: Harry Wentland 
Cc: Ville Syrjala 
Cc: Pekka Paalanen 
Cc: Simon Ser 
Cc: Harry Wentland 
Cc: Melissa Wen 
Cc: Jonas Ådahl 
Cc: Sebastian Wick 
Cc: Shashank Sharma 
Cc: Alexander Goins 
Cc: Joshua Ashton 
Cc: Michel Dänzer 
Cc: Aleix Pol 
Cc: Xaver Hugl 
Cc: Victoria Brekenfeld 
Cc: Sima 
Cc: Uma Shankar 
Cc: Naseer Ahmed 
Cc: Christopher Braga 
Cc: Abhinav Kumar 
Cc: Arthur Grillo 
Cc: Hector Martin 
Cc: Liviu Dudau 
Cc: Sasha McIntosh 
---
 drivers/gpu/drm/drm_colorop.c | 27 +++
 include/drm/drm_colorop.h | 12 
 2 files changed, 39 insertions(+)

diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
index ff6331fe5d5e..bc1250718baf 100644
--- a/drivers/gpu/drm/drm_colorop.c
+++ b/drivers/gpu/drm/drm_colorop.c
@@ -104,6 +104,15 @@ int drm_colorop_init(struct drm_device *dev, struct 
drm_colorop *colorop,
   colorop->curve_1d_type_property,
   0);
 
+   prop = drm_property_create_object(dev, DRM_MODE_PROP_IMMUTABLE | 
DRM_MODE_PROP_ATOMIC,
+   "NEXT", DRM_MODE_OBJECT_COLOROP);
+   if (!prop)
+   return -ENOMEM;
+   colorop->next_property = prop;
+   drm_object_attach_property(&colorop->base,
+  colorop->next_property,
+  0);
+
return ret;
 }
 EXPORT_SYMBOL(drm_colorop_init);
@@ -262,3 +271,21 @@ const char *drm_get_colorop_curve_1d_type_name(enum 
drm_colorop_curve_1d_type ty
 
return colorop_curve_1d_type_name[type];
 }
+
+/**
+ * drm_colorop_set_next_property - sets the next pointer
+ * @colorop: drm colorop
+ * @next: next colorop
+ *
+ * Should be used when constructing the color pipeline
+ */
+void drm_colorop_set_next_property(struct drm_colorop *colorop, struct 
drm_colorop *next)
+{
+   if (!colorop->next_property)
+   return;
+
+   drm_object_property_set_value(&colorop->base,
+ colorop->next_property,
+ next->base.id);
+}
+EXPORT_SYMBOL(drm_colorop_set_next_property);
diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h
index 69636f6752a0..1ddd0e65fe36 100644
--- a/include/drm/drm_colorop.h
+++ b/include/drm/drm_colorop.h
@@ -162,10 +162,20 @@ struct drm_colorop {
 */
struct drm_property *curve_1d_type_property;
 
+   /**
+* @next_property
+*
+* Read-only property to next colorop in the pipeline
+*/
+   struct drm_property *next_property;
+
 };
 
 #define obj_to_colorop(x) container_of(x, struct drm_colorop, base)
 
+
+
+
 /**
  * drm_crtc_find - look up a Colorop object from its ID
  * @dev: DRM device
@@ -212,5 +222,7 @@ static inline unsigned int drm_colorop_index(const struct 
drm_colorop *colorop)
 #define drm_for_each_colorop(colorop, dev) \
list_for_each_entry(colorop, &(dev)->mode_config.colorop_list, head)
 
+void drm_colorop_set_next_property(struct drm_colorop *colorop, struct 
drm_colorop *next);
+
 
 #endif /* __DRM_COLOROP_H__ */
-- 
2.42.0



[RFC PATCH v2 09/17] drm/color: Add 1D Curve subtype

2023-10-19 Thread Harry Wentland
Signed-off-by: Harry Wentland 
Cc: Ville Syrjala 
Cc: Pekka Paalanen 
Cc: Simon Ser 
Cc: Harry Wentland 
Cc: Melissa Wen 
Cc: Jonas Ådahl 
Cc: Sebastian Wick 
Cc: Shashank Sharma 
Cc: Alexander Goins 
Cc: Joshua Ashton 
Cc: Michel Dänzer 
Cc: Aleix Pol 
Cc: Xaver Hugl 
Cc: Victoria Brekenfeld 
Cc: Sima 
Cc: Uma Shankar 
Cc: Naseer Ahmed 
Cc: Christopher Braga 
Cc: Abhinav Kumar 
Cc: Arthur Grillo 
Cc: Hector Martin 
Cc: Liviu Dudau 
Cc: Sasha McIntosh 
---
 drivers/gpu/drm/drm_atomic_uapi.c | 18 ++
 drivers/gpu/drm/drm_colorop.c | 39 +++
 include/drm/drm_colorop.h | 20 
 3 files changed, 72 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c 
b/drivers/gpu/drm/drm_atomic_uapi.c
index f22bd8671236..52b9b48e5757 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -670,11 +670,17 @@ static int drm_atomic_colorop_set_property(struct 
drm_colorop *colorop,
struct drm_colorop_state *state, struct drm_file *file_priv,
struct drm_property *property, uint64_t val)
 {
-   drm_dbg_atomic(colorop->dev,
-   "[COLOROP:%d] unknown property [PROP:%d:%s]]\n",
-   colorop->base.id,
-   property->base.id, property->name);
-   return -EINVAL;
+   if (property == colorop->curve_1d_type_property) {
+   state->curve_1d_type = val;
+   } else {
+   drm_dbg_atomic(colorop->dev,
+  "[COLOROP:%d:%d] unknown property 
[PROP:%d:%s]]\n",
+  colorop->base.id, colorop->type,
+  property->base.id, property->name);
+   return -EINVAL;
+   }
+
+   return 0;
 }
 
 static int
@@ -684,6 +690,8 @@ drm_atomic_colorop_get_property(struct drm_colorop *colorop,
 {
if (property == colorop->type_property) {
*val = colorop->type;
+   } else if (property == colorop->curve_1d_type_property) {
+   *val = state->curve_1d_type;
} else {
return -EINVAL;
}
diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
index 33e7dbf4dbe4..8d8f9461950f 100644
--- a/drivers/gpu/drm/drm_colorop.c
+++ b/drivers/gpu/drm/drm_colorop.c
@@ -36,6 +36,11 @@ static const struct drm_prop_enum_list 
drm_colorop_type_enum_list[] = {
{ DRM_COLOROP_1D_CURVE, "1D Curve" },
 };
 
+static const struct drm_prop_enum_list drm_colorop_curve_1d_type_enum_list[] = 
{
+   { DRM_COLOROP_1D_CURVE_SRGB_EOTF, "sRGB EOTF" },
+   { DRM_COLOROP_1D_CURVE_SRGB_INV_EOTF, "sRGB Inverse EOTF" },
+};
+
 /* Init Helpers */
 
 int drm_colorop_init(struct drm_device *dev, struct drm_colorop *colorop,
@@ -73,6 +78,20 @@ int drm_colorop_init(struct drm_device *dev, struct 
drm_colorop *colorop,
   colorop->type_property,
   colorop->type);
 
+   /* curve_1d_type */
+   /* TODO move to mode_config? */
+   prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
+   "CURVE_1D_TYPE",
+   drm_colorop_curve_1d_type_enum_list,
+   
ARRAY_SIZE(drm_colorop_curve_1d_type_enum_list));
+   if (!prop)
+   return -ENOMEM;
+
+   colorop->curve_1d_type_property = prop;
+   drm_object_attach_property(&colorop->base,
+  colorop->curve_1d_type_property,
+  0);
+
return ret;
 }
 EXPORT_SYMBOL(drm_colorop_init);
@@ -194,6 +213,11 @@ static const char * const colorop_type_name[] = {
[DRM_COLOROP_1D_CURVE] = "1D Curve",
 };
 
+static const char * const colorop_curve_1d_type_name[] = {
+   [DRM_COLOROP_1D_CURVE_SRGB_EOTF] = "sRGB EOTF",
+   [DRM_COLOROP_1D_CURVE_SRGB_INV_EOTF] = "sRGB Inverse EOTF",
+};
+
 /**
  * drm_get_colorop_type_name - return a string for colorop type
  * @range: colorop type to compute name of
@@ -208,3 +232,18 @@ const char *drm_get_colorop_type_name(enum 
drm_colorop_type type)
 
return colorop_type_name[type];
 }
+
+/**
+ * drm_get_colorop_curve_1d_type_name - return a string for 1D curve type
+ * @range: 1d curve type to compute name of
+ *
+ * In contrast to the other drm_get_*_name functions this one here returns a
+ * const pointer and hence is threadsafe.
+ */
+const char *drm_get_colorop_curve_1d_type_name(enum drm_colorop_curve_1d_type 
type)
+{
+   if (WARN_ON(type >= ARRAY_SIZE(colorop_curve_1d_type_name)))
+   return "unknown";
+
+   return colorop_curve_1d_type_name[type];
+}
diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h
index 22a217372428..7701b61ff7e9 100644
--- a/include/drm/drm_colorop.h
+++ b/include/drm/drm_colorop.h
@@ -34,6 +34,11 @@ enum drm_colorop_type {
DRM_COLOROP_1D

[RFC PATCH v2 04/17] drm/vkms: Add kunit tests for VKMS LUT handling

2023-10-19 Thread Harry Wentland
Debugging LUT math is much easier when we can unit test
it. Add kunit functionality to VKMS and add tests for
 - get_lut_index
 - lerp_u16

Signed-off-by: Harry Wentland 
Cc: Ville Syrjala 
Cc: Pekka Paalanen 
Cc: Simon Ser 
Cc: Harry Wentland 
Cc: Melissa Wen 
Cc: Jonas Ådahl 
Cc: Sebastian Wick 
Cc: Shashank Sharma 
Cc: Alexander Goins 
Cc: Joshua Ashton 
Cc: Michel Dänzer 
Cc: Aleix Pol 
Cc: Xaver Hugl 
Cc: Victoria Brekenfeld 
Cc: Sima 
Cc: Uma Shankar 
Cc: Naseer Ahmed 
Cc: Christopher Braga 
Cc: Abhinav Kumar 
Cc: Arthur Grillo 
Cc: Hector Martin 
Cc: Liviu Dudau 
Cc: Sasha McIntosh 
---
 drivers/gpu/drm/vkms/Kconfig  |  5 ++
 drivers/gpu/drm/vkms/Makefile |  2 +
 drivers/gpu/drm/vkms/tests/.kunitconfig   |  4 ++
 drivers/gpu/drm/vkms/tests/Makefile   |  4 ++
 drivers/gpu/drm/vkms/tests/vkms_color_tests.c | 64 +++
 drivers/gpu/drm/vkms/vkms_composer.c  |  4 +-
 drivers/gpu/drm/vkms/vkms_composer.h  | 11 
 7 files changed, 92 insertions(+), 2 deletions(-)
 create mode 100644 drivers/gpu/drm/vkms/tests/.kunitconfig
 create mode 100644 drivers/gpu/drm/vkms/tests/Makefile
 create mode 100644 drivers/gpu/drm/vkms/tests/vkms_color_tests.c
 create mode 100644 drivers/gpu/drm/vkms/vkms_composer.h

diff --git a/drivers/gpu/drm/vkms/Kconfig b/drivers/gpu/drm/vkms/Kconfig
index 1816562381a2..372cc5fa92f1 100644
--- a/drivers/gpu/drm/vkms/Kconfig
+++ b/drivers/gpu/drm/vkms/Kconfig
@@ -13,3 +13,8 @@ config DRM_VKMS
  a VKMS.
 
  If M is selected the module will be called vkms.
+
+config DRM_VKMS_KUNIT_TESTS
+   tristate "Tests for VKMS" if !KUNIT_ALL_TESTS
+   depends on DRM_VKMS && KUNIT
+   default KUNIT_ALL_TESTS
diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
index 1b28a6a32948..d3440f228f46 100644
--- a/drivers/gpu/drm/vkms/Makefile
+++ b/drivers/gpu/drm/vkms/Makefile
@@ -9,3 +9,5 @@ vkms-y := \
vkms_writeback.o
 
 obj-$(CONFIG_DRM_VKMS) += vkms.o
+
+obj-y += tests/
\ No newline at end of file
diff --git a/drivers/gpu/drm/vkms/tests/.kunitconfig 
b/drivers/gpu/drm/vkms/tests/.kunitconfig
new file mode 100644
index ..70e378228cbd
--- /dev/null
+++ b/drivers/gpu/drm/vkms/tests/.kunitconfig
@@ -0,0 +1,4 @@
+CONFIG_KUNIT=y
+CONFIG_DRM=y
+CONFIG_DRM_VKMS=y
+CONFIG_DRM_VKMS_KUNIT_TESTS=y
diff --git a/drivers/gpu/drm/vkms/tests/Makefile 
b/drivers/gpu/drm/vkms/tests/Makefile
new file mode 100644
index ..761465332ff2
--- /dev/null
+++ b/drivers/gpu/drm/vkms/tests/Makefile
@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: GPL-2.0+
+
+obj-$(CONFIG_DRM_VKMS_KUNIT_TESTS) += \
+   vkms_color_tests.o
\ No newline at end of file
diff --git a/drivers/gpu/drm/vkms/tests/vkms_color_tests.c 
b/drivers/gpu/drm/vkms/tests/vkms_color_tests.c
new file mode 100644
index ..843b2e1d607e
--- /dev/null
+++ b/drivers/gpu/drm/vkms/tests/vkms_color_tests.c
@@ -0,0 +1,64 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+
+#include 
+
+#include 
+
+#include "../vkms_composer.h"
+
+#define TEST_LUT_SIZE 16
+
+static struct drm_color_lut test_linear_array[TEST_LUT_SIZE] = {
+   { 0x0, 0x0, 0x0, 0 },
+   { 0x, 0x, 0x, 0 },
+   { 0x, 0x, 0x, 0 },
+   { 0x, 0x, 0x, 0 },
+   { 0x, 0x, 0x, 0 },
+   { 0x, 0x, 0x, 0 },
+   { 0x, 0x, 0x, 0 },
+   { 0x, 0x, 0x, 0 },
+   { 0x, 0x, 0x, 0 },
+   { 0x, 0x, 0x, 0 },
+   { 0x, 0x, 0x, 0 },
+   { 0x, 0x, 0x, 0 },
+   { 0x, 0x, 0x, 0 },
+   { 0x, 0x, 0x, 0 },
+   { 0x, 0x, 0x, 0 },
+   { 0x, 0x, 0x, 0 },
+};
+
+const struct vkms_color_lut test_linear_lut = {
+   .base = test_linear_array,
+   .lut_length = TEST_LUT_SIZE,
+   .channel_value2index_ratio = 0xf000fll
+};
+
+
+static void vkms_color_test_get_lut_index(struct kunit *test)
+{
+   int i;
+
+   KUNIT_EXPECT_EQ(test, drm_fixp2int(get_lut_index(&test_linear_lut, 
test_linear_array[0].red)), 0);
+
+   for (i = 0; i < TEST_LUT_SIZE; i++)
+   KUNIT_EXPECT_EQ(test, 
drm_fixp2int_ceil(get_lut_index(&test_linear_lut, test_linear_array[i].red)), 
i);
+}
+
+static void vkms_color_test_lerp(struct kunit *test)
+{
+   KUNIT_EXPECT_EQ(test, lerp_u16(0x0, 0x10, 0x8000), 0x8);
+}
+
+static struct kunit_case vkms_color_test_cases[] = {
+   KUNIT_CASE(vkms_color_test_get_lut_index),
+   KUNIT_CASE(vkms_color_test_lerp),
+   {}
+};
+
+static struct kunit_suite vkms_color_test_suite = {
+   .name = "vkms-color",
+   .test_cases = vkms_color_test_cases,
+};
+kunit_test_suite(vkms_color_test_suite);
+
+MODULE_LICENSE("GPL");
\ No newline at end of file
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c 
b/drivers/gpu/drm/vkms/vkms_composer.c
index 3c99fb8b54e2..a0a3a6fd2926 100644
--- a/drivers/gpu/drm

[RFC PATCH v2 07/17] drm/colorop: Introduce new drm_colorop mode object

2023-10-19 Thread Harry Wentland
This patches introduces a new drm_colorop mode object. This
object represents color transformations and can be used to
define color pipelines.

We also introduce the drm_colorop_state here, as well as
various helpers and state tracking bits.

Signed-off-by: Harry Wentland 
Cc: Ville Syrjala 
Cc: Pekka Paalanen 
Cc: Simon Ser 
Cc: Harry Wentland 
Cc: Melissa Wen 
Cc: Jonas Ådahl 
Cc: Sebastian Wick 
Cc: Shashank Sharma 
Cc: Alexander Goins 
Cc: Joshua Ashton 
Cc: Michel Dänzer 
Cc: Aleix Pol 
Cc: Xaver Hugl 
Cc: Victoria Brekenfeld 
Cc: Sima 
Cc: Uma Shankar 
Cc: Naseer Ahmed 
Cc: Christopher Braga 
Cc: Abhinav Kumar 
Cc: Arthur Grillo 
Cc: Hector Martin 
Cc: Liviu Dudau 
Cc: Sasha McIntosh 
---
 drivers/gpu/drm/Makefile|   1 +
 drivers/gpu/drm/drm_atomic.c|  79 +
 drivers/gpu/drm/drm_atomic_helper.c |  12 ++
 drivers/gpu/drm/drm_atomic_uapi.c   |  48 
 drivers/gpu/drm/drm_colorop.c   | 169 
 drivers/gpu/drm/drm_mode_config.c   |   7 ++
 drivers/gpu/drm/drm_plane_helper.c  |   2 +-
 include/drm/drm_atomic.h|  82 ++
 include/drm/drm_atomic_uapi.h   |   1 +
 include/drm/drm_colorop.h   | 157 ++
 include/drm/drm_mode_config.h   |  18 +++
 include/drm/drm_plane.h |   2 +
 include/uapi/drm/drm.h  |   3 +
 include/uapi/drm/drm_mode.h |   1 +
 14 files changed, 581 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/drm_colorop.c
 create mode 100644 include/drm/drm_colorop.h

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 8e1bde059170..7ba67f9775e7 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -16,6 +16,7 @@ drm-y := \
drm_client.o \
drm_client_modeset.o \
drm_color_mgmt.o \
+   drm_colorop.o \
drm_connector.o \
drm_crtc.o \
drm_displayid.o \
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index f1a503aafe5a..d55db5a06940 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -42,6 +42,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "drm_crtc_internal.h"
 #include "drm_internal.h"
@@ -108,6 +109,7 @@ void drm_atomic_state_default_release(struct 
drm_atomic_state *state)
kfree(state->connectors);
kfree(state->crtcs);
kfree(state->planes);
+   kfree(state->colorops);
kfree(state->private_objs);
 }
 EXPORT_SYMBOL(drm_atomic_state_default_release);
@@ -139,6 +141,10 @@ drm_atomic_state_init(struct drm_device *dev, struct 
drm_atomic_state *state)
sizeof(*state->planes), GFP_KERNEL);
if (!state->planes)
goto fail;
+   state->colorops = kcalloc(dev->mode_config.num_colorop,
+ sizeof(*state->colorops), GFP_KERNEL);
+   if (!state->colorops)
+   goto fail;
 
/*
 * Because drm_atomic_state can be committed asynchronously we need our
@@ -250,6 +256,20 @@ void drm_atomic_state_default_clear(struct 
drm_atomic_state *state)
state->planes[i].new_state = NULL;
}
 
+   for (i = 0; i < config->num_colorop; i++) {
+   struct drm_colorop *colorop = state->colorops[i].ptr;
+
+   if (!colorop)
+   continue;
+
+   drm_colorop_atomic_destroy_state(colorop,
+state->colorops[i].state);
+   state->colorops[i].ptr = NULL;
+   state->colorops[i].state = NULL;
+   state->colorops[i].old_state = NULL;
+   state->colorops[i].new_state = NULL;
+   }
+
for (i = 0; i < state->num_private_objs; i++) {
struct drm_private_obj *obj = state->private_objs[i].ptr;
 
@@ -571,6 +591,65 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state,
 }
 EXPORT_SYMBOL(drm_atomic_get_plane_state);
 
+
+/**
+ * drm_atomic_get_colorop_state - get colorop state
+ * @state: global atomic state object
+ * @colorop: colorop to get state object for
+ *
+ * This function returns the colorop state for the given colorop, allocating it
+ * if needed. It will also grab the relevant plane lock to make sure that the
+ * state is consistent.
+ *
+ * Returns:
+ *
+ * Either the allocated state or the error code encoded into the pointer. When
+ * the error is EDEADLK then the w/w mutex code has detected a deadlock and the
+ * entire atomic sequence must be restarted. All other errors are fatal.
+ */
+struct drm_colorop_state *
+drm_atomic_get_colorop_state(struct drm_atomic_state *state,
+struct drm_colorop *colorop)
+{
+   int ret, index = drm_colorop_index(colorop);
+   struct drm_colorop_state *colorop_state;
+   struct drm_plane_state *plane_state;
+
+   WARN_ON(!state->acquire_ctx);
+
+   colorop_state = drm_atomic_get_exist

[RFC PATCH v2 06/17] drm/doc/rfc: Describe why prescriptive color pipeline is needed

2023-10-19 Thread Harry Wentland
v2:
 - Update colorop visualizations to match reality (Sebastian, Alex Hung)
 - Updated wording (Pekka)
 - Change BYPASS wording to make it non-mandatory (Sebastian)
 - Drop cover-letter-like paragraph from COLOR_PIPELINE Plane Property
   section (Pekka)
 - Use PQ EOTF instead of its inverse in Pipeline Programming example (Melissa)
 - Add "Driver Implementer's Guide" section (Pekka)
 - Add "Driver Forward/Backward Compatibility" section (Sebastian, Pekka)

Signed-off-by: Harry Wentland 
Cc: Ville Syrjala 
Cc: Pekka Paalanen 
Cc: Simon Ser 
Cc: Harry Wentland 
Cc: Melissa Wen 
Cc: Jonas Ådahl 
Cc: Sebastian Wick 
Cc: Shashank Sharma 
Cc: Alexander Goins 
Cc: Joshua Ashton 
Cc: Michel Dänzer 
Cc: Aleix Pol 
Cc: Xaver Hugl 
Cc: Victoria Brekenfeld 
Cc: Sima 
Cc: Uma Shankar 
Cc: Naseer Ahmed 
Cc: Christopher Braga 
Cc: Abhinav Kumar 
Cc: Arthur Grillo 
Cc: Hector Martin 
Cc: Liviu Dudau 
Cc: Sasha McIntosh 
---
 Documentation/gpu/rfc/color_pipeline.rst | 347 +++
 1 file changed, 347 insertions(+)
 create mode 100644 Documentation/gpu/rfc/color_pipeline.rst

diff --git a/Documentation/gpu/rfc/color_pipeline.rst 
b/Documentation/gpu/rfc/color_pipeline.rst
new file mode 100644
index ..af5f2ea29116
--- /dev/null
+++ b/Documentation/gpu/rfc/color_pipeline.rst
@@ -0,0 +1,347 @@
+
+Linux Color Pipeline API
+
+
+What problem are we solving?
+
+
+We would like to support pre-, and post-blending complex color
+transformations in display controller hardware in order to allow for
+HW-supported HDR use-cases, as well as to provide support to
+color-managed applications, such as video or image editors.
+
+It is possible to support an HDR output on HW supporting the Colorspace
+and HDR Metadata drm_connector properties, but that requires the
+compositor or application to render and compose the content into one
+final buffer intended for display. Doing so is costly.
+
+Most modern display HW offers various 1D LUTs, 3D LUTs, matrices, and other
+operations to support color transformations. These operations are often
+implemented in fixed-function HW and therefore much more power efficient than
+performing similar operations via shaders or CPU.
+
+We would like to make use of this HW functionality to support complex color
+transformations with no, or minimal CPU or shader load.
+
+
+How are other OSes solving this problem?
+
+
+The most widely supported use-cases regard HDR content, whether video or
+gaming.
+
+Most OSes will specify the source content format (color gamut, encoding 
transfer
+function, and other metadata, such as max and average light levels) to a 
driver.
+Drivers will then program their fixed-function HW accordingly to map from a
+source content buffer's space to a display's space.
+
+When fixed-function HW is not available the compositor will assemble a shader 
to
+ask the GPU to perform the transformation from the source content format to the
+display's format.
+
+A compositor's mapping function and a driver's mapping function are usually
+entirely separate concepts. On OSes where a HW vendor has no insight into
+closed-source compositor code such a vendor will tune their color management
+code to visually match the compositor's. On other OSes, where both mapping
+functions are open to an implementer they will ensure both mappings match.
+
+This results in mapping algorithm lock-in, meaning that no-one alone can
+experiment with or introduce new mapping algorithms and achieve
+consistent results regardless of which implementation path is taken.
+
+Why is Linux different?
+===
+
+Unlike other OSes, where there is one compositor for one or more drivers, on
+Linux we have a many-to-many relationship. Many compositors; many drivers.
+In addition each compositor vendor or community has their own view of how
+color management should be done. This is what makes Linux so beautiful.
+
+This means that a HW vendor can now no longer tune their driver to one
+compositor, as tuning it to one could make it look fairly different from
+another compositor's color mapping.
+
+We need a better solution.
+
+
+Descriptive API
+===
+
+An API that describes the source and destination colorspaces is a descriptive
+API. It describes the input and output color spaces but does not describe
+how precisely they should be mapped. Such a mapping includes many minute
+design decision that can greatly affect the look of the final result.
+
+It is not feasible to describe such mapping with enough detail to ensure the
+same result from each implementation. In fact, these mappings are a very active
+research area.
+
+
+Prescriptive API
+
+
+A prescriptive API describes not the source and destination colorspaces. It
+instead prescribes a recipe for how to manipulate pixel values to arrive at the
+desired outcome.
+
+This recipe is generally an orde

[RFC PATCH v2 08/17] drm/colorop: Add TYPE property

2023-10-19 Thread Harry Wentland
Add a read-only TYPE property. The TYPE specifies the colorop
type, such as enumerated curve, 1D LUT, CTM, 3D LUT, PWL LUT,
etc.

For now we're only introducing an enumerated 1D LUT type to
illustrate the concept.

Signed-off-by: Harry Wentland 
Cc: Ville Syrjala 
Cc: Pekka Paalanen 
Cc: Simon Ser 
Cc: Harry Wentland 
Cc: Melissa Wen 
Cc: Jonas Ådahl 
Cc: Sebastian Wick 
Cc: Shashank Sharma 
Cc: Alexander Goins 
Cc: Joshua Ashton 
Cc: Michel Dänzer 
Cc: Aleix Pol 
Cc: Xaver Hugl 
Cc: Victoria Brekenfeld 
Cc: Sima 
Cc: Uma Shankar 
Cc: Naseer Ahmed 
Cc: Christopher Braga 
Cc: Abhinav Kumar 
Cc: Arthur Grillo 
Cc: Hector Martin 
Cc: Liviu Dudau 
Cc: Sasha McIntosh 
---
 drivers/gpu/drm/drm_atomic.c  |  4 +--
 drivers/gpu/drm/drm_atomic_uapi.c |  8 +-
 drivers/gpu/drm/drm_colorop.c | 43 ++-
 include/drm/drm_colorop.h | 21 ++-
 4 files changed, 71 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index d55db5a06940..524bec520287 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -636,8 +636,8 @@ drm_atomic_get_colorop_state(struct drm_atomic_state *state,
state->colorops[index].new_state = colorop_state;
colorop_state->state = state;
 
-   drm_dbg_atomic(colorop->dev, "Added [COLOROP:%d] %p state to %p\n",
-  colorop->base.id, colorop_state, state);
+   drm_dbg_atomic(colorop->dev, "Added [COLOROP:%d:%d] %p state to %p\n",
+  colorop->base.id, colorop->type, colorop_state, state);
 
/* TODO is this necessary? */
 
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c 
b/drivers/gpu/drm/drm_atomic_uapi.c
index 21da1b327ee9..f22bd8671236 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -682,7 +682,13 @@ drm_atomic_colorop_get_property(struct drm_colorop 
*colorop,
const struct drm_colorop_state *state,
struct drm_property *property, uint64_t *val)
 {
-   return -EINVAL;
+   if (property == colorop->type_property) {
+   *val = colorop->type;
+   } else {
+   return -EINVAL;
+   }
+
+   return 0;
 }
 
 static int drm_atomic_set_writeback_fb_for_connector(
diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
index 78d6a0067f5b..33e7dbf4dbe4 100644
--- a/drivers/gpu/drm/drm_colorop.c
+++ b/drivers/gpu/drm/drm_colorop.c
@@ -32,12 +32,17 @@
 
 /* TODO big colorop doc, including properties, etc. */
 
+static const struct drm_prop_enum_list drm_colorop_type_enum_list[] = {
+   { DRM_COLOROP_1D_CURVE, "1D Curve" },
+};
+
 /* Init Helpers */
 
 int drm_colorop_init(struct drm_device *dev, struct drm_colorop *colorop,
-struct drm_plane *plane)
+struct drm_plane *plane, enum drm_colorop_type type)
 {
struct drm_mode_config *config = &dev->mode_config;
+   struct drm_property *prop;
int ret = 0;
 
ret = drm_mode_object_add(dev, &colorop->base, DRM_MODE_OBJECT_COLOROP);
@@ -46,12 +51,28 @@ int drm_colorop_init(struct drm_device *dev, struct 
drm_colorop *colorop,
 
colorop->base.properties = &colorop->properties;
colorop->dev = dev;
+   colorop->type = type;
colorop->plane = plane;
 
list_add_tail(&colorop->head, &config->colorop_list);
colorop->index = config->num_colorop++;
 
/* add properties */
+
+   /* type */
+   prop = drm_property_create_enum(dev,
+   DRM_MODE_PROP_IMMUTABLE | 
DRM_MODE_PROP_ATOMIC,
+   "TYPE", drm_colorop_type_enum_list,
+   ARRAY_SIZE(drm_colorop_type_enum_list));
+   if (!prop)
+   return -ENOMEM;
+
+   colorop->type_property = prop;
+
+   drm_object_attach_property(&colorop->base,
+  colorop->type_property,
+  colorop->type);
+
return ret;
 }
 EXPORT_SYMBOL(drm_colorop_init);
@@ -167,3 +188,23 @@ void drm_colorop_reset(struct drm_colorop *colorop)
__drm_colorop_reset(colorop, colorop->state);
 }
 EXPORT_SYMBOL(drm_colorop_reset);
+
+
+static const char * const colorop_type_name[] = {
+   [DRM_COLOROP_1D_CURVE] = "1D Curve",
+};
+
+/**
+ * drm_get_colorop_type_name - return a string for colorop type
+ * @range: colorop type to compute name of
+ *
+ * In contrast to the other drm_get_*_name functions this one here returns a
+ * const pointer and hence is threadsafe.
+ */
+const char *drm_get_colorop_type_name(enum drm_colorop_type type)
+{
+   if (WARN_ON(type >= ARRAY_SIZE(colorop_type_name)))
+   return "unknown";
+
+   return colorop_type_name[type];
+}
diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h
index 3dd169b0317d..22a217372428 100644
--- a/include/drm/drm_colorop

[RFC PATCH v2 03/17] drm/vkms: Create separate Kconfig file for VKMS

2023-10-19 Thread Harry Wentland
This aligns with most other DRM drivers and will allow
us to add new VKMS config options without polluting
the DRM Kconfig.

Signed-off-by: Harry Wentland 
Cc: Ville Syrjala 
Cc: Pekka Paalanen 
Cc: Simon Ser 
Cc: Harry Wentland 
Cc: Melissa Wen 
Cc: Jonas Ådahl 
Cc: Sebastian Wick 
Cc: Shashank Sharma 
Cc: Alexander Goins 
Cc: Joshua Ashton 
Cc: Michel Dänzer 
Cc: Aleix Pol 
Cc: Xaver Hugl 
Cc: Victoria Brekenfeld 
Cc: Sima 
Cc: Uma Shankar 
Cc: Naseer Ahmed 
Cc: Christopher Braga 
Cc: Abhinav Kumar 
Cc: Arthur Grillo 
Cc: Hector Martin 
Cc: Liviu Dudau 
Cc: Sasha McIntosh 
---
 drivers/gpu/drm/Kconfig  | 14 +-
 drivers/gpu/drm/vkms/Kconfig | 15 +++
 2 files changed, 16 insertions(+), 13 deletions(-)
 create mode 100644 drivers/gpu/drm/vkms/Kconfig

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 48ca28a2e4ff..61ebd682c9b0 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -286,19 +286,7 @@ config DRM_VGEM
  as used by Mesa's software renderer for enhanced performance.
  If M is selected the module will be called vgem.
 
-config DRM_VKMS
-   tristate "Virtual KMS (EXPERIMENTAL)"
-   depends on DRM && MMU
-   select DRM_KMS_HELPER
-   select DRM_GEM_SHMEM_HELPER
-   select CRC32
-   default n
-   help
- Virtual Kernel Mode-Setting (VKMS) is used for testing or for
- running GPU in a headless machines. Choose this option to get
- a VKMS.
-
- If M is selected the module will be called vkms.
+source "drivers/gpu/drm/vkms/Kconfig"
 
 source "drivers/gpu/drm/exynos/Kconfig"
 
diff --git a/drivers/gpu/drm/vkms/Kconfig b/drivers/gpu/drm/vkms/Kconfig
new file mode 100644
index ..1816562381a2
--- /dev/null
+++ b/drivers/gpu/drm/vkms/Kconfig
@@ -0,0 +1,15 @@
+# SPDX-License-Identifier: GPL-2.0+
+
+config DRM_VKMS
+   tristate "Virtual KMS (EXPERIMENTAL)"
+   depends on DRM && MMU
+   select DRM_KMS_HELPER
+   select DRM_GEM_SHMEM_HELPER
+   select CRC32
+   default n
+   help
+ Virtual Kernel Mode-Setting (VKMS) is used for testing or for
+ running GPU in a headless machines. Choose this option to get
+ a VKMS.
+
+ If M is selected the module will be called vkms.
-- 
2.42.0



[RFC PATCH v2 05/17] drm/vkms: Avoid reading beyond LUT array

2023-10-19 Thread Harry Wentland
When the floor LUT index (drm_fixp2int(lut_index) is the last
index of the array the ceil LUT index will point to an entry
beyond the array. Make sure we guard against it and use the
value of the floot LUT index.

Blurb about LUT creation and how first element should be 0x0 and
last one 0x.

Hold on, is that even correct? What should the ends of a LUT be?
How does UNORM work and how does it apply to LUTs?

Signed-off-by: Harry Wentland 
Cc: Ville Syrjala 
Cc: Pekka Paalanen 
Cc: Simon Ser 
Cc: Harry Wentland 
Cc: Melissa Wen 
Cc: Jonas Ådahl 
Cc: Sebastian Wick 
Cc: Shashank Sharma 
Cc: Alexander Goins 
Cc: Joshua Ashton 
Cc: Michel Dänzer 
Cc: Aleix Pol 
Cc: Xaver Hugl 
Cc: Victoria Brekenfeld 
Cc: Sima 
Cc: Uma Shankar 
Cc: Naseer Ahmed 
Cc: Christopher Braga 
Cc: Abhinav Kumar 
Cc: Arthur Grillo 
Cc: Hector Martin 
Cc: Liviu Dudau 
Cc: Sasha McIntosh 
---
 drivers/gpu/drm/vkms/vkms_composer.c | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c 
b/drivers/gpu/drm/vkms/vkms_composer.c
index a0a3a6fd2926..cf1dff162920 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -123,6 +123,8 @@ static u16 apply_lut_to_channel_value(const struct 
vkms_color_lut *lut, u16 chan
  enum lut_channel channel)
 {
s64 lut_index = get_lut_index(lut, channel_value);
+   u16 *floor_lut_value, *ceil_lut_value;
+   u16 floor_channel_value, ceil_channel_value;
 
/*
 * This checks if `struct drm_color_lut` has any gap added by the 
compiler
@@ -130,11 +132,15 @@ static u16 apply_lut_to_channel_value(const struct 
vkms_color_lut *lut, u16 chan
 */
static_assert(sizeof(struct drm_color_lut) == sizeof(__u16) * 4);
 
-   u16 *floor_lut_value = (__u16 *)&lut->base[drm_fixp2int(lut_index)];
-   u16 *ceil_lut_value = (__u16 *)&lut->base[drm_fixp2int_ceil(lut_index)];
+   floor_lut_value = (__u16 *)&lut->base[drm_fixp2int(lut_index)];
+   if (drm_fixp2int(lut_index) == (lut->lut_length - 1))
+   /* We're at the end of the LUT array, use same value for ceil 
and floor */
+   ceil_lut_value = floor_lut_value;
+   else
+   ceil_lut_value = (__u16 
*)&lut->base[drm_fixp2int_ceil(lut_index)];
 
-   u16 floor_channel_value = floor_lut_value[channel];
-   u16 ceil_channel_value = ceil_lut_value[channel];
+   floor_channel_value = floor_lut_value[channel];
+   ceil_channel_value = ceil_lut_value[channel];
 
return lerp_u16(floor_channel_value, ceil_channel_value,
lut_index & DRM_FIXED_DECIMAL_MASK);
-- 
2.42.0



[RFC PATCH v2 02/17] drm: Don't treat 0 as -1 in drm_fixp2int_ceil

2023-10-19 Thread Harry Wentland
Unit testing this in VKMS shows that passing 0 into
this function returns -1, which is highly counter-
intuitive. Fix it by checking whether the input is
>= 0 instead of > 0.

Signed-off-by: Harry Wentland 
Cc: Ville Syrjala 
Cc: Pekka Paalanen 
Cc: Simon Ser 
Cc: Harry Wentland 
Cc: Melissa Wen 
Cc: Jonas Ådahl 
Cc: Sebastian Wick 
Cc: Shashank Sharma 
Cc: Alexander Goins 
Cc: Joshua Ashton 
Cc: Michel Dänzer 
Cc: Aleix Pol 
Cc: Xaver Hugl 
Cc: Victoria Brekenfeld 
Cc: Sima 
Cc: Uma Shankar 
Cc: Naseer Ahmed 
Cc: Christopher Braga 
Cc: Abhinav Kumar 
Cc: Arthur Grillo 
Cc: Hector Martin 
Cc: Liviu Dudau 
Cc: Sasha McIntosh 
---
 include/drm/drm_fixed.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/drm/drm_fixed.h b/include/drm/drm_fixed.h
index 6ea339d5de08..0c9f917a4d4b 100644
--- a/include/drm/drm_fixed.h
+++ b/include/drm/drm_fixed.h
@@ -95,7 +95,7 @@ static inline int drm_fixp2int_round(s64 a)
 
 static inline int drm_fixp2int_ceil(s64 a)
 {
-   if (a > 0)
+   if (a >= 0)
return drm_fixp2int(a + DRM_FIXED_ALMOST_ONE);
else
return drm_fixp2int(a - DRM_FIXED_ALMOST_ONE);
-- 
2.42.0



[RFC PATCH v2 01/17] drm/atomic: Allow get_value for immutable properties on atomic drivers

2023-10-19 Thread Harry Wentland
drm_colorops use immutable properties, for type and next.
Even though drivers create these properties at initialization
they will need to look at the properties when parsing a
color pipeline for programming during an atomic check
or commit operation.

This aligns the get_value call with behavior of the set_value
call.

Signed-off-by: Harry Wentland 
Cc: Ville Syrjala 
Cc: Pekka Paalanen 
Cc: Simon Ser 
Cc: Harry Wentland 
Cc: Melissa Wen 
Cc: Jonas Ådahl 
Cc: Sebastian Wick 
Cc: Shashank Sharma 
Cc: Alexander Goins 
Cc: Joshua Ashton 
Cc: Michel Dänzer 
Cc: Aleix Pol 
Cc: Xaver Hugl 
Cc: Victoria Brekenfeld 
Cc: Sima 
Cc: Uma Shankar 
Cc: Naseer Ahmed 
Cc: Christopher Braga 
Cc: Abhinav Kumar 
Cc: Arthur Grillo 
Cc: Hector Martin 
Cc: Liviu Dudau 
Cc: Sasha McIntosh 
---
 drivers/gpu/drm/drm_mode_object.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_mode_object.c 
b/drivers/gpu/drm/drm_mode_object.c
index ac0d2ce3f870..c9b1cd48547a 100644
--- a/drivers/gpu/drm/drm_mode_object.c
+++ b/drivers/gpu/drm/drm_mode_object.c
@@ -351,7 +351,8 @@ static int __drm_object_property_get_value(struct 
drm_mode_object *obj,
 int drm_object_property_get_value(struct drm_mode_object *obj,
  struct drm_property *property, uint64_t *val)
 {
-   WARN_ON(drm_drv_uses_atomic_modeset(property->dev));
+   WARN_ON(drm_drv_uses_atomic_modeset(property->dev) &&
+   !(property->flags & DRM_MODE_PROP_IMMUTABLE));
 
return __drm_object_property_get_value(obj, property, val);
 }
-- 
2.42.0



[RFC PATCH v2 00/17] Color Pipeline API w/ VKMS

2023-10-19 Thread Harry Wentland
This is an early RFC set for a color pipeline API, along with a
sample implementation in VKMS. All the key API bits are here.
VKMS now supports two named transfer function colorops and we
have an IGT test that confirms that sRGB EOTF, followed by its
inverse gives us expected results within +/- 1 8 bpc codepoint
value.

This patchset is grouped as follows:
 - Patches 1-2: couple general patches/fixes
 - Patches 3-5: introduce kunit to VKMS
 - Patch 6: description of motivation and details behind the
Color Pipeline API. If you're reading nothing else
but are interested in the topic I highly recommend
you take a look at this.
 - Patches 7-15: Add core DRM API bits
 - Patches 15-17: VKMS implementation

There are plenty of things that I would like to see here but
haven't had a chance to look at. These will (hopefully) be
addressed in future iterations:
 - Abandon IOCTLs and discover colorops as clients iterate the pipeline
 - Add color_pipeline client cap and deprecate existing color encoding and
   color range properties.
   See 
https://lists.freedesktop.org/archives/dri-devel/2023-September/422643.html
 - Add CTM colorop to VKMS
 - Add custom LUT colorops to VKMS
 - Add pre-blending 3DLUT with tetrahedral interpolation to VKMS
 - How to support HW which can't bypass entire pipeline?
 - Add ability to create colorops that don't have BYPASS
 - Can we do a LOAD / COMMIT model for LUTs (and other properties)?

IGT tests can be found at
https://gitlab.freedesktop.org/hwentland/igt-gpu-tools/-/merge_requests/1

IGT patches are also being sent to the igt-dev mailing list.

libdrm changes to support the new IOCTLs are at
https://gitlab.freedesktop.org/hwentland/drm/-/merge_requests/1

If you prefer a gitlab MR for review you can find it at
https://gitlab.freedesktop.org/hwentland/linux/-/merge_requests/5

A slightly different approach for a Color Pipeline API was sent by
Uma Shankar and can be found at
https://patchwork.freedesktop.org/series/123024/

The main difference is that his approach is not introducing a new DRM
core object but instead exposes color pipelines via blob properties.
There are pros and cons to both approaches.

v2:
 - Rebased on drm-misc-next
 - Introduce a VKMS Kunit so we can test LUT functionality in vkms_composer
 - Incorporate feedback in color_pipeline.rst doc
 - Add support for sRGB inverse EOTF
 - Add 2nd enumerated TF colorop to VKMS
 - Fix LUTs and some issues with applying LUTs in VKMS

Cc: Ville Syrjala 
Cc: Pekka Paalanen 
Cc: Simon Ser 
Cc: Harry Wentland 
Cc: Melissa Wen 
Cc: Jonas Ådahl 
Cc: Sebastian Wick 
Cc: Shashank Sharma 
Cc: Alexander Goins 
Cc: Joshua Ashton 
Cc: Michel Dänzer 
Cc: Aleix Pol 
Cc: Xaver Hugl 
Cc: Victoria Brekenfeld 
Cc: Sima 
Cc: Uma Shankar 
Cc: Naseer Ahmed 
Cc: Christopher Braga 
Cc: Abhinav Kumar 
Cc: Arthur Grillo 
Cc: Hector Martin 
Cc: Liviu Dudau 
Cc: Sasha McIntosh 

Harry Wentland (17):
  drm/atomic: Allow get_value for immutable properties on atomic drivers
  drm: Don't treat 0 as -1 in drm_fixp2int_ceil
  drm/vkms: Create separate Kconfig file for VKMS
  drm/vkms: Add kunit tests for VKMS LUT handling
  drm/vkms: Avoid reading beyond LUT array
  drm/doc/rfc: Describe why prescriptive color pipeline is needed
  drm/colorop: Introduce new drm_colorop mode object
  drm/colorop: Add TYPE property
  drm/color: Add 1D Curve subtype
  drm/colorop: Add BYPASS property
  drm/colorop: Add NEXT property
  drm/colorop: Add atomic state print for drm_colorop
  drm/colorop: Add new IOCTLs to retrieve drm_colorop objects
  drm/plane: Add COLOR PIPELINE property
  drm/colorop: Add NEXT to colorop state print
  drm/vkms: Add enumerated 1D curve colorop
  drm/vkms: Add kunit tests for linear and sRGB LUTs

 Documentation/gpu/rfc/color_pipeline.rst  | 347 
 drivers/gpu/drm/Kconfig   |  14 +-
 drivers/gpu/drm/Makefile  |   1 +
 drivers/gpu/drm/drm_atomic.c  | 155 
 drivers/gpu/drm/drm_atomic_helper.c   |  12 +
 drivers/gpu/drm/drm_atomic_state_helper.c |   5 +
 drivers/gpu/drm/drm_atomic_uapi.c | 110 +++
 drivers/gpu/drm/drm_colorop.c | 384 +
 drivers/gpu/drm/drm_crtc_internal.h   |   4 +
 drivers/gpu/drm/drm_ioctl.c   |   5 +
 drivers/gpu/drm/drm_mode_config.c |   7 +
 drivers/gpu/drm/drm_mode_object.c |   3 +-
 drivers/gpu/drm/drm_plane_helper.c|   2 +-
 drivers/gpu/drm/vkms/Kconfig  |  20 +
 drivers/gpu/drm/vkms/Makefile |   6 +-
 drivers/gpu/drm/vkms/tests/.kunitconfig   |   4 +
 drivers/gpu/drm/vkms/tests/Makefile   |   4 +
 drivers/gpu/drm/vkms/tests/vkms_color_tests.c | 100 +++
 drivers/gpu/drm/vkms/vkms_colorop.c   |  85 ++
 drivers/gpu/drm/vkms/vkms_composer.c  |  77 +-
 drivers/gpu/drm/vkms/vkms_composer.h  |  25 +
 drivers/gpu/drm/vkms/vkms_drv.h   |   4 +
 dr

Re: [RFC PATCH 10/10] drm/vkms: Add enumerated 1D curve colorop

2023-10-19 Thread Harry Wentland



On 2023-10-10 12:27, Melissa Wen wrote:
> On 09/08, Harry Wentland wrote:
>> Signed-off-by: Harry Wentland 
>> Cc: Ville Syrjala 
>> Cc: Pekka Paalanen 
>> Cc: Simon Ser 
>> Cc: Harry Wentland 
>> Cc: Melissa Wen 
>> Cc: Jonas Ådahl 
>> Cc: Sebastian Wick 
>> Cc: Shashank Sharma 
>> Cc: Alexander Goins 
>> Cc: Joshua Ashton 
>> Cc: Michel Dänzer 
>> Cc: Aleix Pol 
>> Cc: Xaver Hugl 
>> Cc: Victoria Brekenfeld 
>> Cc: Daniel Vetter 
>> Cc: Uma Shankar 
>> Cc: Naseer Ahmed 
>> Cc: Christopher Braga 
>> ---
>>  drivers/gpu/drm/vkms/Makefile|   3 +-
>>  drivers/gpu/drm/vkms/vkms_colorop.c  | 108 +
>>  drivers/gpu/drm/vkms/vkms_composer.c | 316 +++
>>  drivers/gpu/drm/vkms/vkms_drv.h  |   4 +
>>  drivers/gpu/drm/vkms/vkms_plane.c|   2 +
>>  5 files changed, 432 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/gpu/drm/vkms/vkms_colorop.c
>>
>> diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
>> index 1b28a6a32948..bcf508873622 100644
>> --- a/drivers/gpu/drm/vkms/Makefile
>> +++ b/drivers/gpu/drm/vkms/Makefile
>> @@ -6,6 +6,7 @@ vkms-y := \
>>  vkms_formats.o \
>>  vkms_crtc.o \
>>  vkms_composer.o \
>> -vkms_writeback.o
>> +vkms_writeback.o \
>> +vkms_colorop.o
>>  
>>  obj-$(CONFIG_DRM_VKMS) += vkms.o
>> diff --git a/drivers/gpu/drm/vkms/vkms_colorop.c 
>> b/drivers/gpu/drm/vkms/vkms_colorop.c
>> new file mode 100644
>> index ..b3da0705bca7
>> --- /dev/null
>> +++ b/drivers/gpu/drm/vkms/vkms_colorop.c
>> @@ -0,0 +1,108 @@
>> +/*
>> + * Copyright (C) 2023 Advanced Micro Devices, Inc. All rights reserved.
>> + *
>> + * 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.
>> + *
>> + * Authors: AMD
>> + *
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#define MAX_COLOR_PIPELINES 5
>> +
>> +const int vkms_initialize_tf_pipeline(struct drm_plane *plane, struct 
>> drm_prop_enum_list *list)
>> +{
>> +
>> +struct drm_colorop *op, *prev_op;
>> +struct drm_device *dev = plane->dev;
>> +int ret;
>> +
>> +/* 1st op: 1d curve */
>> +op = kzalloc(sizeof(struct drm_colorop), GFP_KERNEL);
>> +if (!op) {
>> +DRM_ERROR("KMS: Failed to allocate colorop\n");
>> +return -ENOMEM;
>> +}
>> +
>> +ret = drm_colorop_init(dev, op, plane, DRM_COLOROP_1D_CURVE);
>> +if (ret)
>> +return ret;
>> +
>> +list->type = op->base.id;
>> +list->name = kasprintf(GFP_KERNEL, "Color Pipeline %d", op->base.id);
>> +
>> +prev_op = op;
>> +
>> +/* 2nd op: 1d curve */
>> +op = kzalloc(sizeof(struct drm_colorop), GFP_KERNEL);
>> +if (!op) {
>> +DRM_ERROR("KMS: Failed to allocate colorop\n");
>> +return -ENOMEM;
>> +}
>> +
>> +ret = drm_colorop_init(dev, op, plane, DRM_COLOROP_1D_CURVE);
>> +if (ret)
>> +return ret;
>> +
>> +drm_colorop_set_next_property(prev_op, op);
>> +
>> +return 0;
>> +}
>> +
>> +int vkms_initialize_colorops(struct drm_plane *plane)
>> +{
>> +struct drm_device *dev = plane->dev;
>> +struct drm_property *prop;
>> +struct drm_prop_enum_list pipelines[MAX_COLOR_PIPELINES];
>> +int len = 0;
>> +int ret;
>> +
>> +/* Add "Bypass" (i.e. NULL) pipeline */
>> +pipelines[len].type = 0;
>> +pipelines[len].name = "Bypass";
>> +len++;
>> +
>> +/* Add pipeline consisting of transfer functions */
>> +ret = vkms_initialize_tf_pipeline(plane, &(pipelines[len]));
>> +if (ret)
>> +return ret;
>> +len++;
>> +
>> +/* Create COLOR_PIPELINE property and attach */
>> +prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
>> +"COLOR_PIPELINE",
>> +pipelines, len);
>> +if (!prop)
>> +return -ENOMEM;
>> +
>> 

Re: [RFC PATCH 02/10] drm/colorop: Introduce new drm_colorop mode object

2023-10-19 Thread Harry Wentland



On 2023-10-10 12:19, Melissa Wen wrote:
> On 09/08, Harry Wentland wrote:
>> This patches introduces a new drm_colorop mode object. This
>> object represents color transformations and can be used to
>> define color pipelines.
>>
>> We also introduce the drm_colorop_state here, as well as
>> various helpers and state tracking bits.
>>
>> Signed-off-by: Harry Wentland 
>> Cc: Ville Syrjala 
>> Cc: Pekka Paalanen 
>> Cc: Simon Ser 
>> Cc: Harry Wentland 
>> Cc: Melissa Wen 
>> Cc: Jonas Ådahl 
>> Cc: Sebastian Wick 
>> Cc: Shashank Sharma 
>> Cc: Alexander Goins 
>> Cc: Joshua Ashton 
>> Cc: Michel Dänzer 
>> Cc: Aleix Pol 
>> Cc: Xaver Hugl 
>> Cc: Victoria Brekenfeld 
>> Cc: Daniel Vetter 
>> Cc: Uma Shankar 
>> Cc: Naseer Ahmed 
>> Cc: Christopher Braga 
>> ---
>>  drivers/gpu/drm/Makefile|   1 +
>>  drivers/gpu/drm/drm_atomic.c|  79 +
>>  drivers/gpu/drm/drm_atomic_helper.c |  12 ++
>>  drivers/gpu/drm/drm_atomic_uapi.c   |  48 
>>  drivers/gpu/drm/drm_colorop.c   | 169 
>>  drivers/gpu/drm/drm_mode_config.c   |   7 ++
>>  drivers/gpu/drm/drm_plane_helper.c  |   2 +-
>>  include/drm/drm_atomic.h|  82 ++
>>  include/drm/drm_atomic_uapi.h   |   1 +
>>  include/drm/drm_colorop.h   | 157 ++
>>  include/drm/drm_mode_config.h   |  18 +++
>>  include/drm/drm_plane.h |   2 +
>>  include/uapi/drm/drm.h  |   3 +
>>  include/uapi/drm/drm_mode.h |   1 +
>>  14 files changed, 581 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/gpu/drm/drm_colorop.c
>>  create mode 100644 include/drm/drm_colorop.h
>>
>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>> index 1855863b4d7a..941de0269709 100644
>> --- a/drivers/gpu/drm/Makefile
>> +++ b/drivers/gpu/drm/Makefile
>> @@ -16,6 +16,7 @@ drm-y := \
>>  drm_client.o \
>>  drm_client_modeset.o \
>>  drm_color_mgmt.o \
>> +drm_colorop.o \
>>  drm_connector.o \
>>  drm_crtc.o \
>>  drm_displayid.o \
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 11f3a130f6f4..d734e9d5bfed 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -42,6 +42,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  
>>  #include "drm_crtc_internal.h"
>>  #include "drm_internal.h"
>> @@ -108,6 +109,7 @@ void drm_atomic_state_default_release(struct 
>> drm_atomic_state *state)
>>  kfree(state->connectors);
>>  kfree(state->crtcs);
>>  kfree(state->planes);
>> +kfree(state->colorops);
>>  kfree(state->private_objs);
>>  }
>>  EXPORT_SYMBOL(drm_atomic_state_default_release);
>> @@ -139,6 +141,10 @@ drm_atomic_state_init(struct drm_device *dev, struct 
>> drm_atomic_state *state)
>>  sizeof(*state->planes), GFP_KERNEL);
>>  if (!state->planes)
>>  goto fail;
>> +state->colorops = kcalloc(dev->mode_config.num_colorop,
>> +  sizeof(*state->colorops), GFP_KERNEL);
>> +if (!state->colorops)
>> +goto fail;
>>  
>>  state->dev = dev;
>>  
>> @@ -244,6 +250,20 @@ void drm_atomic_state_default_clear(struct 
>> drm_atomic_state *state)
>>  state->planes[i].new_state = NULL;
>>  }
>>  
>> +for (i = 0; i < config->num_colorop; i++) {
>> +struct drm_colorop *colorop = state->colorops[i].ptr;
>> +
>> +if (!colorop)
>> +continue;
>> +
>> +drm_colorop_atomic_destroy_state(colorop,
>> + state->colorops[i].state);
>> +state->colorops[i].ptr = NULL;
>> +state->colorops[i].state = NULL;
>> +state->colorops[i].old_state = NULL;
>> +state->colorops[i].new_state = NULL;
>> +}
>> +
>>  for (i = 0; i < state->num_private_objs; i++) {
>>  struct drm_private_obj *obj = state->private_objs[i].ptr;
>>  
>> @@ -562,6 +582,65 @@ drm_atomic_get_plane_state(struct drm_atomic_state 
>> *state,
>>  }
>>  EXPORT_SYMBOL(drm_atomic_get_plane_state);
>>  
>> +
>> +/**
>> + * drm_atomic_get_colorop_state - get colorop state
>> + * @state: global atomic state object
>> + * @colorop: colorop to get state object for
>> + *
>> + * This function returns the colorop state for the given colorop, 
>> allocating it
>> + * if needed. It will also grab the relevant plane lock to make sure that 
>> the
>> + * state is consistent.
>> + *
>> + * Returns:
>> + *
>> + * Either the allocated state or the error code encoded into the pointer. 
>> When
>> + * the error is EDEADLK then the w/w mutex code has detected a deadlock and 
>> the
>> + * entire atomic sequence must be restarted. All other errors are fatal.
>> + */
>> +struct drm_colorop_state *
>> +drm_atomic_get_colorop_state(struct drm_atomic_state *state,
>> + struct drm_colorop *co

Re: [RFC PATCH 01/10] drm/doc/rfc: Describe why prescriptive color pipeline is needed

2023-10-19 Thread Harry Wentland



On 2023-10-10 12:13, Melissa Wen wrote:
> O 09/08, Harry Wentland wrote:
>> Signed-off-by: Harry Wentland 
>> Cc: Ville Syrjala 
>> Cc: Pekka Paalanen 
>> Cc: Simon Ser 
>> Cc: Harry Wentland 
>> Cc: Melissa Wen 
>> Cc: Jonas Ådahl 
>> Cc: Sebastian Wick 
>> Cc: Shashank Sharma 
>> Cc: Alexander Goins 
>> Cc: Joshua Ashton 
>> Cc: Michel Dänzer 
>> Cc: Aleix Pol 
>> Cc: Xaver Hugl 
>> Cc: Victoria Brekenfeld 
>> Cc: Daniel Vetter 
>> Cc: Uma Shankar 
>> Cc: Naseer Ahmed 
>> Cc: Christopher Braga 
>> ---
>>  Documentation/gpu/rfc/color_pipeline.rst | 278 +++
>>  1 file changed, 278 insertions(+)
>>  create mode 100644 Documentation/gpu/rfc/color_pipeline.rst
>>
>> diff --git a/Documentation/gpu/rfc/color_pipeline.rst 
>> b/Documentation/gpu/rfc/color_pipeline.rst
>> new file mode 100644
>> index ..bfa4a8f12087
>> --- /dev/null
>> +++ b/Documentation/gpu/rfc/color_pipeline.rst
>> @@ -0,0 +1,278 @@
>> +
>> +Linux Color Pipeline API
>> +
>> +
>> +What problem are we solving?
>> +
>> +
>> +We would like to support pre-, and post-blending complex color 
>> transformations
>> +in order to allow for HW-supported HDR use-cases, as well as to provide 
>> support
>> +to color-managed applications, such as video or image editors.
>> +
>> +While it is possible to support an HDR output on HW supporting the 
>> Colorspace
>> +and HDR Metadata drm_connector properties that requires the compositor or
>> +application to render and compose the content into one final buffer 
>> intended for
>> +display. Doing so is costly.
>> +
>> +Most modern display HW offers various 1D LUTs, 3D LUTs, matrices, and other
>> +operations to support color transformations. These operations are often
>> +implemented in fixed-function HW and therefore much more power efficient 
>> than
>> +performing similar operations via shaders or CPU.
>> +
>> +We would like to make use of this HW functionality to support complex color
>> +transformations with no, or minimal CPU or shader load.
>> +
>> +
>> +How are other OSes solving this problem?
>> +
>> +
>> +The most widely supported use-cases regard HDR content, whether video or
>> +gaming.
>> +
>> +Most OSes will specify the source content format (color gamut, encoding 
>> transfer
>> +function, and other metadata, such as max and average light levels) to a 
>> driver.
>> +Drivers will then program their fixed-function HW accordingly to map from a
>> +source content buffer's space to a display's space.
>> +
>> +When fixed-function HW is not available the compositor will assemble a 
>> shader to
>> +ask the GPU to perform the transformation from the source content format to 
>> the
>> +display's format.
>> +
>> +A compositor's mapping function and a driver's mapping function are usually
>> +entirely separate concepts. On OSes where a HW vendor has no insight into
>> +closed-source compositor code such a vendor will tune their color management
>> +code to visually match the compositor's. On other OSes, where both mapping
>> +functions are open to an implementer they will ensure both mappings match.
>> +
>> +
>> +Why is Linux different?
>> +===
>> +
>> +Unlike other OSes, where there is one compositor for one or more drivers, on
>> +Linux we have a many-to-many relationship. Many compositors; many drivers.
>> +In addition each compositor vendor or community has their own view of how
>> +color management should be done. This is what makes Linux so beautiful.
>> +
>> +This means that a HW vendor can now no longer tune their driver to one
>> +compositor, as tuning it to one will almost inevitably make it look very
>> +different from another compositor's color mapping.
>> +
>> +We need a better solution.
>> +
>> +
>> +Descriptive API
>> +===
>> +
>> +An API that describes the source and destination colorspaces is a 
>> descriptive
>> +API. It describes the input and output color spaces but does not describe
>> +how precisely they should be mapped. Such a mapping includes many minute
>> +design decision that can greatly affect the look of the final result.
>> +
>> +It is not feasible to describe such mapping with enough detail to ensure the
>> +same result from each implementation. In fact, these mappings are a very 
>> active
>> +research area.
>> +
>> +
>> +Prescriptive API
>> +
>> +
>> +A prescriptive API describes not the source and destination colorspaces. It
>> +instead prescribes a recipe for how to manipulate pixel values to arrive at 
>> the
>> +desired outcome.
>> +
>> +This recipe is generally an order straight-forward operations, with clear
>> +mathematical definitions, such as 1D LUTs, 3D LUTs, matrices, or other
>> +operations that can be described in a precise manner.
>> +
>> +
>> +The Color Pipeline API
>> +==
>> +
>> +HW color management pipelines can significantly differ between 

Re: [RFC PATCH 01/10] drm/doc/rfc: Describe why prescriptive color pipeline is needed

2023-10-19 Thread Harry Wentland



On 2023-09-13 07:29, Pekka Paalanen wrote:
> On Fri, 8 Sep 2023 11:02:26 -0400
> Harry Wentland  wrote:
> 
>> Signed-off-by: Harry Wentland 
>> Cc: Ville Syrjala 
>> Cc: Pekka Paalanen 
>> Cc: Simon Ser 
>> Cc: Harry Wentland 
>> Cc: Melissa Wen 
>> Cc: Jonas Ådahl 
>> Cc: Sebastian Wick 
>> Cc: Shashank Sharma 
>> Cc: Alexander Goins 
>> Cc: Joshua Ashton 
>> Cc: Michel Dänzer 
>> Cc: Aleix Pol 
>> Cc: Xaver Hugl 
>> Cc: Victoria Brekenfeld 
>> Cc: Daniel Vetter 
>> Cc: Uma Shankar 
>> Cc: Naseer Ahmed 
>> Cc: Christopher Braga 
>> ---
>>  Documentation/gpu/rfc/color_pipeline.rst | 278 +++
>>  1 file changed, 278 insertions(+)
>>  create mode 100644 Documentation/gpu/rfc/color_pipeline.rst
> 
> Hi Harry,
> 
> it's really nice to see this!
> 

Thanks for the feedback. I'm just putting a v2 together with comments
(partially) addressed.

> Sebastian started on the backward/forward compatibility, so I'll
> comment on everything else here, and leave the compatibility for that
> thread.
> 
>> diff --git a/Documentation/gpu/rfc/color_pipeline.rst 
>> b/Documentation/gpu/rfc/color_pipeline.rst
>> new file mode 100644
>> index ..bfa4a8f12087
>> --- /dev/null
>> +++ b/Documentation/gpu/rfc/color_pipeline.rst
...
>> +COLOR_PIPELINE Plane Property
>> +=
>> +
>> +Because we don't have existing KMS color properties in the pre-blending
>> +portion of display pipelines (i.e. on drm_planes) we are introducing
>> +color pipelines here first. Eventually we'll want to use the same
>> +concept for the post-blending portion, i.e. drm_crtcs.
> 
> This paragraph might fit better in a cover letter.
> 
>> +
>> +Color Pipelines are created by a driver and advertised via a new
>> +COLOR_PIPELINE enum property on each plane. Values of the property
>> +always include '0', which is the default and means all color processing
>> +is disabled. Additional values will be the object IDs of the first
>> +drm_colorop in a pipeline. A driver can create and advertise none, one,
>> +or more possible color pipelines. A DRM client will select a color
>> +pipeline by setting the COLOR PIPELINE to the respective value.
>> +
>> +In the case where drivers have custom support for pre-blending color
>> +processing those drivers shall reject atomic commits that are trying to
>> +set both the custom color properties, as well as the COLOR_PIPELINE
> 
> s/set/use/ because one of them could be carried-over state from
> previous commits while not literally set in this one.
> 
>> +property.
>> +
>> +An example of a COLOR_PIPELINE property on a plane might look like this::
>> +
>> +Plane 10
>> +├─ "type": immutable enum {Overlay, Primary, Cursor} = Primary
>> +├─ …
>> +└─ "color_pipeline": enum {0, 42, 52} = 0
> 
> Enum values are string names. I presume the intention here is that the
> strings will never need to be parsed, and the uint64_t is always equal
> to the string representation, right?
> 
> That needs a statement here. It differs from all previous uses of
> enums, and e.g. requires a little bit of new API in libweston's
> DRM-backend to handle since it has its own enums referring to the
> string names that get mapped to the uint64_t per owning KMS object.
> 

I'm currently putting the DRM object ID in the "value" and use the
"name" as a descriptive name.

> struct drm_mode_property_enum {
>   __u64 value;
>   char name[DRM_PROP_NAME_LEN];
> };

This works well in IGT and gives us a nice descriptive name for
debugging, but I could consider changing this if it'd simplify
people's lives.

>> +
>> +
>> +Color Pipeline Discovery
>> +
>> +
>> +A DRM client wanting color management on a drm_plane will:
>> +
>> +1. Read all drm_colorop objects
> 
> What does this do?

We probably don't need this, and with it we probably don't need
the new IOCTLs. I added this to align with IGT's current init
procedure where it reads all DRM core objects, like planes, etc.,
before using them. But realistically we can just look at the
colorop ID from the COLOR_PIPELINE property and then retrieve
the other colorops through the NEXT pointer.

> 
>> +2. Get the COLOR_PIPELINE property of the plane
>> +3. iterate all COLOR_PIPELINE enum values
>> +4. for each enum value walk the color pipeline (via the NEXT pointers)
>> +   and see if the available color operations are suitable for the
>> +   desired color management operations
>> +
>> +An example of chained properties to define an AMD pre-blending color
>> +pipeline might look like this::
>> +
>> +Plane 10
>> +├─ "type": immutable enum {Overlay, Primary, Cursor} = Primary
>> +└─ "color_pipeline": enum {0, 42} = 0
>> +Color operation 42 (input CSC)
> 
> I presume the string "(input CSC)" does not come from KMS, and is
> actually just a comment added here by hand?
> 

Exactly. It only exists as a comment here. I'll remove it.

Harry

> 
> Thanks,
> pq
> 
>> +├─ "type": enum {Bypass, Matrix} = Matrix
>> +  

Re: [PATCH RFC v6 01/10] drm: Introduce pixel_source DRM plane property

2023-10-19 Thread Simon Ser
For the uAPI:

Acked-by: Simon Ser 


Re: [PATCH v1] dynamic_debug: add support for logs destination

2023-10-19 Thread Łukasz Bartosik
śr., 18 paź 2023 o 05:08  napisał(a):
>
> adding in Jason, not sure how he got missed.
>
> On Mon, Oct 16, 2023 at 9:13 AM Łukasz Bartosik  wrote:
> >
> > czw., 12 paź 2023 o 20:48  napisał(a):
> > >
> > > > If you want the kernel to keep separate flight recorders I guess we 
> > > > could
> > > > add that, but I don't think it currently exists for the dyndbg stuff at
> > > > least. Maybe a flight recorder v2 feature, once the basics are in.
> > > >
> > >
> > > dyndbg has   +pwrites to syslog
> > > +T  would separately independently write the same to global trace
> > >
> > > This would allow  graceful switchover to tracefs,
> > > without removing logging from dmesg, where most folks
> > > (and any monitor tools) would expect it.
> > >
> > > Lukas (iiuc) wants to steer each site to just 1 destination.
> > > Or maybe (in addition to +p > syslog) one trace destination,
> > > either global via events, or a separate tracebuf
> > >
> > > Im ambivalent, but thinking the smooth rollover from syslog to trace
> > > might be worth having to ease migration / weaning off syslog.
> > >
> > > And we have a 4 byte hole in struct _ddebug we could just use.
> >
> > I'm glad you brought that up. This means we can leave class_id field
> > untouched, have separate +T in flags (for consistency)
> > and dst_id can be easily 8 bits wide.
> >
> > Also can you point me to the latest version of writing debug logs to
> > trace events (+T option).
> > I would like to base trace instances work on that because both are
> > closely related.
> >
>
> Ive got 3 branches, all stacked up on rc6
> (last I checked, they were clean on drm-tip)
>
> https://github.com/jimc/linux/tree/dd-fix-7c
> the regression fix, reposting this version next.
> it also grabs another flag bit: _DPRINTK_FLAGS_INCL_LOOKUP
>
>
> https://github.com/jimc/linux/tree/dd-shrink-3b
> split modname,filename,function to new __dyndbg_sites section
> loads the 3 column values and their intervals into 3 maple trees
> and implements 3 accessor functions to retrieve the vals
> from the descriptor addresses.
> it "works" but doesnt erase intervals properly on rmmod
> it also claims another flag - _DPRINTK_FLAGS_PREFIX_CACHED
> and uses it to mark cached prefix fragment that get created for enabled calls.
>
> https://github.com/jimc/linux/tree/dd-kitchen-sink
> this adds the +T flag stuff.
> its still a little fuzzy, esp around the descriptor arg -
> using it to render the prefix str at buffer-render time
> got UNSAFE warnings - likely due to loadable module
> descriptors which could or did go away between
> capture and render (after rmmod)
>

Great, I will use commits related to adding +T from this tree as a
baseline for adding trace instances.

>
>
>
> > > Unless the align 8 is optional on 32-bits,
> >
> > I verified with "gcc -g -m32 ..." that the align(8) is honored on 32 bits.
> >
>
> this is sorta the opposite of what I was probing, but I think result
> is the same.
> istm  align(8) is only for JUMP_LABEL, nothing else in the struct
> appears to need it
> so moving it to the top trades the hole for padding.
>

Ahh I see what you meant now.  Although I have to admit that looking
at the jump label code and static key usage cases
around the kernel code I don't see where align(8) requirement for jump
label comes from.

>
>
> > > I think we're never gonna close the hole anywhere.
> > >
> >
> > :)
> >
> > > is align 8 a generic expression of an architectural simplifying 
> > > constraint ?
> > > or a need for 1-7 ptr offsets ?
> > >
> > >
> > >
> > >
> > > > > That's my idea of it. It is interesting to see how far the 
> > > > > requirements
> > > > > can be reasonably realised.
> > > >
> > > > I think aside from the "make it available directly to unpriviledged
> > > > userspace" everything sounds reasonable and doable.
> > > >
> > > > More on the process side of things, I think Jim is very much looking for
> > > > acks and tested-by by people who are interested in better drm logging
> > > > infra. That should help that things are moving in a direction that's
> > > > actually useful, even when it's not yet entirely complete.
> > > >
> > >
> > > yes, please.  Now posted at
> > >
> > > https://lore.kernel.org/lkml/20231012172137.3286566-1-jim.cro...@gmail.com/T/#t
> > >
> > > Lukas, I managed to miss your email in the send phase.
> > > please consider yourself a direct recipient :-)
> > >
> > > thanks everyone
> > >
> > > > Cheers, Sima
> > > > --
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > http://blog.ffwll.ch


Re: [PATCH RFC v6 01/10] drm: Introduce pixel_source DRM plane property

2023-10-19 Thread Sebastian Wick
On Tue, Aug 29, 2023 at 10:48:16AM +0300, Pekka Paalanen wrote:
> On Mon, 28 Aug 2023 17:05:07 -0700
> Jessica Zhang  wrote:
> 
> > Add support for pixel_source property to drm_plane and related
> > documentation. In addition, force pixel_source to
> > DRM_PLANE_PIXEL_SOURCE_FB in DRM_IOCTL_MODE_SETPLANE as to not break
> > legacy userspace.
> > 
> > This enum property will allow user to specify a pixel source for the
> > plane. Possible pixel sources will be defined in the
> > drm_plane_pixel_source enum.
> > 
> > Currently, the only pixel sources are DRM_PLANE_PIXEL_SOURCE_FB (the
> > default value) and DRM_PLANE_PIXEL_SOURCE_NONE.
> > 
> > Signed-off-by: Jessica Zhang 
> > ---
> >  drivers/gpu/drm/drm_atomic_state_helper.c |  1 +
> >  drivers/gpu/drm/drm_atomic_uapi.c |  4 ++
> >  drivers/gpu/drm/drm_blend.c   | 90 
> > +++
> >  drivers/gpu/drm/drm_plane.c   | 19 +--
> >  include/drm/drm_blend.h   |  2 +
> >  include/drm/drm_plane.h   | 21 
> >  6 files changed, 133 insertions(+), 4 deletions(-)
> 
> ...
> 
> > diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> > index 6e74de833466..c3c57bae06b7 100644
> > --- a/drivers/gpu/drm/drm_blend.c
> > +++ b/drivers/gpu/drm/drm_blend.c
> > @@ -185,6 +185,21 @@
> >   *  plane does not expose the "alpha" property, then this is
> >   *  assumed to be 1.0
> >   *
> > + * pixel_source:
> > + * pixel_source is set up with drm_plane_create_pixel_source_property().
> > + * It is used to toggle the active source of pixel data for the plane.
> > + * The plane will only display data from the set pixel_source -- any
> > + * data from other sources will be ignored.
> > + *
> > + * Possible values:
> > + *
> > + * "NONE":
> > + * No active pixel source.
> > + * Committing with a NONE pixel source will disable the plane.
> > + *
> > + * "FB":
> > + * Framebuffer source set by the "FB_ID" property.
> > + *
> >   * Note that all the property extensions described here apply either to the
> >   * plane or the CRTC (e.g. for the background color, which currently is not
> >   * exposed and assumed to be black).
> 
> This UAPI:
> Acked-by: Pekka Paalanen 

Thanks Jessica, same for me

Acked-by: Sebastian Wick 

> 
> 
> Thanks,
> pq