[PATCH 00/11] drm: kunit: Switch to kunit actions

[PATCH 00/11] drm: kunit: Switch to kunit actions

[Maxime Ripard]

Hi,

Since v6.5-rc1, kunit gained a devm/drmm-like mechanism that makes tests
resources much easier to cleanup.

This series converts the existing tests to use those new actions were
relevant.

Let me know what you think,
Maxime

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
Maxime Ripard (11):
      drm/tests: helpers: Switch to kunit actions
      drm/tests: client-modeset: Remove call to drm_kunit_helper_free_device()
      drm/tests: modes: Remove call to drm_kunit_helper_free_device()
      drm/tests: probe-helper: Remove call to drm_kunit_helper_free_device()
      drm/tests: helpers: Create an helper to allocate a locking ctx
      drm/tests: helpers: Create an helper to allocate an atomic state
      drm/vc4: tests: pv-muxing: Remove call to drm_kunit_helper_free_device()
      drm/vc4: tests: mock: Use a kunit action to unregister DRM device
      drm/vc4: tests: pv-muxing: Switch to managed locking init
      drm/vc4: tests: Switch to atomic state allocation helper
      drm/vc4: tests: pv-muxing: Document test scenario

 drivers/gpu/drm/tests/drm_client_modeset_test.c |   8 --
 drivers/gpu/drm/tests/drm_kunit_helpers.c       | 112 ++++++++++++++++++++++-
 drivers/gpu/drm/tests/drm_modes_test.c          |   8 --
 drivers/gpu/drm/tests/drm_probe_helper_test.c   |   8 --
 drivers/gpu/drm/vc4/tests/vc4_mock.c            |   5 ++
 drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c  | 115 +++++++++---------------
 include/drm/drm_kunit_helpers.h                 |   7 ++
 7 files changed, 162 insertions(+), 101 deletions(-)
---
base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
change-id: 20230710-kms-kunit-actions-rework-5d163762c93b

Best regards,
-- 
Maxime Ripard <mripard@kernel.org>

[PATCH 01/11] drm/tests: helpers: Switch to kunit actions

[Maxime Ripard]

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/tests/drm_kunit_helpers.c | 32 +++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c b/drivers/gpu/drm/tests/drm_kunit_helpers.c
index 4df47071dc88..38211fea9ae6 100644
--- a/drivers/gpu/drm/tests/drm_kunit_helpers.c
+++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c
@@ -35,8 +35,8 @@ static struct platform_driver fake_platform_driver = {
  * able to leverage the usual infrastructure and most notably the
  * device-managed resources just like a "real" device.
  *
- * Callers need to make sure drm_kunit_helper_free_device() on the
- * device when done.
+ * Resources will be cleaned up automatically, but the removal can be
+ * forced using @drm_kunit_helper_free_device.
  *
  * Returns:
  * A pointer to the new device, or an ERR_PTR() otherwise.
@@ -49,12 +49,31 @@ struct device *drm_kunit_helper_alloc_device(struct kunit *test)
 	ret = platform_driver_register(&fake_platform_driver);
 	KUNIT_ASSERT_EQ(test, ret, 0);
 
+	ret = kunit_add_action_or_reset(test,
+					(kunit_action_t *)platform_driver_unregister,
+					&fake_platform_driver);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
 	pdev = platform_device_alloc(KUNIT_DEVICE_NAME, PLATFORM_DEVID_NONE);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
 
+	ret = kunit_add_action_or_reset(test,
+					(kunit_action_t *)platform_device_put,
+					pdev);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
 	ret = platform_device_add(pdev);
 	KUNIT_ASSERT_EQ(test, ret, 0);
 
+	kunit_remove_action(test,
+			    (kunit_action_t *)platform_device_put,
+			    pdev);
+
+	ret = kunit_add_action_or_reset(test,
+					(kunit_action_t *)platform_device_unregister,
+					pdev);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
 	return &pdev->dev;
 }
 EXPORT_SYMBOL_GPL(drm_kunit_helper_alloc_device);
@@ -70,8 +89,13 @@ void drm_kunit_helper_free_device(struct kunit *test, struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 
-	platform_device_unregister(pdev);
-	platform_driver_unregister(&fake_platform_driver);
+	kunit_release_action(test,
+			     (kunit_action_t *)platform_device_unregister,
+			     pdev);
+
+	kunit_release_action(test,
+			     (kunit_action_t *)platform_driver_unregister,
+			     &fake_platform_driver);
 }
 EXPORT_SYMBOL_GPL(drm_kunit_helper_free_device);
 

-- 
2.41.0

[PATCH 02/11] drm/tests: client-modeset: Remove call to drm_kunit_helper_free_device()

[Maxime Ripard]

Calling drm_kunit_helper_free_device() to clean up the resources
allocated by drm_kunit_helper_alloc_device() is now optional and not
needed in most cases.

Remove it.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/tests/drm_client_modeset_test.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/gpu/drm/tests/drm_client_modeset_test.c b/drivers/gpu/drm/tests/drm_client_modeset_test.c
index 416a279b6dae..7516f6cb36e4 100644
--- a/drivers/gpu/drm/tests/drm_client_modeset_test.c
+++ b/drivers/gpu/drm/tests/drm_client_modeset_test.c
@@ -82,13 +82,6 @@ static int drm_client_modeset_test_init(struct kunit *test)
 	return 0;
 }
 
