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

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

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.

-- 
Best regards,
Dmitry