| Message ID | 20231211220939.215024-4-michal.winiarski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/managed: Add drmm_release_action | expand |
Hi, On Mon, Dec 11, 2023 at 11:09:39PM +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 | 29 ++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/drivers/gpu/drm/tests/drm_managed_test.c b/drivers/gpu/drm/tests/drm_managed_test.c > index 15bd2474440b5..ef5e784afbc6d 100644 > --- a/drivers/gpu/drm/tests/drm_managed_test.c > +++ b/drivers/gpu/drm/tests/drm_managed_test.c > @@ -48,6 +48,34 @@ static void drm_test_managed_run_action(struct kunit *test) > KUNIT_EXPECT_GT_MSG(test, ret, 0, "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. > + */ Thanks, it's much clearer now. > +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; We should call wait_event_* here. > + > + 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_MSG(test, ret, 0, "Unexpected release action call during cleanup"); > +} > + Tests should in general be as fast as possible. Waiting for 100ms for the success case is not ok. We have ~500 tests at the moment, if every test was doing that it would take at least 50s to run all our unit tests, while it takes less than a second at the moment on a capable machine. And also, I'm not sure we actually need to make sure it never happened. If only because nothing actually guarantees it wouldn't have happened after the timeout anyway, so the test isn't definitive. I guess what we could test is whether the action is still in the actions list through a function only exported to tests. If it's no longer in the action list, then it won't be run. But unless we ever have a bug, I'm not sure it's worth testing for that. Maxime
On Fri, Dec 15, 2023 at 05:31:38PM +0100, Maxime Ripard wrote: > Hi, > > On Mon, Dec 11, 2023 at 11:09:39PM +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 | 29 ++++++++++++++++++++++++ > > 1 file changed, 29 insertions(+) > > > > diff --git a/drivers/gpu/drm/tests/drm_managed_test.c b/drivers/gpu/drm/tests/drm_managed_test.c > > index 15bd2474440b5..ef5e784afbc6d 100644 > > --- a/drivers/gpu/drm/tests/drm_managed_test.c > > +++ b/drivers/gpu/drm/tests/drm_managed_test.c > > @@ -48,6 +48,34 @@ static void drm_test_managed_run_action(struct kunit *test) > > KUNIT_EXPECT_GT_MSG(test, ret, 0, "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. > > + */ > > Thanks, it's much clearer now. > > > +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; > > We should call wait_event_* here. > > > + > > + 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_MSG(test, ret, 0, "Unexpected release action call during cleanup"); > > +} > > + > > Tests should in general be as fast as possible. Waiting for 100ms for > the success case is not ok. We have ~500 tests at the moment, if every > test was doing that it would take at least 50s to run all our unit > tests, while it takes less than a second at the moment on a capable > machine. > > And also, I'm not sure we actually need to make sure it never happened. > If only because nothing actually guarantees it wouldn't have happened > after the timeout anyway, so the test isn't definitive. There's no difference in that regard (test being definitive) between drm_test_managed_release_action and pre-existing drm_test_managed_run_action. If the action is executed after the timeout, with run_action we're going to get a false-negative result, and with release_action we're going to get a false-positive result. > I guess what we could test is whether the action is still in the actions > list through a function only exported to tests. If it's no longer in the > action list, then it won't be run. That would require digging into implementation details rather than focusing on the interface, which, in my opinion, is not a very good approach. In the next revision I'll drop the waitqueue completely. If that won't work, I also have a variant that uses bus notifier to make both tests more definitive. Thanks, -Michał > But unless we ever have a bug, I'm not sure it's worth testing for that. > > Maxime
diff --git a/drivers/gpu/drm/tests/drm_managed_test.c b/drivers/gpu/drm/tests/drm_managed_test.c index 15bd2474440b5..ef5e784afbc6d 100644 --- a/drivers/gpu/drm/tests/drm_managed_test.c +++ b/drivers/gpu/drm/tests/drm_managed_test.c @@ -48,6 +48,34 @@ static void drm_test_managed_run_action(struct kunit *test) KUNIT_EXPECT_GT_MSG(test, ret, 0, "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); + + ret = wait_event_interruptible_timeout(priv->action_wq, priv->action_done, + msecs_to_jiffies(TEST_TIMEOUT_MS)); + KUNIT_EXPECT_EQ_MSG(test, ret, 0, "Unexpected release action call during cleanup"); +} + static int drm_managed_test_init(struct kunit *test) { struct managed_test_priv *priv; @@ -76,6 +104,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), {} };
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 | 29 ++++++++++++++++++++++++ 1 file changed, 29 insertions(+)