Re: [RFC PATCH] drm/test: add a test suite for GEM objects backed by shmem

[Marco Pagani]

On 2023-10-24 11:23, Maxime Ripard wrote:
> Hi Marco,
> 
> On Mon, Oct 23, 2023 at 06:45:40PM +0200, Marco Pagani wrote:
>> This patch introduces an initial KUnit test suite for GEM objects
>> backed by shmem buffers.
>>
>> Signed-off-by: Marco Pagani <marpagan@xxxxxxxxxx>
>> ---
>>  drivers/gpu/drm/Kconfig                    |   1 +
>>  drivers/gpu/drm/tests/Makefile             |   3 +-
>>  drivers/gpu/drm/tests/drm_gem_shmem_test.c | 303 +++++++++++++++++++++
>>  3 files changed, 306 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/gpu/drm/tests/drm_gem_shmem_test.c
>>
>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> index 3eee8636f847..f0a77e3e04d7 100644
>> --- a/drivers/gpu/drm/Kconfig
>> +++ b/drivers/gpu/drm/Kconfig
>> @@ -84,6 +84,7 @@ config DRM_KUNIT_TEST
>>  	select DRM_EXPORT_FOR_TESTS if m
>>  	select DRM_KUNIT_TEST_HELPERS
>>  	select DRM_EXEC
>> +	select DRM_GEM_SHMEM_HELPER
> 
> I know that DRM_EXEC already stands out, but these should be ordered
> alphabetically, so it should be before DRM_KUNIT_TEST_HELPERS.
> 
>>  	default KUNIT_ALL_TESTS
>>  	help
>>  	  This builds unit tests for DRM. This option is not useful for
>> diff --git a/drivers/gpu/drm/tests/Makefile b/drivers/gpu/drm/tests/Makefile
>> index ba7baa622675..b8227aab369e 100644
>> --- a/drivers/gpu/drm/tests/Makefile
>> +++ b/drivers/gpu/drm/tests/Makefile
>> @@ -18,6 +18,7 @@ obj-$(CONFIG_DRM_KUNIT_TEST) += \
>>  	drm_plane_helper_test.o \
>>  	drm_probe_helper_test.o \
>>  	drm_rect_test.o	\
>> -	drm_exec_test.o
>> +	drm_exec_test.o \
>> +	drm_gem_shmem_test.o
> 
> Ditto.
>

Right, I'll sort them alphabetically for v2.

>>  CFLAGS_drm_mm_test.o := $(DISABLE_STRUCTLEAK_PLUGIN)
>> diff --git a/drivers/gpu/drm/tests/drm_gem_shmem_test.c b/drivers/gpu/drm/tests/drm_gem_shmem_test.c
>> new file mode 100644
>> index 000000000000..0bf6727f08d2
>> --- /dev/null
>> +++ b/drivers/gpu/drm/tests/drm_gem_shmem_test.c
>> @@ -0,0 +1,303 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * KUnit test suite for GEM objects backed by shmem buffers
>> + *
>> + * Copyright (C) 2023 Red Hat, Inc.
>> + *
>> + * Author: Marco Pagani <marpagan@xxxxxxxxxx>
>> + */
>> +
>> +#include <linux/dma-buf.h>
>> +#include <linux/iosys-map.h>
>> +#include <linux/sizes.h>
>> +
>> +#include <kunit/test.h>
>> +
>> +#include <drm/drm_device.h>
>> +#include <drm/drm_drv.h>
>> +#include <drm/drm_gem.h>
>> +#include <drm/drm_gem_shmem_helper.h>
>> +#include <drm/drm_kunit_helpers.h>
>> +
>> +#define TEST_SIZE		SZ_1M
>> +#define TEST_BYTE		0xae
>> +
>> +struct fake_dev {
>> +	struct drm_device drm_dev;
>> +	struct device *dev;
>> +};
>> +
>> +/* Test creating a shmem GEM object */
>> +static void drm_gem_shmem_test_obj_create(struct kunit *test)
>> +{
>> +	struct fake_dev *fdev = test->priv;
>> +	struct drm_gem_shmem_object *shmem;
>> +
>> +	shmem = drm_gem_shmem_create(&fdev->drm_dev, TEST_SIZE);
>> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, shmem);
>> +	KUNIT_ASSERT_EQ(test, shmem->base.size, TEST_SIZE);
>> +	KUNIT_ASSERT_NOT_NULL(test, shmem->base.filp);
>> +	KUNIT_ASSERT_NOT_NULL(test, shmem->base.funcs);
>> +
>> +	drm_gem_shmem_free(shmem);
>> +}
>> +
>> +/* Test creating a private shmem GEM object from a scatter/gather table */
> 
> Thanks for documenting those tests. I believe we should also document
> what we expect from the test: should the test succeed? fail? if it
> fails, what is the cause of failure?
> 
> Based on the following test, I think something like the following would
> be good:
> 
> Test that creating a private shmem GEM object from a previously
> allocated sg_table succeeds and is properly initialized
> 
> Feel free to change it to something else if you find something missing.
>

