| Message ID | 20250318-drm-vc4-kunit-failures-v1-3-779864d9ab37@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vc4: tests: Fix locking failures | expand |
Hi Maxime, On 18/03/25 11:17, Maxime Ripard wrote: > The vc4-pv-muxing-combinations and vc5-pv-muxing-combinations test > suites use a common test init function which, in part, allocates the > drm atomic state the test will use. > > That allocation relies on drm_kunit_helper_atomic_state_alloc(), and > thus requires a struct drm_modeset_acquire_ctx. This context will then > be stored in the allocated state->acquire_ctx field. > > However, the context is local to the test init function, and is cleared > as soon as drm_kunit_helper_atomic_state_alloc() is done. We thus end up > with an dangling pointer to a cleared context in state->acquire_ctx for > our test to consumes. > > We should really allocate the context and the state in the test > functions, so we can also control when we're done with it. > > Fixes: 30188df0c387 ("drm/tests: Drop drm_kunit_helper_acquire_ctx_alloc()") > Signed-off-by: Maxime Ripard <mripard@kernel.org> > --- > drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c | 41 +++++++++++++++++--------- > 1 file changed, 27 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c > index 992e8f5c5c6ea8d92338a8fe739fa1115ff85338..52c04ef33206bf4f9e21e3c8b7cea932824a67fa 100644 > --- a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c > +++ b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c > @@ -18,11 +18,10 @@ > > #include "vc4_mock.h" > > struct pv_muxing_priv { > struct vc4_dev *vc4; > - struct drm_atomic_state *state; Can't we add `struct drm_modeset_acquire_ctx` here? Then, we can be sure that the context exists during the entire test case. Also, we can add `drm_modeset_drop_locks()` and `drm_modeset_acquire_fini()` to a exit function in the kunit suite. Best Regards, - Maíra > }; > > static bool check_fifo_conflict(struct kunit *test, > const struct drm_atomic_state *state) > { > @@ -675,14 +674,23 @@ KUNIT_ARRAY_PARAM(vc5_test_pv_muxing_invalid, > > static void drm_vc4_test_pv_muxing(struct kunit *test) > { > const struct pv_muxing_param *params = test->param_value; > const struct pv_muxing_priv *priv = test->priv; > - struct drm_atomic_state *state = priv->state; > + struct drm_modeset_acquire_ctx ctx; > + struct drm_atomic_state *state; > + struct drm_device *drm; > + struct vc4_dev *vc4; > unsigned int i; > int ret; > > + drm_modeset_acquire_init(&ctx, 0); > + > + vc4 = priv->vc4; > + drm = &vc4->base; > + state = drm_kunit_helper_atomic_state_alloc(test, drm, &ctx); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state); > for (i = 0; i < params->nencoders; i++) { > enum vc4_encoder_type enc_type = params->encoders[i]; > > ret = vc4_mock_atomic_add_output(test, state, enc_type); > KUNIT_ASSERT_EQ(test, ret, 0); > @@ -698,56 +706,61 @@ static void drm_vc4_test_pv_muxing(struct kunit *test) > enum vc4_encoder_type enc_type = params->encoders[i]; > > KUNIT_EXPECT_TRUE(test, check_channel_for_encoder(test, state, enc_type, > params->check_fn)); > } > + > + drm_modeset_drop_locks(&ctx); > + drm_modeset_acquire_fini(&ctx); > } > > static void drm_vc4_test_pv_muxing_invalid(struct kunit *test) > { > const struct pv_muxing_param *params = test->param_value; > const struct pv_muxing_priv *priv = test->priv; > - struct drm_atomic_state *state = priv->state; > + struct drm_modeset_acquire_ctx ctx; > + struct drm_atomic_state *state; > + struct drm_device *drm; > + struct vc4_dev *vc4; > unsigned int i; > int ret; > > + drm_modeset_acquire_init(&ctx, 0); > + > + vc4 = priv->vc4; > + drm = &vc4->base; > + state = drm_kunit_helper_atomic_state_alloc(test, drm, &ctx); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state); > + > for (i = 0; i < params->nencoders; i++) { > enum vc4_encoder_type enc_type = params->encoders[i]; > > ret = vc4_mock_atomic_add_output(test, state, enc_type); > KUNIT_ASSERT_EQ(test, ret, 0); > } > > ret = drm_atomic_check_only(state); > KUNIT_EXPECT_LT(test, ret, 0); > + > + drm_modeset_drop_locks(&ctx); > + drm_modeset_acquire_fini(&ctx); > } > > static int vc4_pv_muxing_test_init(struct kunit *test) > { > const struct pv_muxing_param *params = test->param_value; > - struct drm_modeset_acquire_ctx ctx; > struct pv_muxing_priv *priv; > - struct drm_device *drm; > struct vc4_dev *vc4; > > priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL); > KUNIT_ASSERT_NOT_NULL(test, priv); > test->priv = priv; > > vc4 = params->mock_fn(test); > KUNIT_ASSERT_NOT_ERR_OR_NULL(test, vc4); > priv->vc4 = vc4; > > - drm_modeset_acquire_init(&ctx, 0); > - > - drm = &vc4->base; > - priv->state = drm_kunit_helper_atomic_state_alloc(test, drm, &ctx); > - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->state); > - > - drm_modeset_drop_locks(&ctx); > - drm_modeset_acquire_fini(&ctx); > - > return 0; > } > > static struct kunit_case vc4_pv_muxing_tests[] = { > KUNIT_CASE_PARAM(drm_vc4_test_pv_muxing, >
On Sun, Mar 23, 2025 at 03:48:17PM -0300, Maíra Canal wrote: > Hi Maxime, > > On 18/03/25 11:17, Maxime Ripard wrote: > > The vc4-pv-muxing-combinations and vc5-pv-muxing-combinations test > > suites use a common test init function which, in part, allocates the > > drm atomic state the test will use. > > > > That allocation relies on drm_kunit_helper_atomic_state_alloc(), and > > thus requires a struct drm_modeset_acquire_ctx. This context will then > > be stored in the allocated state->acquire_ctx field. > > > > However, the context is local to the test init function, and is cleared > > as soon as drm_kunit_helper_atomic_state_alloc() is done. We thus end up > > with an dangling pointer to a cleared context in state->acquire_ctx for > > our test to consumes. > > > > We should really allocate the context and the state in the test > > functions, so we can also control when we're done with it. > > > > Fixes: 30188df0c387 ("drm/tests: Drop drm_kunit_helper_acquire_ctx_alloc()") > > Signed-off-by: Maxime Ripard <mripard@kernel.org> > > --- > > drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c | 41 +++++++++++++++++--------- > > 1 file changed, 27 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c > > index 992e8f5c5c6ea8d92338a8fe739fa1115ff85338..52c04ef33206bf4f9e21e3c8b7cea932824a67fa 100644 > > --- a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c > > +++ b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c > > @@ -18,11 +18,10 @@ > > #include "vc4_mock.h" > > struct pv_muxing_priv { > > struct vc4_dev *vc4; > > - struct drm_atomic_state *state; > > Can't we add `struct drm_modeset_acquire_ctx` here? Then, we can be sure > that the context exists during the entire test case. > > Also, we can add `drm_modeset_drop_locks()` and > `drm_modeset_acquire_fini()` to a exit function in the kunit suite. That's what we were doing before, but the kunit functions and exit functions are run in a separate thread by design, which then cause lockdep to complain. It's not great, but we can't really change either :/ Maxime
Hi Maxime, On 28/03/25 12:32, Maxime Ripard wrote: > On Sun, Mar 23, 2025 at 03:48:17PM -0300, Maíra Canal wrote: >> Hi Maxime, >> >> On 18/03/25 11:17, Maxime Ripard wrote: >>> The vc4-pv-muxing-combinations and vc5-pv-muxing-combinations test >>> suites use a common test init function which, in part, allocates the >>> drm atomic state the test will use. >>> >>> That allocation relies on drm_kunit_helper_atomic_state_alloc(), and >>> thus requires a struct drm_modeset_acquire_ctx. This context will then >>> be stored in the allocated state->acquire_ctx field. >>> >>> However, the context is local to the test init function, and is cleared >>> as soon as drm_kunit_helper_atomic_state_alloc() is done. We thus end up >>> with an dangling pointer to a cleared context in state->acquire_ctx for >>> our test to consumes. >>> >>> We should really allocate the context and the state in the test >>> functions, so we can also control when we're done with it. >>> >>> Fixes: 30188df0c387 ("drm/tests: Drop drm_kunit_helper_acquire_ctx_alloc()") >>> Signed-off-by: Maxime Ripard <mripard@kernel.org> >>> --- >>> drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c | 41 +++++++++++++++++--------- >>> 1 file changed, 27 insertions(+), 14 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c >>> index 992e8f5c5c6ea8d92338a8fe739fa1115ff85338..52c04ef33206bf4f9e21e3c8b7cea932824a67fa 100644 >>> --- a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c >>> +++ b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c >>> @@ -18,11 +18,10 @@ >>> #include "vc4_mock.h" >>> struct pv_muxing_priv { >>> struct vc4_dev *vc4; >>> - struct drm_atomic_state *state; >> >> Can't we add `struct drm_modeset_acquire_ctx` here? Then, we can be sure >> that the context exists during the entire test case. >> >> Also, we can add `drm_modeset_drop_locks()` and >> `drm_modeset_acquire_fini()` to a exit function in the kunit suite. > > That's what we were doing before, but the kunit functions and exit > functions are run in a separate thread by design, which then cause > lockdep to complain. > > It's not great, but we can't really change either :/ > Thanks for the explanation, Maxime. In that case, Reviewed-by: Maíra Canal <mcanal@igalia.com> Best Regards, - Maíra > Maxime
diff --git a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c index 992e8f5c5c6ea8d92338a8fe739fa1115ff85338..52c04ef33206bf4f9e21e3c8b7cea932824a67fa 100644 --- a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c +++ b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c @@ -18,11 +18,10 @@ #include "vc4_mock.h" struct pv_muxing_priv { struct vc4_dev *vc4; - struct drm_atomic_state *state; }; static bool check_fifo_conflict(struct kunit *test, const struct drm_atomic_state *state) { @@ -675,14 +674,23 @@ KUNIT_ARRAY_PARAM(vc5_test_pv_muxing_invalid, static void drm_vc4_test_pv_muxing(struct kunit *test) { const struct pv_muxing_param *params = test->param_value; const struct pv_muxing_priv *priv = test->priv; - struct drm_atomic_state *state = priv->state; + struct drm_modeset_acquire_ctx ctx; + struct drm_atomic_state *state; + struct drm_device *drm; + struct vc4_dev *vc4; unsigned int i; int ret; + drm_modeset_acquire_init(&ctx, 0); + + vc4 = priv->vc4; + drm = &vc4->base; + state = drm_kunit_helper_atomic_state_alloc(test, drm, &ctx); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state); for (i = 0; i < params->nencoders; i++) { enum vc4_encoder_type enc_type = params->encoders[i]; ret = vc4_mock_atomic_add_output(test, state, enc_type); KUNIT_ASSERT_EQ(test, ret, 0); @@ -698,56 +706,61 @@ static void drm_vc4_test_pv_muxing(struct kunit *test) enum vc4_encoder_type enc_type = params->encoders[i]; KUNIT_EXPECT_TRUE(test, check_channel_for_encoder(test, state, enc_type, params->check_fn)); } + + drm_modeset_drop_locks(&ctx); + drm_modeset_acquire_fini(&ctx); } static void drm_vc4_test_pv_muxing_invalid(struct kunit *test) { const struct pv_muxing_param *params = test->param_value; const struct pv_muxing_priv *priv = test->priv; - struct drm_atomic_state *state = priv->state; + struct drm_modeset_acquire_ctx ctx; + struct drm_atomic_state *state; + struct drm_device *drm; + struct vc4_dev *vc4; unsigned int i; int ret; + drm_modeset_acquire_init(&ctx, 0); + + vc4 = priv->vc4; + drm = &vc4->base; + state = drm_kunit_helper_atomic_state_alloc(test, drm, &ctx); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state); + for (i = 0; i < params->nencoders; i++) { enum vc4_encoder_type enc_type = params->encoders[i]; ret = vc4_mock_atomic_add_output(test, state, enc_type); KUNIT_ASSERT_EQ(test, ret, 0); } ret = drm_atomic_check_only(state); KUNIT_EXPECT_LT(test, ret, 0); + + drm_modeset_drop_locks(&ctx); + drm_modeset_acquire_fini(&ctx); } static int vc4_pv_muxing_test_init(struct kunit *test) { const struct pv_muxing_param *params = test->param_value; - struct drm_modeset_acquire_ctx ctx; struct pv_muxing_priv *priv; - struct drm_device *drm; struct vc4_dev *vc4; priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL); KUNIT_ASSERT_NOT_NULL(test, priv); test->priv = priv; vc4 = params->mock_fn(test); KUNIT_ASSERT_NOT_ERR_OR_NULL(test, vc4); priv->vc4 = vc4; - drm_modeset_acquire_init(&ctx, 0); - - drm = &vc4->base; - priv->state = drm_kunit_helper_atomic_state_alloc(test, drm, &ctx); - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->state); - - drm_modeset_drop_locks(&ctx); - drm_modeset_acquire_fini(&ctx); - return 0; } static struct kunit_case vc4_pv_muxing_tests[] = { KUNIT_CASE_PARAM(drm_vc4_test_pv_muxing,
The vc4-pv-muxing-combinations and vc5-pv-muxing-combinations test suites use a common test init function which, in part, allocates the drm atomic state the test will use. That allocation relies on drm_kunit_helper_atomic_state_alloc(), and thus requires a struct drm_modeset_acquire_ctx. This context will then be stored in the allocated state->acquire_ctx field. However, the context is local to the test init function, and is cleared as soon as drm_kunit_helper_atomic_state_alloc() is done. We thus end up with an dangling pointer to a cleared context in state->acquire_ctx for our test to consumes. We should really allocate the context and the state in the test functions, so we can also control when we're done with it. Fixes: 30188df0c387 ("drm/tests: Drop drm_kunit_helper_acquire_ctx_alloc()") Signed-off-by: Maxime Ripard <mripard@kernel.org> --- drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c | 41 +++++++++++++++++--------- 1 file changed, 27 insertions(+), 14 deletions(-)