-static void drm_client_modeset_test_exit(struct kunit *test)
-{
-	struct drm_client_modeset_test_priv *priv = test->priv;
-
-	drm_kunit_helper_free_device(test, priv->dev);
-}
-
 static void drm_test_pick_cmdline_res_1920_1080_60(struct kunit *test)
 {
 	struct drm_client_modeset_test_priv *priv = test->priv;
@@ -188,7 +181,6 @@ static struct kunit_case drm_test_pick_cmdline_tests[] = {
 static struct kunit_suite drm_test_pick_cmdline_test_suite = {
 	.name = "drm_test_pick_cmdline",
 	.init = drm_client_modeset_test_init,
-	.exit = drm_client_modeset_test_exit,
 	.test_cases = drm_test_pick_cmdline_tests
 };
 

-- 
2.41.0

[PATCH 03/11] drm/tests: modes: Remove call to drm_kunit_helper_free_device()

[Maxime Ripard]

Calling drm_kunit_helper_free_device() to clean up the resources
allocated by drm_kunit_helper_alloc_device() is now optional and not
needed in most cases.

Remove it.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/tests/drm_modes_test.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/gpu/drm/tests/drm_modes_test.c b/drivers/gpu/drm/tests/drm_modes_test.c
index bc4aa2ce78be..1e9f63fbfead 100644
--- a/drivers/gpu/drm/tests/drm_modes_test.c
+++ b/drivers/gpu/drm/tests/drm_modes_test.c
@@ -36,13 +36,6 @@ static int drm_test_modes_init(struct kunit *test)
 	return 0;
 }
 
-static void drm_test_modes_exit(struct kunit *test)
-{
-	struct drm_test_modes_priv *priv = test->priv;
-
-	drm_kunit_helper_free_device(test, priv->dev);
-}
-
 static void drm_test_modes_analog_tv_ntsc_480i(struct kunit *test)
 {
 	struct drm_test_modes_priv *priv = test->priv;
@@ -148,7 +141,6 @@ static struct kunit_case drm_modes_analog_tv_tests[] = {
 static struct kunit_suite drm_modes_analog_tv_test_suite = {
 	.name = "drm_modes_analog_tv",
 	.init = drm_test_modes_init,
-	.exit = drm_test_modes_exit,
 	.test_cases = drm_modes_analog_tv_tests,
 };
 

-- 
2.41.0

[PATCH 04/11] drm/tests: probe-helper: Remove call to drm_kunit_helper_free_device()

[Maxime Ripard]

Calling drm_kunit_helper_free_device() to clean up the resources
allocated by drm_kunit_helper_alloc_device() is now optional and not
needed in most cases.

Remove it.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/tests/drm_probe_helper_test.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/gpu/drm/tests/drm_probe_helper_test.c b/drivers/gpu/drm/tests/drm_probe_helper_test.c
index 0ee65828623e..1a2044070a6c 100644
--- a/drivers/gpu/drm/tests/drm_probe_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_probe_helper_test.c
@@ -60,13 +60,6 @@ static int drm_probe_helper_test_init(struct kunit *test)
 	return 0;
 }
 
-static void drm_probe_helper_test_exit(struct kunit *test)
-{
-	struct drm_probe_helper_test_priv *priv = test->priv;
-
-	drm_kunit_helper_free_device(test, priv->dev);
-}
-
 typedef struct drm_display_mode *(*expected_mode_func_t)(struct drm_device *);
 
 struct drm_connector_helper_tv_get_modes_test {
@@ -208,7 +201,6 @@ static struct kunit_case drm_test_connector_helper_tv_get_modes_tests[] = {
 static struct kunit_suite drm_test_connector_helper_tv_get_modes_suite = {
 	.name = "drm_connector_helper_tv_get_modes",
 	.init = drm_probe_helper_test_init,
-	.exit = drm_probe_helper_test_exit,
 	.test_cases = drm_test_connector_helper_tv_get_modes_tests,
 };
 

-- 
2.41.0

[PATCH 05/11] drm/tests: helpers: Create an helper to allocate a locking ctx

[Maxime Ripard]

As we get more and more tests, the locking context initialisation
creates more and more boilerplate, both at creation and destruction.

Let's create a helper that will allocate, initialise a context, and
register kunit actions to clean up once the test is done.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/tests/drm_kunit_helpers.c | 41 +++++++++++++++++++++++++++++++
 include/drm/drm_kunit_helpers.h           |  2 ++
 2 files changed, 43 insertions(+)

diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c b/drivers/gpu/drm/tests/drm_kunit_helpers.c
index 38211fea9ae6..40a27c78d692 100644
--- a/drivers/gpu/drm/tests/drm_kunit_helpers.c
+++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c
@@ -124,5 +124,46 @@ __drm_kunit_helper_alloc_drm_device_with_driver(struct kunit *test,
 }
 EXPORT_SYMBOL_GPL(__drm_kunit_helper_alloc_drm_device_with_driver);
 
+static void action_drm_release_context(void *ptr)
+{
+	struct drm_modeset_acquire_ctx *ctx = ptr;
+
+	drm_modeset_drop_locks(ctx);
+	drm_modeset_acquire_fini(ctx);
+}
+
+/**
+ * drm_kunit_helper_context_alloc - Allocates an acquire context
+ * @test: The test context object
+ *
+ * Allocates and initializes a modeset acquire context.
+ *
+ * The context is tied to the kunit test context, so we must not call
+ * drm_modeset_acquire_fini() on it, it will be done so automatically.
+ *
+ * Returns:
+ * An ERR_PTR on error, a pointer to the newly allocated context otherwise
+ */
+struct drm_modeset_acquire_ctx *
+drm_kunit_helper_acquire_ctx_alloc(struct kunit *test)
+{
+	struct drm_modeset_acquire_ctx *ctx;
+	int ret;
+
+	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, ctx);
+
+	drm_modeset_acquire_init(ctx, 0);
+
+	ret = kunit_add_action_or_reset(test,
+					action_drm_release_context,
+					ctx);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return ctx;
+}
+EXPORT_SYMBOL_GPL(drm_kunit_helper_acquire_ctx_alloc);
+
 MODULE_AUTHOR("Maxime Ripard <maxime@cerno.tech>");
 MODULE_LICENSE("GPL");
diff --git a/include/drm/drm_kunit_helpers.h b/include/drm/drm_kunit_helpers.h
index ed013fdcc1ff..4ba5e10653c6 100644
--- a/include/drm/drm_kunit_helpers.h
+++ b/include/drm/drm_kunit_helpers.h
@@ -87,5 +87,7 @@ __drm_kunit_helper_alloc_drm_device(struct kunit *test,
 						      sizeof(_type),		\
 						      offsetof(_type, _member),	\
 						      _feat))
+struct drm_modeset_acquire_ctx *
+drm_kunit_helper_acquire_ctx_alloc(struct kunit *test);
 
 #endif // DRM_KUNIT_HELPERS_H_

-- 
2.41.0

[PATCH 06/11] drm/tests: helpers: Create an helper to allocate an atomic state

[Maxime Ripard]

As we gain more tests, boilerplate to allocate an atomic state and free
it starts to be there more and more as well.

In order to reduce the allocation boilerplate, we can create an helper
to create that atomic state, and call an action when the test is done.
This will also clean up the exit path.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/tests/drm_kunit_helpers.c | 39 +++++++++++++++++++++++++++++++
 include/drm/drm_kunit_helpers.h           |  5 ++++
 2 files changed, 44 insertions(+)

diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c b/drivers/gpu/drm/tests/drm_kunit_helpers.c
index 40a27c78d692..3f3331bc389f 100644
--- a/drivers/gpu/drm/tests/drm_kunit_helpers.c
+++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 
+#include <drm/drm_atomic.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_kunit_helpers.h>
 #include <drm/drm_managed.h>
@@ -165,5 +166,43 @@ drm_kunit_helper_acquire_ctx_alloc(struct kunit *test)
 }
 EXPORT_SYMBOL_GPL(drm_kunit_helper_acquire_ctx_alloc);
 
+/**
+ * drm_kunit_helper_atomic_state_alloc - Allocates an atomic state
+ * @test: The test context object
+ * @drm: The device to alloc the state for
+ * @ctx: Locking context for that atomic update
+ *
+ * Allocates a empty atomic state.
+ *
+ * The state is tied to the kunit test context, so we must not call
+ * drm_atomic_state_put() on it, it will be done so automatically.
+ *
+ * Returns:
+ * An ERR_PTR on error, a pointer to the newly allocated state otherwise
+ */
+struct drm_atomic_state *
+drm_kunit_helper_atomic_state_alloc(struct kunit *test,
+				    struct drm_device *drm,
+				    struct drm_modeset_acquire_ctx *ctx)
+{
+	struct drm_atomic_state *state;
+	int ret;
+
+	state = drm_atomic_state_alloc(drm);
+	if (!state)
+		return ERR_PTR(-ENOMEM);
+
+	ret = kunit_add_action_or_reset(test,
+					(kunit_action_t *)drm_atomic_state_put,
+					state);
+	if (ret)
+		return ERR_PTR(ret);
+
+	state->acquire_ctx = ctx;
+
+	return state;
+}
+EXPORT_SYMBOL_GPL(drm_kunit_helper_atomic_state_alloc);
+
 MODULE_AUTHOR("Maxime Ripard <maxime@cerno.tech>");
 MODULE_LICENSE("GPL");
diff --git a/include/drm/drm_kunit_helpers.h b/include/drm/drm_kunit_helpers.h
index 4ba5e10653c6..514c8a7a32f0 100644
--- a/include/drm/drm_kunit_helpers.h
+++ b/include/drm/drm_kunit_helpers.h
@@ -90,4 +90,9 @@ __drm_kunit_helper_alloc_drm_device(struct kunit *test,
 struct drm_modeset_acquire_ctx *
 drm_kunit_helper_acquire_ctx_alloc(struct kunit *test);
 
+struct drm_atomic_state *
+drm_kunit_helper_atomic_state_alloc(struct kunit *test,
+				    struct drm_device *drm,
+				    struct drm_modeset_acquire_ctx *ctx);
+
 #endif // DRM_KUNIT_HELPERS_H_

-- 
2.41.0

[PATCH 07/11] drm/vc4: tests: pv-muxing: Remove call to drm_kunit_helper_free_device()

[Maxime Ripard]

Calling drm_kunit_helper_free_device() to clean up the resources
allocated by drm_kunit_helper_alloc_device() is now optional and not
needed in most cases.

Remove it.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c | 4 ----
 1 file changed, 4 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 ae0bd0f81698..6c982e72cae8 100644
--- a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
+++ b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
@@ -762,7 +762,6 @@ static void vc4_pv_muxing_test_exit(struct kunit *test)
 	drm_modeset_drop_locks(&priv->ctx);
 	drm_modeset_acquire_fini(&priv->ctx);
 	drm_dev_unregister(drm);
-	drm_kunit_helper_free_device(test, vc4->dev);
 }
 
 static struct kunit_case vc4_pv_muxing_tests[] = {
@@ -873,7 +872,6 @@ static void drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable(struct kunit *tes
 	drm_modeset_drop_locks(&ctx);
 	drm_modeset_acquire_fini(&ctx);
 	drm_dev_unregister(drm);
-	drm_kunit_helper_free_device(test, vc4->dev);
 }
 
 static void drm_test_vc5_pv_muxing_bugs_stable_fifo(struct kunit *test)
@@ -963,7 +961,6 @@ static void drm_test_vc5_pv_muxing_bugs_stable_fifo(struct kunit *test)
 	drm_modeset_drop_locks(&ctx);
 	drm_modeset_acquire_fini(&ctx);
 	drm_dev_unregister(drm);
-	drm_kunit_helper_free_device(test, vc4->dev);
 }
 
 static void
@@ -1017,7 +1014,6 @@ drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable_too_many_crtc_state(struct ku
 	drm_modeset_drop_locks(&ctx);
 	drm_modeset_acquire_fini(&ctx);
 	drm_dev_unregister(drm);
-	drm_kunit_helper_free_device(test, vc4->dev);
 }
 
 static struct kunit_case vc5_pv_muxing_bugs_tests[] = {

-- 
2.41.0

[PATCH 08/11] drm/vc4: tests: mock: Use a kunit action to unregister DRM device

[Maxime Ripard]

The *_mock_device functions allocate a DRM device that needs to be
released using drm_dev_unregister.

Now that we have a kunit release action API, we can switch to it and
don't require any kind of garbage collection from the caller.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/vc4/tests/vc4_mock.c           | 5 +++++
 drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c | 6 ------
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/vc4/tests/vc4_mock.c b/drivers/gpu/drm/vc4/tests/vc4_mock.c
index a4bed26af32f..00825ddc52f0 100644
--- a/drivers/gpu/drm/vc4/tests/vc4_mock.c
+++ b/drivers/gpu/drm/vc4/tests/vc4_mock.c
@@ -186,6 +186,11 @@ static struct vc4_dev *__mock_device(struct kunit *test, bool is_vc5)
 	ret = drm_dev_register(drm, 0);
 	KUNIT_ASSERT_EQ(test, ret, 0);
 
+	ret = kunit_add_action_or_reset(test,
+					(kunit_action_t *)drm_dev_unregister,
+					drm);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
 	return vc4;
 }
 
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 6c982e72cae8..776a7b01608f 100644
--- a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
+++ b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
@@ -754,14 +754,11 @@ static int vc4_pv_muxing_test_init(struct kunit *test)
 static void vc4_pv_muxing_test_exit(struct kunit *test)
 {
 	struct pv_muxing_priv *priv = test->priv;
-	struct vc4_dev *vc4 = priv->vc4;
-	struct drm_device *drm = &vc4->base;
 	struct drm_atomic_state *state = priv->state;
 
 	drm_atomic_state_put(state);
 	drm_modeset_drop_locks(&priv->ctx);
 	drm_modeset_acquire_fini(&priv->ctx);
-	drm_dev_unregister(drm);
 }
 
 static struct kunit_case vc4_pv_muxing_tests[] = {
@@ -871,7 +868,6 @@ static void drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable(struct kunit *tes
 	drm_atomic_state_put(state);
 	drm_modeset_drop_locks(&ctx);
 	drm_modeset_acquire_fini(&ctx);
-	drm_dev_unregister(drm);
 }
 
 static void drm_test_vc5_pv_muxing_bugs_stable_fifo(struct kunit *test)
@@ -960,7 +956,6 @@ static void drm_test_vc5_pv_muxing_bugs_stable_fifo(struct kunit *test)
 	drm_atomic_state_put(state);
 	drm_modeset_drop_locks(&ctx);
 	drm_modeset_acquire_fini(&ctx);
-	drm_dev_unregister(drm);
 }
 
 static void
@@ -1013,7 +1008,6 @@ drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable_too_many_crtc_state(struct ku
 	drm_atomic_state_put(state);
 	drm_modeset_drop_locks(&ctx);
 	drm_modeset_acquire_fini(&ctx);
-	drm_dev_unregister(drm);
 }
 
 static struct kunit_case vc5_pv_muxing_bugs_tests[] = {

-- 
2.41.0

[PATCH 09/11] drm/vc4: tests: pv-muxing: Switch to managed locking init

[Maxime Ripard]

The new helper to init the locking context allows to remove some
boilerplate.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c | 42 ++++++++++++--------------
 1 file changed, 19 insertions(+), 23 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 776a7b01608f..ff1deaed0cab 100644
--- a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
+++ b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
@@ -20,7 +20,6 @@
 
 struct pv_muxing_priv {
 	struct vc4_dev *vc4;
-	struct drm_modeset_acquire_ctx ctx;
 	struct drm_atomic_state *state;
 };
 
@@ -725,6 +724,7 @@ static void drm_vc4_test_pv_muxing_invalid(struct kunit *test)
 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 drm_atomic_state *state;
 	struct pv_muxing_priv *priv;
 	struct drm_device *drm;
@@ -738,13 +738,14 @@ static int vc4_pv_muxing_test_init(struct kunit *test)
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, vc4);
 	priv->vc4 = vc4;
 
-	drm_modeset_acquire_init(&priv->ctx, 0);
+	ctx = drm_kunit_helper_acquire_ctx_alloc(test);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
 
 	drm = &vc4->base;
 	state = drm_atomic_state_alloc(drm);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
 
-	state->acquire_ctx = &priv->ctx;
+	state->acquire_ctx = ctx;
 
 	priv->state = state;
 
@@ -757,8 +758,6 @@ static void vc4_pv_muxing_test_exit(struct kunit *test)
 	struct drm_atomic_state *state = priv->state;
 
 	drm_atomic_state_put(state);
-	drm_modeset_drop_locks(&priv->ctx);
-	drm_modeset_acquire_fini(&priv->ctx);
 }
 
 static struct kunit_case vc4_pv_muxing_tests[] = {
@@ -798,7 +797,7 @@ static struct kunit_suite vc5_pv_muxing_test_suite = {
  */
 static void drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable(struct kunit *test)
 {
-	struct drm_modeset_acquire_ctx ctx;
+	struct drm_modeset_acquire_ctx *ctx;
 	struct drm_atomic_state *state;
 	struct vc4_crtc_state *new_vc4_crtc_state;
 	struct vc4_hvs_state *new_hvs_state;
@@ -811,13 +810,14 @@ static void drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable(struct kunit *tes
 	vc4 = vc5_mock_device(test);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, vc4);
 
-	drm_modeset_acquire_init(&ctx, 0);
+	ctx = drm_kunit_helper_acquire_ctx_alloc(test);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
 
 	drm = &vc4->base;
 	state = drm_atomic_state_alloc(drm);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
 
-	state->acquire_ctx = &ctx;
+	state->acquire_ctx = ctx;
 
 	ret = vc4_mock_atomic_add_output(test, state, VC4_ENCODER_TYPE_HDMI0);
 	KUNIT_ASSERT_EQ(test, ret, 0);
@@ -844,7 +844,7 @@ static void drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable(struct kunit *tes
 	state = drm_atomic_state_alloc(drm);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
 
-	state->acquire_ctx = &ctx;
+	state->acquire_ctx = ctx;
 
 	ret = vc4_mock_atomic_add_output(test, state, VC4_ENCODER_TYPE_HDMI1);
 	KUNIT_ASSERT_EQ(test, ret, 0);
@@ -866,13 +866,11 @@ static void drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable(struct kunit *tes
 	KUNIT_EXPECT_NE(test, hdmi0_channel, hdmi1_channel);
 
 	drm_atomic_state_put(state);
-	drm_modeset_drop_locks(&ctx);
-	drm_modeset_acquire_fini(&ctx);
 }
 
 static void drm_test_vc5_pv_muxing_bugs_stable_fifo(struct kunit *test)
 {
-	struct drm_modeset_acquire_ctx ctx;
+	struct drm_modeset_acquire_ctx *ctx;
 	struct drm_atomic_state *state;
 	struct vc4_crtc_state *new_vc4_crtc_state;
 	struct vc4_hvs_state *new_hvs_state;
@@ -885,13 +883,14 @@ static void drm_test_vc5_pv_muxing_bugs_stable_fifo(struct kunit *test)
 	vc4 = vc5_mock_device(test);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, vc4);
 
-	drm_modeset_acquire_init(&ctx, 0);
+	ctx = drm_kunit_helper_acquire_ctx_alloc(test);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
 
 	drm = &vc4->base;
 	state = drm_atomic_state_alloc(drm);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
 
-	state->acquire_ctx = &ctx;
+	state->acquire_ctx = ctx;
 
 	ret = vc4_mock_atomic_add_output(test, state, VC4_ENCODER_TYPE_HDMI0);
 	KUNIT_ASSERT_EQ(test, ret, 0);
@@ -929,7 +928,7 @@ static void drm_test_vc5_pv_muxing_bugs_stable_fifo(struct kunit *test)
 	state = drm_atomic_state_alloc(drm);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
 
-	state->acquire_ctx = &ctx;
+	state->acquire_ctx = ctx;
 
 	ret = vc4_mock_atomic_del_output(test, state, VC4_ENCODER_TYPE_HDMI0);
 	KUNIT_ASSERT_EQ(test, ret, 0);
@@ -954,14 +953,12 @@ static void drm_test_vc5_pv_muxing_bugs_stable_fifo(struct kunit *test)
 	}
 
 	drm_atomic_state_put(state);
-	drm_modeset_drop_locks(&ctx);
-	drm_modeset_acquire_fini(&ctx);
 }
 
 static void
 drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable_too_many_crtc_state(struct kunit *test)
 {
-	struct drm_modeset_acquire_ctx ctx;
+	struct drm_modeset_acquire_ctx *ctx;
 	struct drm_atomic_state *state;
 	struct vc4_crtc_state *new_vc4_crtc_state;
 	struct drm_device *drm;
@@ -971,13 +968,14 @@ drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable_too_many_crtc_state(struct ku
 	vc4 = vc5_mock_device(test);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, vc4);
 
-	drm_modeset_acquire_init(&ctx, 0);
+	ctx = drm_kunit_helper_acquire_ctx_alloc(test);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
 
 	drm = &vc4->base;
 	state = drm_atomic_state_alloc(drm);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
 
-	state->acquire_ctx = &ctx;
+	state->acquire_ctx = ctx;
 
 	ret = vc4_mock_atomic_add_output(test, state, VC4_ENCODER_TYPE_HDMI0);
 	KUNIT_ASSERT_EQ(test, ret, 0);
@@ -993,7 +991,7 @@ drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable_too_many_crtc_state(struct ku
 	state = drm_atomic_state_alloc(drm);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
 
-	state->acquire_ctx = &ctx;
+	state->acquire_ctx = ctx;
 
 	ret = vc4_mock_atomic_add_output(test, state, VC4_ENCODER_TYPE_HDMI1);
 	KUNIT_ASSERT_EQ(test, ret, 0);
@@ -1006,8 +1004,6 @@ drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable_too_many_crtc_state(struct ku
 	KUNIT_EXPECT_NULL(test, new_vc4_crtc_state);
 
 	drm_atomic_state_put(state);
-	drm_modeset_drop_locks(&ctx);
-	drm_modeset_acquire_fini(&ctx);
 }
 
 static struct kunit_case vc5_pv_muxing_bugs_tests[] = {

-- 
2.41.0

[PATCH 10/11] drm/vc4: tests: Switch to atomic state allocation helper

[Maxime Ripard]

Now that we have a helper that takes care of an atomic state allocation
and cleanup, we can migrate to it to simplify our tests.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c | 55 ++++----------------------
 1 file changed, 8 insertions(+), 47 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 ff1deaed0cab..5f9f5626329d 100644
--- a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
+++ b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
@@ -725,7 +725,6 @@ 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 drm_atomic_state *state;
 	struct pv_muxing_priv *priv;
 	struct drm_device *drm;
 	struct vc4_dev *vc4;
@@ -742,24 +741,12 @@ static int vc4_pv_muxing_test_init(struct kunit *test)
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
 
 	drm = &vc4->base;
-	state = drm_atomic_state_alloc(drm);
-	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
-
-	state->acquire_ctx = ctx;
-
-	priv->state = state;
+	priv->state = drm_kunit_helper_atomic_state_alloc(test, drm, ctx);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->state);
 
 	return 0;
 }
 
-static void vc4_pv_muxing_test_exit(struct kunit *test)
-{
-	struct pv_muxing_priv *priv = test->priv;
-	struct drm_atomic_state *state = priv->state;
-
-	drm_atomic_state_put(state);
-}
-
 static struct kunit_case vc4_pv_muxing_tests[] = {
 	KUNIT_CASE_PARAM(drm_vc4_test_pv_muxing,
 			 vc4_test_pv_muxing_gen_params),
@@ -771,7 +758,6 @@ static struct kunit_case vc4_pv_muxing_tests[] = {
 static struct kunit_suite vc4_pv_muxing_test_suite = {
 	.name = "vc4-pv-muxing-combinations",
 	.init = vc4_pv_muxing_test_init,
-	.exit = vc4_pv_muxing_test_exit,
 	.test_cases = vc4_pv_muxing_tests,
 };
 
@@ -786,7 +772,6 @@ static struct kunit_case vc5_pv_muxing_tests[] = {
 static struct kunit_suite vc5_pv_muxing_test_suite = {
 	.name = "vc5-pv-muxing-combinations",
 	.init = vc4_pv_muxing_test_init,
-	.exit = vc4_pv_muxing_test_exit,
 	.test_cases = vc5_pv_muxing_tests,
 };
 
@@ -814,11 +799,9 @@ static void drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable(struct kunit *tes
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
 
 	drm = &vc4->base;
-	state = drm_atomic_state_alloc(drm);
+	state = drm_kunit_helper_atomic_state_alloc(test, drm, ctx);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
 
-	state->acquire_ctx = ctx;
-
 	ret = vc4_mock_atomic_add_output(test, state, VC4_ENCODER_TYPE_HDMI0);
 	KUNIT_ASSERT_EQ(test, ret, 0);
 
@@ -839,13 +822,9 @@ static void drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable(struct kunit *tes
 	ret = drm_atomic_helper_swap_state(state, false);
 	KUNIT_ASSERT_EQ(test, ret, 0);
 
-	drm_atomic_state_put(state);
-
-	state = drm_atomic_state_alloc(drm);
+	state = drm_kunit_helper_atomic_state_alloc(test, drm, ctx);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
 
-	state->acquire_ctx = ctx;
-
 	ret = vc4_mock_atomic_add_output(test, state, VC4_ENCODER_TYPE_HDMI1);
 	KUNIT_ASSERT_EQ(test, ret, 0);
 
@@ -864,8 +843,6 @@ static void drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable(struct kunit *tes
 	KUNIT_ASSERT_TRUE(test, new_hvs_state->fifo_state[hdmi1_channel].in_use);
 
 	KUNIT_EXPECT_NE(test, hdmi0_channel, hdmi1_channel);
-
-	drm_atomic_state_put(state);
 }
 
 static void drm_test_vc5_pv_muxing_bugs_stable_fifo(struct kunit *test)
@@ -887,11 +864,9 @@ static void drm_test_vc5_pv_muxing_bugs_stable_fifo(struct kunit *test)
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
 
 	drm = &vc4->base;
-	state = drm_atomic_state_alloc(drm);
+	state = drm_kunit_helper_atomic_state_alloc(test, drm, ctx);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
 
-	state->acquire_ctx = ctx;
-
 	ret = vc4_mock_atomic_add_output(test, state, VC4_ENCODER_TYPE_HDMI0);
 	KUNIT_ASSERT_EQ(test, ret, 0);
 
@@ -923,13 +898,9 @@ static void drm_test_vc5_pv_muxing_bugs_stable_fifo(struct kunit *test)
 	ret = drm_atomic_helper_swap_state(state, false);
 	KUNIT_ASSERT_EQ(test, ret, 0);
 
-	drm_atomic_state_put(state);
-
-	state = drm_atomic_state_alloc(drm);
+	state = drm_kunit_helper_atomic_state_alloc(test, drm, ctx);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
 
-	state->acquire_ctx = ctx;
-
 	ret = vc4_mock_atomic_del_output(test, state, VC4_ENCODER_TYPE_HDMI0);
 	KUNIT_ASSERT_EQ(test, ret, 0);
 
@@ -951,8 +922,6 @@ static void drm_test_vc5_pv_muxing_bugs_stable_fifo(struct kunit *test)
 
 		KUNIT_EXPECT_EQ(test, old_hdmi1_channel, hdmi1_channel);
 	}
-
-	drm_atomic_state_put(state);
 }
 
 static void
@@ -972,11 +941,9 @@ drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable_too_many_crtc_state(struct ku
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
 
 	drm = &vc4->base;
-	state = drm_atomic_state_alloc(drm);
+	state = drm_kunit_helper_atomic_state_alloc(test, drm, ctx);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
 
-	state->acquire_ctx = ctx;
-
 	ret = vc4_mock_atomic_add_output(test, state, VC4_ENCODER_TYPE_HDMI0);
 	KUNIT_ASSERT_EQ(test, ret, 0);
 
@@ -986,13 +953,9 @@ drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable_too_many_crtc_state(struct ku
 	ret = drm_atomic_helper_swap_state(state, false);
 	KUNIT_ASSERT_EQ(test, ret, 0);
 
-	drm_atomic_state_put(state);
-
-	state = drm_atomic_state_alloc(drm);
+	state = drm_kunit_helper_atomic_state_alloc(test, drm, ctx);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
 
-	state->acquire_ctx = ctx;
-
 	ret = vc4_mock_atomic_add_output(test, state, VC4_ENCODER_TYPE_HDMI1);
 	KUNIT_ASSERT_EQ(test, ret, 0);
 
@@ -1002,8 +965,6 @@ drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable_too_many_crtc_state(struct ku
 	new_vc4_crtc_state = get_vc4_crtc_state_for_encoder(test, state,
 							    VC4_ENCODER_TYPE_HDMI0);
 	KUNIT_EXPECT_NULL(test, new_vc4_crtc_state);
-
-	drm_atomic_state_put(state);
 }
 
 static struct kunit_case vc5_pv_muxing_bugs_tests[] = {

-- 
2.41.0

[PATCH 11/11] drm/vc4: tests: pv-muxing: Document test scenario

[Maxime Ripard]

We've had a couple of tests that weren't really obvious, nor did they
document what they were supposed to test. Document that to make it
hopefully more obvious.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c | 22 ++++++++++++++++++++++
 1 file changed, 22 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 5f9f5626329d..61622e951031 100644
--- a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
+++ b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
@@ -845,6 +845,13 @@ static void drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable(struct kunit *tes
 	KUNIT_EXPECT_NE(test, hdmi0_channel, hdmi1_channel);
 }
 
+/*
+ * This test makes sure that we never change the FIFO of an active HVS
+ * channel if we disable a FIFO with a lower index.
+ *
+ * Doing so would result in a FIFO stall and would disrupt an output
+ * supposed to be unaffected by the commit.
+ */
 static void drm_test_vc5_pv_muxing_bugs_stable_fifo(struct kunit *test)
 {
 	struct drm_modeset_acquire_ctx *ctx;
@@ -924,6 +931,21 @@ static void drm_test_vc5_pv_muxing_bugs_stable_fifo(struct kunit *test)
 	}
 }
 
+/*
+ * Test that if we affect a single output, only the CRTC state of that
+ * output will be pulled in the global atomic state.
+ *
+ * This is relevant for two things:
+ *
+ *   - If we don't have that state at all, we are unlikely to affect the
+ *     FIFO muxing. This is somewhat redundant with
+ *     drm_test_vc5_pv_muxing_bugs_stable_fifo()
+ *
+ *   - KMS waits for page flips to occur on all the CRTC found in the
+ *     CRTC state. Since the CRTC is unaffected, we would over-wait, but
+ *     most importantly run into corner cases like waiting on an
+ *     inactive CRTC that never completes.
+ */
 static void
 drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable_too_many_crtc_state(struct kunit *test)
 {

-- 
2.41.0

Re: [PATCH 00/11] drm: kunit: Switch to kunit actions

[suijingfeng]

Hi,

On 2023/7/10 15:47, Maxime Ripard wrote:
> Hi,
>
> Since v6.5-rc1, kunit gained a devm/drmm-like mechanism that makes tests
> resources much easier to cleanup.
>
> This series converts the existing tests to use those new actions were
> relevant.

Is the word 'were' here means that 'where' relevant ?

Or it is means that it were relevant, after applied you patch it is not 
relevant anymore ?

> Let me know what you think,
> Maxime
>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
> Maxime Ripard (11):
>        drm/tests: helpers: Switch to kunit actions
>        drm/tests: client-modeset: Remove call to drm_kunit_helper_free_device()
>        drm/tests: modes: Remove call to drm_kunit_helper_free_device()
>        drm/tests: probe-helper: Remove call to drm_kunit_helper_free_device()
>        drm/tests: helpers: Create an helper to allocate a locking ctx
>        drm/tests: helpers: Create an helper to allocate an atomic state

a helper or an helper ?

Should this two patch be re-titled as following ?

I search it on the internet[1], mostly using a helper.


       drm/tests: helpers: Create a helper to allocate a locking ctx
       drm/tests: helpers: Create a helper to allocate an atomic state

[1] https://www.a-or-an.com/a_an/helper

Sorry about the noise if I'm wrong.

>        drm/vc4: tests: pv-muxing: Remove call to drm_kunit_helper_free_device()
>        drm/vc4: tests: mock: Use a kunit action to unregister DRM device
>        drm/vc4: tests: pv-muxing: Switch to managed locking init
>        drm/vc4: tests: Switch to atomic state allocation helper
>        drm/vc4: tests: pv-muxing: Document test scenario
>
>   drivers/gpu/drm/tests/drm_client_modeset_test.c |   8 --
>   drivers/gpu/drm/tests/drm_kunit_helpers.c       | 112 ++++++++++++++++++++++-
>   drivers/gpu/drm/tests/drm_modes_test.c          |   8 --
>   drivers/gpu/drm/tests/drm_probe_helper_test.c   |   8 --
>   drivers/gpu/drm/vc4/tests/vc4_mock.c            |   5 ++
>   drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c  | 115 +++++++++---------------
>   include/drm/drm_kunit_helpers.h                 |   7 ++
>   7 files changed, 162 insertions(+), 101 deletions(-)
> ---
> base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
> change-id: 20230710-kms-kunit-actions-rework-5d163762c93b
>
> Best regards,

Re: [05/11] drm/tests: helpers: Create an helper to allocate a locking ctx

[suijingfeng]

Hi,

On 2023/7/10 15:47, Maxime Ripard wrote:
> As we get more and more tests, the locking context initialisation
> creates more and more boilerplate, both at creation and destruction.
>
> Let's create a helper that will allocate, initialise a context, and
> register kunit actions to clean up once the test is done.
>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>   drivers/gpu/drm/tests/drm_kunit_helpers.c | 41 +++++++++++++++++++++++++++++++
>   include/drm/drm_kunit_helpers.h           |  2 ++
>   2 files changed, 43 insertions(+)
>
> diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c b/drivers/gpu/drm/tests/drm_kunit_helpers.c
> index 38211fea9ae6..40a27c78d692 100644
> --- a/drivers/gpu/drm/tests/drm_kunit_helpers.c
> +++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c
> @@ -124,5 +124,46 @@ __drm_kunit_helper_alloc_drm_device_with_driver(struct kunit *test,
>   }
>   EXPORT_SYMBOL_GPL(__drm_kunit_helper_alloc_drm_device_with_driver);
>   
> +static void action_drm_release_context(void *ptr)
> +{
> +	struct drm_modeset_acquire_ctx *ctx = ptr;
> +
> +	drm_modeset_drop_locks(ctx);
> +	drm_modeset_acquire_fini(ctx);
> +}
> +
> +/**
> + * drm_kunit_helper_context_alloc - Allocates an acquire context
> + * @test: The test context object
> + *
> + * Allocates and initializes a modeset acquire context.
> + *
> + * The context is tied to the kunit test context, so we must not call
> + * drm_modeset_acquire_fini() on it, it will be done so automatically.
> + *
> + * Returns:
> + * An ERR_PTR on error, a pointer to the newly allocated context otherwise
> + */
> +struct drm_modeset_acquire_ctx *
> +drm_kunit_helper_acquire_ctx_alloc(struct kunit *test)
> +{
> +	struct drm_modeset_acquire_ctx *ctx;
> +	int ret;
> +
> +	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_NULL(test, ctx);
> +
> +	drm_modeset_acquire_init(ctx, 0);
> +
> +	ret = kunit_add_action_or_reset(test,
> +					action_drm_release_context,
> +					ctx);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return ctx;
> +}
> +EXPORT_SYMBOL_GPL(drm_kunit_helper_acquire_ctx_alloc);
> +

I think all of the patch inside this series are quite well.

Personally, I can't find problems in it.


But I still want to ask a question:

Should the managed functions you introduced be prefixed with drmm_ 
(instead of drm_) ?

As mindless programmer may still want to call drm_modeset_acquire_fini() 
on the pointer returned by

drm_kunit_helper_acquire_ctx_alloc()?


>   MODULE_AUTHOR("Maxime Ripard <maxime@cerno.tech>");
>   MODULE_LICENSE("GPL");
> diff --git a/include/drm/drm_kunit_helpers.h b/include/drm/drm_kunit_helpers.h
> index ed013fdcc1ff..4ba5e10653c6 100644
> --- a/include/drm/drm_kunit_helpers.h
> +++ b/include/drm/drm_kunit_helpers.h
> @@ -87,5 +87,7 @@ __drm_kunit_helper_alloc_drm_device(struct kunit *test,
>   						      sizeof(_type),		\
>   						      offsetof(_type, _member),	\
>   						      _feat))
> +struct drm_modeset_acquire_ctx *
> +drm_kunit_helper_acquire_ctx_alloc(struct kunit *test);
>   
>   #endif // DRM_KUNIT_HELPERS_H_

Re: [05/11] drm/tests: helpers: Create an helper to allocate a locking ctx

[suijingfeng]

Hi,

On 2023/7/10 15:47, Maxime Ripard wrote:
> As we get more and more tests, the locking context initialisation
> creates more and more boilerplate, both at creation and destruction.
>
> Let's create a helper that will allocate, initialise a context, and
> register kunit actions to clean up once the test is done.
>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>   drivers/gpu/drm/tests/drm_kunit_helpers.c | 41 +++++++++++++++++++++++++++++++
>   include/drm/drm_kunit_helpers.h           |  2 ++
>   2 files changed, 43 insertions(+)
>
> diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c b/drivers/gpu/drm/tests/drm_kunit_helpers.c
> index 38211fea9ae6..40a27c78d692 100644
> --- a/drivers/gpu/drm/tests/drm_kunit_helpers.c
> +++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c
> @@ -124,5 +124,46 @@ __drm_kunit_helper_alloc_drm_device_with_driver(struct kunit *test,
>   }
>   EXPORT_SYMBOL_GPL(__drm_kunit_helper_alloc_drm_device_with_driver);
>   
> +static void action_drm_release_context(void *ptr)
> +{
> +	struct drm_modeset_acquire_ctx *ctx = ptr;
> +
> +	drm_modeset_drop_locks(ctx);
> +	drm_modeset_acquire_fini(ctx);
> +}
> +
> +/**
> + * drm_kunit_helper_context_alloc - Allocates an acquire context
> + * @test: The test context object
> + *
> + * Allocates and initializes a modeset acquire context.
> + *
> + * The context is tied to the kunit test context, so we must not call
> + * drm_modeset_acquire_fini() on it, it will be done so automatically.
> + *
> + * Returns:
> + * An ERR_PTR on error, a pointer to the newly allocated context otherwise
> + */
> +struct drm_modeset_acquire_ctx *
> +drm_kunit_helper_acquire_ctx_alloc(struct kunit *test)
> +{
> +	struct drm_modeset_acquire_ctx *ctx;
> +	int ret;
> +
> +	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);

Because kunit_kzalloc() is also managed,

Is there any possibility that kfree(ctx) get called before 
action_drm_release_context(ctx) ?

Currently, I can't find where the order is guaranteed.

> +	KUNIT_ASSERT_NOT_NULL(test, ctx);
> +
> +	drm_modeset_acquire_init(ctx, 0);
> +
> +	ret = kunit_add_action_or_reset(test,
> +					action_drm_release_context,
> +					ctx);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return ctx;
> +}
> +EXPORT_SYMBOL_GPL(drm_kunit_helper_acquire_ctx_alloc);
> +
>   MODULE_AUTHOR("Maxime Ripard <maxime@cerno.tech>");
>   MODULE_LICENSE("GPL");
> diff --git a/include/drm/drm_kunit_helpers.h b/include/drm/drm_kunit_helpers.h
> index ed013fdcc1ff..4ba5e10653c6 100644
> --- a/include/drm/drm_kunit_helpers.h
> +++ b/include/drm/drm_kunit_helpers.h
> @@ -87,5 +87,7 @@ __drm_kunit_helper_alloc_drm_device(struct kunit *test,
>   						      sizeof(_type),		\
>   						      offsetof(_type, _member),	\
>   						      _feat))
> +struct drm_modeset_acquire_ctx *
> +drm_kunit_helper_acquire_ctx_alloc(struct kunit *test);
>   
>   #endif // DRM_KUNIT_HELPERS_H_

Re: [09/11] drm/vc4: tests: pv-muxing: Switch to managed locking init

[suijingfeng]

Hi,


On 2023/7/10 15:47, Maxime Ripard wrote:
> The new helper to init the locking context allows to remove some
> boilerplate.
>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>   drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c | 42 ++++++++++++--------------
>   1 file changed, 19 insertions(+), 23 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 776a7b01608f..ff1deaed0cab 100644
> --- a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
> +++ b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
> @@ -20,7 +20,6 @@
>   
>   struct pv_muxing_priv {
>   	struct vc4_dev *vc4;
> -	struct drm_modeset_acquire_ctx ctx;
>   	struct drm_atomic_state *state;
>   };
>   
> @@ -725,6 +724,7 @@ static void drm_vc4_test_pv_muxing_invalid(struct kunit *test)
>   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 drm_atomic_state *state;
>   	struct pv_muxing_priv *priv;
>   	struct drm_device *drm;
> @@ -738,13 +738,14 @@ static int vc4_pv_muxing_test_init(struct kunit *test)
>   	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, vc4);
>   	priv->vc4 = vc4;
>   
> -	drm_modeset_acquire_init(&priv->ctx, 0);
> +	ctx = drm_kunit_helper_acquire_ctx_alloc(test);

> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
>   

The pointer returned by drm_kunit_helper_acquire_ctx_alloc() function 
can't be NULL,

if ctx is NULL, the current kthread will be terminated by the 
KUNIT_ASSERT_NOT_NULL() in the drm_kunit_helper_acquire_ctx_alloc().

so only a PTR_ERR is possible, right?

If so, probably invent a KUNIT_ASSERT_NOT_ERR() function to call is enough.

I'm fine with this patch, but I feel the checking if the ctx is NULL is 
redundant.

>   	drm = &vc4->base;
>   	state = drm_atomic_state_alloc(drm);
>   	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
>   
> -	state->acquire_ctx = &priv->ctx;
> +	state->acquire_ctx = ctx;
>   
>   	priv->state = state;
>   
> @@ -757,8 +758,6 @@ static void vc4_pv_muxing_test_exit(struct kunit *test)
>   	struct drm_atomic_state *state = priv->state;
>   
>   	drm_atomic_state_put(state);
> -	drm_modeset_drop_locks(&priv->ctx);
> -	drm_modeset_acquire_fini(&priv->ctx);
>   }
>   
>   static struct kunit_case vc4_pv_muxing_tests[] = {
> @@ -798,7 +797,7 @@ static struct kunit_suite vc5_pv_muxing_test_suite = {
>    */
>   static void drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable(struct kunit *test)
>   {
> -	struct drm_modeset_acquire_ctx ctx;
> +	struct drm_modeset_acquire_ctx *ctx;
>   	struct drm_atomic_state *state;
>   	struct vc4_crtc_state *new_vc4_crtc_state;
>   	struct vc4_hvs_state *new_hvs_state;
> @@ -811,13 +810,14 @@ static void drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable(struct kunit *tes
>   	vc4 = vc5_mock_device(test);
>   	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, vc4);
>   
> -	drm_modeset_acquire_init(&ctx, 0);
> +	ctx = drm_kunit_helper_acquire_ctx_alloc(test);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
>   
>   	drm = &vc4->base;
>   	state = drm_atomic_state_alloc(drm);
>   	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
>   
> -	state->acquire_ctx = &ctx;
> +	state->acquire_ctx = ctx;
>   
>   	ret = vc4_mock_atomic_add_output(test, state, VC4_ENCODER_TYPE_HDMI0);
>   	KUNIT_ASSERT_EQ(test, ret, 0);
> @@ -844,7 +844,7 @@ static void drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable(struct kunit *tes
>   	state = drm_atomic_state_alloc(drm);
>   	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
>   
> -	state->acquire_ctx = &ctx;
> +	state->acquire_ctx = ctx;
>   
>   	ret = vc4_mock_atomic_add_output(test, state, VC4_ENCODER_TYPE_HDMI1);
>   	KUNIT_ASSERT_EQ(test, ret, 0);
> @@ -866,13 +866,11 @@ static void drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable(struct kunit *tes
>   	KUNIT_EXPECT_NE(test, hdmi0_channel, hdmi1_channel);
>   
>   	drm_atomic_state_put(state);
> -	drm_modeset_drop_locks(&ctx);
> -	drm_modeset_acquire_fini(&ctx);
>   }
>   
>   static void drm_test_vc5_pv_muxing_bugs_stable_fifo(struct kunit *test)
>   {
> -	struct drm_modeset_acquire_ctx ctx;
> +	struct drm_modeset_acquire_ctx *ctx;
>   	struct drm_atomic_state *state;
>   	struct vc4_crtc_state *new_vc4_crtc_state;
>   	struct vc4_hvs_state *new_hvs_state;
> @@ -885,13 +883,14 @@ static void drm_test_vc5_pv_muxing_bugs_stable_fifo(struct kunit *test)
>   	vc4 = vc5_mock_device(test);
>   	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, vc4);
>   
> -	drm_modeset_acquire_init(&ctx, 0);
> +	ctx = drm_kunit_helper_acquire_ctx_alloc(test);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
>   
>   	drm = &vc4->base;
>   	state = drm_atomic_state_alloc(drm);
>   	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
>   
> -	state->acquire_ctx = &ctx;
> +	state->acquire_ctx = ctx;
>   
>   	ret = vc4_mock_atomic_add_output(test, state, VC4_ENCODER_TYPE_HDMI0);
>   	KUNIT_ASSERT_EQ(test, ret, 0);
> @@ -929,7 +928,7 @@ static void drm_test_vc5_pv_muxing_bugs_stable_fifo(struct kunit *test)
>   	state = drm_atomic_state_alloc(drm);
>   	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
>   
> -	state->acquire_ctx = &ctx;
> +	state->acquire_ctx = ctx;
>   
>   	ret = vc4_mock_atomic_del_output(test, state, VC4_ENCODER_TYPE_HDMI0);
>   	KUNIT_ASSERT_EQ(test, ret, 0);
> @@ -954,14 +953,12 @@ static void drm_test_vc5_pv_muxing_bugs_stable_fifo(struct kunit *test)
>   	}
>   
>   	drm_atomic_state_put(state);
> -	drm_modeset_drop_locks(&ctx);
> -	drm_modeset_acquire_fini(&ctx);
>   }
>   
>   static void
>   drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable_too_many_crtc_state(struct kunit *test)
>   {
> -	struct drm_modeset_acquire_ctx ctx;
> +	struct drm_modeset_acquire_ctx *ctx;
>   	struct drm_atomic_state *state;
>   	struct vc4_crtc_state *new_vc4_crtc_state;
>   	struct drm_device *drm;
> @@ -971,13 +968,14 @@ drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable_too_many_crtc_state(struct ku
>   	vc4 = vc5_mock_device(test);
>   	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, vc4);
>   
> -	drm_modeset_acquire_init(&ctx, 0);
> +	ctx = drm_kunit_helper_acquire_ctx_alloc(test);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
>   
>   	drm = &vc4->base;
>   	state = drm_atomic_state_alloc(drm);
>   	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
>   
> -	state->acquire_ctx = &ctx;
> +	state->acquire_ctx = ctx;
>   
>   	ret = vc4_mock_atomic_add_output(test, state, VC4_ENCODER_TYPE_HDMI0);
>   	KUNIT_ASSERT_EQ(test, ret, 0);
> @@ -993,7 +991,7 @@ drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable_too_many_crtc_state(struct ku
>   	state = drm_atomic_state_alloc(drm);
>   	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
>   
> -	state->acquire_ctx = &ctx;
> +	state->acquire_ctx = ctx;
>   
>   	ret = vc4_mock_atomic_add_output(test, state, VC4_ENCODER_TYPE_HDMI1);
>   	KUNIT_ASSERT_EQ(test, ret, 0);
> @@ -1006,8 +1004,6 @@ drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable_too_many_crtc_state(struct ku
>   	KUNIT_EXPECT_NULL(test, new_vc4_crtc_state);
>   
>   	drm_atomic_state_put(state);
> -	drm_modeset_drop_locks(&ctx);
> -	drm_modeset_acquire_fini(&ctx);
>   }
>   
>   static struct kunit_case vc5_pv_muxing_bugs_tests[] = {

Re: [PATCH 01/11] drm/tests: helpers: Switch to kunit actions

[Javier Martinez Canillas]

Maxime Ripard <mripard@kernel.org> writes:

Hello Maxime,

The patch looks good to me. I've two questions below though.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>  drivers/gpu/drm/tests/drm_kunit_helpers.c | 32 +++++++++++++++++++++++++++----
>  1 file changed, 28 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c b/drivers/gpu/drm/tests/drm_kunit_helpers.c
> index 4df47071dc88..38211fea9ae6 100644
> --- a/drivers/gpu/drm/tests/drm_kunit_helpers.c
> +++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c
> @@ -35,8 +35,8 @@ static struct platform_driver fake_platform_driver = {
>   * able to leverage the usual infrastructure and most notably the
>   * device-managed resources just like a "real" device.
>   *
> - * Callers need to make sure drm_kunit_helper_free_device() on the
> - * device when done.
> + * Resources will be cleaned up automatically, but the removal can be
> + * forced using @drm_kunit_helper_free_device.
>   *
>   * Returns:
>   * A pointer to the new device, or an ERR_PTR() otherwise.
> @@ -49,12 +49,31 @@ struct device *drm_kunit_helper_alloc_device(struct kunit *test)
>  	ret = platform_driver_register(&fake_platform_driver);
>  	KUNIT_ASSERT_EQ(test, ret, 0);
>  
> +	ret = kunit_add_action_or_reset(test,
> +					(kunit_action_t *)platform_driver_unregister,
> +					&fake_platform_driver);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
>  	pdev = platform_device_alloc(KUNIT_DEVICE_NAME, PLATFORM_DEVID_NONE);
>  	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
>  
> +	ret = kunit_add_action_or_reset(test,
> +					(kunit_action_t *)platform_device_put,
> +					pdev);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
>  	ret = platform_device_add(pdev);
>  	KUNIT_ASSERT_EQ(test, ret, 0);
>  
> +	kunit_remove_action(test,
> +			    (kunit_action_t *)platform_device_put,
> +			    pdev);
> +

I understand that this action removal is because platform_device_put() is
not needed anymore after the platform_device_unregister() remove action is
registered since that already takes care of the platform_device_put().

But maybe add a comment to make more clear for someone who is not familiar
with these details of the platform core ?

>  EXPORT_SYMBOL_GPL(drm_kunit_helper_alloc_device);
> @@ -70,8 +89,13 @@ void drm_kunit_helper_free_device(struct kunit *test, struct device *dev)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
>  
> -	platform_device_unregister(pdev);
> -	platform_driver_unregister(&fake_platform_driver);
> +	kunit_release_action(test,
> +			     (kunit_action_t *)platform_device_unregister,
> +			     pdev);
> +
> +	kunit_release_action(test,
> +			     (kunit_action_t *)platform_driver_unregister,
> +			     &fake_platform_driver);
>  }
>  EXPORT_SYMBOL_GPL(drm_kunit_helper_free_device);
>

I thought the point of using the kunit cleanup actions was that you could
just make the kunit framework handle the release of resources and not do
it manually?

Can you just remove this function helper or is still needed in some cases?

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 02/11] drm/tests: client-modeset: Remove call to drm_kunit_helper_free_device()

[Javier Martinez Canillas]

Maxime Ripard <mripard@kernel.org> writes:

> Calling drm_kunit_helper_free_device() to clean up the resources
> allocated by drm_kunit_helper_alloc_device() is now optional and not
> needed in most cases.
>
> Remove it.
>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 03/11] drm/tests: modes: Remove call to drm_kunit_helper_free_device()

[Javier Martinez Canillas]

Maxime Ripard <mripard@kernel.org> writes:

> Calling drm_kunit_helper_free_device() to clean up the resources
> allocated by drm_kunit_helper_alloc_device() is now optional and not
> needed in most cases.
>
> Remove it.
>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 04/11] drm/tests: probe-helper: Remove call to drm_kunit_helper_free_device()

[Javier Martinez Canillas]

Maxime Ripard <mripard@kernel.org> writes:

> Calling drm_kunit_helper_free_device() to clean up the resources
> allocated by drm_kunit_helper_alloc_device() is now optional and not
> needed in most cases.
>
> Remove it.
>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---

I wonder if makes sense to just squash 2-3 and this one as a single patch.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 05/11] drm/tests: helpers: Create an helper to allocate a locking ctx

[Javier Martinez Canillas]

Maxime Ripard <mripard@kernel.org> writes:

> As we get more and more tests, the locking context initialisation
> creates more and more boilerplate, both at creation and destruction.
>
> Let's create a helper that will allocate, initialise a context, and
> register kunit actions to clean up once the test is done.
>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [05/11] drm/tests: helpers: Create an helper to allocate a locking ctx

[Javier Martinez Canillas]

suijingfeng <suijingfeng@loongson.cn> writes:

> Hi,
>
> On 2023/7/10 15:47, Maxime Ripard wrote:

[...]

>> +
>> +/**
>> + * drm_kunit_helper_context_alloc - Allocates an acquire context
>> + * @test: The test context object
>> + *
>> + * Allocates and initializes a modeset acquire context.
>> + *
>> + * The context is tied to the kunit test context, so we must not call
>> + * drm_modeset_acquire_fini() on it, it will be done so automatically.
>> + *
>> + * Returns:
>> + * An ERR_PTR on error, a pointer to the newly allocated context otherwise
>> + */
>> +struct drm_modeset_acquire_ctx *
>> +drm_kunit_helper_acquire_ctx_alloc(struct kunit *test)
>> +{
>> +	struct drm_modeset_acquire_ctx *ctx;
>> +	int ret;
>> +
>> +	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
>> +	KUNIT_ASSERT_NOT_NULL(test, ctx);
>> +
>> +	drm_modeset_acquire_init(ctx, 0);
>> +
>> +	ret = kunit_add_action_or_reset(test,
>> +					action_drm_release_context,
>> +					ctx);
>> +	if (ret)
>> +		return ERR_PTR(ret);
>> +
>> +	return ctx;
>> +}
>> +EXPORT_SYMBOL_GPL(drm_kunit_helper_acquire_ctx_alloc);
>> +
>
> I think all of the patch inside this series are quite well.
>
> Personally, I can't find problems in it.
>
>
> But I still want to ask a question:
>
> Should the managed functions you introduced be prefixed with drmm_ 
> (instead of drm_) ?
>

That's a good question. But personally I think that the drmm_ prefix
should be reserved for drm_device managed resources and helpers.

> As mindless programmer may still want to call drm_modeset_acquire_fini() 
> on the pointer returned by
>
> drm_kunit_helper_acquire_ctx_alloc()?
>

The function kernel-doc already mentions that there's no need to do that
and that will be done automatically by kunit. So shouldn't be different of
other functions helper where the programmer didn't read the documentation.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [05/11] drm/tests: helpers: Create an helper to allocate a locking ctx

[Javier Martinez Canillas]

suijingfeng <suijingfeng@loongson.cn> writes:

> Hi,
>
> On 2023/7/10 15:47, Maxime Ripard wrote:
>> As we get more and more tests, the locking context initialisation

[...]

>> +/**
>> + * drm_kunit_helper_context_alloc - Allocates an acquire context
>> + * @test: The test context object
>> + *
>> + * Allocates and initializes a modeset acquire context.
>> + *
>> + * The context is tied to the kunit test context, so we must not call
>> + * drm_modeset_acquire_fini() on it, it will be done so automatically.
>> + *
>> + * Returns:
>> + * An ERR_PTR on error, a pointer to the newly allocated context otherwise
>> + */
>> +struct drm_modeset_acquire_ctx *
>> +drm_kunit_helper_acquire_ctx_alloc(struct kunit *test)
>> +{
>> +	struct drm_modeset_acquire_ctx *ctx;
>> +	int ret;
>> +
>> +	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
>
> Because kunit_kzalloc() is also managed,
>
> Is there any possibility that kfree(ctx) get called before 
> action_drm_release_context(ctx) ?
>
> Currently, I can't find where the order is guaranteed.
>

It isn't documented indeed in Documentation/dev-tools/kunit/usage.rst but
the kunit_add_action() kernel-doc says:

"All functions registered with kunit_add_action() will execute in the
opposite order to that they were registered in".

And now that kunit_kzalloc() and friends are also implemented using the
cleanup actions, it will be part of that execution chain.

Probably the kunit docs can make this more clear.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 06/11] drm/tests: helpers: Create an helper to allocate an atomic state

[Javier Martinez Canillas]

Maxime Ripard <mripard@kernel.org> writes:

> As we gain more tests, boilerplate to allocate an atomic state and free
> it starts to be there more and more as well.
>
> In order to reduce the allocation boilerplate, we can create an helper
> to create that atomic state, and call an action when the test is done.
> This will also clean up the exit path.
>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 07/11] drm/vc4: tests: pv-muxing: Remove call to drm_kunit_helper_free_device()

[Javier Martinez Canillas]

Maxime Ripard <mripard@kernel.org> writes:

> Calling drm_kunit_helper_free_device() to clean up the resources
> allocated by drm_kunit_helper_alloc_device() is now optional and not
> needed in most cases.
>
> Remove it.
>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 08/11] drm/vc4: tests: mock: Use a kunit action to unregister DRM device

[Javier Martinez Canillas]

Maxime Ripard <mripard@kernel.org> writes:

> The *_mock_device functions allocate a DRM device that needs to be
> released using drm_dev_unregister.
>
> Now that we have a kunit release action API, we can switch to it and
> don't require any kind of garbage collection from the caller.
>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 09/11] drm/vc4: tests: pv-muxing: Switch to managed locking init

[Javier Martinez Canillas]

Maxime Ripard <mripard@kernel.org> writes:

> The new helper to init the locking context allows to remove some
> boilerplate.
>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 10/11] drm/vc4: tests: Switch to atomic state allocation helper

[Javier Martinez Canillas]

Maxime Ripard <mripard@kernel.org> writes:

> Now that we have a helper that takes care of an atomic state allocation
> and cleanup, we can migrate to it to simplify our tests.
>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 11/11] drm/vc4: tests: pv-muxing: Document test scenario