Sounds good to me. I'll improve the documentation for v2.

>> +static void drm_gem_shmem_test_obj_create_private(struct kunit *test)
>> +{
>> +	struct fake_dev *fdev = test->priv;
>> +	struct drm_gem_shmem_object *shmem;
>> +	struct drm_gem_object *gem_obj;
>> +	struct dma_buf buf_mock;
>> +	struct dma_buf_attachment attach_mock;
>> +	struct sg_table *sgt;
>> +	char *buf;
>> +	int ret;
>> +
>> +	/* Create a mock scatter/gather table */
>> +	buf = kunit_kzalloc(test, TEST_SIZE, GFP_KERNEL);
>> +	KUNIT_ASSERT_NOT_NULL(test, buf);
>> +
>> +	sgt = kzalloc(sizeof(*sgt), GFP_KERNEL);
>> +	KUNIT_ASSERT_NOT_NULL(test, sgt);
>> +
>> +	ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
>> +	KUNIT_ASSERT_EQ(test, ret, 0);
>> +	sg_init_one(sgt->sgl, buf, TEST_SIZE);
>> +
>> +	/* Init a mock DMA-BUF */
>> +	buf_mock.size = TEST_SIZE;
>> +	attach_mock.dmabuf = &buf_mock;
>> +
>> +	gem_obj = drm_gem_shmem_prime_import_sg_table(&fdev->drm_dev, &attach_mock, sgt);
>> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, gem_obj);
>> +	KUNIT_ASSERT_EQ(test, gem_obj->size, TEST_SIZE);
>> +	KUNIT_ASSERT_NULL(test, gem_obj->filp);
>> +	KUNIT_ASSERT_NOT_NULL(test, gem_obj->funcs);
>> +
>> +	shmem = to_drm_gem_shmem_obj(gem_obj);
>> +	KUNIT_ASSERT_PTR_EQ(test, shmem->sgt, sgt);
>> +
>> +	/* The scatter/gather table is freed by drm_gem_shmem_free */
>> +	drm_gem_shmem_free(shmem);
>> +}
> 
> KUNIT_ASSERT_* will stop the execution of the test on failure, you
> should probably use a bit more of KUNIT_EXPECT_* calls otherwise you'll
> leak resources.
> 
> You also probably want to use a kunit_action to clean up and avoid that
> whole discussion
>

You are right. I slightly prefer using KUnit expectations (unless actions
are strictly necessary) since I feel using actions makes test cases a bit
less straightforward to understand. Is this okay for you?

