[PATCH 0/4] drm/vkms: speed up composition with a precomputed gamma LUT

[PATCH 0/4] drm/vkms: speed up composition with a precomputed gamma LUT

[Oarora Etimis]

Four patches: a fix so that a frame takes the gamma LUT from the state it
is built from, a table that turns applying it into a lookup, a test, and
the CI entries that can go once the timeouts are gone.

apply_lut() samples the gamma LUT: every channel of every pixel gets a
fixed-point index and an interpolation between two neighbouring entries.
On a 1024x768 mode that is 2.36 million interpolations per frame, and
kms_plane@pixel-format takes 160.1-167.8 s while
pixel-format-source-clamping takes 78.9-83.8 s, over ten runs each. CI
lists both as Timeout in drivers/gpu/drm/ci/xfails/vkms-none-fails.txt.

A channel value is a u16, so a channel has only 65536 possible outputs.
Patch 2 computes them once per LUT change into three tables (384 KiB) and
applies the LUT by indexing them, which leaves the composed output
bit-identical to the interpolated path. The two subtests take 4.87-4.90 s
and 9.70-9.84 s.

Patch 1 is the fix I posted on 22 October
(<20261022215803.621996-1-oaroraetimis@gmail.com>), and patch 2 builds on
it. Patch 4 removes the two xfail entries and has to be merged with patch
2: on its own the subtests still time out.

The other approach I considered. drm_fixp_msbset() computes fls64() with a
62-step loop; replacing it with fls64() is three lines, needs no memory,
no cache and no lifetime, and returns the same values. In a userspace copy
of the same functions the LUT phase goes from 153.9 ms to 10.5 ms per
frame, ~15x, where the tables give 0.9 ms, ~170x. I have not measured the
subtests with it, so I have no wall clock for it. It is not part of this
series.

---
Oarora Etimis (4):
      drm/vkms: use the gamma LUT of the state being composed
      drm/vkms: precompute the gamma LUT for every channel value
      drm/vkms: add a KUnit test for the precomputed LUT table
      drm/ci: drop the kms_plane pixel-format xfails for vkms

 drivers/gpu/drm/ci/xfails/vkms-none-fails.txt |  2 -
 drivers/gpu/drm/vkms/tests/vkms_color_test.c  | 54 +++++++++++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_composer.c          | 47 ++++++++++++++++++-----
 drivers/gpu/drm/vkms/vkms_composer.h          |  3 ++
 drivers/gpu/drm/vkms/vkms_crtc.c              | 24 ++++++++++++
 drivers/gpu/drm/vkms/vkms_drv.h               |  6 +++
 6 files changed, 125 insertions(+), 11 deletions(-)
---
base-commit: 33ed26235db93162935d1719bc6f538c384a2fd8
change-id: 20261108-lut-4fde3dee44ce

Best regards,
--  
Oarora Etimis <oaroraetimis@gmail.com>

[PATCH 1/4] drm/vkms: use the gamma LUT of the state being composed

[Oarora Etimis]

The worker can fall behind a commit, so crtc->state may be newer than the
state the frame is built from, and the frame would mix planes of one state
with the LUT of another. Take the LUT from crtc_state->base.gamma_lut.

Clear lut_length and channel_value2index_ratio in the else branch as well,
so that gamma_lut is either complete or entirely empty. Nothing reads them
while base is NULL - apply_lut() returns on that - but the next patch
decides whether to rebuild its table from lut_length alone, and a non-zero
length next to a NULL base would have it build from a NULL LUT.

A zero-length blob takes that branch too. It used to leave lut_length at 0
and channel_value2index_ratio at drm_fixp_div(-1, 0xffff), which nothing
read either.

Fixes: db1f254f2cfa ("drm/vkms: Add support to 1D gamma LUT")
Signed-off-by: Oarora Etimis <oaroraetimis@gmail.com>
---
 drivers/gpu/drm/vkms/vkms_composer.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index 0fc915a954ba..1d5658bbd493 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -609,6 +609,7 @@ void vkms_composer_worker(struct work_struct *work)
 							  struct vkms_crtc_state,
 							  composer_work);
 	struct drm_crtc *crtc = crtc_state->base.crtc;
