On Wed, Sep 7, 2022 at 3:25 AM Dmitry Osipenko <dmitry.osipenko@xxxxxxxxxxxxx> wrote: > > On 8/23/22 19:47, Rob Clark wrote: > > On Tue, Aug 23, 2022 at 3:01 AM Christian König > > <ckoenig.leichtzumerken@xxxxxxxxx> 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? For anything involving the GPU, userspace already needs to sync access (ie. GPUs are asynchronous).. this is done using drm_gem_object::resv (which comes from dma_buf::resv).. Possibly we should require that dma_buf_begin_cpu_access()/dma_buf_end_cpu_access().. but in practice if the gl/vk driver needs CPU access it is to read the results of it's own GPU rendering. Disallow that and all of deqp/piglit/etc will break on systems where "window" surfaces are dma-buf imports (like android), which would be a rather unpopular outcome ;-) BR, -R