[Javier Martinez Canillas]

Maxime Ripard <mripard@kernel.org> writes:

> We've had a couple of tests that weren't really obvious, nor did they
> document what they were supposed to test. Document that to make it
> hopefully more obvious.
>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 00/11] drm: kunit: Switch to kunit actions

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1547 bytes --]

Hi,

On Mon, Jul 17, 2023 at 11:24:13PM +0800, suijingfeng wrote:
> On 2023/7/10 15:47, Maxime Ripard wrote:
> > Hi,
> > 
> > Since v6.5-rc1, kunit gained a devm/drmm-like mechanism that makes tests
> > resources much easier to cleanup.
> > 
> > This series converts the existing tests to use those new actions were
> > relevant.
> 
> Is the word 'were' here means that 'where' relevant ?

Yes :)

> Or it is means that it were relevant, after applied you patch it is not
> relevant anymore ?
> 
> > Let me know what you think,
> > Maxime
> > 
> > Signed-off-by: Maxime Ripard <mripard@kernel.org>
> > ---
> > Maxime Ripard (11):
> >        drm/tests: helpers: Switch to kunit actions
> >        drm/tests: client-modeset: Remove call to drm_kunit_helper_free_device()
> >        drm/tests: modes: Remove call to drm_kunit_helper_free_device()
> >        drm/tests: probe-helper: Remove call to drm_kunit_helper_free_device()
> >        drm/tests: helpers: Create an helper to allocate a locking ctx
> >        drm/tests: helpers: Create an helper to allocate an atomic state
> 
> a helper or an helper ?
> 
> Should this two patch be re-titled as following ?
> 
> I search it on the internet[1], mostly using a helper.
> 
> 
>       drm/tests: helpers: Create a helper to allocate a locking ctx
>       drm/tests: helpers: Create a helper to allocate an atomic state
> 
> [1] https://www.a-or-an.com/a_an/helper
> 
> Sorry about the noise if I'm wrong.

