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
v4 -> v5:
- Simplify the release test (Maxime)
- Rename suite to "drm_managed" (Maxime)
- Drop redundant messages from asserts (Maxime)
Michał Winiarski (5):
drm/managed: Add drmm_release_action
drm/tests: managed: Rename the suite name to match other DRM tests
drm/tests: managed: Add comments about test intent
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 | 77 +++++++++++++++++++-----
include/drm/drm_managed.h | 4 ++
3 files changed, 104 insertions(+), 16 deletions(-)
--
2.43.0
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 <[email protected]>
Reviewed-by: Maxime Ripard <[email protected]>
---
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
DRM tests use "_" rather than "-" as word separator. Rename the test
suite to match other tests.
Signed-off-by: Michał Winiarski <[email protected]>
---
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..f85dada4de0a9 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_managed",
.test_cases = drm_managed_tests
};
--
2.43.0
Add comments explaining the intention behind the test and certain
implementation details related to device lifetime.
Signed-off-by: Michał Winiarski <[email protected]>
---
drivers/gpu/drm/tests/drm_managed_test.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/gpu/drm/tests/drm_managed_test.c b/drivers/gpu/drm/tests/drm_managed_test.c
index f85dada4de0a9..b5cf46d9f5cf8 100644
--- a/drivers/gpu/drm/tests/drm_managed_test.c
+++ b/drivers/gpu/drm/tests/drm_managed_test.c
@@ -24,6 +24,10 @@ static void drm_action(struct drm_device *drm, void *ptr)
wake_up_interruptible(&priv->action_wq);
}
+/*
+ * 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;
@@ -38,6 +42,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);
--
2.43.0
Add a simple test that checks whether the action is called when
drmm_managed_release is called.
Signed-off-by: Michał Winiarski <[email protected]>
---
drivers/gpu/drm/tests/drm_managed_test.c | 25 ++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/drivers/gpu/drm/tests/drm_managed_test.c b/drivers/gpu/drm/tests/drm_managed_test.c
index d936c879a4a30..76eb273c9b364 100644
--- a/drivers/gpu/drm/tests/drm_managed_test.c
+++ b/drivers/gpu/drm/tests/drm_managed_test.c
@@ -25,6 +25,30 @@ static void drm_action(struct drm_device *drm, void *ptr)
wake_up_interruptible(&priv->action_wq);
}
+/*
+ * The test verifies that the release action is called when
+ * drmm_release_action is called.
+ */
+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);
+ ret = wait_event_interruptible_timeout(priv->action_wq, priv->action_done,
+ msecs_to_jiffies(TEST_TIMEOUT_MS));
+ KUNIT_EXPECT_GT(test, ret, 0);
+
+ drm_dev_unregister(priv->drm);
+ drm_kunit_helper_free_device(test, priv->drm->dev);
+}
+
/*
* The test verifies that the release action is called automatically when the
* device is released.
@@ -75,6 +99,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
It simplifies the process of extending the test suite with additional
test cases without unnecessary duplication.
Signed-off-by: Michał Winiarski <[email protected]>
Acked-by: Maxime Ripard <[email protected]>
---
drivers/gpu/drm/tests/drm_managed_test.c | 41 +++++++++++++++---------
1 file changed, 26 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/tests/drm_managed_test.c b/drivers/gpu/drm/tests/drm_managed_test.c
index b5cf46d9f5cf8..d936c879a4a30 100644
--- a/drivers/gpu/drm/tests/drm_managed_test.c
+++ b/drivers/gpu/drm/tests/drm_managed_test.c
@@ -12,6 +12,7 @@
#define TEST_TIMEOUT_MS 100
struct managed_test_priv {
+ struct drm_device *drm;
bool action_done;
wait_queue_head_t action_wq;
};
@@ -29,11 +30,28 @@ 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);
+
+ ret = wait_event_interruptible_timeout(priv->action_wq, priv->action_done,
+ msecs_to_jiffies(TEST_TIMEOUT_MS));
+ KUNIT_EXPECT_GT(test, ret, 0);
+}
+
+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);
@@ -47,21 +65,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);
-
- ret = wait_event_interruptible_timeout(priv->action_wq, priv->action_done,
- msecs_to_jiffies(TEST_TIMEOUT_MS));
- KUNIT_EXPECT_GT(test, ret, 0);
+ return 0;
}
static struct kunit_case drm_managed_tests[] = {
@@ -71,6 +81,7 @@ static struct kunit_case drm_managed_tests[] = {
static struct kunit_suite drm_managed_test_suite = {
.name = "drm_managed",
+ .init = drm_managed_test_init,
.test_cases = drm_managed_tests
};
--
2.43.0
On Mon, 15 Jan 2024 18:13:47 +0100, Michał Winiarski wrote:
> Similar to devres equivalent, it allows to call the "release" action
> directly and remove the resource from the managed resources list.
>
>
Applied to drm/drm-misc (drm-misc-next).
Thanks!
Maxime
On Mon, 15 Jan 2024 18:13:48 +0100, Michał Winiarski wrote:
> DRM tests use "_" rather than "-" as word separator. Rename the test
> suite to match other tests.
>
>
Applied to drm/drm-misc (drm-misc-next).
Thanks!
Maxime
On Mon, 15 Jan 2024 18:13:49 +0100, Michał Winiarski wrote:
> Add comments explaining the intention behind the test and certain
> implementation details related to device lifetime.
>
>
Applied to drm/drm-misc (drm-misc-next).
Thanks!
Maxime
On Mon, 15 Jan 2024 18:13:50 +0100, Michał Winiarski wrote:
> It simplifies the process of extending the test suite with additional
> test cases without unnecessary duplication.
>
>
Applied to drm/drm-misc (drm-misc-next).
Thanks!
Maxime
On Mon, 15 Jan 2024 18:13:51 +0100, Michał Winiarski wrote:
> Add a simple test that checks whether the action is called when
> drmm_managed_release is called.
>
>
Applied to drm/drm-misc (drm-misc-next).
Thanks!
Maxime