On 2024-07-16 14:07, Ruhl, Michael J wrote:
>> -----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?
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@xxxxxxxxxx>
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@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
>