You're right, I'll fix it

Thanks!
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 01/11] drm/tests: helpers: Switch to kunit actions

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3615 bytes --]

Hi Javier,

Thanks for reviewing the series

On Wed, Jul 19, 2023 at 08:42:51PM +0200, Javier Martinez Canillas wrote:
> > diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c b/drivers/gpu/drm/tests/drm_kunit_helpers.c
> > index 4df47071dc88..38211fea9ae6 100644
> > --- a/drivers/gpu/drm/tests/drm_kunit_helpers.c
> > +++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c
> > @@ -35,8 +35,8 @@ static struct platform_driver fake_platform_driver = {
> >   * able to leverage the usual infrastructure and most notably the
> >   * device-managed resources just like a "real" device.
> >   *
> > - * Callers need to make sure drm_kunit_helper_free_device() on the
> > - * device when done.
> > + * Resources will be cleaned up automatically, but the removal can be
> > + * forced using @drm_kunit_helper_free_device.
> >   *
> >   * Returns:
> >   * A pointer to the new device, or an ERR_PTR() otherwise.
> > @@ -49,12 +49,31 @@ struct device *drm_kunit_helper_alloc_device(struct kunit *test)
> >  	ret = platform_driver_register(&fake_platform_driver);
> >  	KUNIT_ASSERT_EQ(test, ret, 0);
> >  
> > +	ret = kunit_add_action_or_reset(test,
> > +					(kunit_action_t *)platform_driver_unregister,
> > +					&fake_platform_driver);
> > +	KUNIT_ASSERT_EQ(test, ret, 0);
> > +
> >  	pdev = platform_device_alloc(KUNIT_DEVICE_NAME, PLATFORM_DEVID_NONE);
> >  	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
> >  
> > +	ret = kunit_add_action_or_reset(test,
> > +					(kunit_action_t *)platform_device_put,
> > +					pdev);
> > +	KUNIT_ASSERT_EQ(test, ret, 0);
> > +
> >  	ret = platform_device_add(pdev);
> >  	KUNIT_ASSERT_EQ(test, ret, 0);
> >  
> > +	kunit_remove_action(test,
> > +			    (kunit_action_t *)platform_device_put,
> > +			    pdev);
> > +
> 
> I understand that this action removal is because platform_device_put() is
> not needed anymore after the platform_device_unregister() remove action is
> registered since that already takes care of the platform_device_put().

It's not so much that it's not needed after platform_device_unregister,
but rather that once you've called paltform_device_add my understanding
was that you didn't need it anymore.

I can't find where I got that from though, so I might be wrong. It also
looks like I could just use platform_device_del directly and not cancel
platform_device_put which will make it more obvious.

> But maybe add a comment to make more clear for someone who is not familiar
> with these details of the platform core ?
> 
> >  EXPORT_SYMBOL_GPL(drm_kunit_helper_alloc_device);
> > @@ -70,8 +89,13 @@ void drm_kunit_helper_free_device(struct kunit *test, struct device *dev)
> >  {
> >  	struct platform_device *pdev = to_platform_device(dev);
> >  
> > -	platform_device_unregister(pdev);
> > -	platform_driver_unregister(&fake_platform_driver);
> > +	kunit_release_action(test,
> > +			     (kunit_action_t *)platform_device_unregister,
> > +			     pdev);
> > +
> > +	kunit_release_action(test,
> > +			     (kunit_action_t *)platform_driver_unregister,
> > +			     &fake_platform_driver);
> >  }
> >  EXPORT_SYMBOL_GPL(drm_kunit_helper_free_device);
> >
> 
> I thought the point of using the kunit cleanup actions was that you could
> just make the kunit framework handle the release of resources and not do
> it manually?

You're right

> Can you just remove this function helper or is still needed in some cases?

We still need it for the drmm execution test where we would force the
removal of the device. All other tests at the moment shouldn't call it.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [05/11] drm/tests: helpers: Create an helper to allocate a locking ctx

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2328 bytes --]

