From 3bf0583db07d07a67e87eea16a9c464c4ac680c7 Mon Sep 17 00:00:00 2001 From: jj Date: Mon, 20 Jul 2026 08:34:58 -0400 Subject: [PATCH 1/2] Add software GAMMA_LUT/CTM color management on the CRTC evdi's virtual CRTC never called drm_crtc_enable_color_mgmt(), so it never exposed GAMMA_LUT/CTM properties. Compositors that apply color correction (e.g. GNOME's Night Light) exclusively through those DRM/KMS CRTC properties therefore have nothing to set on an evdi output, and silently do nothing there, while native hardware CRTCs are corrected normally. Since evdi has no real display hardware behind it, there is no gamma/ CTM block to program. Instead, register the properties (256-entry gamma LUT, 3x3 CTM) via drm_crtc_enable_color_mgmt() and apply the resulting transform in software, in evdi_painter.c's copy_primary_pixels()/copy_primary_pixels_on_xe() - the only place the driver has direct access to raw pixel bytes before they are copied out to userspace via the GRABPIX ioctl. The transform is skipped entirely (zero added cost) unless a client has actually set a LUT or CTM. evdi_color.c/.h hold the transform math (LUT + fixed-point CTM, reusing drm_fixed.h's S32.32 helpers) as a self-contained, unit-tested component; module/tests/test_evdi_color.c covers identity/gamma/CTM/ channel-order behavior without requiring hardware. Signed-off-by: jj --- module/Makefile | 4 +- module/evdi_color.c | 176 +++++++++++++++++++++++++++++++++ module/evdi_color.h | 63 ++++++++++++ module/evdi_drm_drv.c | 1 + module/evdi_drm_drv.h | 2 + module/evdi_modeset.c | 14 +++ module/evdi_painter.c | 44 ++++++++- module/tests/Makefile | 2 +- module/tests/test_evdi_color.c | 114 +++++++++++++++++++++ 9 files changed, 412 insertions(+), 8 deletions(-) create mode 100644 module/evdi_color.c create mode 100644 module/evdi_color.h create mode 100644 module/tests/test_evdi_color.c diff --git a/module/Makefile b/module/Makefile index 13106a29..211f08e3 100644 --- a/module/Makefile +++ b/module/Makefile @@ -36,7 +36,7 @@ ifneq ($(DKMS_BUILD),) KERN_DIR := /lib/modules/$(KERNELRELEASE)/build ccflags-y := -Iinclude/uapi/drm -Iinclude/drm -evdi-y := evdi_platform_drv.o evdi_platform_dev.o evdi_sysfs.o evdi_modeset.o evdi_connector.o evdi_encoder.o evdi_drm_drv.o evdi_fb.o evdi_gem.o evdi_painter.o evdi_params.o evdi_cursor.o evdi_debug.o evdi_i2c.o +evdi-y := evdi_platform_drv.o evdi_platform_dev.o evdi_sysfs.o evdi_modeset.o evdi_connector.o evdi_encoder.o evdi_drm_drv.o evdi_fb.o evdi_gem.o evdi_painter.o evdi_params.o evdi_cursor.o evdi_color.o evdi_debug.o evdi_i2c.o evdi-$(CONFIG_COMPAT) += evdi_ioc32.o obj-m := evdi.o $(eval $(call evdi_conftest)) @@ -59,7 +59,7 @@ ifneq ($(KERNELRELEASE),) # inside kbuild # Note: this can be removed once it is in kernel tree and Kconfig is properly used ccflags-y := -isystem include/uapi/drm $(CFLAGS) -evdi-y := evdi_platform_drv.o evdi_platform_dev.o evdi_sysfs.o evdi_modeset.o evdi_connector.o evdi_encoder.o evdi_drm_drv.o evdi_fb.o evdi_gem.o evdi_painter.o evdi_params.o evdi_cursor.o evdi_debug.o evdi_i2c.o +evdi-y := evdi_platform_drv.o evdi_platform_dev.o evdi_sysfs.o evdi_modeset.o evdi_connector.o evdi_encoder.o evdi_drm_drv.o evdi_fb.o evdi_gem.o evdi_painter.o evdi_params.o evdi_cursor.o evdi_color.o evdi_debug.o evdi_i2c.o evdi-$(CONFIG_COMPAT) += evdi_ioc32.o CONFIG_DRM_EVDI ?= m obj-$(CONFIG_DRM_EVDI) := evdi.o diff --git a/module/evdi_color.c b/module/evdi_color.c new file mode 100644 index 00000000..4d6ca4b6 --- /dev/null +++ b/module/evdi_color.c @@ -0,0 +1,176 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (c) 2026 DisplayLink (UK) Ltd. + * + * This file is subject to the terms and conditions of the GNU General Public + * License v2. See the file COPYING in the main directory of this archive for + * more details. + */ + +#include +#include +#include +#include +#include +#include + +#include "evdi_color.h" + +/* + * struct drm_color_ctm stores each entry as S31.32 sign-magnitude (bit 63 + * is the sign, bits 0-62 are the unsigned value) rather than the two's + * complement drm_fixed.h (drm_fixp_t) format the drm_fixp_* helpers use. + * Some newer kernels provide drm_sm2fixp() for this, but it isn't present + * across the full 4.15+ range evdi supports, so convert it ourselves. + */ +static s64 evdi_sm2fixp(u64 sm) +{ + s64 magnitude = (s64)(sm & ~BIT_ULL(63)); + + return (sm & BIT_ULL(63)) ? -magnitude : magnitude; +} + +static void evdi_color_update_gamma(struct evdi_color_data *data, + struct drm_crtc_state *crtc_state) +{ + const struct drm_color_lut *lut; + unsigned int lut_len; + int i, c; + + if (!crtc_state->gamma_lut) { + data->has_gamma = false; + return; + } + + lut = (const struct drm_color_lut *)crtc_state->gamma_lut->data; + lut_len = crtc_state->gamma_lut->length / sizeof(*lut); + + if (lut_len == 0) { + data->has_gamma = false; + return; + } + + /* + * Build a direct 256-entry table indexed by raw 8-bit channel value, + * regardless of the length of the LUT userspace actually uploaded + * (expected to be EVDI_GAMMA_LUT_SIZE, since that's what we advertise + * via drm_mode_crtc_set_gamma_size(), but not all clients necessarily + * respect that hint). + */ + for (i = 0; i < EVDI_GAMMA_LUT_SIZE; ++i) { + s64 idx_fp = drm_fixp_div(drm_int2fixp(i * (lut_len - 1)), + drm_int2fixp(EVDI_GAMMA_LUT_SIZE - 1)); + int idx_floor = drm_fixp2int(idx_fp); + int idx_ceil = min_t(int, idx_floor + 1, lut_len - 1); + s64 frac = idx_fp - drm_int2fixp(idx_floor); + u16 raw[3] = { lut[idx_floor].red, lut[idx_floor].green, lut[idx_floor].blue }; + u16 raw_ceil[3] = { lut[idx_ceil].red, lut[idx_ceil].green, lut[idx_ceil].blue }; + + for (c = 0; c < 3; ++c) { + s64 floor_fp = drm_int2fixp(raw[c]); + s64 ceil_fp = drm_int2fixp(raw_ceil[c]); + s64 interp = floor_fp + drm_fixp_mul(ceil_fp - floor_fp, frac); + int val16 = clamp(drm_fixp2int(interp), 0, 65535); + + /* 16-bit LUT entry -> 8-bit raw pixel channel */ + data->gamma[c][i] = val16 >> 8; + } + } + + data->has_gamma = true; +} + +static void evdi_color_update_ctm(struct evdi_color_data *data, + struct drm_crtc_state *crtc_state) +{ + const struct drm_color_ctm *ctm; + int r, c; + + if (!crtc_state->ctm) { + data->has_ctm = false; + return; + } + + ctm = (const struct drm_color_ctm *)crtc_state->ctm->data; + for (r = 0; r < 3; ++r) + for (c = 0; c < 3; ++c) + data->ctm[r][c] = evdi_sm2fixp(ctm->matrix[r * 3 + c]); + + data->has_ctm = true; +} + +void evdi_color_transform_init(struct evdi_color_transform *color) +{ + mutex_init(&color->lock); + memset(&color->data, 0, sizeof(color->data)); +} + +void evdi_color_transform_update(struct evdi_color_transform *color, + struct drm_crtc_state *crtc_state) +{ + if (!crtc_state->color_mgmt_changed) + return; + + mutex_lock(&color->lock); + evdi_color_update_gamma(&color->data, crtc_state); + evdi_color_update_ctm(&color->data, crtc_state); + color->data.active = color->data.has_gamma || color->data.has_ctm; + mutex_unlock(&color->lock); +} + +bool evdi_color_transform_snapshot(struct evdi_color_transform *color, + struct evdi_color_data *snapshot) +{ + mutex_lock(&color->lock); + *snapshot = color->data; + mutex_unlock(&color->lock); + + return snapshot->active; +} + +static s64 evdi_color_apply_ctm_channel(const struct evdi_color_data *snapshot, + int row, s64 r, s64 g, s64 b) +{ + return drm_fixp_mul(snapshot->ctm[row][0], r) + + drm_fixp_mul(snapshot->ctm[row][1], g) + + drm_fixp_mul(snapshot->ctm[row][2], b); +} + +void evdi_color_transform_apply_row(const struct evdi_color_data *snapshot, + void *row, int width_px, bool swap_rb) +{ + u8 *px = row; + const int r_idx = swap_rb ? 2 : 0; + const int b_idx = swap_rb ? 0 : 2; + int i; + + if (!snapshot->active) + return; + + for (i = 0; i < width_px; ++i, px += 4) { + int val[3] = { px[r_idx], px[1], px[b_idx] }; + + if (snapshot->has_ctm) { + s64 r = drm_int2fixp(val[0]); + s64 g = drm_int2fixp(val[1]); + s64 b = drm_int2fixp(val[2]); + + val[0] = clamp(drm_fixp2int_round( + evdi_color_apply_ctm_channel(snapshot, 0, r, g, b)), 0, 255); + val[1] = clamp(drm_fixp2int_round( + evdi_color_apply_ctm_channel(snapshot, 1, r, g, b)), 0, 255); + val[2] = clamp(drm_fixp2int_round( + evdi_color_apply_ctm_channel(snapshot, 2, r, g, b)), 0, 255); + } + + if (snapshot->has_gamma) { + val[0] = snapshot->gamma[0][val[0]]; + val[1] = snapshot->gamma[1][val[1]]; + val[2] = snapshot->gamma[2][val[2]]; + } + + px[r_idx] = val[0]; + px[1] = val[1]; + px[b_idx] = val[2]; + } +} diff --git a/module/evdi_color.h b/module/evdi_color.h new file mode 100644 index 00000000..082b1979 --- /dev/null +++ b/module/evdi_color.h @@ -0,0 +1,63 @@ +/* SPDX-License-Identifier: GPL-2.0-only + * Copyright (c) 2026 DisplayLink (UK) Ltd. + * + * This file is subject to the terms and conditions of the GNU General Public + * License v2. See the file COPYING in the main directory of this archive for + * more details. + */ + +#ifndef EVDI_COLOR_H +#define EVDI_COLOR_H + +#include +#include + +struct drm_crtc_state; + +/* + * evdi has no hardware CRTC gamma/CTM block, so Night Light and other + * DRM color management clients need the transform applied in software + * to the raw framebuffer bytes before they leave the driver. + * + * Advertised via drm_mode_crtc_set_gamma_size()/drm_crtc_enable_color_mgmt() + * and consumed here as a direct 8-bit-indexed lookup table, independent of + * whatever length LUT userspace actually uploads (see evdi_color_transform_update()). + */ +#define EVDI_GAMMA_LUT_SIZE 256 + +struct evdi_color_data { + bool active; + bool has_gamma; + bool has_ctm; + u8 gamma[3][EVDI_GAMMA_LUT_SIZE]; + /* row-major 3x3, drm_fixed.h S32.32 two's-complement fixed point */ + s64 ctm[3][3]; +}; + +struct evdi_color_transform { + struct mutex lock; + struct evdi_color_data data; +}; + +void evdi_color_transform_init(struct evdi_color_transform *color); + +/* Recomputes the LUT/CTM from crtc_state; cheap no-op unless + * crtc_state->color_mgmt_changed is set (i.e. on actual property writes). + */ +void evdi_color_transform_update(struct evdi_color_transform *color, + struct drm_crtc_state *crtc_state); + +/* Copies the current transform out under lock. Returns whether it's a + * no-op identity transform, letting the caller skip apply_row() entirely. + */ +bool evdi_color_transform_snapshot(struct evdi_color_transform *color, + struct evdi_color_data *snapshot); + +/* Applies gamma/CTM in place to one packed 32bpp scanline of width_px + * pixels. swap_rb selects XBGR/ABGR (R and B swapped vs XRGB/ARGB) byte + * order; the alpha/padding byte is always left untouched. + */ +void evdi_color_transform_apply_row(const struct evdi_color_data *snapshot, + void *row, int width_px, bool swap_rb); + +#endif diff --git a/module/evdi_drm_drv.c b/module/evdi_drm_drv.c index 59f73198..57011641 100644 --- a/module/evdi_drm_drv.c +++ b/module/evdi_drm_drv.c @@ -188,6 +188,7 @@ static int evdi_drm_device_init(struct drm_device *dev) ret = evdi_cursor_init(&evdi->cursor); if (ret) goto err_free; + evdi_color_transform_init(&evdi->color); evdi_modeset_init(dev); diff --git a/module/evdi_drm_drv.h b/module/evdi_drm_drv.h index ad6ff9e6..160dc51a 100644 --- a/module/evdi_drm_drv.h +++ b/module/evdi_drm_drv.h @@ -38,6 +38,7 @@ #include #include +#include "evdi_color.h" #include "evdi_debug.h" #include "tests/evdi_test.h" @@ -49,6 +50,7 @@ struct evdi_device { struct drm_connector *conn; struct evdi_cursor *cursor; bool cursor_events_enabled; + struct evdi_color_transform color; uint32_t pixel_area_limit; uint32_t pixel_per_second_limit; diff --git a/module/evdi_modeset.c b/module/evdi_modeset.c index 6fe779f8..9989f14f 100644 --- a/module/evdi_modeset.c +++ b/module/evdi_modeset.c @@ -23,12 +23,14 @@ #include #endif #include +#include #include #include #include #include #include "evdi_drm.h" #include "evdi_drm_drv.h" +#include "evdi_color.h" #include "evdi_cursor.h" #include "evdi_params.h" #ifdef EVDI_HAVE_DRM_GEM_PLANE_HELPER_PREPARE_FB @@ -87,6 +89,8 @@ static void evdi_crtc_atomic_flush( (crtc_state->mode_changed || evdi_painter_needs_full_modeset(evdi->painter)); bool notify_dpms = crtc_state->active_changed || evdi_painter_needs_full_modeset(evdi->painter); + evdi_color_transform_update(&evdi->color, crtc_state); + if (notify_mode_changed) evdi_painter_mode_changed_notify(evdi, &crtc_state->adjusted_mode); @@ -503,6 +507,16 @@ static int evdi_crtc_init(struct drm_device *dev) EVDI_DEBUG("drm_crtc_init: %d p%p\n", status, primary_plane); drm_crtc_helper_add(crtc, &evdi_helper_funcs); + /* + * evdi has no hardware gamma/CTM block to program; the properties + * registered here are applied in software to the raw framebuffer + * bytes in evdi_painter.c's copy_primary_pixels()/_on_xe(), since + * that's the only place the driver has direct access to pixel data + * before it's handed to userspace. + */ + drm_mode_crtc_set_gamma_size(crtc, EVDI_GAMMA_LUT_SIZE); + drm_crtc_enable_color_mgmt(crtc, 0, true, EVDI_GAMMA_LUT_SIZE); + return 0; } diff --git a/module/evdi_painter.c b/module/evdi_painter.c index 5cc69ad4..53583da7 100644 --- a/module/evdi_painter.c +++ b/module/evdi_painter.c @@ -23,6 +23,7 @@ #include #include "evdi_drm.h" #include "evdi_drm_drv.h" +#include "evdi_color.h" #include "evdi_cursor.h" #include "evdi_params.h" #include "evdi_i2c.h" @@ -177,16 +178,23 @@ static void collapse_dirty_rects(struct drm_clip_rect *rects, int *count) *count = 1; } +static bool evdi_format_swaps_rb(uint32_t format) +{ + return format == DRM_FORMAT_XBGR8888 || format == DRM_FORMAT_ABGR8888; +} + #if defined(EVDI_HAVE_IOSYS_MAP) && defined(CONFIG_X86) static int copy_primary_pixels_on_xe(struct evdi_framebuffer *efb, char __user *buffer, int buf_byte_stride, int const max_x, - int const max_y) + int const max_y, + const struct evdi_color_data *color) { int y; struct drm_framebuffer *fb = &efb->base; const int byte_span = max_x * 4; + const bool swap_rb = evdi_format_swaps_rb(fb->format->format); struct iosys_map dst_mapping = IOSYS_MAP_INIT_VADDR(vmalloc(max_x * 4)); if (!dst_mapping.vaddr) @@ -200,6 +208,8 @@ static int copy_primary_pixels_on_xe(struct evdi_framebuffer *efb, drm_clflush_virt_range(src_mapping.vaddr, byte_span); drm_memcpy_from_wc(&dst_mapping, &src_mapping, byte_span); + if (color->active) + evdi_color_transform_apply_row(color, dst_mapping.vaddr, max_x, swap_rb); if (copy_to_user(dst, dst_mapping.vaddr, byte_span)) { vfree(dst_mapping.vaddr); return -EFAULT; @@ -220,18 +230,27 @@ static int copy_primary_pixels(struct evdi_framebuffer *efb, int buf_byte_stride, int num_rects, struct drm_clip_rect *rects, int const max_x, - int const max_y) + int const max_y, + const struct evdi_color_data *color) { struct drm_framebuffer *fb = &efb->base; struct drm_clip_rect *r; + const bool swap_rb = evdi_format_swaps_rb(fb->format->format); + char *scratch_row = NULL; EVDI_CHECKPT(); #if defined(EVDI_HAVE_IOSYS_MAP) && defined(CONFIG_X86) if (efb->is_from_xe) - return copy_primary_pixels_on_xe(efb, buffer, buf_byte_stride, max_x, max_y); + return copy_primary_pixels_on_xe(efb, buffer, buf_byte_stride, max_x, max_y, color); #endif + if (color->active) { + scratch_row = vmalloc(max_x * 4); + if (!scratch_row) + return -ENOMEM; + } + for (r = rects; r != rects + num_rects; ++r) { const int byte_offset = r->x1 * 4; const int byte_span = (r->x2 - r->x1) * 4; @@ -245,6 +264,7 @@ static int copy_primary_pixels(struct evdi_framebuffer *efb, /* rect size may correspond to previous resolution */ if (max_x < r->x2 || max_y < r->y2) { EVDI_WARN("Rect size beyond expected dimensions\n"); + vfree(scratch_row); return -EFAULT; } @@ -255,14 +275,24 @@ static int copy_primary_pixels(struct evdi_framebuffer *efb, #if defined(CONFIG_X86) drm_clflush_virt_range((void *)src, byte_span); #endif - if (copy_to_user(dst, src, byte_span)) + if (color->active) { + memcpy(scratch_row, src, byte_span); + evdi_color_transform_apply_row(color, scratch_row, + byte_span / 4, swap_rb); + if (copy_to_user(dst, scratch_row, byte_span)) { + vfree(scratch_row); + return -EFAULT; + } + } else if (copy_to_user(dst, src, byte_span)) { return -EFAULT; + } src += fb->pitches[0]; dst += buf_byte_stride; } } + vfree(scratch_row); return 0; } @@ -1074,6 +1104,9 @@ int evdi_painter_grabpix_ioctl(struct drm_device *drm_dev, void *data, int err; int ret; struct dma_buf_attachment *import_attach; + struct evdi_color_data color; + + evdi_color_transform_snapshot(&evdi->color, &color); EVDI_CHECKPT(); @@ -1176,7 +1209,8 @@ int evdi_painter_grabpix_ioctl(struct drm_device *drm_dev, void *data, cmd->num_rects, dirty_rects, cmd->buf_width, - cmd->buf_height); + cmd->buf_height, + &color); if (err == 0 && !evdi->cursor_events_enabled) copy_cursor_pixels(efb, cmd->buffer, diff --git a/module/tests/Makefile b/module/tests/Makefile index 5714ba7a..ad1e9bd1 100644 --- a/module/tests/Makefile +++ b/module/tests/Makefile @@ -1,5 +1,5 @@ ccflags-$(CONFIG_DRM_EVDI_KUNIT_TEST) += -I$(srctree)/drivers/gpu/drm/evdi -obj-$(CONFIG_DRM_EVDI_KUNIT_TEST) += evdi_test.o test_evdi_vt_switch.o evdi_fake_user_client.o evdi_fake_compositor.o +obj-$(CONFIG_DRM_EVDI_KUNIT_TEST) += evdi_test.o test_evdi_vt_switch.o test_evdi_color.o evdi_fake_user_client.o evdi_fake_compositor.o diff --git a/module/tests/test_evdi_color.c b/module/tests/test_evdi_color.c new file mode 100644 index 00000000..d8363295 --- /dev/null +++ b/module/tests/test_evdi_color.c @@ -0,0 +1,114 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (c) 2026 DisplayLink (UK) Ltd. + * + * This file is subject to the terms and conditions of the GNU General Public + * License v2. See the file COPYING in the main directory of this archive for + * more details. + */ + +#include +#include +#include + +#include "evdi_color.h" + +static void test_identity_is_noop(struct kunit *test) +{ + struct evdi_color_data data = { 0 }; + u8 row[8] = { 10, 20, 30, 40, 200, 100, 50, 255 }; + u8 expected[8]; + + memcpy(expected, row, sizeof(row)); + + evdi_color_transform_apply_row(&data, row, 2, false); + + KUNIT_EXPECT_MEMEQ(test, row, expected, sizeof(row)); +} + +static void test_gamma_lut_applied_per_channel(struct kunit *test) +{ + struct evdi_color_data data = { 0 }; + u8 row[4] = { 10, 20, 30, 0xAA }; /* B, G, R, X (XRGB8888 byte order) */ + int i; + + data.active = true; + data.has_gamma = true; + for (i = 0; i < EVDI_GAMMA_LUT_SIZE; ++i) { + data.gamma[0][i] = 255 - i; /* invert blue */ + data.gamma[1][i] = i; /* identity green */ + data.gamma[2][i] = 0; /* zero red */ + } + + evdi_color_transform_apply_row(&data, row, 1, false); + + KUNIT_EXPECT_EQ(test, row[0], (u8)(255 - 10)); + KUNIT_EXPECT_EQ(test, row[1], (u8)20); + KUNIT_EXPECT_EQ(test, row[2], (u8)0); + KUNIT_EXPECT_EQ(test, row[3], (u8)0xAA); /* X/alpha byte untouched */ +} + +static void test_ctm_swaps_channels(struct kunit *test) +{ + struct evdi_color_data data = { 0 }; + /* out = [ [0 0 1] [0 1 0] [1 0 0] ] x in : swaps R and B, keeps G */ + u8 row[4] = { 10, 20, 30, 0 }; /* B=10 G=20 R=30 (XRGB8888) */ + + data.active = true; + data.has_ctm = true; + data.ctm[0][2] = drm_int2fixp(1); /* out_r = in_b */ + data.ctm[1][1] = drm_int2fixp(1); /* out_g = in_g */ + data.ctm[2][0] = drm_int2fixp(1); /* out_b = in_r */ + + evdi_color_transform_apply_row(&data, row, 1, false); + + /* row[0]=B should become old R (30), row[2]=R should become old B (10) */ + KUNIT_EXPECT_EQ(test, row[0], (u8)30); + KUNIT_EXPECT_EQ(test, row[1], (u8)20); + KUNIT_EXPECT_EQ(test, row[2], (u8)10); +} + +static void test_swap_rb_targets_correct_bytes(struct kunit *test) +{ + struct evdi_color_data data = { 0 }; + u8 row_xrgb[4] = { 1, 2, 3, 9 }; /* B, G, R, X */ + u8 row_xbgr[4] = { 3, 2, 1, 9 }; /* R, G, B, X - same logical pixel */ + int i; + + data.active = true; + data.has_gamma = true; + for (i = 0; i < EVDI_GAMMA_LUT_SIZE; ++i) { + data.gamma[0][i] = i * 2 > 255 ? 255 : i * 2; /* boost red */ + data.gamma[1][i] = i; + data.gamma[2][i] = i; + } + + evdi_color_transform_apply_row(&data, row_xrgb, 1, false); + evdi_color_transform_apply_row(&data, row_xbgr, 1, true); + + /* Both encode the same logical (R,G,B); result must match once + * accounting for the swapped byte layout. + */ + KUNIT_EXPECT_EQ(test, row_xrgb[2], row_xbgr[0]); /* R byte */ + KUNIT_EXPECT_EQ(test, row_xrgb[1], row_xbgr[1]); /* G byte */ + KUNIT_EXPECT_EQ(test, row_xrgb[0], row_xbgr[2]); /* B byte */ + KUNIT_EXPECT_EQ(test, row_xrgb[3], (u8)9); + KUNIT_EXPECT_EQ(test, row_xbgr[3], (u8)9); +} + +static struct kunit_case evdi_color_test_cases[] = { + KUNIT_CASE(test_identity_is_noop), + KUNIT_CASE(test_gamma_lut_applied_per_channel), + KUNIT_CASE(test_ctm_swaps_channels), + KUNIT_CASE(test_swap_rb_targets_correct_bytes), + {} +}; + +static struct kunit_suite evdi_color_test_suite = { + .name = "drm_evdi_color_tests", + .test_cases = evdi_color_test_cases, +}; + +kunit_test_suite(evdi_color_test_suite); + +MODULE_LICENSE("GPL"); From 8a229d8b968315d48db9081be86132671e7aee1b Mon Sep 17 00:00:00 2001 From: jj Date: Mon, 20 Jul 2026 08:54:17 -0400 Subject: [PATCH 2/2] Fix R/B channel swap in evdi_color_transform_apply_row() drm_fourcc.h documents DRM_FORMAT_XRGB8888/ARGB8888 as "[31:0] x:R:G:B ... little endian", which puts B at byte 0 and R at byte 2 in memory (XBGR8888/ABGR8888 swap that). The r_idx/b_idx assignment had this backwards, so a gamma/CTM transform meant for the red channel was applied to the blue byte and vice versa. Confirmed on real hardware: before this fix, Night Light produced a static purple/cool tint that didn't track the configured color temperature; after, it correctly renders as a warm orange shift that tracks temperature changes. Also corrects module/tests/test_evdi_color.c's expected values, which had been written to match the buggy output rather than derived independently from the true byte layout - the previous version of this test suite could not have caught this bug. Signed-off-by: jj --- module/evdi_color.c | 9 +++++++-- module/tests/test_evdi_color.c | 19 +++++++++++++------ 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/module/evdi_color.c b/module/evdi_color.c index 4d6ca4b6..fa89b0bc 100644 --- a/module/evdi_color.c +++ b/module/evdi_color.c @@ -139,9 +139,14 @@ static s64 evdi_color_apply_ctm_channel(const struct evdi_color_data *snapshot, void evdi_color_transform_apply_row(const struct evdi_color_data *snapshot, void *row, int width_px, bool swap_rb) { + /* + * XRGB8888/ARGB8888 ("x:R:G:B" MSB-to-LSB, little endian per + * drm_fourcc.h) store B at byte 0 and R at byte 2; XBGR8888/ + * ABGR8888 swap that. G (byte 1) and X/alpha (byte 3) never move. + */ u8 *px = row; - const int r_idx = swap_rb ? 2 : 0; - const int b_idx = swap_rb ? 0 : 2; + const int r_idx = swap_rb ? 0 : 2; + const int b_idx = swap_rb ? 2 : 0; int i; if (!snapshot->active) diff --git a/module/tests/test_evdi_color.c b/module/tests/test_evdi_color.c index d8363295..7eb38f4b 100644 --- a/module/tests/test_evdi_color.c +++ b/module/tests/test_evdi_color.c @@ -29,22 +29,29 @@ static void test_identity_is_noop(struct kunit *test) static void test_gamma_lut_applied_per_channel(struct kunit *test) { struct evdi_color_data data = { 0 }; - u8 row[4] = { 10, 20, 30, 0xAA }; /* B, G, R, X (XRGB8888 byte order) */ + /* XRGB8888 is "x:R:G:B" MSB-to-LSB, little endian (drm_fourcc.h): + * byte0=B=10, byte1=G=20, byte2=R=30, byte3=X=0xAA. + */ + u8 row[4] = { 10, 20, 30, 0xAA }; int i; data.active = true; data.has_gamma = true; for (i = 0; i < EVDI_GAMMA_LUT_SIZE; ++i) { - data.gamma[0][i] = 255 - i; /* invert blue */ - data.gamma[1][i] = i; /* identity green */ - data.gamma[2][i] = 0; /* zero red */ + data.gamma[0][i] = 255 - i; /* R: invert */ + data.gamma[1][i] = i; /* G: identity */ + data.gamma[2][i] = 0; /* B: zero */ } evdi_color_transform_apply_row(&data, row, 1, false); - KUNIT_EXPECT_EQ(test, row[0], (u8)(255 - 10)); + /* byte0 (true B, was 10) must go through the B curve (-> 0), not + * the R curve - this is the exact split that would catch an R/B + * byte-offset swap. + */ + KUNIT_EXPECT_EQ(test, row[0], (u8)0); KUNIT_EXPECT_EQ(test, row[1], (u8)20); - KUNIT_EXPECT_EQ(test, row[2], (u8)0); + KUNIT_EXPECT_EQ(test, row[2], (u8)(255 - 30)); KUNIT_EXPECT_EQ(test, row[3], (u8)0xAA); /* X/alpha byte untouched */ }