| Message ID | 20231023164541.92913-1-marpagan@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] drm/test: add a test suite for GEM objects backed by shmem | expand |
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@redhat.com> > --- > 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. > 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@redhat.com> > + */ > + > +#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. > +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 > + > +/* 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
On 2023-10-25 10:43, Maxime Ripard wrote: > Hi, > > On Tue, Oct 24, 2023 at 07:14:25PM +0200, Marco Pagani wrote: >>>> +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? > > I disagree. Actions make it easier to reason about, even when comparing > assertion vs expectation > > Like, for the call to sg_alloc_table and > drm_gem_shmem_prime_import_sg_table(), the reasonable use of assert vs > expect would be something like: > > 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); > > /* > * Here, it's already not super clear whether you want to expect vs > * assert. expect will make you handle the failure case later, assert will > * force you to call kfree on sgt. Both kind of suck in their own ways. > */ > > sg_init_one(sgt->sgl, buf, TEST_SIZE); > > gem_obj = drm_gem_shmem_prime_import_sg_table(&fdev->drm_dev, &attach_mock, sgt); > KUNIT_ASSERT_NOT_ERR_OR_NULL(test, gem_obj); > > /* > * If the assert fails, we forgot to call sg_free_table(sgt) and kfree(sgt). > */ > > KUNIT_EXPECT_EQ(test, gem_obj->size, TEST_SIZE); > KUNIT_EXPECT_NULL(test, gem_obj->filp); > KUNIT_EXPECT_NOT_NULL(test, gem_obj->funcs); > > /* > * And here we have to handle the case where the expectation was wrong, > * but the test still continued. > */ > > But if you're not using an action, you still have to call kfree(sgt), > which means that you might still > > shmem = to_drm_gem_shmem_obj(gem_obj); > KUNIT_ASSERT_PTR_EQ(test, shmem->sgt, sgt); > > /* > * If the assertion fails, we now have to call drm_gem_shmem_free(shmem) > */ > > /* The scatter/gather table is freed by drm_gem_shmem_free */ > drm_gem_shmem_free(shmem); > > /* everything's fine now */ > > The semantics around drm_gem_shmem_free make it a bit convoluted, but > doing it using goto/labels, plus handling the assertions and error > reporting would be difficult. > > Using actions, we have: > > sgt = kzalloc(sizeof(*sgt), GFP_KERNEL); > KUNIT_ASSERT_NOT_NULL(test, sgt); > > ret = kunit_add_action_or_reset(test, kfree_wrapper, sgt); > KUNIT_ASSERT_EQ(test, ret, 0); > > ret = sg_alloc_table(sgt, 1, GFP_KERNEL); > KUNIT_ASSERT_EQ(test, ret, 0); > > ret = kunit_add_action_or_reset(test, sg_free_table_wrapper, sgt); > KUNIT_ASSERT_EQ(test, ret, 0); > > sg_init_one(sgt->sgl, buf, TEST_SIZE); > > 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_EXPECT_EQ(test, gem_obj->size, TEST_SIZE); > KUNIT_EXPECT_NULL(test, gem_obj->filp); > KUNIT_EXPECT_NOT_NULL(test, gem_obj->funcs); > > /* drm_gem_shmem_free will free the struct sg_table itself */ > kunit_remove_action(test, sg_free_table_wrapper, sgt); > kunit_remove_action(test, kfree_wrapper, sgt); I agree that using actions makes error handling cleaner. However, I still have some concerns about the additional complexity that actions introduce. For instance, I feel these two lines make the testing harness more complex without asserting any additional property of the component under test. In some sense, I wonder if it is worth worrying about memory leaks when a test case fails. At that point, the system is already in an inconsistent state due to a bug in the component under test, so it is unsafe to continue anyway. > > shmem = to_drm_gem_shmem_obj(gem_obj); > KUNIT_ASSERT_PTR_EQ(test, shmem->sgt, sgt); > > ret = kunit_add_action_or_reset(test, drm_gem_shmem_free_wrapper, shmem); > KUNIT_ASSERT_EQ(test, ret, 0); > > The last one is arguable, but for the previous ones it makes error > handling much more convenient and easy to reason about. > > The wrappers are also a bit inconvenient to use, but it's mostly there > to avoid a compiler warning at the moment. > > This patch will help hopefully: > https://lore.kernel.org/linux-kselftest/20230915050125.3609689-1-davidgow@google.com/ > > Maxime Thanks, Marco
On Mon, Oct 30, 2023 at 11:58:20AM +0100, Marco Pagani wrote: > On 2023-10-25 10:43, Maxime Ripard wrote: > > On Tue, Oct 24, 2023 at 07:14:25PM +0200, Marco Pagani wrote: > >>>> +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? > > > > I disagree. Actions make it easier to reason about, even when comparing > > assertion vs expectation > > > > Like, for the call to sg_alloc_table and > > drm_gem_shmem_prime_import_sg_table(), the reasonable use of assert vs > > expect would be something like: > > > > 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); > > > > /* > > * Here, it's already not super clear whether you want to expect vs > > * assert. expect will make you handle the failure case later, assert will > > * force you to call kfree on sgt. Both kind of suck in their own ways. > > */ > > > > sg_init_one(sgt->sgl, buf, TEST_SIZE); > > > > gem_obj = drm_gem_shmem_prime_import_sg_table(&fdev->drm_dev, &attach_mock, sgt); > > KUNIT_ASSERT_NOT_ERR_OR_NULL(test, gem_obj); > > > > /* > > * If the assert fails, we forgot to call sg_free_table(sgt) and kfree(sgt). > > */ > > > > KUNIT_EXPECT_EQ(test, gem_obj->size, TEST_SIZE); > > KUNIT_EXPECT_NULL(test, gem_obj->filp); > > KUNIT_EXPECT_NOT_NULL(test, gem_obj->funcs); > > > > /* > > * And here we have to handle the case where the expectation was wrong, > > * but the test still continued. > > */ > > > > But if you're not using an action, you still have to call kfree(sgt), > > which means that you might still > > > > shmem = to_drm_gem_shmem_obj(gem_obj); > > KUNIT_ASSERT_PTR_EQ(test, shmem->sgt, sgt); > > > > /* > > * If the assertion fails, we now have to call drm_gem_shmem_free(shmem) > > */ > > > > /* The scatter/gather table is freed by drm_gem_shmem_free */ > > drm_gem_shmem_free(shmem); > > > > /* everything's fine now */ > > > > The semantics around drm_gem_shmem_free make it a bit convoluted, but > > doing it using goto/labels, plus handling the assertions and error > > reporting would be difficult. > > > > Using actions, we have: > > > > sgt = kzalloc(sizeof(*sgt), GFP_KERNEL); > > KUNIT_ASSERT_NOT_NULL(test, sgt); > > > > ret = kunit_add_action_or_reset(test, kfree_wrapper, sgt); > > KUNIT_ASSERT_EQ(test, ret, 0); > > > > ret = sg_alloc_table(sgt, 1, GFP_KERNEL); > > KUNIT_ASSERT_EQ(test, ret, 0); > > > > ret = kunit_add_action_or_reset(test, sg_free_table_wrapper, sgt); > > KUNIT_ASSERT_EQ(test, ret, 0); > > > > sg_init_one(sgt->sgl, buf, TEST_SIZE); > > > > 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_EXPECT_EQ(test, gem_obj->size, TEST_SIZE); > > KUNIT_EXPECT_NULL(test, gem_obj->filp); > > KUNIT_EXPECT_NOT_NULL(test, gem_obj->funcs); > > > > /* drm_gem_shmem_free will free the struct sg_table itself */ > > kunit_remove_action(test, sg_free_table_wrapper, sgt); > > kunit_remove_action(test, kfree_wrapper, sgt); > > I agree that using actions makes error handling cleaner. However, I still > have some concerns about the additional complexity that actions introduce. > For instance, I feel these two lines make the testing harness more complex > without asserting any additional property of the component under test. If anything, the API makes it more difficult to deal with. It would actually be harder/messier to handle without an action. > In some sense, I wonder if it is worth worrying about memory leaks when > a test case fails. At that point, the system is already in an inconsistent > state due to a bug in the component under test, so it is unsafe to continue > anyway. I guess the larger issue is: once that code will be merged, we're going to have patches to convert to actions because they make it nicer and fix a couple of issues anyway. So, if it's still the state we're going to end up in, why not doing it right from the beginning? Maxime
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 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 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@redhat.com> + */ + +#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 */ +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); +} + +/* 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); +} + +static struct kunit_case drm_gem_shmem_test_cases[] = { + KUNIT_CASE(drm_gem_shmem_test_obj_create), + KUNIT_CASE(drm_gem_shmem_test_obj_create_private), + KUNIT_CASE(drm_gem_shmem_test_pin_pages), + KUNIT_CASE(drm_gem_shmem_test_vmap), + KUNIT_CASE(drm_gem_shmem_test_get_pages_sgt), + KUNIT_CASE(drm_gem_shmem_test_get_sg_table), + KUNIT_CASE(drm_gem_shmem_test_madvise), + KUNIT_CASE(drm_gem_shmem_test_purge), + {} +}; + +static struct kunit_suite drm_gem_shmem_suite = { + .name = "drm_gem_shmem", + .init = drm_gem_shmem_test_init, + .exit = drm_gem_shmem_test_exit, + .test_cases = drm_gem_shmem_test_cases +}; + +kunit_test_suite(drm_gem_shmem_suite);
This patch introduces an initial KUnit test suite for GEM objects backed by shmem buffers. Signed-off-by: Marco Pagani <marpagan@redhat.com> --- 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 base-commit: 3f5ba636d6987ddffeaa056dea1c524da63912f3