On Wed, Jul 19, 2023 at 09:12:14PM +0200, Javier Martinez Canillas wrote:
> suijingfeng <suijingfeng@loongson.cn> writes:
> 
> > Hi,
> >
> > On 2023/7/10 15:47, Maxime Ripard wrote:
> 
> [...]
> 
> >> +
> >> +/**
> >> + * drm_kunit_helper_context_alloc - Allocates an acquire context
> >> + * @test: The test context object
> >> + *
> >> + * Allocates and initializes a modeset acquire context.
> >> + *
> >> + * The context is tied to the kunit test context, so we must not call
> >> + * drm_modeset_acquire_fini() on it, it will be done so automatically.
> >> + *
> >> + * Returns:
> >> + * An ERR_PTR on error, a pointer to the newly allocated context otherwise
> >> + */
> >> +struct drm_modeset_acquire_ctx *
> >> +drm_kunit_helper_acquire_ctx_alloc(struct kunit *test)
> >> +{
> >> +	struct drm_modeset_acquire_ctx *ctx;
> >> +	int ret;
> >> +
> >> +	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
> >> +	KUNIT_ASSERT_NOT_NULL(test, ctx);
> >> +
> >> +	drm_modeset_acquire_init(ctx, 0);
> >> +
> >> +	ret = kunit_add_action_or_reset(test,
> >> +					action_drm_release_context,
> >> +					ctx);
> >> +	if (ret)
> >> +		return ERR_PTR(ret);
> >> +
> >> +	return ctx;
> >> +}
> >> +EXPORT_SYMBOL_GPL(drm_kunit_helper_acquire_ctx_alloc);
> >> +
> >
> > I think all of the patch inside this series are quite well.
> >
> > Personally, I can't find problems in it.
> >
> >
> > But I still want to ask a question:
> >
> > Should the managed functions you introduced be prefixed with drmm_ 
> > (instead of drm_) ?
> >
> 
> That's a good question. But personally I think that the drmm_ prefix
> should be reserved for drm_device managed resources and helpers.

