Re: [PATCH RFC 22/25] drm/tests: hdmi: Add "Game Mode" VRR tests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] drm/tests: test_edid_hdmi_vrr defined as static const in header
--

commit 77a0520912816fb52f18f64df569bc989ec9d0ad
Author: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

drm/tests: hdmi: Add "Game Mode" VRR tests

This commit adds kunit tests to verify "Game Mode" VRR functionality
in the DRM HDMI state helper. It checks successful VRR validation on
capable sinks, failure on incapable sinks, and proper behavior of
flush and vsync helpers updating the cur_vtotal values.

> diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> index b2e347863a776..1486c16c2f78e 100644
> --- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> +++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
[ ... ]
> +static void drm_test_check_hdmi_vrr(struct kunit *test)
> +{
[ ... ]
> +	ret = drm_atomic_commit(state);
> +	if (ret == -EDEADLK) {
> +		drm_atomic_commit_clear(state);
> +		ret = drm_modeset_backoff(&ctx);
> +		if (!ret)
> +			goto retry_crtc_state;
> +	}
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> +	KUNIT_ASSERT_NOT_NULL(test, crtc_state);

[Severity: Medium]
This is a pre-existing issue in these testing patterns, but if these
assertions fail, does the function return immediately and leak the modeset
ww_mutex locks?

Since the local drm_modeset_acquire_ctx ctx locks are not automatically
managed, returning early skips calling drm_modeset_drop_locks(&ctx) and
drm_modeset_acquire_fini(&ctx). This could leave dangling lock list nodes
pointing to out-of-scope stack memory.

This pattern also appears in drm_test_check_hdmi_vrr_sink_fail() and
drm_test_check_hdmi_vrr_flush_vsync() where early exits on assertion failures
bypass the cleanup block at the end of the functions.

> +
> +	KUNIT_EXPECT_GE(test, crtc_state->vrr_state.base_vtotal, preferred->crtc_vtotal);
> +	KUNIT_EXPECT_GE(test, crtc_state->vrr_state.max_vtotal, preferred->crtc_vtotal);
> +
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +}
[ ... ]
> diff --git a/drivers/gpu/drm/tests/drm_kunit_edid.h b/drivers/gpu/drm/tests/drm_kunit_edid.h
> index 28b4df93a5554..10175c9e1b2ee 100644
> --- a/drivers/gpu/drm/tests/drm_kunit_edid.h
> +++ b/drivers/gpu/drm/tests/drm_kunit_edid.h
[ ... ]
> + * EDID conformity: PASS
> + */
> +static const unsigned char test_edid_hdmi_vrr[] = {
> +	0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x31, 0xd8, 0x43, 0x00,

[Severity: Low]
Does this definition cause the 256-byte array to be duplicated in every
translation unit that includes drm_kunit_edid.h?

It might also trigger -Wunused-const-variable compiler warnings if a file
includes the header but does not reference test_edid_hdmi_vrr. Should this be
declared as extern const here with its definition placed in a .c file,
similar to the other EDID arrays?

> +	0x00, 0x00, 0x00, 0x00, 0x01, 0x24, 0x01, 0x03, 0x80, 0x60, 0x36, 0x78,
> +	0x0f, 0xee, 0x91, 0xa3, 0x54, 0x4c, 0x99, 0x26, 0x0f, 0x50, 0x54, 0x20,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921-vrr-limiter-uapi-v1-0-2fcd7d011646@collabora.com?part=22