Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Medium] drm/tests: modeset lock leak on KUNIT_ASSERT failure - [Medium] drm/tests: mock connector lacks atomic_create_state causing test failure - [Medium] drm/tests: plane is not enabled before reset in fill_with_defaults test --
commit 181cb0103d707708bd4c194386205faf1d9fca47 Author: Maxime Ripard <[email protected]> drm/tests: atomic: Add tests for drm_atomic_commit_fill_with_defaults() This commit adds KUnit test coverage for drm_atomic_commit_fill_with_defaults() to ensure it properly resets CRTC, plane, and connector states. The tests verify both the reset state and a subsequent modeset commit sequence. > diff --git a/drivers/gpu/drm/tests/drm_atomic_test.c > b/drivers/gpu/drm/tests/drm_atomic_test.c > index ea91bec6569e6..988ec5523147d 100644 > --- a/drivers/gpu/drm/tests/drm_atomic_test.c > +++ b/drivers/gpu/drm/tests/drm_atomic_test.c [ ... ] > @@ -140,13 +140,137 @@ static struct kunit_case > drm_atomic_get_connector_for_encoder_tests[] = { > /* > * Test that after enabling a CRTC, plane, and connector, filling with > * defaults produces a state where all objects are disabled: CRTC is [ ... ] > +static void drm_test_check_fill_with_defaults(struct kunit *test) > +{ [ ... ] > + drm_modeset_acquire_init(&ctx, 0); > + > +retry_enable: > + ret = drm_kunit_helper_enable_crtc_connector(test, drm, > + priv->crtc, > + &priv->connector, > + mode, &ctx); [Severity: Medium] The commit message states the test "enables a CRTC, plane, and connector", but drm_kunit_helper_enable_crtc_connector() configures the CRTC and connector without assigning a framebuffer or CRTC to the plane. Since the plane is never actively enabled before the reset, does this test only verify that an already-disabled plane remains disabled rather than testing the teardown transition of an active plane? > + if (ret == -EDEADLK) { > + ret = drm_modeset_backoff(&ctx); > + if (!ret) > + goto retry_enable; > + } > + KUNIT_ASSERT_EQ(test, ret, 0); > + > + state = drm_kunit_helper_atomic_state_alloc(test, drm, &ctx); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state); > + > + ret = drm_atomic_commit_fill_with_defaults(state); [Severity: Medium] Will drm_atomic_commit_fill_with_defaults() unconditionally return -EOPNOTSUPP here? The mock connector initialized with drm_atomic_init_connector_funcs lacks the .atomic_create_state hook. Since drm_atomic_commit_fill_with_defaults() iterates over connectors and requires this hook, this seems like it will unconditionally fail the test. > + KUNIT_ASSERT_EQ(test, ret, 0); [Severity: Medium] If this assertion (or any subsequent KUNIT_ASSERT_*) fails, it forces an immediate return from the function. Since drm_modeset_acquire_init() and drm_kunit_helper_enable_crtc_connector() acquired modeset locks, would an early return bypass drm_modeset_drop_locks(&ctx) and leak the ww_mutexes and ww_acquire_ctx? Should KUNIT_EXPECT_* macros combined with a cleanup goto label or KUnit deferred actions be used instead of KUNIT_ASSERT_* to ensure locks are released? > + > + new_crtc_state = drm_atomic_get_new_crtc_state(state, priv->crtc); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_crtc_state); [ ... ] > +static void drm_test_check_fill_with_defaults_commit(struct kunit *test) > +{ [ ... ] > + drm_modeset_acquire_init(&ctx, 0); > + > +retry_enable: > + ret = drm_kunit_helper_enable_crtc_connector(test, drm, > + priv->crtc, > + &priv->connector, > + mode, &ctx); [Severity: Medium] Similar to the first test, this helper omits enabling the plane before performing the reset round-trip. Could this leave the plane teardown untested as well? > + if (ret == -EDEADLK) { > + ret = drm_modeset_backoff(&ctx); > + if (!ret) > + goto retry_enable; > + } > + KUNIT_ASSERT_EQ(test, ret, 0); > + > + state = drm_kunit_helper_atomic_state_alloc(test, drm, &ctx); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state); > + > + ret = drm_atomic_commit_fill_with_defaults(state); > + KUNIT_ASSERT_EQ(test, ret, 0); [Severity: Medium] As in the previous function, an assertion failure here would skip the cleanup block at the end of the function. Can this leak the modeset locks acquired above? > + > + state->allow_modeset = true; > + > + ret = drm_atomic_commit(state); > + KUNIT_EXPECT_EQ(test, ret, 0); > + > + drm_modeset_drop_locks(&ctx); > + drm_modeset_acquire_fini(&ctx); > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=11
