>-----Original Message-----
>From: Daniel Vetter <daniel.vetter@xxxxxxxx>
>Sent: Tuesday, July 16, 2024 5:34 AM
>To: Ruhl, Michael J <michael.j.ruhl@xxxxxxxxx>
>Cc: Dave Airlie <airlied@xxxxxxxxx>; dri-devel@xxxxxxxxxxxxxxxxxxxxx
>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@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of
>Dave
>> >Airlie
>> >Sent: Monday, July 15, 2024 4:36 AM
>> >To: dri-devel@xxxxxxxxxxxxxxxxxxxxx
>> >Subject: [PATCH] drm/test: fix the gem shmem test to map the sg table.
>> >
>> >From: Dave Airlie <airlied@xxxxxxxxxx>
>> >
>> >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@xxxxxxxxx>
>
>Acked-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
>
>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@xxxxxxxxxxxxxxx
>> >Signed-off-by: Dave Airlie <airlied@xxxxxxxxxx>
>> >---
>> > 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
