* [PATCH 0/4] drm/vc4: tests: Fix locking failures
@ 2025-03-18 14:17 Maxime Ripard
2025-03-18 14:17 ` [PATCH 1/4] drm/vc4: tests: Use return instead of assert Maxime Ripard
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Maxime Ripard @ 2025-03-18 14:17 UTC (permalink / raw)
To: Dave Stevenson, Maíra Canal, Raspberry Pi Kernel Maintenance,
Maarten Lankhorst, Thomas Zimmermann, David Airlie, Simona Vetter
Cc: dri-devel, linux-kernel, Maxime Ripard
Hi,
This series deals with (lack of) EDEADLK handling in vc4 PV muxing
tests.
This was leading to failures with CONFIG_DEBUG_WW_MUTEX_SLOWPATH
enabled.
Maxime
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
Maxime Ripard (4):
drm/vc4: tests: Use return instead of assert
drm/vc4: tests: Document output handling functions
drm/vc4: tests: Stop allocating the state in test init
drm/vc4: tests: Retry pv-muxing tests when EDEADLK
drivers/gpu/drm/vc4/tests/vc4_mock_output.c | 62 +++++++++++++++----
drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c | 85 +++++++++++++++++++++-----
2 files changed, 121 insertions(+), 26 deletions(-)
---
base-commit: c0988d693eb10e115d95747f8a0cccc83babb3fc
change-id: 20250318-drm-vc4-kunit-failures-313b4775c438
Best regards,
--
Maxime Ripard <mripard@kernel.org>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/4] drm/vc4: tests: Use return instead of assert
2025-03-18 14:17 [PATCH 0/4] drm/vc4: tests: Fix locking failures Maxime Ripard
@ 2025-03-18 14:17 ` Maxime Ripard
2025-03-23 18:12 ` Maíra Canal
2025-03-18 14:17 ` [PATCH 2/4] drm/vc4: tests: Document output handling functions Maxime Ripard
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Maxime Ripard @ 2025-03-18 14:17 UTC (permalink / raw)
To: Dave Stevenson, Maíra Canal, Raspberry Pi Kernel Maintenance,
Maarten Lankhorst, Thomas Zimmermann, David Airlie, Simona Vetter
Cc: dri-devel, linux-kernel, Maxime Ripard
The vc4_mock_atomic_add_output() and vc4_mock_atomic_del_output() assert
that the functions they are calling didn't fail. Since some of them can
return EDEADLK, we can't properly deal with it.
Since both functions are expected to return an int, and all caller check
the return value, let's just properly propagate the errors when they
occur.
Fixes: f759f5b53f1c ("drm/vc4: tests: Introduce a mocking infrastructure")
Fixes: 76ec18dc5afa ("drm/vc4: tests: Add unit test suite for the PV muxing")
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
drivers/gpu/drm/vc4/tests/vc4_mock_output.c | 36 +++++++++++++++++++----------
1 file changed, 24 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/vc4/tests/vc4_mock_output.c b/drivers/gpu/drm/vc4/tests/vc4_mock_output.c
index e70d7c3076acf168782c48301f3b3dfb9be21f22..f0ddc223c1f839e8a14f37fdcbb72e7b2c836aa1 100644
--- a/drivers/gpu/drm/vc4/tests/vc4_mock_output.c
+++ b/drivers/gpu/drm/vc4/tests/vc4_mock_output.c
@@ -73,28 +73,34 @@ int vc4_mock_atomic_add_output(struct kunit *test,
struct drm_encoder *encoder;
struct drm_crtc *crtc;
int ret;
encoder = vc4_find_encoder_by_type(drm, type);
- KUNIT_ASSERT_NOT_ERR_OR_NULL(test, encoder);
+ if (!encoder)
+ return -ENODEV;
crtc = vc4_find_crtc_for_encoder(test, drm, encoder);
- KUNIT_ASSERT_NOT_ERR_OR_NULL(test, crtc);
+ if (!crtc)
+ return -ENODEV;
output = encoder_to_vc4_dummy_output(encoder);
conn = &output->connector;
conn_state = drm_atomic_get_connector_state(state, conn);
- KUNIT_ASSERT_NOT_ERR_OR_NULL(test, conn_state);
+ if (IS_ERR(conn_state))
+ return PTR_ERR(conn_state);
ret = drm_atomic_set_crtc_for_connector(conn_state, crtc);
- KUNIT_EXPECT_EQ(test, ret, 0);
+ if (ret)
+ return ret;
crtc_state = drm_atomic_get_crtc_state(state, crtc);
- KUNIT_ASSERT_NOT_ERR_OR_NULL(test, crtc_state);
+ if (IS_ERR(crtc_state))
+ return PTR_ERR(crtc_state);
ret = drm_atomic_set_mode_for_crtc(crtc_state, &default_mode);
- KUNIT_EXPECT_EQ(test, ret, 0);
+ if (ret)
+ return ret;
crtc_state->active = true;
return 0;
}
@@ -111,28 +117,34 @@ int vc4_mock_atomic_del_output(struct kunit *test,
struct drm_encoder *encoder;
struct drm_crtc *crtc;
int ret;
encoder = vc4_find_encoder_by_type(drm, type);
- KUNIT_ASSERT_NOT_ERR_OR_NULL(test, encoder);
+ if (!encoder)
+ return -ENODEV;
crtc = vc4_find_crtc_for_encoder(test, drm, encoder);
- KUNIT_ASSERT_NOT_ERR_OR_NULL(test, crtc);
+ if (!crtc)
+ return -ENODEV;
crtc_state = drm_atomic_get_crtc_state(state, crtc);
- KUNIT_ASSERT_NOT_ERR_OR_NULL(test, crtc_state);
+ if (IS_ERR(crtc_state))
+ return PTR_ERR(crtc_state);
crtc_state->active = false;
ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
- KUNIT_ASSERT_EQ(test, ret, 0);
+ if (ret)
+ return ret;
output = encoder_to_vc4_dummy_output(encoder);
conn = &output->connector;
conn_state = drm_atomic_get_connector_state(state, conn);
- KUNIT_ASSERT_NOT_ERR_OR_NULL(test, conn_state);
+ if (IS_ERR(conn_state))
+ return PTR_ERR(conn_state);
ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
- KUNIT_ASSERT_EQ(test, ret, 0);
+ if (ret)
+ return ret;
return 0;
}
--
2.48.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/4] drm/vc4: tests: Document output handling functions
2025-03-18 14:17 [PATCH 0/4] drm/vc4: tests: Fix locking failures Maxime Ripard
2025-03-18 14:17 ` [PATCH 1/4] drm/vc4: tests: Use return instead of assert Maxime Ripard
@ 2025-03-18 14:17 ` Maxime Ripard
2025-03-23 18:34 ` Maíra Canal
2025-03-18 14:17 ` [PATCH 3/4] drm/vc4: tests: Stop allocating the state in test init Maxime Ripard
2025-03-18 14:17 ` [PATCH 4/4] drm/vc4: tests: Retry pv-muxing tests when EDEADLK Maxime Ripard
3 siblings, 1 reply; 12+ messages in thread
From: Maxime Ripard @ 2025-03-18 14:17 UTC (permalink / raw)
To: Dave Stevenson, Maíra Canal, Raspberry Pi Kernel Maintenance,
Maarten Lankhorst, Thomas Zimmermann, David Airlie, Simona Vetter
Cc: dri-devel, linux-kernel, Maxime Ripard
vc4_mock_atomic_add_output() and vc4_mock_atomic_del_output() public but
aren't documented. Let's provide the documentation.
In particular, special care should be taken to deal with EDEADLK.
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
drivers/gpu/drm/vc4/tests/vc4_mock_output.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/drivers/gpu/drm/vc4/tests/vc4_mock_output.c b/drivers/gpu/drm/vc4/tests/vc4_mock_output.c
index f0ddc223c1f839e8a14f37fdcbb72e7b2c836aa1..577d9a9563696791632aec614c381a214886bf27 100644
--- a/drivers/gpu/drm/vc4/tests/vc4_mock_output.c
+++ b/drivers/gpu/drm/vc4/tests/vc4_mock_output.c
@@ -59,10 +59,23 @@ struct vc4_dummy_output *vc4_dummy_output(struct kunit *test,
static const struct drm_display_mode default_mode = {
DRM_SIMPLE_MODE(640, 480, 64, 48)
};
+/**
+ * vc4_mock_atomic_add_output() - Enables an output in a state
+ * @test: The test context object
+ * @state: Atomic state to enable the output in.
+ * @type: Type of the output encoder
+ *
+ * Adds an output CRTC and connector to a state, and enables them.
+ *
+ * Returns:
+ * 0 on success, a negative error code on failure. If the error is
+ * EDEADLK, the entire atomic sequence must be restarted. All other
+ * errors are fatal.
+ */
int vc4_mock_atomic_add_output(struct kunit *test,
struct drm_atomic_state *state,
enum vc4_encoder_type type)
{
struct drm_device *drm = state->dev;
@@ -103,10 +116,23 @@ int vc4_mock_atomic_add_output(struct kunit *test,
crtc_state->active = true;
return 0;
}
+/**
+ * vc4_mock_atomic_del_output() - Disables an output in a state
+ * @test: The test context object
+ * @state: Atomic state to disable the output in.
+ * @type: Type of the output encoder
+ *
+ * Adds an output CRTC and connector to a state, and disables them.
+ *
+ * Returns:
+ * 0 on success, a negative error code on failure. If the error is
+ * EDEADLK, the entire atomic sequence must be restarted. All other
+ * errors are fatal.
+ */
int vc4_mock_atomic_del_output(struct kunit *test,
struct drm_atomic_state *state,
enum vc4_encoder_type type)
{
struct drm_device *drm = state->dev;
--
2.48.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/4] drm/vc4: tests: Stop allocating the state in test init
2025-03-18 14:17 [PATCH 0/4] drm/vc4: tests: Fix locking failures Maxime Ripard
2025-03-18 14:17 ` [PATCH 1/4] drm/vc4: tests: Use return instead of assert Maxime Ripard
2025-03-18 14:17 ` [PATCH 2/4] drm/vc4: tests: Document output handling functions Maxime Ripard
@ 2025-03-18 14:17 ` Maxime Ripard
2025-03-23 18:48 ` Maíra Canal
2025-03-18 14:17 ` [PATCH 4/4] drm/vc4: tests: Retry pv-muxing tests when EDEADLK Maxime Ripard
3 siblings, 1 reply; 12+ messages in thread
From: Maxime Ripard @ 2025-03-18 14:17 UTC (permalink / raw)
To: Dave Stevenson, Maíra Canal, Raspberry Pi Kernel Maintenance,
Maarten Lankhorst, Thomas Zimmermann, David Airlie, Simona Vetter
Cc: dri-devel, linux-kernel, Maxime Ripard
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.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 4/4] drm/vc4: tests: Retry pv-muxing tests when EDEADLK
2025-03-18 14:17 [PATCH 0/4] drm/vc4: tests: Fix locking failures Maxime Ripard
` (2 preceding siblings ...)
2025-03-18 14:17 ` [PATCH 3/4] drm/vc4: tests: Stop allocating the state in test init Maxime Ripard
@ 2025-03-18 14:17 ` Maxime Ripard
2025-03-23 18:07 ` Maíra Canal
3 siblings, 1 reply; 12+ messages in thread
From: Maxime Ripard @ 2025-03-18 14:17 UTC (permalink / raw)
To: Dave Stevenson, Maíra Canal, Raspberry Pi Kernel Maintenance,
Maarten Lankhorst, Thomas Zimmermann, David Airlie, Simona Vetter
Cc: dri-devel, linux-kernel, Maxime Ripard
Some functions used by the HVS->PV muxing tests can return with EDEADLK,
meaning the entire sequence should be restarted. It's not a fatal error
and we should treat it as a recoverable error, and recover, instead of
failing the test like we currently do.
Fixes: 76ec18dc5afa ("drm/vc4: tests: Add unit test suite for the PV muxing")
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c | 44 ++++++++++++++++++++++++++
1 file changed, 44 insertions(+)
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 52c04ef33206bf4f9e21e3c8b7cea932824a67fa..94e05bddb630a79aab189d9bc16f09a9d84ce396 100644
--- a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
+++ b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
@@ -685,20 +685,26 @@ static void drm_vc4_test_pv_muxing(struct kunit *test)
drm_modeset_acquire_init(&ctx, 0);
vc4 = priv->vc4;
drm = &vc4->base;
+
+retry:
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);
+ if (ret == -EDEADLK)
+ goto retry;
KUNIT_ASSERT_EQ(test, ret, 0);
}
ret = drm_atomic_check_only(state);
+ if (ret == -EDEADLK)
+ goto retry;
KUNIT_EXPECT_EQ(test, ret, 0);
KUNIT_EXPECT_TRUE(test,
check_fifo_conflict(test, state));
@@ -726,21 +732,27 @@ static void drm_vc4_test_pv_muxing_invalid(struct kunit *test)
drm_modeset_acquire_init(&ctx, 0);
vc4 = priv->vc4;
drm = &vc4->base;
+
+retry:
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);
+ if (ret == -EDEADLK)
+ goto retry;
KUNIT_ASSERT_EQ(test, ret, 0);
}
ret = drm_atomic_check_only(state);
+ if (ret == -EDEADLK)
+ goto retry;
KUNIT_EXPECT_LT(test, ret, 0);
drm_modeset_drop_locks(&ctx);
drm_modeset_acquire_fini(&ctx);
}
@@ -811,17 +823,22 @@ static void drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable(struct kunit *tes
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, vc4);
drm_modeset_acquire_init(&ctx, 0);
drm = &vc4->base;
+retry_first:
state = drm_kunit_helper_atomic_state_alloc(test, drm, &ctx);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
ret = vc4_mock_atomic_add_output(test, state, VC4_ENCODER_TYPE_HDMI0);
+ if (ret == -EDEADLK)
+ goto retry_first;
KUNIT_ASSERT_EQ(test, ret, 0);
ret = drm_atomic_check_only(state);
+ if (ret == -EDEADLK)
+ goto retry_first;
KUNIT_ASSERT_EQ(test, ret, 0);
new_hvs_state = vc4_hvs_get_new_global_state(state);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_hvs_state);
@@ -834,17 +851,22 @@ static void drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable(struct kunit *tes
KUNIT_ASSERT_TRUE(test, new_hvs_state->fifo_state[hdmi0_channel].in_use);
ret = drm_atomic_helper_swap_state(state, false);
KUNIT_ASSERT_EQ(test, ret, 0);
+retry_second:
state = drm_kunit_helper_atomic_state_alloc(test, drm, &ctx);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
ret = vc4_mock_atomic_add_output(test, state, VC4_ENCODER_TYPE_HDMI1);
+ if (ret == -EDEADLK)
+ goto retry_second;
KUNIT_ASSERT_EQ(test, ret, 0);
ret = drm_atomic_check_only(state);
+ if (ret == -EDEADLK)
+ goto retry_second;
KUNIT_ASSERT_EQ(test, ret, 0);
new_hvs_state = vc4_hvs_get_new_global_state(state);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_hvs_state);
@@ -885,20 +907,27 @@ static void drm_test_vc5_pv_muxing_bugs_stable_fifo(struct kunit *test)
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, vc4);
drm_modeset_acquire_init(&ctx, 0);
drm = &vc4->base;
+retry_first:
state = drm_kunit_helper_atomic_state_alloc(test, drm, &ctx);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
ret = vc4_mock_atomic_add_output(test, state, VC4_ENCODER_TYPE_HDMI0);
+ if (ret == -EDEADLK)
+ goto retry_first;
KUNIT_ASSERT_EQ(test, ret, 0);
ret = vc4_mock_atomic_add_output(test, state, VC4_ENCODER_TYPE_HDMI1);
+ if (ret == -EDEADLK)
+ goto retry_first;
KUNIT_ASSERT_EQ(test, ret, 0);
ret = drm_atomic_check_only(state);
+ if (ret == -EDEADLK)
+ goto retry_first;
KUNIT_ASSERT_EQ(test, ret, 0);
new_hvs_state = vc4_hvs_get_new_global_state(state);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_hvs_state);
@@ -919,17 +948,22 @@ static void drm_test_vc5_pv_muxing_bugs_stable_fifo(struct kunit *test)
KUNIT_ASSERT_TRUE(test, new_hvs_state->fifo_state[old_hdmi1_channel].in_use);
ret = drm_atomic_helper_swap_state(state, false);
KUNIT_ASSERT_EQ(test, ret, 0);
+retry_second:
state = drm_kunit_helper_atomic_state_alloc(test, drm, &ctx);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
ret = vc4_mock_atomic_del_output(test, state, VC4_ENCODER_TYPE_HDMI0);
+ if (ret == -EDEADLK)
+ goto retry_second;
KUNIT_ASSERT_EQ(test, ret, 0);
ret = drm_atomic_check_only(state);
+ if (ret == -EDEADLK)
+ goto retry_second;
KUNIT_ASSERT_EQ(test, ret, 0);
new_hvs_state = vc4_hvs_get_new_global_state(state);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_hvs_state);
@@ -979,29 +1013,39 @@ drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable_too_many_crtc_state(struct ku
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, vc4);
drm_modeset_acquire_init(&ctx, 0);
drm = &vc4->base;
+retry_first:
state = drm_kunit_helper_atomic_state_alloc(test, drm, &ctx);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
ret = vc4_mock_atomic_add_output(test, state, VC4_ENCODER_TYPE_HDMI0);
+ if (ret == -EDEADLK)
+ goto retry_first;
KUNIT_ASSERT_EQ(test, ret, 0);
ret = drm_atomic_check_only(state);
+ if (ret == -EDEADLK)
+ goto retry_first;
KUNIT_ASSERT_EQ(test, ret, 0);
ret = drm_atomic_helper_swap_state(state, false);
KUNIT_ASSERT_EQ(test, ret, 0);
+retry_second:
state = drm_kunit_helper_atomic_state_alloc(test, drm, &ctx);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
ret = vc4_mock_atomic_add_output(test, state, VC4_ENCODER_TYPE_HDMI1);
+ if (ret == -EDEADLK)
+ goto retry_second;
KUNIT_ASSERT_EQ(test, ret, 0);
ret = drm_atomic_check_only(state);
+ if (ret == -EDEADLK)
+ goto retry_second;
KUNIT_ASSERT_EQ(test, ret, 0);
new_vc4_crtc_state = get_vc4_crtc_state_for_encoder(test, state,
VC4_ENCODER_TYPE_HDMI0);
KUNIT_EXPECT_NULL(test, new_vc4_crtc_state);
--
2.48.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 4/4] drm/vc4: tests: Retry pv-muxing tests when EDEADLK
2025-03-18 14:17 ` [PATCH 4/4] drm/vc4: tests: Retry pv-muxing tests when EDEADLK Maxime Ripard
@ 2025-03-23 18:07 ` Maíra Canal
0 siblings, 0 replies; 12+ messages in thread
From: Maíra Canal @ 2025-03-23 18:07 UTC (permalink / raw)
To: Maxime Ripard, Dave Stevenson, Raspberry Pi Kernel Maintenance,
Maarten Lankhorst, Thomas Zimmermann, David Airlie, Simona Vetter
Cc: dri-devel, linux-kernel
Hi Maxime,
On 18/03/25 11:17, Maxime Ripard wrote:
> Some functions used by the HVS->PV muxing tests can return with EDEADLK,
> meaning the entire sequence should be restarted. It's not a fatal error
> and we should treat it as a recoverable error, and recover, instead of
> failing the test like we currently do.
>
> Fixes: 76ec18dc5afa ("drm/vc4: tests: Add unit test suite for the PV muxing")
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
> drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c | 44 ++++++++++++++++++++++++++
> 1 file changed, 44 insertions(+)
>
> 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 52c04ef33206bf4f9e21e3c8b7cea932824a67fa..94e05bddb630a79aab189d9bc16f09a9d84ce396 100644
> --- a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
> +++ b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
> @@ -685,20 +685,26 @@ static void drm_vc4_test_pv_muxing(struct kunit *test)
>
> drm_modeset_acquire_init(&ctx, 0);
>
> vc4 = priv->vc4;
> drm = &vc4->base;
> +
> +retry:
> 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);
> + if (ret == -EDEADLK)
> + goto retry;
> KUNIT_ASSERT_EQ(test, ret, 0);
> }
>
> ret = drm_atomic_check_only(state);
> + if (ret == -EDEADLK)
> + goto retry;
Shouldn't we call `drm_modeset_backoff()` before retrying (maybe
`drm_atomic_state_clear()` as well)?
Best Regards,
- Maíra
> KUNIT_EXPECT_EQ(test, ret, 0);
>
> KUNIT_EXPECT_TRUE(test,
> check_fifo_conflict(test, state));
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/4] drm/vc4: tests: Use return instead of assert
2025-03-18 14:17 ` [PATCH 1/4] drm/vc4: tests: Use return instead of assert Maxime Ripard
@ 2025-03-23 18:12 ` Maíra Canal
0 siblings, 0 replies; 12+ messages in thread
From: Maíra Canal @ 2025-03-23 18:12 UTC (permalink / raw)
To: Maxime Ripard, Dave Stevenson, Raspberry Pi Kernel Maintenance,
Maarten Lankhorst, Thomas Zimmermann, David Airlie, Simona Vetter
Cc: dri-devel, linux-kernel
Hi Maxime,
On 18/03/25 11:17, Maxime Ripard wrote:
> The vc4_mock_atomic_add_output() and vc4_mock_atomic_del_output() assert
> that the functions they are calling didn't fail. Since some of them can
> return EDEADLK, we can't properly deal with it.
>
> Since both functions are expected to return an int, and all caller check
> the return value, let's just properly propagate the errors when they
> occur.
>
> Fixes: f759f5b53f1c ("drm/vc4: tests: Introduce a mocking infrastructure")
> Fixes: 76ec18dc5afa ("drm/vc4: tests: Add unit test suite for the PV muxing")
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
Reviewed-by: Maíra Canal <mcanal@igalia.com>
Best Regards,
- Maíra
> ---
> drivers/gpu/drm/vc4/tests/vc4_mock_output.c | 36 +++++++++++++++++++----------
> 1 file changed, 24 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/tests/vc4_mock_output.c b/drivers/gpu/drm/vc4/tests/vc4_mock_output.c
> index e70d7c3076acf168782c48301f3b3dfb9be21f22..f0ddc223c1f839e8a14f37fdcbb72e7b2c836aa1 100644
> --- a/drivers/gpu/drm/vc4/tests/vc4_mock_output.c
> +++ b/drivers/gpu/drm/vc4/tests/vc4_mock_output.c
> @@ -73,28 +73,34 @@ int vc4_mock_atomic_add_output(struct kunit *test,
> struct drm_encoder *encoder;
> struct drm_crtc *crtc;
> int ret;
>
> encoder = vc4_find_encoder_by_type(drm, type);
> - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, encoder);
> + if (!encoder)
> + return -ENODEV;
>
> crtc = vc4_find_crtc_for_encoder(test, drm, encoder);
> - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, crtc);
> + if (!crtc)
> + return -ENODEV;
>
> output = encoder_to_vc4_dummy_output(encoder);
> conn = &output->connector;
> conn_state = drm_atomic_get_connector_state(state, conn);
> - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, conn_state);
> + if (IS_ERR(conn_state))
> + return PTR_ERR(conn_state);
>
> ret = drm_atomic_set_crtc_for_connector(conn_state, crtc);
> - KUNIT_EXPECT_EQ(test, ret, 0);
> + if (ret)
> + return ret;
>
> crtc_state = drm_atomic_get_crtc_state(state, crtc);
> - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, crtc_state);
> + if (IS_ERR(crtc_state))
> + return PTR_ERR(crtc_state);
>
> ret = drm_atomic_set_mode_for_crtc(crtc_state, &default_mode);
> - KUNIT_EXPECT_EQ(test, ret, 0);
> + if (ret)
> + return ret;
>
> crtc_state->active = true;
>
> return 0;
> }
> @@ -111,28 +117,34 @@ int vc4_mock_atomic_del_output(struct kunit *test,
> struct drm_encoder *encoder;
> struct drm_crtc *crtc;
> int ret;
>
> encoder = vc4_find_encoder_by_type(drm, type);
> - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, encoder);
> + if (!encoder)
> + return -ENODEV;
>
> crtc = vc4_find_crtc_for_encoder(test, drm, encoder);
> - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, crtc);
> + if (!crtc)
> + return -ENODEV;
>
> crtc_state = drm_atomic_get_crtc_state(state, crtc);
> - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, crtc_state);
> + if (IS_ERR(crtc_state))
> + return PTR_ERR(crtc_state);
>
> crtc_state->active = false;
>
> ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
> - KUNIT_ASSERT_EQ(test, ret, 0);
> + if (ret)
> + return ret;
>
> output = encoder_to_vc4_dummy_output(encoder);
> conn = &output->connector;
> conn_state = drm_atomic_get_connector_state(state, conn);
> - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, conn_state);
> + if (IS_ERR(conn_state))
> + return PTR_ERR(conn_state);
>
> ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
> - KUNIT_ASSERT_EQ(test, ret, 0);
> + if (ret)
> + return ret;
>
> return 0;
> }
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/4] drm/vc4: tests: Document output handling functions
2025-03-18 14:17 ` [PATCH 2/4] drm/vc4: tests: Document output handling functions Maxime Ripard
@ 2025-03-23 18:34 ` Maíra Canal
2025-03-28 15:30 ` Maxime Ripard
0 siblings, 1 reply; 12+ messages in thread
From: Maíra Canal @ 2025-03-23 18:34 UTC (permalink / raw)
To: Maxime Ripard, Dave Stevenson, Raspberry Pi Kernel Maintenance,
Maarten Lankhorst, Thomas Zimmermann, David Airlie, Simona Vetter
Cc: dri-devel, linux-kernel
Hi Maxime,
On 18/03/25 11:17, Maxime Ripard wrote:
> vc4_mock_atomic_add_output() and vc4_mock_atomic_del_output() public but
Nit: s/public/are public
> aren't documented. Let's provide the documentation.
>
> In particular, special care should be taken to deal with EDEADLK.
>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
> drivers/gpu/drm/vc4/tests/vc4_mock_output.c | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/drivers/gpu/drm/vc4/tests/vc4_mock_output.c b/drivers/gpu/drm/vc4/tests/vc4_mock_output.c
> index f0ddc223c1f839e8a14f37fdcbb72e7b2c836aa1..577d9a9563696791632aec614c381a214886bf27 100644
> --- a/drivers/gpu/drm/vc4/tests/vc4_mock_output.c
> +++ b/drivers/gpu/drm/vc4/tests/vc4_mock_output.c
> @@ -59,10 +59,23 @@ struct vc4_dummy_output *vc4_dummy_output(struct kunit *test,
>
> static const struct drm_display_mode default_mode = {
> DRM_SIMPLE_MODE(640, 480, 64, 48)
> };
>
> +/**
> + * vc4_mock_atomic_add_output() - Enables an output in a state
> + * @test: The test context object
> + * @state: Atomic state to enable the output in.
> + * @type: Type of the output encoder
> + *
> + * Adds an output CRTC and connector to a state, and enables them.
> + *
> + * Returns:
> + * 0 on success, a negative error code on failure. If the error is
> + * EDEADLK, the entire atomic sequence must be restarted. All other
> + * errors are fatal.
> + */
> int vc4_mock_atomic_add_output(struct kunit *test,
> struct drm_atomic_state *state,
> enum vc4_encoder_type type)
> {
> struct drm_device *drm = state->dev;
> @@ -103,10 +116,23 @@ int vc4_mock_atomic_add_output(struct kunit *test,
> crtc_state->active = true;
>
> return 0;
> }
>
> +/**
> + * vc4_mock_atomic_del_output() - Disables an output in a state
> + * @test: The test context object
> + * @state: Atomic state to disable the output in.
> + * @type: Type of the output encoder
> + *
> + * Adds an output CRTC and connector to a state, and disables them.
I'm not sure if "Adds" properly expresses what the function does. How
about "Finds an output CRTC and connector in a state, and disables
them"?
Best Regards,
- Maíra
> + *
> + * Returns:
> + * 0 on success, a negative error code on failure. If the error is
> + * EDEADLK, the entire atomic sequence must be restarted. All other
> + * errors are fatal.
> + */
> int vc4_mock_atomic_del_output(struct kunit *test,
> struct drm_atomic_state *state,
> enum vc4_encoder_type type)
> {
> struct drm_device *drm = state->dev;
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/4] drm/vc4: tests: Stop allocating the state in test init
2025-03-18 14:17 ` [PATCH 3/4] drm/vc4: tests: Stop allocating the state in test init Maxime Ripard
@ 2025-03-23 18:48 ` Maíra Canal
2025-03-28 15:32 ` Maxime Ripard
0 siblings, 1 reply; 12+ messages in thread
From: Maíra Canal @ 2025-03-23 18:48 UTC (permalink / raw)
To: Maxime Ripard, Dave Stevenson, Raspberry Pi Kernel Maintenance,
Maarten Lankhorst, Thomas Zimmermann, David Airlie, Simona Vetter
Cc: dri-devel, linux-kernel
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,
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/4] drm/vc4: tests: Document output handling functions
2025-03-23 18:34 ` Maíra Canal
@ 2025-03-28 15:30 ` Maxime Ripard
0 siblings, 0 replies; 12+ messages in thread
From: Maxime Ripard @ 2025-03-28 15:30 UTC (permalink / raw)
To: Maíra Canal
Cc: Dave Stevenson, Raspberry Pi Kernel Maintenance,
Maarten Lankhorst, Thomas Zimmermann, David Airlie, Simona Vetter,
dri-devel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2733 bytes --]
On Sun, Mar 23, 2025 at 03:34:50PM -0300, Maíra Canal wrote:
> Hi Maxime,
>
> On 18/03/25 11:17, Maxime Ripard wrote:
> > vc4_mock_atomic_add_output() and vc4_mock_atomic_del_output() public but
>
> Nit: s/public/are public
Good catch, thanks!
> > aren't documented. Let's provide the documentation.
> >
> > In particular, special care should be taken to deal with EDEADLK.
> >
> > Signed-off-by: Maxime Ripard <mripard@kernel.org>
> > ---
> > drivers/gpu/drm/vc4/tests/vc4_mock_output.c | 26 ++++++++++++++++++++++++++
> > 1 file changed, 26 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/vc4/tests/vc4_mock_output.c b/drivers/gpu/drm/vc4/tests/vc4_mock_output.c
> > index f0ddc223c1f839e8a14f37fdcbb72e7b2c836aa1..577d9a9563696791632aec614c381a214886bf27 100644
> > --- a/drivers/gpu/drm/vc4/tests/vc4_mock_output.c
> > +++ b/drivers/gpu/drm/vc4/tests/vc4_mock_output.c
> > @@ -59,10 +59,23 @@ struct vc4_dummy_output *vc4_dummy_output(struct kunit *test,
> > static const struct drm_display_mode default_mode = {
> > DRM_SIMPLE_MODE(640, 480, 64, 48)
> > };
> > +/**
> > + * vc4_mock_atomic_add_output() - Enables an output in a state
> > + * @test: The test context object
> > + * @state: Atomic state to enable the output in.
> > + * @type: Type of the output encoder
> > + *
> > + * Adds an output CRTC and connector to a state, and enables them.
> > + *
> > + * Returns:
> > + * 0 on success, a negative error code on failure. If the error is
> > + * EDEADLK, the entire atomic sequence must be restarted. All other
> > + * errors are fatal.
> > + */
> > int vc4_mock_atomic_add_output(struct kunit *test,
> > struct drm_atomic_state *state,
> > enum vc4_encoder_type type)
> > {
> > struct drm_device *drm = state->dev;
> > @@ -103,10 +116,23 @@ int vc4_mock_atomic_add_output(struct kunit *test,
> > crtc_state->active = true;
> > return 0;
> > }
> > +/**
> > + * vc4_mock_atomic_del_output() - Disables an output in a state
> > + * @test: The test context object
> > + * @state: Atomic state to disable the output in.
> > + * @type: Type of the output encoder
> > + *
> > + * Adds an output CRTC and connector to a state, and disables them.
>
> I'm not sure if "Adds" properly expresses what the function does. How
> about "Finds an output CRTC and connector in a state, and disables
> them"?
Yeah, I see what you mean, but it doesn't really work either. The state
is empty before, we do try to find them to add a new state for them in
the global one, and that new state will disable them.
So we can't find them in that state prior to adding them, which is what
this function does.
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/4] drm/vc4: tests: Stop allocating the state in test init
2025-03-23 18:48 ` Maíra Canal
@ 2025-03-28 15:32 ` Maxime Ripard
2025-03-30 18:10 ` Maíra Canal
0 siblings, 1 reply; 12+ messages in thread
From: Maxime Ripard @ 2025-03-28 15:32 UTC (permalink / raw)
To: Maíra Canal
Cc: Dave Stevenson, Raspberry Pi Kernel Maintenance,
Maarten Lankhorst, Thomas Zimmermann, David Airlie, Simona Vetter,
dri-devel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2202 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/4] drm/vc4: tests: Stop allocating the state in test init
2025-03-28 15:32 ` Maxime Ripard
@ 2025-03-30 18:10 ` Maíra Canal
0 siblings, 0 replies; 12+ messages in thread
From: Maíra Canal @ 2025-03-30 18:10 UTC (permalink / raw)
To: Maxime Ripard
Cc: Dave Stevenson, Raspberry Pi Kernel Maintenance,
Maarten Lankhorst, Thomas Zimmermann, David Airlie, Simona Vetter,
dri-devel, linux-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2025-03-30 18:10 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-18 14:17 [PATCH 0/4] drm/vc4: tests: Fix locking failures Maxime Ripard
2025-03-18 14:17 ` [PATCH 1/4] drm/vc4: tests: Use return instead of assert Maxime Ripard
2025-03-23 18:12 ` Maíra Canal
2025-03-18 14:17 ` [PATCH 2/4] drm/vc4: tests: Document output handling functions Maxime Ripard
2025-03-23 18:34 ` Maíra Canal
2025-03-28 15:30 ` Maxime Ripard
2025-03-18 14:17 ` [PATCH 3/4] drm/vc4: tests: Stop allocating the state in test init Maxime Ripard
2025-03-23 18:48 ` Maíra Canal
2025-03-28 15:32 ` Maxime Ripard
2025-03-30 18:10 ` Maíra Canal
2025-03-18 14:17 ` [PATCH 4/4] drm/vc4: tests: Retry pv-muxing tests when EDEADLK Maxime Ripard
2025-03-23 18:07 ` Maíra Canal
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).