Date:   Wed, 7 Sep 2022 13:25:09 +0300
From:   Dmitry Osipenko <dmitry.osipenko@...labora.com>
To:     Rob Clark <robdclark@...il.com>,
        Christian König <ckoenig.leichtzumerken@...il.com>
Cc:     Christian König <christian.koenig@....com>,
        David Airlie <airlied@...ux.ie>,
        Gerd Hoffmann <kraxel@...hat.com>,
        Gurchetan Singh <gurchetansingh@...omium.org>,
        Chia-I Wu <olvaffe@...il.com>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Emil Velikov <emil.l.velikov@...il.com>,
        Thomas Hellström <thomas_os@...pmail.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        "open list:VIRTIO GPU DRIVER" 
        <virtualization@...ts.linux-foundation.org>,
        linux-tegra@...r.kernel.org, Dmitry Osipenko <digetx@...il.com>,
        kernel@...labora.com, Daniel Vetter <daniel@...ll.ch>
Subject: Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs

On 8/23/22 19:47, Rob Clark wrote:
> On Tue, Aug 23, 2022 at 3:01 AM Christian König
> <ckoenig.leichtzumerken@...il.com> wrote:
>>
>> Am 22.08.22 um 19:26 schrieb Dmitry Osipenko:
>>> On 8/16/22 22:55, Dmitry Osipenko wrote:
>>>> On 8/16/22 15:03, Christian König wrote:
>>>>> Am 16.08.22 um 13:44 schrieb Dmitry Osipenko:
>>>>>> [SNIP]
>>>>>>> The other complication I noticed is that we don't seem to keep around
>>>>>>> the fd after importing to a GEM handle.  And I could imagine that
>>>>>>> doing so could cause issues with too many fd's.  So I guess the best
>>>>>>> thing is to keep the status quo and let drivers that cannot mmap
>>>>>>> imported buffers just fail mmap?
>>>>>> That actually should be all the drivers excluding those that use
>>>>>> DRM-SHMEM because only DRM-SHMEM uses dma_buf_mmap(), that's why it
>>>>>> works for Panfrost. I'm pretty sure mmaping of imported GEMs doesn't
>>>>>> work for the MSM driver, isn't it?
>>>>>>
>>>>>> Intel and AMD drivers don't allow to map the imported dma-bufs. Both
>>>>>> refuse to do the mapping.
>>>>>>
>>>>>> Although, AMDGPU "succeeds" to do the mapping using
>>>>>> AMDGPU_GEM_DOMAIN_GTT, but then touching the mapping causes bus fault,
>>>>>> hence mapping actually fails. I think it might be the AMDGPU
>>>>>> driver/libdrm bug, haven't checked yet.
>>>>> That's then certainly broken somehow. Amdgpu should nerve ever have
>>>>> allowed to mmap() imported DMA-bufs and the last time I check it didn't.
>>>> I'll take a closer look. So far I can only tell that it's a kernel
>>>> driver issue because once I re-applied this "Don't map imported GEMs"
>>>> patch, AMDGPU began to refuse mapping AMDGPU_GEM_DOMAIN_GTT.
>>>>
>>>>>> So we're back to the point that neither of DRM drivers need to map
>>>>>> imported dma-bufs and this was never tested. In this case this patch is
>>>>>> valid, IMO.
>>>> Actually, I'm now looking at Etnaviv and Nouveau and seems they should
>>>> map imported dma-buf properly. I know that people ran Android on
>>>> Etnaviv. So maybe devices with a separated GPU/display need to map
>>>> imported display BO for Android support. Wish somebody who ran Android
>>>> on one of these devices using upstream drivers could give a definitive
>>>> answer. I may try to test Nouveau later on.
>>>>
>>> Nouveau+Intel combo doesn't work because of [1] that says:
>>>
>>> "Refuse to fault imported pages. This should be handled (if at all) by
>>> redirecting mmap to the exporter."
>>>
>>> [1]
>>> https://elixir.bootlin.com/linux/v5.19/source/drivers/gpu/drm/ttm/ttm_bo_vm.c#L154
>>>
>>> Interestingly, I noticed that there are IGT tests which check prime
>>> mmaping of Nouveau+Intel [2] (added 9 years ago), but they fail as well,
>>> as expected. The fact that IGT has such tests is interesting because it
>>> suggests that the mapping worked in the past. It's also surprising that
>>> nobody cared to fix the failing tests. For the reference, I checked
>>> v5.18 and today's linux-next.
>>>
>>> [2]
>>> https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/prime_nv_test.c#L132
>>>
>>> Starting subtest: nv_write_i915_cpu_mmap_read
>>> Received signal SIGBUS.
>>> Stack trace:
>>>   #0 [fatal_sig_handler+0x163]
>>>   #1 [__sigaction+0x50]
>>>   #2 [__igt_unique____real_main354+0x406]
>>>   #3 [main+0x23]
>>>   #4 [__libc_start_call_main+0x80]
>>>   #5 [__libc_start_main+0x89]
>>>   #6 [_start+0x25]
>>> Subtest nv_write_i915_cpu_mmap_read: CRASH (0,005s)
>>>
>>> Starting subtest: nv_write_i915_gtt_mmap_read
>>> Received signal SIGBUS.
>>> Stack trace:
>>>   #0 [fatal_sig_handler+0x163]
>>>   #1 [__sigaction+0x50]
>>>   #2 [__igt_unique____real_main354+0x33d]
>>>   #3 [main+0x23]
>>>   #4 [__libc_start_call_main+0x80]
>>>   #5 [__libc_start_main+0x89]
>>>   #6 [_start+0x25]
>>> Subtest nv_write_i915_gtt_mmap_read: CRASH (0,004s)
>>>
>>> I'm curious about the Etnaviv driver because it uses own shmem
>>> implementation and maybe it has a working mmaping of imported GEMs since
>>> it imports the dma-buf pages into Entaviv BO. Although, it should be
>>> risking to map pages using a different caching attributes (WC) from the
>>> exporter, which is prohibited on ARM ad then one may try to map imported
>>> udmabuf.

I see now that Etnaviv uses dma_buf_mmap(), so it should be okay.

>>> Apparently, the Intel DG TTM driver should be able to map imported
>>> dma-buf because it sets TTM_TT_FLAG_EXTERNAL_MAPPABLE.
>>
>> Even with that flag set it is illegal to map the pages directly by an
>> importer.
>>
>> If that ever worked then the only real solution is to redirect mmap()
>> calls on importer BOs to dma_buf_mmap().
> 
> Yeah, I think this is the best option.  Forcing userspace to hang on
> to the fd just in case someone calls readpix would be pretty harsh.

Actually, I proposed this couple months ago [1].

[1] https://patchwork.freedesktop.org/patch/487481/

What's not clear to me is how userspace is supposed to sync CPU accesses
for imported GEMs. Either userspace need to use dma_buf_sync UAPI for
dmabuf of imported GEM or importer driver should do that, or there is
some other option?

-- 
Best regards,
Dmitry

Powered by blists - more mailing lists