Yeah, to me drmm functions are meant for resources that are tied to the
DRM device lifetime. These resources are tied to the test lifetime, so
they shouldn't share the same prefix.

> > As mindless programmer may still want to call drm_modeset_acquire_fini() 
> > on the pointer returned by
> >
> > drm_kunit_helper_acquire_ctx_alloc()?
> >
> 
> The function kernel-doc already mentions that there's no need to do that
> and that will be done automatically by kunit. So shouldn't be different of
> other functions helper where the programmer didn't read the documentation.

Right

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [05/11] drm/tests: helpers: Create an helper to allocate a locking ctx

[suijingfeng]

Hi,


On 2023/7/20 18:07, Maxime Ripard wrote:
> On Wed, Jul 19, 2023 at 09:12:14PM +0200, Javier Martinez Canillas wrote:
>> suijingfeng <suijingfeng@loongson.cn> writes:
>>
>>> Hi,
>>>
>>> On 2023/7/10 15:47, Maxime Ripard wrote:
>> [...]
>>
>>>> +
>>>> +/**
>>>> + * drm_kunit_helper_context_alloc - Allocates an acquire context
>>>> + * @test: The test context object
>>>> + *
>>>> + * Allocates and initializes a modeset acquire context.
>>>> + *
>>>> + * The context is tied to the kunit test context, so we must not call
>>>> + * drm_modeset_acquire_fini() on it, it will be done so automatically.
>>>> + *
>>>> + * Returns:
>>>> + * An ERR_PTR on error, a pointer to the newly allocated context otherwise
>>>> + */
>>>> +struct drm_modeset_acquire_ctx *
>>>> +drm_kunit_helper_acquire_ctx_alloc(struct kunit *test)
>>>> +{
>>>> +	struct drm_modeset_acquire_ctx *ctx;
>>>> +	int ret;
>>>> +
>>>> +	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
>>>> +	KUNIT_ASSERT_NOT_NULL(test, ctx);
>>>> +
>>>> +	drm_modeset_acquire_init(ctx, 0);
>>>> +
>>>> +	ret = kunit_add_action_or_reset(test,
>>>> +					action_drm_release_context,
>>>> +					ctx);
>>>> +	if (ret)
>>>> +		return ERR_PTR(ret);
>>>> +
>>>> +	return ctx;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(drm_kunit_helper_acquire_ctx_alloc);
>>>> +
>>> I think all of the patch inside this series are quite well.
>>>
>>> Personally, I can't find problems in it.
>>>
>>>
>>> But I still want to ask a question:
>>>
>>> Should the managed functions you introduced be prefixed with drmm_
>>> (instead of drm_) ?
>>>
>> That's a good question. But personally I think that the drmm_ prefix
>> should be reserved for drm_device managed resources and helpers.
> Yeah, to me drmm functions are meant for resources that are tied to the
> DRM device lifetime. These resources are tied to the test lifetime, so
> they shouldn't share the same prefix.


Okay then,

Thanks for reply.


>>> As mindless programmer may still want to call drm_modeset_acquire_fini()
>>> on the pointer returned by
>>>
>>> drm_kunit_helper_acquire_ctx_alloc()?
>>>
>> The function kernel-doc already mentions that there's no need to do that
>> and that will be done automatically by kunit. So shouldn't be different of
>> other functions helper where the programmer didn't read the documentation.
> Right
>
> Maxime

Re: [09/11] drm/vc4: tests: pv-muxing: Switch to managed locking init

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2142 bytes --]

