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)
Michał Winiarski (3):
drm/managed: Add drmm_release_action
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 | 63 ++++++++++++++++++------
include/drm/drm_managed.h | 4 ++
3 files changed, 91 insertions(+), 15 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]>
---
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
It simplifies the process of extending the test suite with additional
test cases without unnecessary duplication.
Additionally, store drm_device inside priv to simplify the lifetime
management by tying priv lifetime with parent struct device.
Signed-off-by: Michał Winiarski <[email protected]>
---
drivers/gpu/drm/tests/drm_managed_test.c | 43 ++++++++++++++----------
1 file changed, 26 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/tests/drm_managed_test.c b/drivers/gpu/drm/tests/drm_managed_test.c
index 1652dca11d30c..cabe6360aef71 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;
};
@@ -26,35 +27,42 @@ static void drm_action(struct drm_device *drm, void *ptr)
static void drm_test_managed_run_action(struct kunit *test)
{
- struct managed_test_priv *priv;
- struct drm_device *drm;
- struct device *dev;
+ struct managed_test_priv *priv = test->priv;
int ret;
- 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);
-
- drm = __drm_kunit_helper_alloc_drm_device(test, dev, sizeof(*drm), 0, DRIVER_MODESET);
- KUNIT_ASSERT_NOT_ERR_OR_NULL(test, drm);
-
- ret = drmm_add_action_or_reset(drm, drm_action, priv);
+ ret = drmm_add_action_or_reset(&priv->drm, drm_action, priv);
KUNIT_EXPECT_EQ(test, ret, 0);
- ret = drm_dev_register(drm, 0);
+ ret = drm_dev_register(&priv->drm, 0);
KUNIT_ASSERT_EQ(test, ret, 0);
- drm_dev_unregister(drm);
- drm_kunit_helper_free_device(test, dev);
+ 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 device *dev;
+
+ dev = drm_kunit_helper_alloc_device(test);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev);
+
+ priv = drm_kunit_helper_alloc_drm_device(test, dev, struct managed_test_priv, drm,
+ DRIVER_MODESET);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+
+ init_waitqueue_head(&priv->action_wq);
+
+ test->priv = priv;
+
+ return 0;
+}
+
static struct kunit_case drm_managed_tests[] = {
KUNIT_CASE(drm_test_managed_run_action),
{}
@@ -62,6 +70,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
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 <[email protected]>
---
drivers/gpu/drm/tests/drm_managed_test.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/drivers/gpu/drm/tests/drm_managed_test.c b/drivers/gpu/drm/tests/drm_managed_test.c
index cabe6360aef71..8dfbea21c35c5 100644
--- a/drivers/gpu/drm/tests/drm_managed_test.c
+++ b/drivers/gpu/drm/tests/drm_managed_test.c
@@ -44,6 +44,29 @@ static void drm_test_managed_run_action(struct kunit *test)
KUNIT_EXPECT_GT(test, ret, 0);
}
+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(test, priv->action_done);
+ priv->action_done = false;
+
+ 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_EQ(test, ret, 0);
+}
+
static int drm_managed_test_init(struct kunit *test)
{
struct managed_test_priv *priv;
@@ -65,6 +88,7 @@ static int drm_managed_test_init(struct kunit *test)
static struct kunit_case drm_managed_tests[] = {
KUNIT_CASE(drm_test_managed_run_action),
+ KUNIT_CASE(drm_test_managed_release_action),
{}
};
--
2.43.0
Hi,
Thanks for the rework
On Tue, Dec 05, 2023 at 02:22:10AM +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 <[email protected]>
> ---
> drivers/gpu/drm/tests/drm_managed_test.c | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/drivers/gpu/drm/tests/drm_managed_test.c b/drivers/gpu/drm/tests/drm_managed_test.c
> index cabe6360aef71..8dfbea21c35c5 100644
> --- a/drivers/gpu/drm/tests/drm_managed_test.c
> +++ b/drivers/gpu/drm/tests/drm_managed_test.c
> @@ -44,6 +44,29 @@ static void drm_test_managed_run_action(struct kunit *test)
> KUNIT_EXPECT_GT(test, ret, 0);
> }
>
We should have a description of the intent of the test here.
> +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(test, priv->action_done);
> + priv->action_done = false;
> +
> + 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_EQ(test, ret, 0);
This tests that we have reached the timeout, thus the action never ran.
It's not clear to me what your intent is here.
This test is:
- Registering an action
- Registering the DRM device
- Calling drmm_release_action on the previously registered action
- Checking that the action has been run
- Clearing the flag saying the action has been run
- Unregistering the DRM device
- Freeing the DRM device
- Waiting for the action to run, but expecting it to never do?
I guess something like
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_ASSERT_EQ(test, ret, 0);
KUNIT_ASSERT_FALSE(test, priv->action_done);
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);
KUNIT_EXPECT_TRUE(test, priv->action_done);
}
would be enough?
> +}
> +
> static int drm_managed_test_init(struct kunit *test)
> {
> struct managed_test_priv *priv;
> @@ -65,6 +88,7 @@ static int drm_managed_test_init(struct kunit *test)
>
> static struct kunit_case drm_managed_tests[] = {
> KUNIT_CASE(drm_test_managed_run_action),
> + KUNIT_CASE(drm_test_managed_release_action),
Also, tests should be organized by alphabetical order
Maxime
On Tue, Dec 05, 2023 at 02:50:22PM +0100, Maxime Ripard wrote:
> Hi,
>
> Thanks for the rework
>
> On Tue, Dec 05, 2023 at 02:22:10AM +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 <[email protected]>
> > ---
> > drivers/gpu/drm/tests/drm_managed_test.c | 24 ++++++++++++++++++++++++
> > 1 file changed, 24 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/tests/drm_managed_test.c b/drivers/gpu/drm/tests/drm_managed_test.c
> > index cabe6360aef71..8dfbea21c35c5 100644
> > --- a/drivers/gpu/drm/tests/drm_managed_test.c
> > +++ b/drivers/gpu/drm/tests/drm_managed_test.c
> > @@ -44,6 +44,29 @@ static void drm_test_managed_run_action(struct kunit *test)
> > KUNIT_EXPECT_GT(test, ret, 0);
> > }
> >
>
> We should have a description of the intent of the test here.
The test checks that the release action is called immediately after
calling drmm_release_action, and that it is successfully removed from
the list of resources managed by DRMM (by verifying that release action
is not called upon device cleanup).
Would it be enough to expand the messages in KUNIT_EXPECT?
>
> > +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(test, priv->action_done);
KUNIT_EXPECT_TRUE_MSG(test, priv->action, "Release action was not called");
> > + priv->action_done = false;
> > +
> > + 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_EQ(test, ret, 0);
KUNIT_EXPECT_EQ_MSG(test, ret, 0, "Unexpected release action call during cleanup");
Or a comment on top? Kerneldoc? Not sure what the recommendation is, as
most (all?) tests in DRM don't have a description.
>
> This tests that we have reached the timeout, thus the action never ran.
That's the intent, yes.
> It's not clear to me what your intent is here.
>
> This test is:
>
> - Registering an action
> - Registering the DRM device
> - Calling drmm_release_action on the previously registered action
> - Checking that the action has been run
> - Clearing the flag saying the action has been run
> - Unregistering the DRM device
> - Freeing the DRM device
> - Waiting for the action to run, but expecting it to never do?
>
> I guess something like
>
> 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_ASSERT_EQ(test, ret, 0);
>
> KUNIT_ASSERT_FALSE(test, priv->action_done);
>
> 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);
> KUNIT_EXPECT_TRUE(test, priv->action_done);
> }
>
> would be enough?
It would only check that the action is called immediately, but not that
it was removed from the managed resources list. And we don't need to
wait for that - it should be called immediately.
We do need to wait when we expect it to be called (or not to be called -
in which case we expect a timeout) as part of device cleanup, otherwise
we would get a false positive when delayed release
(CONFIG_DEBUG_KOBJECT_RELEASE) is used.
I assumed that this was the intention behind introducing waitqueue in
drm_test_managed_run_action, is my understanding correct?
And BTW, now that I'm thinking about resource lifetimes. We can't really
tie priv lifetime with the device. It introduces use-after-free in both
tests, when checking if the action was called after
drm_kunit_helper_free_device() has a chance to invoke device cleanup.
Priv should be gone at that point, so I think we should go back to test
init from v1. Do you agree?
>
> > +}
> > +
> > static int drm_managed_test_init(struct kunit *test)
> > {
> > struct managed_test_priv *priv;
> > @@ -65,6 +88,7 @@ static int drm_managed_test_init(struct kunit *test)
> >
> > static struct kunit_case drm_managed_tests[] = {
> > KUNIT_CASE(drm_test_managed_run_action),
> > + KUNIT_CASE(drm_test_managed_release_action),
>
> Also, tests should be organized by alphabetical order
Sure, I'll reorder it. I wasn't aware of that recommendation, as most of
the tests in DRM don't follow it.
Thanks,
-Michał
>
> Maxime