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

[Rob Clark]

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

BR,
-R

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