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;
};
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,
--
2.48.1Hi 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
© 2016 - 2025 Red Hat, Inc.