[PATCH] drm/test: fix the gem shmem test to map the sg table.

[Ruhl, Michael J]

>-----Original Message-----
>From: Daniel Vetter <daniel.vetter at ffwll.ch>
>Sent: Tuesday, July 16, 2024 5:34 AM
>To: Ruhl, Michael J <michael.j.ruhl at intel.com>
>Cc: Dave Airlie <airlied at gmail.com>; dri-devel at lists.freedesktop.org
>Subject: Re: [PATCH] drm/test: fix the gem shmem test to map the sg table.
>
>On Mon, Jul 15, 2024 at 04:07:57PM +0000, Ruhl, Michael J wrote:
>> >-----Original Message-----
>> >From: dri-devel <dri-devel-bounces at lists.freedesktop.org> On Behalf Of
>Dave
>> >Airlie
>> >Sent: Monday, July 15, 2024 4:36 AM
>> >To: dri-devel at lists.freedesktop.org
>> >Subject: [PATCH] drm/test: fix the gem shmem test to map the sg table.
>> >
>> >From: Dave Airlie <airlied at redhat.com>
>> >
>> >The test here creates an sg table, but never maps it, when
>> >we get to drm_gem_shmem_free, the helper tries to unmap and this
>> >causes warnings on some platforms and debug kernels.
>>
>> This looks pretty straightforward...
>>
>> However, should drm_gem_shmem_free() really give an error if the mapping
>> didn't happen?
>>
>> I.e. just because you have an sgt pointer, should you also have a mapping?
>
>Yes, I think only allocating an sgt but not setting it up is a bug. So the
>fix looks correct, and isn't just papering over noise.

I guess my concern here is that the mapping could fail. 

If that happens, what is the error path?

Can I call _shmem_free?

Mike

>> If the errors are really just noise (form the specific platforms), and this patch is
>covering
>> for that, then:
>>
>> Reviewed-by: Michael J. Ruhl <michael.j.ruhl at intel.com>
>
>Acked-by: Daniel Vetter <daniel.vetter at ffwll.ch>
>
>Cheers, Sima
>>
>> Thanks,
>>
>> Mike
>>
>> >This also sets a 64-bit dma mask, as I see an swiotlb warning if I
>> >stick with the default 32-bit one.
>> >
>> >Fixes: 93032ae634d4 ("drm/test: add a test suite for GEM objects backed by
>> >shmem")
>> >Cc: stable at vger.kernel.org
>> >Signed-off-by: Dave Airlie <airlied at redhat.com>
>> >---
>> > drivers/gpu/drm/tests/drm_gem_shmem_test.c | 11 +++++++++++
>> > 1 file changed, 11 insertions(+)
>> >
>> >diff --git a/drivers/gpu/drm/tests/drm_gem_shmem_test.c
>> >b/drivers/gpu/drm/tests/drm_gem_shmem_test.c
>> >index 91202e40cde9..eb3a7a84be90 100644
>> >--- a/drivers/gpu/drm/tests/drm_gem_shmem_test.c
>> >+++ b/drivers/gpu/drm/tests/drm_gem_shmem_test.c
>> >@@ -102,6 +102,17 @@ static void
>> >drm_gem_shmem_test_obj_create_private(struct kunit *test)
>> >
>> > 	sg_init_one(sgt->sgl, buf, TEST_SIZE);
>> >
>> >+	/*
>> >+	 * Set the DMA mask to 64-bits and map the sgtables
>> >+	 * otherwise drm_gem_shmem_free will cause a warning
>> >+	 * on debug kernels.
>> >+	 * */
>> >+	ret = dma_set_mask(drm_dev->dev, DMA_BIT_MASK(64));
>> >+	KUNIT_ASSERT_EQ(test, ret, 0);
>> >+
>> >+	ret = dma_map_sgtable(drm_dev->dev, sgt, DMA_BIDIRECTIONAL, 0);
>> >+	KUNIT_ASSERT_EQ(test, ret, 0);
>> >+
>> > 	/* Init a mock DMA-BUF */
>> > 	buf_mock.size = TEST_SIZE;
>> > 	attach_mock.dmabuf = &buf_mock;
>> >--
>> >2.45.0
>>
>
>--
>Daniel Vetter
>Software Engineer, Intel Corporation
>http://blog.ffwll.ch