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 <[email protected]> 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/[email protected]?part=22
