Re: [PATCH v7 13/16] 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 2 potential issue(s) to consider:
- [Medium] drm/tests: missing atomic_create_state in drm_atomic_init_connector_funcs
- [Low] drm/tests: commit message does not match added tests in drm_atomic_test.c
--

commit 0885e84f724b909721af519687158e094325a32c
Author: Maxime Ripard <mripard@kernel.org>

drm/tests: atomic: Add tests for drm_atomic_commit_fill_with_defaults()

This commit adds a KUnit test suite in drivers/gpu/drm/tests/drm_atomic_test.c
to exercise drm_atomic_commit_fill_with_defaults().

>     drm/tests: atomic: Add tests for drm_atomic_commit_fill_with_defaults()
>     
>     drm_atomic_commit_fill_with_defaults() is used by the
>     DRM_MODE_ATOMIC_RESET flag to bring all objects back to their pristine
>     state. However, it has no dedicated test coverage.
>     
>     Add two tests that verify the reset cycle. The first enables a CRTC,
>     plane, and connector, then calls drm_atomic_commit_fill_with_defaults()
>     and verifies all objects return to their disabled state: CRTC inactive,
>     plane disabled with no framebuffer, and connector unbound. The second
>     goes further and commits the default-filled state to verify the
>     init -> enable -> reset -> commit sequence completes without errors.
>     
>     Suggested-by: Daniel Stone <daniel@fooishbar.org>
>     Signed-off-by: Maxime Ripard <mripard@kernel.org>

[Severity: Low]
This isn't a bug, but does the commit message match the tests added by this
patch?

The commit message states that two tests are added and that the first test
enables a CRTC, plane, and connector. However, drm_atomic_fill_with_defaults_tests
adds four test cases (also including drm_test_check_fill_with_defaults_color_mgmt
and drm_test_check_fill_with_defaults_no_modeset), and as noted in the comment
above drm_test_check_fill_with_defaults(), the test only enables the CRTC and
connector via drm_kunit_helper_enable_crtc_connector() without enabling the
plane.

> diff --git a/drivers/gpu/drm/tests/drm_atomic_test.c b/drivers/gpu/drm/tests/drm_atomic_test.c
> index ea91bec6569e6..e8f18b8f6361e 100644
> --- a/drivers/gpu/drm/tests/drm_atomic_test.c
> +++ b/drivers/gpu/drm/tests/drm_atomic_test.c

[ ... ]

> @@ -140,13 +141,321 @@ static struct kunit_case drm_atomic_get_connector_for_encoder_tests[] = {
>  	{ }
>  };
>  
> -
>  static struct kunit_suite drm_atomic_get_connector_for_encoder_test_suite = {
>  	.name = "drm_test_atomic_get_connector_for_encoder",
>  	.test_cases = drm_atomic_get_connector_for_encoder_tests,
>  };
>  
> -kunit_test_suite(drm_atomic_get_connector_for_encoder_test_suite);
> +/*
> + * Test that after enabling a CRTC and connector, filling with defaults
> + * produces a state where all objects are disabled: CRTC is inactive,
> + * plane is disabled, and connector is unbound.
> + *
> + * This is done without any lease, so all objects are expected to be
> + * reset.
> + *
> + * The plane is not explicitly enabled with a framebuffer here, so
> + * this validates that fill_with_defaults produces a consistent
> + * disabled state rather than testing an active plane teardown.
> + */
> +static void drm_test_check_fill_with_defaults(struct kunit *test)
> +{
> +	struct drm_atomic_test_priv *priv;
> +	struct drm_modeset_acquire_ctx ctx;
> +	struct drm_connector_state *new_conn_state;
> +	struct drm_crtc_state *new_crtc_state;
> +	struct drm_plane_state *new_plane_state;
> +	struct drm_atomic_commit *state;
> +	struct drm_display_mode *mode;
> +	struct drm_device *drm;
> +	int ret;
> +
> +	priv = create_device(test);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
> +
> +	drm = &priv->drm;
> +
> +	mode = drm_kunit_display_mode_from_cea_vic(test, drm, 16);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, mode);
> +
> +	drm_modeset_acquire_init(&ctx, 0);
> +
> +retry_enable:
> +	ret = drm_kunit_helper_enable_crtc_connector(test, drm,
> +						     priv->crtc,
> +						     &priv->connector,
> +						     mode, &ctx);
> +	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);
> +
> +retry_fill:
> +	ret = drm_atomic_commit_fill_with_defaults(NULL, state);
> +	if (ret == -EDEADLK) {
> +		drm_atomic_commit_clear(state);
> +		ret = drm_modeset_backoff(&ctx);
> +		if (!ret)
> +			goto retry_fill;
> +	}
> +	KUNIT_ASSERT_EQ(test, ret, 0);

[Severity: Medium]
As reported by Sashiko on v6 [1], won't drm_atomic_commit_fill_with_defaults()
fail with -EOPNOTSUPP in all four tests here?

Each test calls create_device(), which initializes priv->connector with
drm_atomic_init_connector_funcs:

drivers/gpu/drm/tests/drm_atomic_test.c:
static const struct drm_connector_funcs drm_atomic_init_connector_funcs = {
	.atomic_destroy_state	= drm_atomic_helper_connector_destroy_state,
	.atomic_duplicate_state	= drm_atomic_helper_connector_duplicate_state,
	.reset			= drm_atomic_helper_connector_reset,
};

Because .atomic_create_state is left NULL on drm_atomic_init_connector_funcs,
drm_atomic_commit_fill_with_defaults() acquires the modeset locks on
state->acquire_ctx and then returns -EOPNOTSUPP when iterating over connectors:

drivers/gpu/drm/drm_atomic.c:drm_atomic_commit_fill_with_defaults()
		if (!connector->funcs->atomic_create_state) {
			drm_connector_list_iter_end(&conn_iter);
			return -EOPNOTSUPP;
		}

This causes KUNIT_ASSERT_EQ(test, ret, 0) in drm_test_check_fill_with_defaults(),
drm_test_check_fill_with_defaults_commit(),
drm_test_check_fill_with_defaults_color_mgmt(), and
drm_test_check_fill_with_defaults_no_modeset() to fail and abort the test
before drm_modeset_drop_locks(&ctx) and drm_modeset_acquire_fini(&ctx) are
reached, leaving the acquired modeset locks linked to the stack-allocated ctx
during KUnit device teardown.

Should drm_atomic_init_connector_funcs set .atomic_create_state to
drm_atomic_helper_connector_create_state?

[1] https://lore.kernel.org/r/20261022-drm-reset-state-flag-v6-12-f36d640b7e76@kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030-drm-reset-state-flag-v7-0-4e55eaeae683@kernel.org?part=13