Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs

[Christian König]

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.
>
> 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().

Regards,
Christian.

> Overall, it still questionable to me whether it's worthwhile to allow
> the mmaping of imported GEMs since only Panfrost/Lima can do it out of
> all drivers and h/w that I tested. Feels like drivers that can do the
> mapping have it just because they can and not because they need.