diff mbox series

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

Message ID 20240715083551.777807-1-airlied@gmail.com (mailing list archive)
State New, archived
Headers show
Series drm/test: fix the gem shmem test to map the sg table. | expand

Commit Message

Dave Airlie July 15, 2024, 8:35 a.m. UTC 
From: Dave Airlie <airlied@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 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@vger.kernel.org
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/tests/drm_gem_shmem_test.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Michael J. Ruhl July 15, 2024, 4:07 p.m. UTC | #1 
>-----Original Message-----
>From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Dave
>Airlie
>Sent: Monday, July 15, 2024 4:36 AM
>To: dri-devel@lists.freedesktop.org
>Subject: [PATCH] drm/test: fix the gem shmem test to map the sg table.
>
>From: Dave Airlie <airlied@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?

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@intel.com>

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@vger.kernel.org
>Signed-off-by: Dave Airlie <airlied@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 July 16, 2024, 9:33 a.m. UTC | #2 
On Mon, Jul 15, 2024 at 04:07:57PM +0000, Ruhl, Michael J wrote:
> >-----Original Message-----
> >From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Dave
> >Airlie
> >Sent: Monday, July 15, 2024 4:36 AM
> >To: dri-devel@lists.freedesktop.org
> >Subject: [PATCH] drm/test: fix the gem shmem test to map the sg table.
> >
> >From: Dave Airlie <airlied@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.

> 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@intel.com>

Acked-by: Daniel Vetter <daniel.vetter@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@vger.kernel.org
> >Signed-off-by: Dave Airlie <airlied@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
>
Michael J. Ruhl July 16, 2024, 12:07 p.m. UTC | #3 
>-----Original Message-----
>From: Daniel Vetter <daniel.vetter@ffwll.ch>
>Sent: Tuesday, July 16, 2024 5:34 AM
>To: Ruhl, Michael J <michael.j.ruhl@intel.com>
>Cc: Dave Airlie <airlied@gmail.com>; dri-devel@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@lists.freedesktop.org> On Behalf Of
>Dave
>> >Airlie
>> >Sent: Monday, July 15, 2024 4:36 AM
>> >To: dri-devel@lists.freedesktop.org
>> >Subject: [PATCH] drm/test: fix the gem shmem test to map the sg table.
>> >
>> >From: Dave Airlie <airlied@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@intel.com>
>
>Acked-by: Daniel Vetter <daniel.vetter@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@vger.kernel.org
>> >Signed-off-by: Dave Airlie <airlied@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
Marco Pagani July 16, 2024, 2:45 p.m. UTC | #4 
On 2024-07-16 14:07, Ruhl, Michael J wrote:
>> -----Original Message-----
>> From: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Sent: Tuesday, July 16, 2024 5:34 AM
>> To: Ruhl, Michael J <michael.j.ruhl@intel.com>
>> Cc: Dave Airlie <airlied@gmail.com>; dri-devel@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@lists.freedesktop.org> On Behalf Of
>> Dave
>>>> Airlie
>>>> Sent: Monday, July 15, 2024 4:36 AM
>>>> To: dri-devel@lists.freedesktop.org
>>>> Subject: [PATCH] drm/test: fix the gem shmem test to map the sg table.
>>>>
>>>> From: Dave Airlie <airlied@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?

In this case, if the mapping fails, the test case will be aborted, and the
sg_table will be freed by the action that calls sg_free_table_wrapper().
However, I also think drm_gem_shmem_free() should behave well even if the
sg_table is not mapped. Is there any advantage in issuing a warning when
freeing the object if the sg_table is not mapped?

Reviewed-by: Marco Pagani <marpagan@redhat.com>

Thanks,
Marco

> 
> 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@intel.com>
>>
>> Acked-by: Daniel Vetter <daniel.vetter@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@vger.kernel.org
>>>> Signed-off-by: Dave Airlie <airlied@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
>
Maxime Ripard Aug. 2, 2024, 7:26 a.m. UTC | #5 
On Mon, 15 Jul 2024 18:35:51 +1000, Dave Airlie wrote:
> 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 also sets a 64-bit dma mask, as I see an swiotlb warning if I
> stick with the default 32-bit one.
> 
> [...]

Applied to misc/kernel.git (drm-misc-fixes).

Thanks!
Maxime
diff mbox series

Patch

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;