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@xxxxxxxxx>
> > ---
> > 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
