Re: [PATCH v6 12/15] 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
- [Medium] drm/tests: unhandled -EDEADLK in drm_atomic_commit_fill_with_defaults() tests
--

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

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

This commit adds two KUnit tests for drm_atomic_commit_fill_with_defaults()
to verify resetting an enabled CRTC, plane, and connector back to their
disabled state and committing the default-filled state.

> diff --git a/drivers/gpu/drm/tests/drm_atomic_test.c b/drivers/gpu/drm/tests/drm_atomic_test.c
> index ea91bec6569e6..52db445b4eadb 100644
> --- a/drivers/gpu/drm/tests/drm_atomic_test.c
> +++ b/drivers/gpu/drm/tests/drm_atomic_test.c
> @@ -140,13 +140,150 @@ 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);
> +
> +	ret = drm_atomic_commit_fill_with_defaults(NULL, state);
> +	KUNIT_ASSERT_EQ(test, ret, 0);

[Severity: Medium]
Will drm_atomic_commit_fill_with_defaults() always return -EOPNOTSUPP here
and in drm_test_check_fill_with_defaults_commit() below?

create_device() initializes priv->connector with
drm_atomic_init_connector_funcs in 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 not set on drm_atomic_init_connector_funcs,
drm_atomic_commit_fill_with_defaults() in drivers/gpu/drm/drm_atomic.c locks
the plane, CRTC, and connection_mutex into state->acquire_ctx, and then hits:

    if (!connector->funcs->atomic_create_state) {
            drm_connector_list_iter_end(&conn_iter);
            return -EOPNOTSUPP;
    }

KUNIT_ASSERT_EQ(test, ret, 0) then aborts the test before
drm_modeset_drop_locks(&ctx) and drm_modeset_acquire_fini(&ctx) can run,
leaving those modeset locks held on the stack-allocated ctx when drmm
teardown runs drm_modeset_lock_fini().

Should drm_atomic_init_connector_funcs wire up .atomic_create_state to
drm_atomic_helper_connector_create_state?

> +
> +	new_crtc_state = drm_atomic_get_new_crtc_state(state, priv->crtc);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_crtc_state);
> +	KUNIT_EXPECT_FALSE(test, new_crtc_state->enable);
> +	KUNIT_EXPECT_FALSE(test, new_crtc_state->active);
> +
> +	new_plane_state = drm_atomic_get_new_plane_state(state, priv->plane);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_plane_state);
> +	KUNIT_EXPECT_NULL(test, new_plane_state->crtc);
> +	KUNIT_EXPECT_NULL(test, new_plane_state->fb);
> +
> +	new_conn_state = drm_atomic_get_new_connector_state(state, &priv->connector);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_conn_state);
> +	KUNIT_EXPECT_NULL(test, new_conn_state->crtc);
> +
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +}
> +
> +/*
> + * Test that after enabling a CRTC and connector, fill_with_defaults
> + * produces a state that can be successfully committed.
> + *
> + * This is done without any lease, so all objects are reset.
> + *
> + * The plane is not explicitly enabled with a framebuffer, 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_commit(struct kunit *test)
> +{
[ ... ]
> +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);
> +
> +	ret = drm_atomic_commit_fill_with_defaults(NULL, state);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	state->allow_modeset = true;
> +
> +	ret = drm_atomic_commit(state);
> +	KUNIT_EXPECT_EQ(test, ret, 0);

[Severity: Medium]
Can drm_atomic_commit_fill_with_defaults() (both here and in
drm_test_check_fill_with_defaults()) and drm_atomic_commit() fail with
-EDEADLK when CONFIG_DEBUG_WW_MUTEX_SLOWPATH is enabled?

drm_kunit_helper_enable_crtc_connector() only locks
drm->mode_config.connection_mutex and priv->crtc->mutex on ctx, leaving
priv->plane->mutex unlocked. When drm_atomic_commit_fill_with_defaults() is
subsequently called with the same ctx, it acquires priv->plane->mutex for the
first time:

    drm_for_each_plane(plane, dev) {
            ...
            ret = drm_modeset_lock(&plane->mutex, commit->acquire_ctx);
            if (ret)
                    return ret;

Under CONFIG_DEBUG_WW_MUTEX_SLOWPATH, acquiring a new ww_mutex on an acquire
context that already holds locks injects -EDEADLK when the context's
deadlock_inject_countdown reaches zero. Without a retry loop calling
drm_atomic_commit_clear(state) and drm_modeset_backoff(&ctx), will
KUNIT_ASSERT_EQ(test, ret, 0) fail spuriously and abort with ctx.contended
and held locks still active?

> +
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-drm-reset-state-flag-v6-0-f36d640b7e76@kernel.org?part=12