[Intel-xe] [PATCH 2/3] drm/tests/drm_exec: Add a test for object freeing within drm_exec_fini()

[Maxime Ripard]

On Tue, Sep 05, 2023 at 02:32:38PM +0200, Thomas Hellström wrote:
> Hi,
> 
> On 9/5/23 14:05, Maxime Ripard wrote:
> > Hi,
> > 
> > On Tue, Sep 05, 2023 at 10:58:31AM +0200, Thomas Hellström wrote:
> > > Check that object freeing from within drm_exec_fini() works as expected
> > > and doesn't generate any warnings.
> > > 
> > > Cc: Christian König <christian.koenig at amd.com>
> > > Cc: dri-devel at lists.freedesktop.org
> > > Signed-off-by: Thomas Hellström <thomas.hellstrom at linux.intel.com>
> > > ---
> > >   drivers/gpu/drm/tests/drm_exec_test.c | 47 +++++++++++++++++++++++++++
> > >   1 file changed, 47 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/tests/drm_exec_test.c b/drivers/gpu/drm/tests/drm_exec_test.c
> > > index 563949d777dd..294c25f49cc7 100644
> > > --- a/drivers/gpu/drm/tests/drm_exec_test.c
> > > +++ b/drivers/gpu/drm/tests/drm_exec_test.c
> > > @@ -170,6 +170,52 @@ static void test_prepare_array(struct kunit *test)
> > >   	drm_gem_private_object_fini(&gobj2);
> > >   }
> > > +static const struct drm_gem_object_funcs put_funcs = {
> > > +	.free = (void *)kfree,
> > > +};
> > > +
> > > +/*
> > > + * Check that freeing objects from within drm_exec_fini()
> > > + * behaves as expected.
> > > + */
> > > +static void test_early_put(struct kunit *test)
> > > +{
> > > +	struct drm_exec_priv *priv = test->priv;
> > > +	struct drm_gem_object *gobj1;
> > > +	struct drm_gem_object *gobj2;
> > > +	struct drm_gem_object *array[2];
> > > +	struct drm_exec exec;
> > > +	int ret;
> > > +
> > > +	gobj1 = kzalloc(sizeof(*gobj1), GFP_KERNEL);
> > > +	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, gobj1);
> > > +	if (!gobj1)
> > > +		return;
> > > +
> > > +	gobj2 = kzalloc(sizeof(*gobj2), GFP_KERNEL);
> > > +	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, gobj2);
> > > +	if (!gobj2) {
> > > +		kfree(gobj1);
> > > +		return;
> > > +	}
> > > +
> > > +	gobj1->funcs = &put_funcs;
> > > +	gobj2->funcs = &put_funcs;
> > > +	array[0] = gobj1;
> > > +	array[1] = gobj2;
> > > +	drm_gem_private_object_init(priv->drm, gobj1, PAGE_SIZE);
> > > +	drm_gem_private_object_init(priv->drm, gobj2, PAGE_SIZE);
> > > +
> > > +	drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT);
> > > +	drm_exec_until_all_locked(&exec)
> > > +		ret = drm_exec_prepare_array(&exec, array, ARRAY_SIZE(array),
> > > +					     1);
> > > +	KUNIT_EXPECT_EQ(test, ret, 0);
> > > +	drm_gem_object_put(gobj1);
> > > +	drm_gem_object_put(gobj2);
> > > +	drm_exec_fini(&exec);
> > It doesn't look like you actually check that "freeing objects from
> > within drm_exec_fini() behaves as expected." What is the expectation
> > here, and how is it checked?
> 
> Hm. Good question, I've been manually checking dmesg for lockdep splats. Is
> there a way to automate that?

I'm not familiar with the drm_exec API, but judging by the code I'd
assume you want to check that gobj1 and gobj2 are actually freed using
kfree?

If so, I've used tested for that by creating a waitqueue and completing
it from the free function. You won't be certain that you have gone
through kfree, but you'll know that drm_gem_object_funcs.free will have
been called which is what you actually care about I think?

So something along those lines would work I think:

struct test_gem_object {
	struct drm_gem_object base;
	wait_queue_head_t freed_wq;
	bool freed_done;
};

void free_test_gem_object(struct drm_gem_object *obj)
{
	struct test_gem_object *test_obj =
		container_of(obj, struct test_gem_object, base)

	test_obj->freed_done = true;
	wake_up(&test_obj->freed_wq);
};

static const struct drm_gem_object_funcs put_funcs = {
	.free = free_test_gem_object,
}

static void test_early_put(struct kunit *test)
{
	struct test_gem_object *gobj1;
	...
	gobj1 = kunit_kzalloc(test, sizeof(*gobj1), GFP_KERNEL);
	...
	gobj1->base.funcs = &put_funcs;
	...
	array[0] = &gobj1->base;

	drm_exec_fini(&exec);

	ret = wait_event_interruptible_timeout(gobj1->freed_wq, gobj1->freed_done,
                                               msecs_to_jiffies(TIMEOUT_MS));
	KUNIT_EXPECT_GT(test, ret, 0);
}

I guess?

Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20230905/4216739f/attachment.sig>