>> +
>> +/* Test pinning backing pages */
>> +static void drm_gem_shmem_test_pin_pages(struct kunit *test)
>> +{
>> +	struct fake_dev *fdev = test->priv;
>> +	struct drm_gem_shmem_object *shmem;
>> +	int i, ret;
>> +
>> +	shmem = drm_gem_shmem_create(&fdev->drm_dev, TEST_SIZE);
>> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, shmem);
>> +	KUNIT_ASSERT_NULL(test, shmem->pages);
>> +	KUNIT_ASSERT_EQ(test, shmem->pages_use_count, 0);
>> +
>> +	ret = drm_gem_shmem_pin(shmem);
>> +	KUNIT_ASSERT_EQ(test, ret, 0);
>> +	KUNIT_ASSERT_NOT_NULL(test, shmem->pages);
>> +	KUNIT_ASSERT_EQ(test, shmem->pages_use_count, 1);
>> +
>> +	for (i = 0; i < (shmem->base.size >> PAGE_SHIFT); i++)
>> +		KUNIT_ASSERT_NOT_NULL(test, shmem->pages[i]);
>> +
>> +	drm_gem_shmem_unpin(shmem);
>> +	KUNIT_ASSERT_NULL(test, shmem->pages);
>> +	KUNIT_ASSERT_EQ(test, shmem->pages_use_count, 0);
>> +
>> +	drm_gem_shmem_free(shmem);
>> +}
>> +
>> +/* Test creating a virtual mapping */
>> +static void drm_gem_shmem_test_vmap(struct kunit *test)
>> +{
>> +	struct fake_dev *fdev = test->priv;
>> +	struct drm_gem_shmem_object *shmem;
>> +	struct iosys_map map;
>> +	int ret, i;
>> +
>> +	shmem = drm_gem_shmem_create(&fdev->drm_dev, TEST_SIZE);
>> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, shmem);
>> +	KUNIT_ASSERT_NULL(test, shmem->vaddr);
>> +	KUNIT_ASSERT_EQ(test, shmem->vmap_use_count, 0);
>> +
>> +	ret = drm_gem_shmem_vmap(shmem, &map);
>> +	KUNIT_ASSERT_EQ(test, ret, 0);
>> +	KUNIT_ASSERT_NOT_NULL(test, shmem->vaddr);
>> +	KUNIT_ASSERT_EQ(test, shmem->vmap_use_count, 1);
>> +	KUNIT_ASSERT_FALSE(test, iosys_map_is_null(&map));
>> +
>> +	iosys_map_memset(&map, 0, TEST_BYTE, TEST_SIZE);
>> +	for (i = 0; i < TEST_SIZE; i++)
>> +		KUNIT_ASSERT_EQ(test, iosys_map_rd(&map, i, u8), TEST_BYTE);
>> +
>> +	drm_gem_shmem_vunmap(shmem, &map);
>> +	KUNIT_ASSERT_NULL(test, shmem->vaddr);
>> +	KUNIT_ASSERT_EQ(test, shmem->vmap_use_count, 0);
>> +
>> +	drm_gem_shmem_free(shmem);
>> +}
>> +
>> +/* Test exporting a scatter/gather table */
>> +static void drm_gem_shmem_test_get_pages_sgt(struct kunit *test)
>> +{
>> +	struct fake_dev *fdev = test->priv;
>> +	struct drm_gem_shmem_object *shmem;
>> +	struct sg_table *sgt;
>> +	struct scatterlist *sg;
>> +	unsigned int si, len = 0;
>> +	int ret;
>> +
>> +	shmem = drm_gem_shmem_create(&fdev->drm_dev, TEST_SIZE);
>> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, shmem);
>> +
>> +	ret = drm_gem_shmem_pin(shmem);
>> +	KUNIT_ASSERT_EQ(test, ret, 0);
>> +
>> +	sgt = drm_gem_shmem_get_sg_table(shmem);
>> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, sgt);
>> +	KUNIT_ASSERT_NULL(test, shmem->sgt);
>> +
>> +	for_each_sgtable_sg(sgt, sg, si) {
>> +		KUNIT_ASSERT_NOT_NULL(test, sg);
>> +		len += sg->length;
>> +	}
>> +	KUNIT_ASSERT_GE(test, len, TEST_SIZE);
>> +
>> +	kfree(sgt);
>> +	drm_gem_shmem_free(shmem);
>> +}
>> +
>> +/* Test exporting a scatter/gather pinned table for PRIME */
>> +static void drm_gem_shmem_test_get_sg_table(struct kunit *test)
>> +{
>> +	struct fake_dev *fdev = test->priv;
>> +	struct drm_gem_shmem_object *shmem;
>> +	struct sg_table *sgt;
>> +	struct scatterlist *sg;
>> +	unsigned int si, len = 0;
>> +
>> +	shmem = drm_gem_shmem_create(&fdev->drm_dev, TEST_SIZE);
>> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, shmem);
>> +
>> +	sgt = drm_gem_shmem_get_pages_sgt(shmem);
>> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, sgt);
>> +	KUNIT_ASSERT_PTR_EQ(test, sgt, shmem->sgt);
>> +
>> +	for_each_sgtable_sg(sgt, sg, si) {
>> +		KUNIT_ASSERT_NOT_NULL(test, sg);
>> +		len += sg->length;
>> +	}
>> +	KUNIT_ASSERT_GE(test, len, TEST_SIZE);
>> +
>> +	/* The scatter/gather table is freed by drm_gem_shmem_free */
>> +	drm_gem_shmem_free(shmem);
>> +}
>> +
>> +/* Test updating madvise status */
>> +static void drm_gem_shmem_test_madvise(struct kunit *test)
>> +{
>> +	struct fake_dev *fdev = test->priv;
>> +	struct drm_gem_shmem_object *shmem;
>> +	int ret;
>> +
>> +	shmem = drm_gem_shmem_create(&fdev->drm_dev, TEST_SIZE);
>> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, shmem);
>> +	KUNIT_ASSERT_EQ(test, shmem->madv, 0);
>> +
>> +	ret = drm_gem_shmem_madvise(shmem, 1);
>> +	KUNIT_ASSERT_TRUE(test, ret);
>> +	KUNIT_ASSERT_EQ(test, shmem->madv, 1);
>> +
>> +	ret = drm_gem_shmem_madvise(shmem, -1);
>> +	KUNIT_ASSERT_FALSE(test, ret);
>> +	KUNIT_ASSERT_EQ(test, shmem->madv, -1);
>> +
>> +	ret = drm_gem_shmem_madvise(shmem, 0);
>> +	KUNIT_ASSERT_FALSE(test, ret);
>> +	KUNIT_ASSERT_EQ(test, shmem->madv, -1);
>> +
>> +	drm_gem_shmem_free(shmem);
>> +}
>> +
>> +/* Test purging */
>> +static void drm_gem_shmem_test_purge(struct kunit *test)
>> +{
>> +	struct fake_dev *fdev = test->priv;
>> +	struct drm_gem_shmem_object *shmem;
>> +	struct sg_table *sgt;
>> +	int ret;
>> +
>> +	shmem = drm_gem_shmem_create(&fdev->drm_dev, TEST_SIZE);
>> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, shmem);
>> +
>> +	ret = drm_gem_shmem_is_purgeable(shmem);
>> +	KUNIT_ASSERT_FALSE(test, ret);
>> +
>> +	ret = drm_gem_shmem_madvise(shmem, 1);
>> +	KUNIT_ASSERT_TRUE(test, ret);
>> +
>> +	sgt = drm_gem_shmem_get_pages_sgt(shmem);
>> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, sgt);
>> +
>> +	ret = drm_gem_shmem_is_purgeable(shmem);
>> +	KUNIT_ASSERT_TRUE(test, ret);
>> +
>> +	drm_gem_shmem_purge(shmem);
>> +	KUNIT_ASSERT_TRUE(test, ret);
>> +
>> +	drm_gem_shmem_free(shmem);
>> +}
>> +
>> +static int drm_gem_shmem_test_init(struct kunit *test)
>> +{
>> +	struct fake_dev *fdev;
>> +	struct device *dev;
>> +
>> +	/* Allocate a parent device */
>> +	dev = drm_kunit_helper_alloc_device(test);
>> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev);
>> +
>> +	/*
>> +	 * The DRM core will automatically initialize the GEM core and create
>> +	 * a DRM Memory Manager object which provides an address space pool
>> +	 * for GEM objects allocation.
>> +	 */
>> +	fdev = drm_kunit_helper_alloc_drm_device(test, dev, struct fake_dev,
>> +						 drm_dev, DRIVER_GEM);
>> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, fdev);
>> +
>> +	fdev->dev = dev;
>> +	test->priv = fdev;
>> +
>> +	return 0;
>> +}
>> +
>> +static void drm_gem_shmem_test_exit(struct kunit *test)
>> +{
>> +	struct fake_dev *fdev = test->priv;
>> +
>> +	drm_kunit_helper_free_device(test, fdev->dev);
>> +}
> 
> You don't need to call drm_kunit_helper_free_device() anymore
> 
> Maxime

Right. Initially, I accidentally used an older tree that did not include
commit 4f2b0b583baa4. I'll remove the whole exit function for v2.


Thanks,
Marco