Re: [PATCH 3/4] drm/vc4: tests: Stop allocating the state in test init

["Maíra Canal" <mcanal@igalia.com>]

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,
>