+	struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
 	struct vkms_writeback_job *active_wb = crtc_state->active_writeback;
 	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
 	bool crc_pending, wb_pending;
@@ -625,19 +626,22 @@ void vkms_composer_worker(struct work_struct *work)
 	crtc_state->frame_end = 0;
 	crtc_state->crc_pending = false;
 
-	if (crtc->state->gamma_lut) {
+	/* crtc->state may be newer than the state this frame is built from. */
+	if (gamma_lut && gamma_lut->length) {
 		s64 max_lut_index_fp;
 		s64 u16_max_fp = drm_int2fixp(0xffff);
 
-		crtc_state->gamma_lut.base = (struct drm_color_lut *)crtc->state->gamma_lut->data;
+		crtc_state->gamma_lut.base = (struct drm_color_lut *)gamma_lut->data;
 		crtc_state->gamma_lut.lut_length =
-			crtc->state->gamma_lut->length / sizeof(struct drm_color_lut);
+			gamma_lut->length / sizeof(struct drm_color_lut);
 		max_lut_index_fp = drm_int2fixp(crtc_state->gamma_lut.lut_length - 1);
 		crtc_state->gamma_lut.channel_value2index_ratio = drm_fixp_div(max_lut_index_fp,
 									       u16_max_fp);
 
 	} else {
 		crtc_state->gamma_lut.base = NULL;
+		crtc_state->gamma_lut.lut_length = 0;
+		crtc_state->gamma_lut.channel_value2index_ratio = 0;
 	}
 
 	spin_unlock_irq(&out->composer_lock);

-- 
2.55.0

[PATCH 2/4] drm/vkms: precompute the gamma LUT for every channel value

[Oarora Etimis]

apply_lut() interpolates every channel of every pixel: three calls to
apply_lut_to_channel_value(), each computing a fixed-point LUT index and
lerping the two neighbouring entries. In virtme-ng with two CPUs, that
makes kms_plane@pixel-format-source-clamping take 78.9-83.8 s and
kms_plane@pixel-format 160.1-167.8 s over ten runs.

A channel value is a u16, so a channel has only 65536 possible inputs.
Compute them once per LUT change into three tables of 65536 u16, in one
allocation of 384 KiB, and let apply_lut() index them by the channel value
itself; composing a pixel then costs three reads. The two subtests drop to
4.87-4.90 s and 9.70-9.84 s.

Entries come from the same apply_lut_to_channel_value(), so the composed
output stays bit-identical to the per-pixel path.

The tables are built in vkms_composer_worker(), outside the composer lock,
on a frame with a CRC pending whose blob differs from the one they were
built from. Holding a reference on that blob keeps its address from being
recycled, so comparing pointers stays a valid identity test for contents
that are immutable once published. They are allocated with kvmalloc() in
vkms_crtc_init() and freed by a device action that also drops the blob
reference - registered before the workqueue, so the workqueue is destroyed
first.

When there is no LUT the tables keep their previous contents; apply_lut()
returns on the NULL base before indexing them.

This patch depends on patch 1: it declares gamma_lut and clears
lut_length when there is no LUT. Applied alone, this patch does not
compile, and the rebuild check would build from a NULL base.

Signed-off-by: Oarora Etimis <oaroraetimis@gmail.com>
---
 drivers/gpu/drm/vkms/vkms_composer.c | 37 ++++++++++++++++++++++++++++++------
 drivers/gpu/drm/vkms/vkms_composer.h |  3 +++
 drivers/gpu/drm/vkms/vkms_crtc.c     | 24 +++++++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_drv.h      |  6 ++++++
 4 files changed, 64 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index 1d5658bbd493..5bbc34113250 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -111,21 +111,37 @@ VISIBLE_IF_KUNIT u16 apply_lut_to_channel_value(const struct vkms_color_lut *lut
 }
 EXPORT_SYMBOL_IF_KUNIT(apply_lut_to_channel_value);
 
+/*
+ * Precompute the LUT for every 16-bit channel value, so that composing a
+ * pixel becomes one table lookup instead of an interpolation.
+ */
+VISIBLE_IF_KUNIT void build_lut_table(const struct vkms_color_lut *lut, u16 *lut_table)
+{
+	enum lut_channel channel;
+	int i;
+
+	for (channel = LUT_RED; channel < LUT_RESERVED; channel++) {
+		for (i = 0; i < LUT_TABLE_ENTRIES; i++)
+			lut_table[channel * LUT_TABLE_ENTRIES + i] =
+				apply_lut_to_channel_value(lut, i, channel);
+	}
+}
+EXPORT_SYMBOL_IF_KUNIT(build_lut_table);
 
 static void apply_lut(const struct vkms_crtc_state *crtc_state, struct line_buffer *output_buffer)
 {
-	if (!crtc_state->gamma_lut.base)
-		return;
+	const struct vkms_output *out = drm_crtc_to_vkms_output(crtc_state->base.crtc);
+	const u16 *table = out->gamma_lut_table;
 
-	if (!crtc_state->gamma_lut.lut_length)
+	if (!crtc_state->gamma_lut.base || !crtc_state->gamma_lut.lut_length)
 		return;
 
 	for (size_t x = 0; x < output_buffer->n_pixels; x++) {
 		struct pixel_argb_u16 *pixel = &output_buffer->pixels[x];
 
-		pixel->r = apply_lut_to_channel_value(&crtc_state->gamma_lut, pixel->r, LUT_RED);
-		pixel->g = apply_lut_to_channel_value(&crtc_state->gamma_lut, pixel->g, LUT_GREEN);
-		pixel->b = apply_lut_to_channel_value(&crtc_state->gamma_lut, pixel->b, LUT_BLUE);
+		pixel->r = table[LUT_RED * LUT_TABLE_ENTRIES + pixel->r];
+		pixel->g = table[LUT_GREEN * LUT_TABLE_ENTRIES + pixel->g];
+		pixel->b = table[LUT_BLUE * LUT_TABLE_ENTRIES + pixel->b];
 	}
 }
 
@@ -653,6 +669,15 @@ void vkms_composer_worker(struct work_struct *work)
 	if (!crc_pending)
 		return;
 
+	/* Rebuild only when the LUT differs from the one the table was built from. */
+	if (gamma_lut != out->gamma_lut_blob) {
+		if (crtc_state->gamma_lut.lut_length)
+			build_lut_table(&crtc_state->gamma_lut, out->gamma_lut_table);
+
+		drm_property_blob_put(out->gamma_lut_blob);
+		out->gamma_lut_blob = gamma_lut ? drm_property_blob_get(gamma_lut) : NULL;
+	}
+
 	if (wb_pending)
 		ret = compose_active_planes(active_wb, crtc_state, &crc32);
 	else
diff --git a/drivers/gpu/drm/vkms/vkms_composer.h b/drivers/gpu/drm/vkms/vkms_composer.h
index 04dd5646f672..b0ee93ba6dbe 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.h
+++ b/drivers/gpu/drm/vkms/vkms_composer.h
@@ -17,11 +17,14 @@ enum lut_channel {
 	LUT_RESERVED
 };
 
+#define LUT_TABLE_ENTRIES	(1U << 16)
+
 #if IS_ENABLED(CONFIG_KUNIT)
 u16 lerp_u16(u16 a, u16 b, s64 t);
 s64 get_lut_index(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);
+void build_lut_table(const struct vkms_color_lut *lut, u16 *lut_table);
 void apply_3x4_matrix(struct pixel_argb_s32 *pixel, const struct drm_color_ctm_3x4 *matrix);
 #endif
 
diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index fe17f49305a9..c6ac90fbca2b 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -11,6 +11,7 @@
 #include <drm/drm_vblank.h>
 #include <drm/drm_vblank_helper.h>
 
+#include "vkms_composer.h"
 #include "vkms_drv.h"
 
 static bool vkms_crtc_handle_vblank_timeout(struct drm_crtc *crtc)
@@ -203,6 +204,14 @@ static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
 	.handle_vblank_timeout = vkms_crtc_handle_vblank_timeout,
 };
 
