* [PATCH v4 0/6] drm/managed: Add drmm_release_action
@ 2024-01-05 10:13 Michał Winiarski
2024-01-05 10:13 ` [PATCH v4 1/6] " Michał Winiarski
` (5 more replies)
0 siblings, 6 replies; 13+ messages in thread
From: Michał Winiarski @ 2024-01-05 10:13 UTC (permalink / raw)
To: intel-xe, dri-devel, linux-kernel
Cc: Rodrigo Vivi, Michal Wajdeczko, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Daniel Vetter,
Javier Martinez Canillas, Maíra Canal,
Michał Winiarski
Upcoming Intel Xe driver will need to have a more fine-grained control
over DRM managed actions - namely, the ability to release a given
action, triggering it manually at a different point in time than the
final drm_dev_put().
This series adds a drmm_release_action function (which is similar to
devres devm_release_action) and a simple test that uses it.
v1 -> v2:
- Split the test changes (Maxime)
- Simplify priv lifetime management (Maxime)
v2 -> v3:
- Order tests alphabetically (Maxime)
- Add comments explaining the intention behind the tests and the reason
why DRM device can't be embedded inside test priv (Maxime)
- Bring back priv lifetime management from v1 to avoid use-after-free
v3 -> v4:
- Split test changes into smaller patches (Maxime)
- Remove the waitqueue usage in tests
- Rename the test suite to match other DRM tests
Michał Winiarski (6):
drm/managed: Add drmm_release_action
drm/tests: managed: Rename the suite name to match other DRM tests
drm/tests: managed: Remove the waitqueue usage
drm/tests: managed: Add comments and expect fail messages
drm/tests: managed: Extract device initialization into test init
drm/tests: managed: Add a simple test for drmm_managed_release
drivers/gpu/drm/drm_managed.c | 39 +++++++++++
drivers/gpu/drm/tests/drm_managed_test.c | 84 +++++++++++++++++-------
include/drm/drm_managed.h | 4 ++
3 files changed, 105 insertions(+), 22 deletions(-)
--
2.43.0
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v4 1/6] drm/managed: Add drmm_release_action
2024-01-05 10:13 [PATCH v4 0/6] drm/managed: Add drmm_release_action Michał Winiarski
@ 2024-01-05 10:13 ` Michał Winiarski
2024-01-05 10:13 ` [PATCH v4 2/6] drm/tests: managed: Rename the suite name to match other DRM tests Michał Winiarski
` (4 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Michał Winiarski @ 2024-01-05 10:13 UTC (permalink / raw)
To: intel-xe, dri-devel, linux-kernel
Cc: Rodrigo Vivi, Michal Wajdeczko, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Daniel Vetter,
Javier Martinez Canillas, Maíra Canal,
Michał Winiarski
Similar to devres equivalent, it allows to call the "release" action
directly and remove the resource from the managed resources list.
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Reviewed-by: Maxime Ripard <mripard@kernel.org>
---
drivers/gpu/drm/drm_managed.c | 39 +++++++++++++++++++++++++++++++++++
include/drm/drm_managed.h | 4 ++++
2 files changed, 43 insertions(+)
diff --git a/drivers/gpu/drm/drm_managed.c b/drivers/gpu/drm/drm_managed.c
index bcd111404b128..7646f67bda4e4 100644
--- a/drivers/gpu/drm/drm_managed.c
+++ b/drivers/gpu/drm/drm_managed.c
@@ -176,6 +176,45 @@ int __drmm_add_action_or_reset(struct drm_device *dev,
}
EXPORT_SYMBOL(__drmm_add_action_or_reset);
+/**
+ * drmm_release_action - release a managed action from a &drm_device
+ * @dev: DRM device
+ * @action: function which would be called when @dev is released
+ * @data: opaque pointer, passed to @action
+ *
+ * This function calls the @action previously added by drmm_add_action()
+ * immediately.
+ * The @action is removed from the list of cleanup actions for @dev,
+ * which means that it won't be called in the final drm_dev_put().
+ */
+void drmm_release_action(struct drm_device *dev,
+ drmres_release_t action,
+ void *data)
+{
+ struct drmres *dr_match = NULL, *dr;
+ unsigned long flags;
+
+ spin_lock_irqsave(&dev->managed.lock, flags);
+ list_for_each_entry_reverse(dr, &dev->managed.resources, node.entry) {
+ if (dr->node.release == action) {
+ if (!data || (data && *(void **)dr->data == data)) {
+ dr_match = dr;
+ del_dr(dev, dr_match);
+ break;
+ }
+ }
+ }
+ spin_unlock_irqrestore(&dev->managed.lock, flags);
+
+ if (WARN_ON(!dr_match))
+ return;
+
+ action(dev, data);
+
+ free_dr(dr_match);
+}
+EXPORT_SYMBOL(drmm_release_action);
+
/**
* drmm_kmalloc - &drm_device managed kmalloc()
* @dev: DRM device
diff --git a/include/drm/drm_managed.h b/include/drm/drm_managed.h
index ad08f834af408..f547b09ca0239 100644
--- a/include/drm/drm_managed.h
+++ b/include/drm/drm_managed.h
@@ -45,6 +45,10 @@ int __must_check __drmm_add_action_or_reset(struct drm_device *dev,
drmres_release_t action,
void *data, const char *name);
+void drmm_release_action(struct drm_device *dev,
+ drmres_release_t action,
+ void *data);
+
void *drmm_kmalloc(struct drm_device *dev, size_t size, gfp_t gfp) __malloc;
/**
--
2.43.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v4 2/6] drm/tests: managed: Rename the suite name to match other DRM tests
2024-01-05 10:13 [PATCH v4 0/6] drm/managed: Add drmm_release_action Michał Winiarski
2024-01-05 10:13 ` [PATCH v4 1/6] " Michał Winiarski
@ 2024-01-05 10:13 ` Michał Winiarski
2024-01-08 10:37 ` Maxime Ripard
2024-01-05 10:13 ` [PATCH v4 3/6] drm/tests: managed: Remove the waitqueue usage Michał Winiarski
` (3 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: Michał Winiarski @ 2024-01-05 10:13 UTC (permalink / raw)
To: intel-xe, dri-devel, linux-kernel
Cc: Rodrigo Vivi, Michal Wajdeczko, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Daniel Vetter,
Javier Martinez Canillas, Maíra Canal,
Michał Winiarski
DRM tests use "_" rather than "-" as word separator. Rename the test
suite to match other tests.
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
drivers/gpu/drm/tests/drm_managed_test.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tests/drm_managed_test.c b/drivers/gpu/drm/tests/drm_managed_test.c
index 1652dca11d30c..659af5abb8014 100644
--- a/drivers/gpu/drm/tests/drm_managed_test.c
+++ b/drivers/gpu/drm/tests/drm_managed_test.c
@@ -61,7 +61,7 @@ static struct kunit_case drm_managed_tests[] = {
};
static struct kunit_suite drm_managed_test_suite = {
- .name = "drm-test-managed",
+ .name = "drm_test_managed",
.test_cases = drm_managed_tests
};
--
2.43.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v4 3/6] drm/tests: managed: Remove the waitqueue usage
2024-01-05 10:13 [PATCH v4 0/6] drm/managed: Add drmm_release_action Michał Winiarski
2024-01-05 10:13 ` [PATCH v4 1/6] " Michał Winiarski
2024-01-05 10:13 ` [PATCH v4 2/6] drm/tests: managed: Rename the suite name to match other DRM tests Michał Winiarski
@ 2024-01-05 10:13 ` Michał Winiarski
2024-01-08 10:45 ` Maxime Ripard
2024-01-05 10:13 ` [PATCH v4 4/6] drm/tests: managed: Add comments and expect fail messages Michał Winiarski
` (2 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: Michał Winiarski @ 2024-01-05 10:13 UTC (permalink / raw)
To: intel-xe, dri-devel, linux-kernel
Cc: Rodrigo Vivi, Michal Wajdeczko, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Daniel Vetter,
Javier Martinez Canillas, Maíra Canal,
Michał Winiarski
DRM managed release (drm_managed_release) is called as part of devres
release (devres_release_all), which is not async.
The release action should have already been executed once
drm_kunit_helper_free_device exits, meaning that there's no need to use
a waitqueue - we can just inspect the "action_done" state directly.
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
drivers/gpu/drm/tests/drm_managed_test.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/tests/drm_managed_test.c b/drivers/gpu/drm/tests/drm_managed_test.c
index 659af5abb8014..e4790ae838ba7 100644
--- a/drivers/gpu/drm/tests/drm_managed_test.c
+++ b/drivers/gpu/drm/tests/drm_managed_test.c
@@ -8,12 +8,8 @@
#include <linux/device.h>
-/* Ought to be enough for anybody */
-#define TEST_TIMEOUT_MS 100
-
struct managed_test_priv {
bool action_done;
- wait_queue_head_t action_wq;
};
static void drm_action(struct drm_device *drm, void *ptr)
@@ -21,7 +17,6 @@ static void drm_action(struct drm_device *drm, void *ptr)
struct managed_test_priv *priv = ptr;
priv->action_done = true;
- wake_up_interruptible(&priv->action_wq);
}
static void drm_test_managed_run_action(struct kunit *test)
@@ -33,7 +28,6 @@ static void drm_test_managed_run_action(struct kunit *test)
priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
- init_waitqueue_head(&priv->action_wq);
dev = drm_kunit_helper_alloc_device(test);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev);
@@ -50,9 +44,7 @@ static void drm_test_managed_run_action(struct kunit *test)
drm_dev_unregister(drm);
drm_kunit_helper_free_device(test, dev);
- ret = wait_event_interruptible_timeout(priv->action_wq, priv->action_done,
- msecs_to_jiffies(TEST_TIMEOUT_MS));
- KUNIT_EXPECT_GT(test, ret, 0);
+ KUNIT_EXPECT_TRUE(test, priv->action_done);
}
static struct kunit_case drm_managed_tests[] = {
--
2.43.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v4 4/6] drm/tests: managed: Add comments and expect fail messages
2024-01-05 10:13 [PATCH v4 0/6] drm/managed: Add drmm_release_action Michał Winiarski
` (2 preceding siblings ...)
2024-01-05 10:13 ` [PATCH v4 3/6] drm/tests: managed: Remove the waitqueue usage Michał Winiarski
@ 2024-01-05 10:13 ` Michał Winiarski
2024-01-08 10:47 ` Maxime Ripard
2024-01-05 10:13 ` [PATCH v4 5/6] drm/tests: managed: Extract device initialization into test init Michał Winiarski
2024-01-05 10:13 ` [PATCH v4 6/6] drm/tests: managed: Add a simple test for drmm_managed_release Michał Winiarski
5 siblings, 1 reply; 13+ messages in thread
From: Michał Winiarski @ 2024-01-05 10:13 UTC (permalink / raw)
To: intel-xe, dri-devel, linux-kernel
Cc: Rodrigo Vivi, Michal Wajdeczko, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Daniel Vetter,
Javier Martinez Canillas, Maíra Canal,
Michał Winiarski
Add comments explaining the intention behind the test and certain
implementation details related to device lifetime.
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
drivers/gpu/drm/tests/drm_managed_test.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tests/drm_managed_test.c b/drivers/gpu/drm/tests/drm_managed_test.c
index e4790ae838ba7..986a38c9144a5 100644
--- a/drivers/gpu/drm/tests/drm_managed_test.c
+++ b/drivers/gpu/drm/tests/drm_managed_test.c
@@ -19,6 +19,10 @@ static void drm_action(struct drm_device *drm, void *ptr)
priv->action_done = true;
}
+/*
+ * The test verifies that the release action is called automatically when the
+ * device is released.
+ */
static void drm_test_managed_run_action(struct kunit *test)
{
struct managed_test_priv *priv;
@@ -32,6 +36,11 @@ static void drm_test_managed_run_action(struct kunit *test)
dev = drm_kunit_helper_alloc_device(test);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev);
+ /*
+ * DRM device can't be embedded in priv, since priv->action_done needs
+ * to remain allocated beyond both parent device and drm_device
+ * lifetime.
+ */
drm = __drm_kunit_helper_alloc_drm_device(test, dev, sizeof(*drm), 0, DRIVER_MODESET);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, drm);
@@ -44,7 +53,7 @@ static void drm_test_managed_run_action(struct kunit *test)
drm_dev_unregister(drm);
drm_kunit_helper_free_device(test, dev);
- KUNIT_EXPECT_TRUE(test, priv->action_done);
+ KUNIT_EXPECT_TRUE_MSG(test, priv->action_done, "Release action was not called");
}
static struct kunit_case drm_managed_tests[] = {
--
2.43.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v4 5/6] drm/tests: managed: Extract device initialization into test init
2024-01-05 10:13 [PATCH v4 0/6] drm/managed: Add drmm_release_action Michał Winiarski
` (3 preceding siblings ...)
2024-01-05 10:13 ` [PATCH v4 4/6] drm/tests: managed: Add comments and expect fail messages Michał Winiarski
@ 2024-01-05 10:13 ` Michał Winiarski
2024-01-08 10:52 ` Maxime Ripard
2024-01-05 10:13 ` [PATCH v4 6/6] drm/tests: managed: Add a simple test for drmm_managed_release Michał Winiarski
5 siblings, 1 reply; 13+ messages in thread
From: Michał Winiarski @ 2024-01-05 10:13 UTC (permalink / raw)
To: intel-xe, dri-devel, linux-kernel
Cc: Rodrigo Vivi, Michal Wajdeczko, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Daniel Vetter,
Javier Martinez Canillas, Maíra Canal,
Michał Winiarski
It simplifies the process of extending the test suite with additional
test cases without unnecessary duplication.
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
drivers/gpu/drm/tests/drm_managed_test.c | 37 +++++++++++++++---------
1 file changed, 24 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/tests/drm_managed_test.c b/drivers/gpu/drm/tests/drm_managed_test.c
index 986a38c9144a5..c1fc1f0aac9b2 100644
--- a/drivers/gpu/drm/tests/drm_managed_test.c
+++ b/drivers/gpu/drm/tests/drm_managed_test.c
@@ -9,6 +9,7 @@
#include <linux/device.h>
struct managed_test_priv {
+ struct drm_device *drm;
bool action_done;
};
@@ -24,11 +25,26 @@ static void drm_action(struct drm_device *drm, void *ptr)
* device is released.
*/
static void drm_test_managed_run_action(struct kunit *test)
+{
+ struct managed_test_priv *priv = test->priv;
+ int ret;
+
+ ret = drmm_add_action_or_reset(priv->drm, drm_action, priv);
+ KUNIT_EXPECT_EQ(test, ret, 0);
+
+ ret = drm_dev_register(priv->drm, 0);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ drm_dev_unregister(priv->drm);
+ drm_kunit_helper_free_device(test, priv->drm->dev);
+
+ KUNIT_EXPECT_TRUE_MSG(test, priv->action_done, "Release action was not called");
+}
+
+static int drm_managed_test_init(struct kunit *test)
{
struct managed_test_priv *priv;
- struct drm_device *drm;
struct device *dev;
- int ret;
priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
@@ -41,19 +57,13 @@ static void drm_test_managed_run_action(struct kunit *test)
* to remain allocated beyond both parent device and drm_device
* lifetime.
*/
- drm = __drm_kunit_helper_alloc_drm_device(test, dev, sizeof(*drm), 0, DRIVER_MODESET);
- KUNIT_ASSERT_NOT_ERR_OR_NULL(test, drm);
+ priv->drm = __drm_kunit_helper_alloc_drm_device(test, dev, sizeof(*priv->drm), 0,
+ DRIVER_MODESET);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->drm);
- ret = drmm_add_action_or_reset(drm, drm_action, priv);
- KUNIT_EXPECT_EQ(test, ret, 0);
+ test->priv = priv;
- ret = drm_dev_register(drm, 0);
- KUNIT_ASSERT_EQ(test, ret, 0);
-
- drm_dev_unregister(drm);
- drm_kunit_helper_free_device(test, dev);
-
- KUNIT_EXPECT_TRUE_MSG(test, priv->action_done, "Release action was not called");
+ return 0;
}
static struct kunit_case drm_managed_tests[] = {
@@ -63,6 +73,7 @@ static struct kunit_case drm_managed_tests[] = {
static struct kunit_suite drm_managed_test_suite = {
.name = "drm_test_managed",
+ .init = drm_managed_test_init,
.test_cases = drm_managed_tests
};
--
2.43.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v4 6/6] drm/tests: managed: Add a simple test for drmm_managed_release
2024-01-05 10:13 [PATCH v4 0/6] drm/managed: Add drmm_release_action Michał Winiarski
` (4 preceding siblings ...)
2024-01-05 10:13 ` [PATCH v4 5/6] drm/tests: managed: Extract device initialization into test init Michał Winiarski
@ 2024-01-05 10:13 ` Michał Winiarski
2024-01-10 15:56 ` Maxime Ripard
5 siblings, 1 reply; 13+ messages in thread
From: Michał Winiarski @ 2024-01-05 10:13 UTC (permalink / raw)
To: intel-xe, dri-devel, linux-kernel
Cc: Rodrigo Vivi, Michal Wajdeczko, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Daniel Vetter,
Javier Martinez Canillas, Maíra Canal,
Michał Winiarski
Add a simple test that checks whether the action is indeed called right
away and that it is not called on the final drm_dev_put().
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
drivers/gpu/drm/tests/drm_managed_test.c | 28 ++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/drivers/gpu/drm/tests/drm_managed_test.c b/drivers/gpu/drm/tests/drm_managed_test.c
index c1fc1f0aac9b2..91863642efc13 100644
--- a/drivers/gpu/drm/tests/drm_managed_test.c
+++ b/drivers/gpu/drm/tests/drm_managed_test.c
@@ -41,6 +41,33 @@ static void drm_test_managed_run_action(struct kunit *test)
KUNIT_EXPECT_TRUE_MSG(test, priv->action_done, "Release action was not called");
}
+/*
+ * The test verifies that the release action is called immediately when
+ * drmm_release_action is called and that it is not called for a second time
+ * when the device is released.
+ */
+static void drm_test_managed_release_action(struct kunit *test)
+{
+ struct managed_test_priv *priv = test->priv;
+ int ret;
+
+ ret = drmm_add_action_or_reset(priv->drm, drm_action, priv);
+ KUNIT_EXPECT_EQ(test, ret, 0);
+
+ ret = drm_dev_register(priv->drm, 0);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ drmm_release_action(priv->drm, drm_action, priv);
+ KUNIT_EXPECT_TRUE_MSG(test, priv->action_done, "Release action was not called");
+ priv->action_done = false;
+
+ drm_dev_unregister(priv->drm);
+ drm_kunit_helper_free_device(test, priv->drm->dev);
+
+ KUNIT_EXPECT_FALSE_MSG(test, priv->action_done,
+ "Unexpected release action call during cleanup");
+}
+
static int drm_managed_test_init(struct kunit *test)
{
struct managed_test_priv *priv;
@@ -67,6 +94,7 @@ static int drm_managed_test_init(struct kunit *test)
}
static struct kunit_case drm_managed_tests[] = {
+ KUNIT_CASE(drm_test_managed_release_action),
KUNIT_CASE(drm_test_managed_run_action),
{}
};
--
2.43.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/6] drm/tests: managed: Rename the suite name to match other DRM tests
2024-01-05 10:13 ` [PATCH v4 2/6] drm/tests: managed: Rename the suite name to match other DRM tests Michał Winiarski
@ 2024-01-08 10:37 ` Maxime Ripard
0 siblings, 0 replies; 13+ messages in thread
From: Maxime Ripard @ 2024-01-08 10:37 UTC (permalink / raw)
To: Michał Winiarski
Cc: intel-xe, dri-devel, linux-kernel, Rodrigo Vivi,
Michal Wajdeczko, Maarten Lankhorst, Thomas Zimmermann,
David Airlie, Daniel Vetter, Javier Martinez Canillas,
Maíra Canal
[-- Attachment #1: Type: text/plain, Size: 931 bytes --]
On Fri, Jan 05, 2024 at 11:13:20AM +0100, Michał Winiarski wrote:
> DRM tests use "_" rather than "-" as word separator. Rename the test
> suite to match other tests.
>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> ---
> drivers/gpu/drm/tests/drm_managed_test.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/tests/drm_managed_test.c b/drivers/gpu/drm/tests/drm_managed_test.c
> index 1652dca11d30c..659af5abb8014 100644
> --- a/drivers/gpu/drm/tests/drm_managed_test.c
> +++ b/drivers/gpu/drm/tests/drm_managed_test.c
> @@ -61,7 +61,7 @@ static struct kunit_case drm_managed_tests[] = {
> };
>
> static struct kunit_suite drm_managed_test_suite = {
> - .name = "drm-test-managed",
> + .name = "drm_test_managed",
> .test_cases = drm_managed_tests
> };
I guess if we were to truly follow the trend, it would be drm_managed?
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 3/6] drm/tests: managed: Remove the waitqueue usage
2024-01-05 10:13 ` [PATCH v4 3/6] drm/tests: managed: Remove the waitqueue usage Michał Winiarski
@ 2024-01-08 10:45 ` Maxime Ripard
0 siblings, 0 replies; 13+ messages in thread
From: Maxime Ripard @ 2024-01-08 10:45 UTC (permalink / raw)
To: Michał Winiarski
Cc: intel-xe, dri-devel, linux-kernel, Rodrigo Vivi,
Michal Wajdeczko, Maarten Lankhorst, Thomas Zimmermann,
David Airlie, Daniel Vetter, Javier Martinez Canillas,
Maíra Canal
[-- Attachment #1: Type: text/plain, Size: 815 bytes --]
On Fri, Jan 05, 2024 at 11:13:21AM +0100, Michał Winiarski wrote:
> DRM managed release (drm_managed_release) is called as part of devres
> release (devres_release_all), which is not async.
> The release action should have already been executed once
> drm_kunit_helper_free_device exits, meaning that there's no need to use
> a waitqueue - we can just inspect the "action_done" state directly.
>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
I disagree, nothing guarantees in the API that it will be executed right
away. Since it might be asynchronous (if something else holds a
reference for example), we need the workqueue.
Fortunately, it turns out that it's actually done right away, which also
means we'll never hit the timeout and thus never stall the test run.
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 4/6] drm/tests: managed: Add comments and expect fail messages
2024-01-05 10:13 ` [PATCH v4 4/6] drm/tests: managed: Add comments and expect fail messages Michał Winiarski
@ 2024-01-08 10:47 ` Maxime Ripard
0 siblings, 0 replies; 13+ messages in thread
From: Maxime Ripard @ 2024-01-08 10:47 UTC (permalink / raw)
To: Michał Winiarski
Cc: intel-xe, dri-devel, linux-kernel, Rodrigo Vivi,
Michal Wajdeczko, Maarten Lankhorst, Thomas Zimmermann,
David Airlie, Daniel Vetter, Javier Martinez Canillas,
Maíra Canal
[-- Attachment #1: Type: text/plain, Size: 1988 bytes --]
On Fri, Jan 05, 2024 at 11:13:22AM +0100, Michał Winiarski wrote:
> Add comments explaining the intention behind the test and certain
> implementation details related to device lifetime.
>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> ---
> drivers/gpu/drm/tests/drm_managed_test.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/tests/drm_managed_test.c b/drivers/gpu/drm/tests/drm_managed_test.c
> index e4790ae838ba7..986a38c9144a5 100644
> --- a/drivers/gpu/drm/tests/drm_managed_test.c
> +++ b/drivers/gpu/drm/tests/drm_managed_test.c
> @@ -19,6 +19,10 @@ static void drm_action(struct drm_device *drm, void *ptr)
> priv->action_done = true;
> }
>
> +/*
> + * The test verifies that the release action is called automatically when the
> + * device is released.
> + */
> static void drm_test_managed_run_action(struct kunit *test)
> {
> struct managed_test_priv *priv;
> @@ -32,6 +36,11 @@ static void drm_test_managed_run_action(struct kunit *test)
> dev = drm_kunit_helper_alloc_device(test);
> KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev);
>
> + /*
> + * DRM device can't be embedded in priv, since priv->action_done needs
> + * to remain allocated beyond both parent device and drm_device
> + * lifetime.
> + */
> drm = __drm_kunit_helper_alloc_drm_device(test, dev, sizeof(*drm), 0, DRIVER_MODESET);
> KUNIT_ASSERT_NOT_ERR_OR_NULL(test, drm);
>
> @@ -44,7 +53,7 @@ static void drm_test_managed_run_action(struct kunit *test)
> drm_dev_unregister(drm);
> drm_kunit_helper_free_device(test, dev);
>
> - KUNIT_EXPECT_TRUE(test, priv->action_done);
> + KUNIT_EXPECT_TRUE_MSG(test, priv->action_done, "Release action was not called");
I'm fine with the other two comments, but I'm not really sure what that
message brings to the table. It should be pretty obvious from the
function name, variable name and comments already.
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 5/6] drm/tests: managed: Extract device initialization into test init
2024-01-05 10:13 ` [PATCH v4 5/6] drm/tests: managed: Extract device initialization into test init Michał Winiarski
@ 2024-01-08 10:52 ` Maxime Ripard
0 siblings, 0 replies; 13+ messages in thread
From: Maxime Ripard @ 2024-01-08 10:52 UTC (permalink / raw)
To: Michał Winiarski
Cc: intel-xe, dri-devel, linux-kernel, Rodrigo Vivi,
Michal Wajdeczko, Maarten Lankhorst, Thomas Zimmermann,
David Airlie, Daniel Vetter, Javier Martinez Canillas,
Maíra Canal
[-- Attachment #1: Type: text/plain, Size: 1533 bytes --]
On Fri, Jan 05, 2024 at 11:13:23AM +0100, Michał Winiarski wrote:
> It simplifies the process of extending the test suite with additional
> test cases without unnecessary duplication.
>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> ---
> drivers/gpu/drm/tests/drm_managed_test.c | 37 +++++++++++++++---------
> 1 file changed, 24 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/tests/drm_managed_test.c b/drivers/gpu/drm/tests/drm_managed_test.c
> index 986a38c9144a5..c1fc1f0aac9b2 100644
> --- a/drivers/gpu/drm/tests/drm_managed_test.c
> +++ b/drivers/gpu/drm/tests/drm_managed_test.c
> @@ -9,6 +9,7 @@
> #include <linux/device.h>
>
> struct managed_test_priv {
> + struct drm_device *drm;
> bool action_done;
> };
>
> @@ -24,11 +25,26 @@ static void drm_action(struct drm_device *drm, void *ptr)
> * device is released.
> */
> static void drm_test_managed_run_action(struct kunit *test)
> +{
> + struct managed_test_priv *priv = test->priv;
> + int ret;
> +
> + ret = drmm_add_action_or_reset(priv->drm, drm_action, priv);
> + KUNIT_EXPECT_EQ(test, ret, 0);
> +
> + ret = drm_dev_register(priv->drm, 0);
> + KUNIT_ASSERT_EQ(test, ret, 0);
> +
> + drm_dev_unregister(priv->drm);
> + drm_kunit_helper_free_device(test, priv->drm->dev);
> +
> + KUNIT_EXPECT_TRUE_MSG(test, priv->action_done, "Release action was not called");
Aside from the message here I already pointed out,
Acked-by: Maxime Ripard <mripard@kernel.org>
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 6/6] drm/tests: managed: Add a simple test for drmm_managed_release
2024-01-05 10:13 ` [PATCH v4 6/6] drm/tests: managed: Add a simple test for drmm_managed_release Michał Winiarski
@ 2024-01-10 15:56 ` Maxime Ripard
2024-01-15 17:11 ` Michał Winiarski
0 siblings, 1 reply; 13+ messages in thread
From: Maxime Ripard @ 2024-01-10 15:56 UTC (permalink / raw)
To: Michał Winiarski
Cc: intel-xe, dri-devel, linux-kernel, Rodrigo Vivi,
Michal Wajdeczko, Maarten Lankhorst, Thomas Zimmermann,
David Airlie, Daniel Vetter, Javier Martinez Canillas,
Maíra Canal
[-- Attachment #1: Type: text/plain, Size: 1970 bytes --]
On Fri, Jan 05, 2024 at 11:13:24AM +0100, Michał Winiarski wrote:
> Add a simple test that checks whether the action is indeed called right
> away and that it is not called on the final drm_dev_put().
>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> ---
> drivers/gpu/drm/tests/drm_managed_test.c | 28 ++++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
>
> diff --git a/drivers/gpu/drm/tests/drm_managed_test.c b/drivers/gpu/drm/tests/drm_managed_test.c
> index c1fc1f0aac9b2..91863642efc13 100644
> --- a/drivers/gpu/drm/tests/drm_managed_test.c
> +++ b/drivers/gpu/drm/tests/drm_managed_test.c
> @@ -41,6 +41,33 @@ static void drm_test_managed_run_action(struct kunit *test)
> KUNIT_EXPECT_TRUE_MSG(test, priv->action_done, "Release action was not called");
> }
>
> +/*
> + * The test verifies that the release action is called immediately when
> + * drmm_release_action is called and that it is not called for a second time
> + * when the device is released.
> + */
> +static void drm_test_managed_release_action(struct kunit *test)
> +{
> + struct managed_test_priv *priv = test->priv;
> + int ret;
> +
> + ret = drmm_add_action_or_reset(priv->drm, drm_action, priv);
> + KUNIT_EXPECT_EQ(test, ret, 0);
> +
> + ret = drm_dev_register(priv->drm, 0);
> + KUNIT_ASSERT_EQ(test, ret, 0);
> +
> + drmm_release_action(priv->drm, drm_action, priv);
> + KUNIT_EXPECT_TRUE_MSG(test, priv->action_done, "Release action was not called");
> + priv->action_done = false;
> +
> + drm_dev_unregister(priv->drm);
> + drm_kunit_helper_free_device(test, priv->drm->dev);
> +
> + KUNIT_EXPECT_FALSE_MSG(test, priv->action_done,
> + "Unexpected release action call during cleanup");
> +}
> +
I guess we can have something simpler if we switch action_done to a
counter and just check that the counter didn't increase.
And I think the custom messages should be removed there too.
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Re: [PATCH v4 6/6] drm/tests: managed: Add a simple test for drmm_managed_release
2024-01-10 15:56 ` Maxime Ripard
@ 2024-01-15 17:11 ` Michał Winiarski
0 siblings, 0 replies; 13+ messages in thread
From: Michał Winiarski @ 2024-01-15 17:11 UTC (permalink / raw)
To: Maxime Ripard
Cc: intel-xe, dri-devel, linux-kernel, Rodrigo Vivi,
Michal Wajdeczko, Maarten Lankhorst, Thomas Zimmermann,
David Airlie, Daniel Vetter, Javier Martinez Canillas,
Maíra Canal
On Wed, Jan 10, 2024 at 04:56:27PM +0100, Maxime Ripard wrote:
> On Fri, Jan 05, 2024 at 11:13:24AM +0100, Michał Winiarski wrote:
> > Add a simple test that checks whether the action is indeed called right
> > away and that it is not called on the final drm_dev_put().
> >
> > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> > ---
> > drivers/gpu/drm/tests/drm_managed_test.c | 28 ++++++++++++++++++++++++
> > 1 file changed, 28 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/tests/drm_managed_test.c b/drivers/gpu/drm/tests/drm_managed_test.c
> > index c1fc1f0aac9b2..91863642efc13 100644
> > --- a/drivers/gpu/drm/tests/drm_managed_test.c
> > +++ b/drivers/gpu/drm/tests/drm_managed_test.c
> > @@ -41,6 +41,33 @@ static void drm_test_managed_run_action(struct kunit *test)
> > KUNIT_EXPECT_TRUE_MSG(test, priv->action_done, "Release action was not called");
> > }
> >
> > +/*
> > + * The test verifies that the release action is called immediately when
> > + * drmm_release_action is called and that it is not called for a second time
> > + * when the device is released.
> > + */
> > +static void drm_test_managed_release_action(struct kunit *test)
> > +{
> > + struct managed_test_priv *priv = test->priv;
> > + int ret;
> > +
> > + ret = drmm_add_action_or_reset(priv->drm, drm_action, priv);
> > + KUNIT_EXPECT_EQ(test, ret, 0);
> > +
> > + ret = drm_dev_register(priv->drm, 0);
> > + KUNIT_ASSERT_EQ(test, ret, 0);
> > +
> > + drmm_release_action(priv->drm, drm_action, priv);
> > + KUNIT_EXPECT_TRUE_MSG(test, priv->action_done, "Release action was not called");
> > + priv->action_done = false;
> > +
> > + drm_dev_unregister(priv->drm);
> > + drm_kunit_helper_free_device(test, priv->drm->dev);
> > +
> > + KUNIT_EXPECT_FALSE_MSG(test, priv->action_done,
> > + "Unexpected release action call during cleanup");
> > +}
> > +
>
> I guess we can have something simpler if we switch action_done to a
> counter and just check that the counter didn't increase.
>
> And I think the custom messages should be removed there too.
>
> Maxime
I'll drop the custom messages here and in the previous patch.
I'll also simplify this test in the way you suggested in previous
revision, to not check for release action call on cleanup.
Thanks,
-Michał
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2024-01-15 17:12 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-05 10:13 [PATCH v4 0/6] drm/managed: Add drmm_release_action Michał Winiarski
2024-01-05 10:13 ` [PATCH v4 1/6] " Michał Winiarski
2024-01-05 10:13 ` [PATCH v4 2/6] drm/tests: managed: Rename the suite name to match other DRM tests Michał Winiarski
2024-01-08 10:37 ` Maxime Ripard
2024-01-05 10:13 ` [PATCH v4 3/6] drm/tests: managed: Remove the waitqueue usage Michał Winiarski
2024-01-08 10:45 ` Maxime Ripard
2024-01-05 10:13 ` [PATCH v4 4/6] drm/tests: managed: Add comments and expect fail messages Michał Winiarski
2024-01-08 10:47 ` Maxime Ripard
2024-01-05 10:13 ` [PATCH v4 5/6] drm/tests: managed: Extract device initialization into test init Michał Winiarski
2024-01-08 10:52 ` Maxime Ripard
2024-01-05 10:13 ` [PATCH v4 6/6] drm/tests: managed: Add a simple test for drmm_managed_release Michał Winiarski
2024-01-10 15:56 ` Maxime Ripard
2024-01-15 17:11 ` Michał Winiarski
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).