On Tue, Jul 18, 2023 at 01:24:29AM +0800, suijingfeng wrote:
> On 2023/7/10 15:47, Maxime Ripard wrote:
> > The new helper to init the locking context allows to remove some
> > boilerplate.
> > 
> > Signed-off-by: Maxime Ripard <mripard@kernel.org>
> > ---
> >   drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c | 42 ++++++++++++--------------
> >   1 file changed, 19 insertions(+), 23 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 776a7b01608f..ff1deaed0cab 100644
> > --- a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
> > +++ b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
> > @@ -20,7 +20,6 @@
> >   struct pv_muxing_priv {
> >   	struct vc4_dev *vc4;
> > -	struct drm_modeset_acquire_ctx ctx;
> >   	struct drm_atomic_state *state;
> >   };
> > @@ -725,6 +724,7 @@ static void drm_vc4_test_pv_muxing_invalid(struct kunit *test)
> >   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 drm_atomic_state *state;
> >   	struct pv_muxing_priv *priv;
> >   	struct drm_device *drm;
> > @@ -738,13 +738,14 @@ static int vc4_pv_muxing_test_init(struct kunit *test)
> >   	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, vc4);
> >   	priv->vc4 = vc4;
> > -	drm_modeset_acquire_init(&priv->ctx, 0);
> > +	ctx = drm_kunit_helper_acquire_ctx_alloc(test);
> 
> > +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
> 
> The pointer returned by drm_kunit_helper_acquire_ctx_alloc() function can't
> be NULL,
> 
> if ctx is NULL, the current kthread will be terminated by the
> KUNIT_ASSERT_NOT_NULL() in the drm_kunit_helper_acquire_ctx_alloc().
> 
> so only a PTR_ERR is possible, right?
> 
> If so, probably invent a KUNIT_ASSERT_NOT_ERR() function to call is enough.
> 
> I'm fine with this patch, but I feel the checking if the ctx is NULL is
> redundant.

I guess, but we're still reference that pointer later on, so making sure
that it's a valid pointer still makes sense.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]