+static void vkms_release_gamma_lut(struct drm_device *dev, void *data)
+{
+	struct vkms_output *out = data;
+
+	kvfree(out->gamma_lut_table);
+	drm_property_blob_put(out->gamma_lut_blob);
+}
+
 struct vkms_output *vkms_crtc_init(struct drm_device *dev, struct drm_plane *primary,
 				   struct drm_plane *cursor)
 {
@@ -235,6 +244,21 @@ struct vkms_output *vkms_crtc_init(struct drm_device *dev, struct drm_plane *pri
 	spin_lock_init(&vkms_out->lock);
 	spin_lock_init(&vkms_out->composer_lock);
 
+	/*
+	 * Allocate before composer_workq: drmm frees in reverse order, so the
+	 * workqueue (and any running worker) is destroyed before this table is
+	 * freed.
+	 */
+	vkms_out->gamma_lut_table = kvmalloc_array(LUT_RESERVED * LUT_TABLE_ENTRIES,
+						   sizeof(*vkms_out->gamma_lut_table),
+						   GFP_KERNEL | __GFP_ZERO);
+	if (!vkms_out->gamma_lut_table)
+		return ERR_PTR(-ENOMEM);
+
+	ret = drmm_add_action_or_reset(dev, vkms_release_gamma_lut, vkms_out);
+	if (ret)
+		return ERR_PTR(ret);
+
 	vkms_out->composer_workq = drmm_alloc_ordered_workqueue(dev, "vkms_composer", 0);
 	if (IS_ERR(vkms_out->composer_workq))
 		return ERR_CAST(vkms_out->composer_workq);
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 381483aa4fb0..065a7b07b4ab 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -214,6 +214,10 @@ struct vkms_crtc_state {
  *		      writeback)
  * @composer_state: Protected by @lock, current state of this VKMS output
  * @composer_lock: Lock used internally to protect @composer_state members
+ * @gamma_lut_table: Precomputed gamma LUT indexed by the 16-bit channel value. Allocated
+ *		     in vkms_crtc_init() and freed with the device.
+ * @gamma_lut_blob: Blob @gamma_lut_table was built from. Holds a reference so that the
+ *		    blob cannot be freed and reused at the same address with other contents.
  */
 struct vkms_output {
 	struct drm_crtc crtc;
@@ -226,6 +230,8 @@ struct vkms_output {
 	struct vkms_crtc_state *composer_state;
 
 	spinlock_t composer_lock;
+	u16 *gamma_lut_table;
+	struct drm_property_blob *gamma_lut_blob;
 };
 
 struct vkms_config;

-- 
2.55.0

[PATCH 3/4] drm/vkms: add a KUnit test for the precomputed LUT table

[Oarora Etimis]

build_lut_table() turns a per-pixel interpolation into a table lookup, so
a wrong entry changes the composed output with no other symptom.

Compare every entry against apply_lut_to_channel_value() for the same
channel and value, for a 4-entry LUT whose channels differ and for the
256-entry sRGB one. The check stops at the first mismatch and prints its
channel, value and both results.

The tables are built with apply_lut_to_channel_value(), so this proves the
lookup matches the interpolation, not that the LUT arithmetic is right;
vkms_color_srgb_inv_srgb() covers the latter.

Signed-off-by: Oarora Etimis <oaroraetimis@gmail.com>
---
 drivers/gpu/drm/vkms/tests/vkms_color_test.c | 54 ++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/drivers/gpu/drm/vkms/tests/vkms_color_test.c b/drivers/gpu/drm/vkms/tests/vkms_color_test.c
index 1a1c7cac2f15..cdbb74d16b5e 100644
--- a/drivers/gpu/drm/vkms/tests/vkms_color_test.c
+++ b/drivers/gpu/drm/vkms/tests/vkms_color_test.c
@@ -88,6 +88,21 @@ static const struct vkms_color_lut test_linear_lut = {
 	.channel_value2index_ratio = 0xf000fll
 };
 
+/* Channels differ, so a table built with the wrong channel does not match. */
+static struct drm_color_lut test_distinct_array[] = {
+	{ 0x0000, 0x0000, 0x0000, 0 },
+	{ 0x1000, 0x6000, 0xb000, 0 },
+	{ 0x2000, 0x4000, 0x9000, 0 },
+	{ 0xffff, 0xffff, 0xffff, 0 },
+};
+
+static const struct vkms_color_lut test_distinct_lut = {
+	.base = test_distinct_array,
+	.lut_length = ARRAY_SIZE(test_distinct_array),
+	/* drm_fixp_div(drm_int2fixp(lut_length - 1), drm_int2fixp(0xffff)) */
+	.channel_value2index_ratio = 0x30003ll
+};
+
 static void vkms_color_test_get_lut_index(struct kunit *test)
 {
 	s64 lut_index;
@@ -393,6 +408,44 @@ static void vkms_color_ctm_3x4_bt709(struct kunit *test)
 	KUNIT_EXPECT_LT(test, out.b, 0x100);
 }
 
+static void vkms_color_test_check_lut_table(struct kunit *test,
+					    const struct vkms_color_lut *lut,
+					    const u16 *table)
+{
+	int i;
+
+	for (i = 0; i < LUT_RESERVED * LUT_TABLE_ENTRIES; i++) {
+		enum lut_channel channel = i / LUT_TABLE_ENTRIES;
+		u16 value = i % LUT_TABLE_ENTRIES;
+		u16 expected = apply_lut_to_channel_value(lut, value, channel);
+
+		if (table[i] != expected) {
+			KUNIT_FAIL(test, "channel %d, value %#06x: 0x%04x != 0x%04x",
+				   channel, value, table[i], expected);
+			return;
+		}
+	}
+}
+
+static void vkms_color_test_build_lut_table(struct kunit *test)
+{
+	const struct vkms_color_lut *luts[] = {
+		&test_distinct_lut,
+		&srgb_eotf,
+	};
+	u16 *table;
+	int l;
+
+	table = kunit_kzalloc(test, LUT_RESERVED * LUT_TABLE_ENTRIES * sizeof(*table),
+			      GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, table);
+
+	for (l = 0; l < ARRAY_SIZE(luts); l++) {
+		build_lut_table(luts[l], table);
+		vkms_color_test_check_lut_table(test, luts[l], table);
+	}
+}
+
 static struct kunit_case vkms_color_test_cases[] = {
 	KUNIT_CASE(vkms_color_test_get_lut_index),
 	KUNIT_CASE(vkms_color_test_lerp),
@@ -400,6 +453,7 @@ static struct kunit_case vkms_color_test_cases[] = {
 	KUNIT_CASE(vkms_color_srgb_inv_srgb),
 	KUNIT_CASE(vkms_color_ctm_3x4_50_desat),
 	KUNIT_CASE(vkms_color_ctm_3x4_bt709),
+	KUNIT_CASE(vkms_color_test_build_lut_table),
 	{}
 };
 

-- 
2.55.0

[PATCH 4/4] drm/ci: drop the kms_plane pixel-format xfails for vkms

[Oarora Etimis]

kms_plane@pixel-format and pixel-format-source-clamping are listed as
Timeout in vkms-none-fails.txt. With the per-pixel interpolation they take
160.1-167.8 s and 78.9-83.8 s locally; with the precomputed LUT tables
they take 9.70-9.84 s and 4.87-4.90 s, so the entries no longer describe
the driver.

This patch depends on patch 2: removing the entries only helps once the
subtests stop timing out, so a job that takes this patch without patch 2
fails.

Signed-off-by: Oarora Etimis <oaroraetimis@gmail.com>
---
 drivers/gpu/drm/ci/xfails/vkms-none-fails.txt | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/ci/xfails/vkms-none-fails.txt b/drivers/gpu/drm/ci/xfails/vkms-none-fails.txt
index 029bd4956e85..6ebcc7d89fbd 100644
--- a/drivers/gpu/drm/ci/xfails/vkms-none-fails.txt
+++ b/drivers/gpu/drm/ci/xfails/vkms-none-fails.txt
@@ -16,8 +16,6 @@ kms_flip@flip-vs-panning-vs-hang,Fail
 kms_flip@flip-vs-suspend,Fail
 kms_flip@flip-vs-suspend-interruptible,Fail
 kms_lease@lease-uevent,Fail
-kms_plane@pixel-format-source-clamping,Timeout
-kms_plane@pixel-format,Timeout
 kms_writeback@writeback-check-output,Fail
 kms_writeback@writeback-check-output-XRGB2101010,Fail
 kms_writeback@writeback-fb-id,Fail

-- 
2.55.0