Re: [RFC PATCH v3 04/23] drm/vkms: Add kunit tests for VKMS LUT handling
On Wed, 8 Nov 2023 11:36:23 -0500
Harry Wentland wrote:
> 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
>
> v3:
> - Use include way of testing static functions (Arthur)
>
> Signed-off-by: Harry Wentland
> Cc: Arthur Grillo
> ---
> drivers/gpu/drm/vkms/Kconfig | 5 ++
> drivers/gpu/drm/vkms/tests/.kunitconfig | 4 ++
> drivers/gpu/drm/vkms/tests/vkms_color_tests.c | 62 +++
> drivers/gpu/drm/vkms/vkms_composer.c | 8 ++-
> 4 files changed, 77 insertions(+), 2 deletions(-)
> create mode 100644 drivers/gpu/drm/vkms/tests/.kunitconfig
> create mode 100644 drivers/gpu/drm/vkms/tests/vkms_color_tests.c
>
> diff --git a/drivers/gpu/drm/vkms/Kconfig b/drivers/gpu/drm/vkms/Kconfig
> index b9ecdebecb0b..c1f8b343ff0e 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/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/vkms_color_tests.c
> b/drivers/gpu/drm/vkms/tests/vkms_color_tests.c
> new file mode 100644
> index ..b995114cf6b8
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/tests/vkms_color_tests.c
> @@ -0,0 +1,62 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +
> +#include
> +
> +#include
> +
> +#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);
Hi,
what about testing with values not directly hitting a LUT element?
> +}
> +
> +static void vkms_color_test_lerp(struct kunit *test)
> +{
> + KUNIT_EXPECT_EQ(test, lerp_u16(0x0, 0x10, 0x8000), 0x8);
It would raise much more confidence in lerp_u16() if there were more cases:
- odd a
- odd b
- odd b-a
- b = a
- b = a + 1
For each of the above:
- t = 0.0
- t = 1.0
- t = 0.0 + 1
- t = 1.0 - 1
- t chosen so that result must round/truncate
- t chosen to verify the flipping point of result a (or b) to a+1 (or b-1)
I think those are the fragile points in a lerp implementation.
Thanks,
pq
> +}
> +
> +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..6f942896036e 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -91,7 +91,7 @@ static void fill_background(const struct pixel_argb_u16
> *background_color,
> }
>
> // lerp(a, b, t) = a + (b - a) * t
> -static u16 lerp_u16(u16 a, u16 b, s64 t)
> +u16 lerp_u16(u16 a, u16 b, s64 t)
> {
> s64 a_fp = drm_int2fixp(a);
> s64 b_fp = drm_int2fixp(b);
> @@ -101,7 +101,7 @@ static u16 lerp_u16(u16 a, u16 b, s64 t)
> return drm_fixp2int(a_fp + delta);
> }
>
> -static s64 get_lut_index(const struct vkms_color_lut *lut, u16 channel_value)
> +s64 get_lut_index(const struct vkms_color_lut *lut, u16 channe
what are the protocol rules about uniqueness of event and request names?
Can a single interface have an event and a request with the same name?
SEGFAULT encountered in wl_event_queue_release
Hello, I'm writing a wayland application which utilizes gtk4-layer-shell and gtk4. I'm testing how the application handles removal of monitors. The application creates a panel on all monitors and will remove the panel, which is a gtk4-layer-shell window, when a monitor's invalidate signal is encountered. However, I'm having a bit of an issue where destroying the TopLevel gtk4-layer-shell window throws a segfault in `wl_event_queue_release`. Oddly enough, the segfault occurs due to what seems like a corrupt pointer to the name field in proxy->object.interface->name. I'm using current HEAD:82d8b21827c4f58013a4061fb3c813d9ae80407e Here is the GDB backtrace output for the invalid memory: 0x7f40de1c4c99 in wl_log (fmt=0x38161602d000fe0 ) at ../src/wayland-util.c:449 I know there are a few components here. For one, the client is being used by Gtk4, not myself, so it's possible this is not an issue in Wayland but an issue of its usage in Gtk4. However, I'm just trying to pinpoint if this is a bug and where it exists. I'll end with a full backtrace of the segfault followed by the source file which does the bulk of the work: GDB BT- #0 __strlen_evex () at ../sysdeps/x86_64/multiarch/strlen-evex.S:228 #1 0x7f40de9354f8 in __printf_buffer (buf=buf@entry=0x7ffe99279ae0, format=0x7f40de1c5770 " %s#%u still attached\n", ap=0x7ffe99279d40, mode_flags=mode_flags@entry=2) at /usr/src/debug/glibc-2.38-11.fc39.x86_64/stdio-common/vfprintf-process-arg.c:435 #2 0x7f40de956af0 in __vasprintf_internal (result_ptr=result_ptr@entry=0x7ffe99279c40, format=, args=, mode_flags=mode_flags@entry=2) at vasprintf.c:102 #3 0x7f40de9f932a in __vasprintf_chk (result_ptr=result_ptr@entry=0x7ffe99279c40, flag=flag@entry=2, format=, ap=) at vasprintf_chk.c:36 #4 0x7f40debd05d3 in vasprintf (__ap=, __fmt=, __ptr=0x7ffe99279c40) at /usr/include/bits/stdio2.h:169 #5 g_vasprintf (string=0x7ffe99279c40, format=, args=) at ../glib/gprintf.c:340 #6 0x7f40deba25d5 in g_strdup_vprintf (format=, args=) at ../glib/gstrfuncs.c:553 #7 0x7f40deb82f80 in format_string (out_allocated_string=, args=0x7ffe99279d40, format=0x7f40de1c5770 " %s#%u still attached\n") at ../glib/gmessages.c:3363 #8 g_logv (log_domain=0x7f40df4c3349 "\300t\bH\213\005\03515", log_level=G_LOG_LEVEL_DEBUG, format=0x7f40de1c5770 " %s#%u still attached\n", args=0x7ffe99279d40) at ../glib/gmessages.c:1319 #9 0x7f40de1c4c99 in wl_log (fmt=0x38161602d000fe0 ) at ../src/wayland-util.c:449 #10 0x7f40de1bf595 in wl_event_queue_release (queue=queue@entry=0x3787780) at ../src/wayland-client.c:315 #11 0x7f40de1bf4d1 in wl_event_queue_destroy (queue=0x3787780) at ../src/wayland-client.c:352 #12 0x7f40df3f7cf4 in gdk_wayland_surface_dispose (object=0x3786a50) at ../gdk/wayland/gdksurface-wayland.c:538 #13 gdk_wayland_surface_dispose (object=0x3786a50) at ../gdk/wayland/gdksurface-wayland.c:522 #14 0x7f40deae49b4 in g_object_unref (_object=0x3786a50) at ../gobject/gobject.c:3894 #15 g_object_unref (_object=0x3786a50) at ../gobject/gobject.c:3805 #16 0x7f40df430e73 in gdk_draw_context_dispose (gobject=0x3787e70) at ../gdk/gdkdrawcontext.c:80 #17 0x7f40deae49b4 in g_object_unref (_object=0x3787e70) at ../gobject/gobject.c:3894 #18 g_object_unref (_object=0x3787e70) at ../gobject/gobject.c:3805 #19 0x7f40df443708 in gdk_gl_context_make_current (context=0x383d210) at ../gdk/gdkglcontext.c:1639 #20 gdk_gl_context_make_current (context=0x383d210) at ../gdk/gdkglcontext.c:1604 #21 0x7f40df4781b9 in gsk_gl_renderer_unrealize (renderer=0x383ca80) at ../gsk/gl/gskglrenderer.c:175 #22 0x7f40df458890 in gsk_renderer_unrealize (renderer=0x383ca80) at ../gsk/gskrenderer.c:325 #23 0x7f40df288ada in gtk_window_unrealize (widget=0x37952c0) at ../gtk/gtkwindow.c:4478 #24 0x7f40deaf5e85 in _g_closure_invoke_va (param_types=0x0, n_params=, args=0x7ffe9927a270, instance=0x37952c0, return_value=0x0, closure=0xf713c0) at ../gobject/gclosure.c:895 #25 signal_emit_valist_unlocked (instance=instance@entry=0x37952c0, signal_id=signal_id@entry=56, detail=detail@entry=0, var_args=var_args@entry=0x7ffe9927a270) at ../gobject/gsignal.c:3516 #26 0x7f40deaf5f91 in g_signal_emit_valist (instance=0x37952c0, signal_id=56, detail=0, var_args=var_args@entry=0x7ffe9927a270) at ../gobject/gsignal.c:3355 #27 0x7f40deaf6053 in g_signal_emit (instance=instance@entry=0x37952c0, signal_id=, detail=detail@entry=0) at ../gobject/gsignal.c:3675 #28 0x7f40df26d127 in gtk_widget_unrealize (widget=0x37952c0) at ../gtk/gtkwidget.c:3476 #29 0x7f40df28c044 in gtk_window_destroy (window=0x37952c0) at ../gtk/gtkwindow.c:6802 #30 0x7f40df27d199 in gtk_window_close (window=0x37952c0) at ../gtk/gtkwindow.c:1356 #31 gtk_window_close (window=0x37952c0) at ../gtk/gtkwindow.c:1343 #32 0x0040259b in _bar_free (mon=0x3817690, ba
Re: what are the protocol rules about uniqueness of event and request names?
The generated C code be full of conflicts. The MY_PROTOCOL_REQUESTEVENT_SINCE_VERSION define for a start. I think it might compile in C, but other generators exist that might not and it's making life much more confusing than it needs to be. I would strongly avoid it. David
Re: what are the protocol rules about uniqueness of event and request names?
On Thu, 7 Dec 2023 22:06:07 + David Edmundson wrote: > The generated C code be full of conflicts. The > MY_PROTOCOL_REQUESTEVENT_SINCE_VERSION define for a start. > > I think it might compile in C, but other generators exist that might > not and it's making life much more confusing than it needs to be. I > would strongly avoid it. > > David To be clear, I wasn't intending it to sound like I wanted to add an event and a request with the same name myself. I'm writing some middleware that sits between a Wayland compositor and some of its clients, and I would like to know if it might encounter an interface that has an event and a request with the same name. I think you've answered that it's not a good idea for a protocol author to do that, but it also sounds like it's a possibility that someone could do it anyway because there's no direct rule against it. So maybe I should take the necessary precautions. Thanks, Jonathan
