On Tue, Sep 05, 2023 at 03:42:58PM +0200, Thomas Hellström wrote:

Hi, Maxime

On 9/5/23 15:16, Maxime Ripard wrote:

...

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@amd.com Cc: dri-devel@lists.freedesktop.org Signed-off-by: Thomas Hellström thomas.hellstrom@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?

Actually not. What's important here is that the call to drm_exec_fini(), which puts the last references to gobj1 and gobj2 doesn't trigger any lockdep splats, like the one in the commit message of patch 3/3. So to make more sense, the test could perhaps be conditioned on CONFIG_DEBUG_LOCK_ALLOC. Still it would require manual checking of dmesg() after being run.

I'm not aware of something to check on lockdep's status when running a kunit test, but I'm not sure anyone is expected to look at the dmesg trace when running kunit to find out whether the test succeeded or not.

It looks like there was an attempt at some point to fail the test if there was a lockdep error: https://lore.kernel.org/all/20200814205527.1833459-1-urielguajardojr@gmail.c...

It doesn't look like it's been merged though. David, Brendan, do you know why it wasn't merged or if there is a good option for us there?

At the very least, I think a comment after the call to drm_exec_fini to make it clear that the error would be in the kernel logs, and a better one on the test definition to explicitly say what you want to make sure of, and how one can check it's been done would be great.

Maxime