Re: [PATCH v3 3/3] drm/tests: managed: Add a simple test for drmm_managed_release

[Michał Winiarski]

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