Re: [PATCH v5 11/14] drm/tests: atomic: Add tests for drm_atomic_commit_fill_with_defaults()

[sashiko-bot@kernel.org]

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 <mripard@kernel.org>

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/20261016-drm-reset-state-flag-v5-0-dad2d64b693b@kernel.org?part=11