Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs
On Wed, Sep 7, 2022 at 3:25 AM Dmitry Osipenko wrote: > > On 8/23/22 19:47, Rob Clark wrote: > > On Tue, Aug 23, 2022 at 3:01 AM Christian König > > 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_uniquereal_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_uniquereal_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
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 > 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_uniquereal_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_uniquereal_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 d
Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs
On Tue, Aug 23, 2022 at 3:01 AM Christian König 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_uniquereal_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_uniquereal_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. > > >
Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs
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_uniquereal_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_uniquereal_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.
Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs
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_uniquereal_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_uniquereal_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
Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs
On Tue, Aug 16, 2022 at 4:45 AM Dmitry Osipenko
wrote:
>
> On 8/12/22 18:01, Rob Clark wrote:
> > On Fri, Aug 12, 2022 at 7:57 AM Rob Clark wrote:
> >>
> >> On Fri, Aug 12, 2022 at 4:26 AM Dmitry Osipenko
> >> wrote:
> >>>
> >>> On 8/11/22 02:19, Rob Clark wrote:
> On Wed, Aug 10, 2022 at 3:23 PM Dmitry Osipenko
> wrote:
> >
> > On 8/11/22 01:03, Rob Clark wrote:
> >> On Wed, Aug 10, 2022 at 12:26 PM Dmitry Osipenko
> >> wrote:
> >>>
> >>> On 8/10/22 18:08, Rob Clark wrote:
> On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter
> wrote:
> >
> > On Wed, Jul 06, 2022 at 10:02:07AM +0300, Dmitry Osipenko wrote:
> >> On 7/6/22 00:48, Rob Clark wrote:
> >>> On Tue, Jul 5, 2022 at 4:51 AM Christian König
> >>> wrote:
>
> Am 01.07.22 um 11:02 schrieb Dmitry Osipenko:
> > Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers
> > don't
> > handle imported dma-bufs properly, which results in mapping of
> > something
> > else than the imported dma-buf. On NVIDIA Tegra we get a hard
> > lockup when
> > userspace writes to the memory mapping of a dma-buf that was
> > imported into
> > Tegra's DRM GEM.
> >
> > Majority of DRM drivers prohibit mapping of the imported GEM
> > objects.
> > Mapping of imported GEMs require special care from userspace
> > since it
> > should sync dma-buf because mapping coherency of the exporter
> > device may
> > not match the DRM device. Let's prohibit the mapping for all
> > DRM drivers
> > for consistency.
> >
> > Suggested-by: Thomas Hellström
> >
> > Signed-off-by: Dmitry Osipenko
>
> I'm pretty sure that this is the right approach, but it's
> certainly more
> than possible that somebody abused this already.
> >>>
> >>> I suspect that this is abused if you run deqp cts on android..
> >>> ie. all
> >>> winsys buffers are dma-buf imports from gralloc. And then when
> >>> you
> >>> hit readpix...
> >>>
> >>> You might only hit this in scenarios with separate gpu and
> >>> display (or
> >>> dGPU+iGPU) because self-imports are handled differently in
> >>> drm_gem_prime_import_dev().. and maybe not in cases where you end
> >>> up
> >>> with a blit from tiled/compressed to linear.. maybe that narrows
> >>> the
> >>> scope enough to just fix it in userspace?
> >>
> >> Given that that only drivers which use DRM-SHMEM potentially
> >> could've
> >> map imported dma-bufs (Panfrost, Lima) and they already don't
> >> allow to
> >> do that, I think we're good.
> >
> > So can I have an ack from Rob here or are there still questions
> > that this
> > might go boom?
> >
> > Dmitry, since you have a bunch of patches merged now I think would
> > also be
> > good to get commit rights so you can drive this more yourself. I've
> > asked
> > Daniel Stone to help you out with getting that.
>
> I *think* we'd be ok with this on msm, mostly just by dumb luck.
> Because the dma-buf's we import will be self-import. I'm less sure
> about panfrost (src/panfrost/lib/pan_bo.c doesn't seem to have a
> special path for imported dma-bufs either, and in that case they
> won't
> be self-imports.. but I guess no one has tried to run android cts on
> panfrost).
> >>>
> >>> The last time I tried to mmap dma-buf imported to Panfrost didn't work
> >>> because Panfrost didn't implement something needed for that. I'll need
> >>> to take a look again because can't recall what it was.
> >>> Upd: I re-checked Panfrost using today's linux-next and mapping of
> >>> imported dma-buf works, I mapped imported buf from video decoder.
> >>> Apparently previously I had some local kernel change that broke the
> >>> mapping.
> >>>
> What about something less drastic to start, like (apologies for
> hand-edited patch):
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 86d670c71286..fc9ec42fa0ab 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1034,6 +1034,10 @@ int drm_gem_mmap_obj(struct drm_gem_object
> *obj, unsigned long obj_size,
> {
> int ret;
>
> + WARN_ON_ONCE(obj->import_attach);
> >>>
> >>> This will hang NVIDIA Tegra, which is what
Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs
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. -- Best regards, Dmitry
Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs
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. Regards, Christian. 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.
Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs
On 8/16/22 14:44, Dmitry Osipenko wrote:
> On 8/12/22 18:01, Rob Clark wrote:
>> On Fri, Aug 12, 2022 at 7:57 AM Rob Clark wrote:
>>>
>>> On Fri, Aug 12, 2022 at 4:26 AM Dmitry Osipenko
>>> wrote:
On 8/11/22 02:19, Rob Clark wrote:
> On Wed, Aug 10, 2022 at 3:23 PM Dmitry Osipenko
> wrote:
>>
>> On 8/11/22 01:03, Rob Clark wrote:
>>> On Wed, Aug 10, 2022 at 12:26 PM Dmitry Osipenko
>>> wrote:
On 8/10/22 18:08, Rob Clark wrote:
> On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter wrote:
>>
>> On Wed, Jul 06, 2022 at 10:02:07AM +0300, Dmitry Osipenko wrote:
>>> On 7/6/22 00:48, Rob Clark wrote:
On Tue, Jul 5, 2022 at 4:51 AM Christian König
wrote:
>
> Am 01.07.22 um 11:02 schrieb Dmitry Osipenko:
>> Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers
>> don't
>> handle imported dma-bufs properly, which results in mapping of
>> something
>> else than the imported dma-buf. On NVIDIA Tegra we get a hard
>> lockup when
>> userspace writes to the memory mapping of a dma-buf that was
>> imported into
>> Tegra's DRM GEM.
>>
>> Majority of DRM drivers prohibit mapping of the imported GEM
>> objects.
>> Mapping of imported GEMs require special care from userspace
>> since it
>> should sync dma-buf because mapping coherency of the exporter
>> device may
>> not match the DRM device. Let's prohibit the mapping for all DRM
>> drivers
>> for consistency.
>>
>> Suggested-by: Thomas Hellström
>> Signed-off-by: Dmitry Osipenko
>
> I'm pretty sure that this is the right approach, but it's
> certainly more
> than possible that somebody abused this already.
I suspect that this is abused if you run deqp cts on android.. ie.
all
winsys buffers are dma-buf imports from gralloc. And then when you
hit readpix...
You might only hit this in scenarios with separate gpu and display
(or
dGPU+iGPU) because self-imports are handled differently in
drm_gem_prime_import_dev().. and maybe not in cases where you end
up
with a blit from tiled/compressed to linear.. maybe that narrows
the
scope enough to just fix it in userspace?
>>>
>>> Given that that only drivers which use DRM-SHMEM potentially
>>> could've
>>> map imported dma-bufs (Panfrost, Lima) and they already don't allow
>>> to
>>> do that, I think we're good.
>>
>> So can I have an ack from Rob here or are there still questions that
>> this
>> might go boom?
>>
>> Dmitry, since you have a bunch of patches merged now I think would
>> also be
>> good to get commit rights so you can drive this more yourself. I've
>> asked
>> Daniel Stone to help you out with getting that.
>
> I *think* we'd be ok with this on msm, mostly just by dumb luck.
> Because the dma-buf's we import will be self-import. I'm less sure
> about panfrost (src/panfrost/lib/pan_bo.c doesn't seem to have a
> special path for imported dma-bufs either, and in that case they won't
> be self-imports.. but I guess no one has tried to run android cts on
> panfrost).
The last time I tried to mmap dma-buf imported to Panfrost didn't work
because Panfrost didn't implement something needed for that. I'll need
to take a look again because can't recall what it was.
Upd: I re-checked Panfrost using today's linux-next and mapping of
imported dma-buf works, I mapped imported buf from video decoder.
Apparently previously I had some local kernel change that broke the
mapping.
> What about something less drastic to start, like (apologies for
> hand-edited patch):
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 86d670c71286..fc9ec42fa0ab 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1034,6 +1034,10 @@ int drm_gem_mmap_obj(struct drm_gem_object
> *obj, unsigned long obj_size,
> {
> int ret;
>
> + WARN_ON_ONCE(obj->import_attach);
This will hang NVIDIA Tegra, which is what this patch fixed initially.
If neither of upstream DRM drivers need to map imported dma-bufs and
never needed, then why do we need this?
>>>
>>> oh, tegra i
Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs
On 8/12/22 18:01, Rob Clark wrote:
> On Fri, Aug 12, 2022 at 7:57 AM Rob Clark wrote:
>>
>> On Fri, Aug 12, 2022 at 4:26 AM Dmitry Osipenko
>> wrote:
>>>
>>> On 8/11/22 02:19, Rob Clark wrote:
On Wed, Aug 10, 2022 at 3:23 PM Dmitry Osipenko
wrote:
>
> On 8/11/22 01:03, Rob Clark wrote:
>> On Wed, Aug 10, 2022 at 12:26 PM Dmitry Osipenko
>> wrote:
>>>
>>> On 8/10/22 18:08, Rob Clark wrote:
On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter wrote:
>
> On Wed, Jul 06, 2022 at 10:02:07AM +0300, Dmitry Osipenko wrote:
>> On 7/6/22 00:48, Rob Clark wrote:
>>> On Tue, Jul 5, 2022 at 4:51 AM Christian König
>>> wrote:
Am 01.07.22 um 11:02 schrieb Dmitry Osipenko:
> Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers
> don't
> handle imported dma-bufs properly, which results in mapping of
> something
> else than the imported dma-buf. On NVIDIA Tegra we get a hard
> lockup when
> userspace writes to the memory mapping of a dma-buf that was
> imported into
> Tegra's DRM GEM.
>
> Majority of DRM drivers prohibit mapping of the imported GEM
> objects.
> Mapping of imported GEMs require special care from userspace
> since it
> should sync dma-buf because mapping coherency of the exporter
> device may
> not match the DRM device. Let's prohibit the mapping for all DRM
> drivers
> for consistency.
>
> Suggested-by: Thomas Hellström
> Signed-off-by: Dmitry Osipenko
I'm pretty sure that this is the right approach, but it's
certainly more
than possible that somebody abused this already.
>>>
>>> I suspect that this is abused if you run deqp cts on android.. ie.
>>> all
>>> winsys buffers are dma-buf imports from gralloc. And then when you
>>> hit readpix...
>>>
>>> You might only hit this in scenarios with separate gpu and display
>>> (or
>>> dGPU+iGPU) because self-imports are handled differently in
>>> drm_gem_prime_import_dev().. and maybe not in cases where you end up
>>> with a blit from tiled/compressed to linear.. maybe that narrows the
>>> scope enough to just fix it in userspace?
>>
>> Given that that only drivers which use DRM-SHMEM potentially could've
>> map imported dma-bufs (Panfrost, Lima) and they already don't allow
>> to
>> do that, I think we're good.
>
> So can I have an ack from Rob here or are there still questions that
> this
> might go boom?
>
> Dmitry, since you have a bunch of patches merged now I think would
> also be
> good to get commit rights so you can drive this more yourself. I've
> asked
> Daniel Stone to help you out with getting that.
I *think* we'd be ok with this on msm, mostly just by dumb luck.
Because the dma-buf's we import will be self-import. I'm less sure
about panfrost (src/panfrost/lib/pan_bo.c doesn't seem to have a
special path for imported dma-bufs either, and in that case they won't
be self-imports.. but I guess no one has tried to run android cts on
panfrost).
>>>
>>> The last time I tried to mmap dma-buf imported to Panfrost didn't work
>>> because Panfrost didn't implement something needed for that. I'll need
>>> to take a look again because can't recall what it was.
>>> Upd: I re-checked Panfrost using today's linux-next and mapping of
>>> imported dma-buf works, I mapped imported buf from video decoder.
>>> Apparently previously I had some local kernel change that broke the mapping.
>>>
What about something less drastic to start, like (apologies for
hand-edited patch):
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 86d670c71286..fc9ec42fa0ab 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1034,6 +1034,10 @@ int drm_gem_mmap_obj(struct drm_gem_object
*obj, unsigned long obj_size,
{
int ret;
+ WARN_ON_ONCE(obj->import_attach);
>>>
>>> This will hang NVIDIA Tegra, which is what this patch fixed initially.
>>> If neither of upstream DRM drivers need to map imported dma-bufs and
>>> never needed, then why do we need this?
>>
>> oh, tegra isn't using shmem helpers? I assumed it was. Well my point
>> was to make a more targeted fail on tegra, and a WARN_ON for everyone
>> else to make it clear that what they are doing is undef
Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs
On Fri, Aug 12, 2022 at 7:57 AM Rob Clark wrote:
>
> On Fri, Aug 12, 2022 at 4:26 AM Dmitry Osipenko
> wrote:
> >
> > On 8/11/22 02:19, Rob Clark wrote:
> > > On Wed, Aug 10, 2022 at 3:23 PM Dmitry Osipenko
> > > wrote:
> > >>
> > >> On 8/11/22 01:03, Rob Clark wrote:
> > >>> On Wed, Aug 10, 2022 at 12:26 PM Dmitry Osipenko
> > >>> wrote:
> >
> > On 8/10/22 18:08, Rob Clark wrote:
> > > On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter wrote:
> > >>
> > >> On Wed, Jul 06, 2022 at 10:02:07AM +0300, Dmitry Osipenko wrote:
> > >>> On 7/6/22 00:48, Rob Clark wrote:
> > On Tue, Jul 5, 2022 at 4:51 AM Christian König
> > wrote:
> > >
> > > Am 01.07.22 um 11:02 schrieb Dmitry Osipenko:
> > >> Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers
> > >> don't
> > >> handle imported dma-bufs properly, which results in mapping of
> > >> something
> > >> else than the imported dma-buf. On NVIDIA Tegra we get a hard
> > >> lockup when
> > >> userspace writes to the memory mapping of a dma-buf that was
> > >> imported into
> > >> Tegra's DRM GEM.
> > >>
> > >> Majority of DRM drivers prohibit mapping of the imported GEM
> > >> objects.
> > >> Mapping of imported GEMs require special care from userspace
> > >> since it
> > >> should sync dma-buf because mapping coherency of the exporter
> > >> device may
> > >> not match the DRM device. Let's prohibit the mapping for all DRM
> > >> drivers
> > >> for consistency.
> > >>
> > >> Suggested-by: Thomas Hellström
> > >> Signed-off-by: Dmitry Osipenko
> > >
> > > I'm pretty sure that this is the right approach, but it's
> > > certainly more
> > > than possible that somebody abused this already.
> >
> > I suspect that this is abused if you run deqp cts on android.. ie.
> > all
> > winsys buffers are dma-buf imports from gralloc. And then when you
> > hit readpix...
> >
> > You might only hit this in scenarios with separate gpu and display
> > (or
> > dGPU+iGPU) because self-imports are handled differently in
> > drm_gem_prime_import_dev().. and maybe not in cases where you end
> > up
> > with a blit from tiled/compressed to linear.. maybe that narrows
> > the
> > scope enough to just fix it in userspace?
> > >>>
> > >>> Given that that only drivers which use DRM-SHMEM potentially
> > >>> could've
> > >>> map imported dma-bufs (Panfrost, Lima) and they already don't allow
> > >>> to
> > >>> do that, I think we're good.
> > >>
> > >> So can I have an ack from Rob here or are there still questions that
> > >> this
> > >> might go boom?
> > >>
> > >> Dmitry, since you have a bunch of patches merged now I think would
> > >> also be
> > >> good to get commit rights so you can drive this more yourself. I've
> > >> asked
> > >> Daniel Stone to help you out with getting that.
> > >
> > > I *think* we'd be ok with this on msm, mostly just by dumb luck.
> > > Because the dma-buf's we import will be self-import. I'm less sure
> > > about panfrost (src/panfrost/lib/pan_bo.c doesn't seem to have a
> > > special path for imported dma-bufs either, and in that case they won't
> > > be self-imports.. but I guess no one has tried to run android cts on
> > > panfrost).
> >
> > The last time I tried to mmap dma-buf imported to Panfrost didn't work
> > because Panfrost didn't implement something needed for that. I'll need
> > to take a look again because can't recall what it was.
> > Upd: I re-checked Panfrost using today's linux-next and mapping of
> > imported dma-buf works, I mapped imported buf from video decoder.
> > Apparently previously I had some local kernel change that broke the mapping.
> >
> > > What about something less drastic to start, like (apologies for
> > > hand-edited patch):
> > >
> > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > > index 86d670c71286..fc9ec42fa0ab 100644
> > > --- a/drivers/gpu/drm/drm_gem.c
> > > +++ b/drivers/gpu/drm/drm_gem.c
> > > @@ -1034,6 +1034,10 @@ int drm_gem_mmap_obj(struct drm_gem_object
> > > *obj, unsigned long obj_size,
> > > {
> > > int ret;
> > >
> > > + WARN_ON_ONCE(obj->import_attach);
> >
> > This will hang NVIDIA Tegra, which is what this patch fixed initially.
> > If neither of upstream DRM drivers need to map imported dma-bufs and
> > never needed, then why do we need this?
> > >>>
> > >>> oh, tegra isn't using shmem helpers? I assumed it was. Well my point
> > >>> was to make a more targeted fai
Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs
On Fri, Aug 12, 2022 at 4:26 AM Dmitry Osipenko
wrote:
>
> On 8/11/22 02:19, Rob Clark wrote:
> > On Wed, Aug 10, 2022 at 3:23 PM Dmitry Osipenko
> > wrote:
> >>
> >> On 8/11/22 01:03, Rob Clark wrote:
> >>> On Wed, Aug 10, 2022 at 12:26 PM Dmitry Osipenko
> >>> wrote:
>
> On 8/10/22 18:08, Rob Clark wrote:
> > On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter wrote:
> >>
> >> On Wed, Jul 06, 2022 at 10:02:07AM +0300, Dmitry Osipenko wrote:
> >>> On 7/6/22 00:48, Rob Clark wrote:
> On Tue, Jul 5, 2022 at 4:51 AM Christian König
> wrote:
> >
> > Am 01.07.22 um 11:02 schrieb Dmitry Osipenko:
> >> Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers
> >> don't
> >> handle imported dma-bufs properly, which results in mapping of
> >> something
> >> else than the imported dma-buf. On NVIDIA Tegra we get a hard
> >> lockup when
> >> userspace writes to the memory mapping of a dma-buf that was
> >> imported into
> >> Tegra's DRM GEM.
> >>
> >> Majority of DRM drivers prohibit mapping of the imported GEM
> >> objects.
> >> Mapping of imported GEMs require special care from userspace since
> >> it
> >> should sync dma-buf because mapping coherency of the exporter
> >> device may
> >> not match the DRM device. Let's prohibit the mapping for all DRM
> >> drivers
> >> for consistency.
> >>
> >> Suggested-by: Thomas Hellström
> >> Signed-off-by: Dmitry Osipenko
> >
> > I'm pretty sure that this is the right approach, but it's certainly
> > more
> > than possible that somebody abused this already.
>
> I suspect that this is abused if you run deqp cts on android.. ie.
> all
> winsys buffers are dma-buf imports from gralloc. And then when you
> hit readpix...
>
> You might only hit this in scenarios with separate gpu and display
> (or
> dGPU+iGPU) because self-imports are handled differently in
> drm_gem_prime_import_dev().. and maybe not in cases where you end up
> with a blit from tiled/compressed to linear.. maybe that narrows the
> scope enough to just fix it in userspace?
> >>>
> >>> Given that that only drivers which use DRM-SHMEM potentially could've
> >>> map imported dma-bufs (Panfrost, Lima) and they already don't allow to
> >>> do that, I think we're good.
> >>
> >> So can I have an ack from Rob here or are there still questions that
> >> this
> >> might go boom?
> >>
> >> Dmitry, since you have a bunch of patches merged now I think would
> >> also be
> >> good to get commit rights so you can drive this more yourself. I've
> >> asked
> >> Daniel Stone to help you out with getting that.
> >
> > I *think* we'd be ok with this on msm, mostly just by dumb luck.
> > Because the dma-buf's we import will be self-import. I'm less sure
> > about panfrost (src/panfrost/lib/pan_bo.c doesn't seem to have a
> > special path for imported dma-bufs either, and in that case they won't
> > be self-imports.. but I guess no one has tried to run android cts on
> > panfrost).
>
> The last time I tried to mmap dma-buf imported to Panfrost didn't work
> because Panfrost didn't implement something needed for that. I'll need
> to take a look again because can't recall what it was.
> Upd: I re-checked Panfrost using today's linux-next and mapping of
> imported dma-buf works, I mapped imported buf from video decoder.
> Apparently previously I had some local kernel change that broke the mapping.
>
> > What about something less drastic to start, like (apologies for
> > hand-edited patch):
> >
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index 86d670c71286..fc9ec42fa0ab 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -1034,6 +1034,10 @@ int drm_gem_mmap_obj(struct drm_gem_object
> > *obj, unsigned long obj_size,
> > {
> > int ret;
> >
> > + WARN_ON_ONCE(obj->import_attach);
>
> This will hang NVIDIA Tegra, which is what this patch fixed initially.
> If neither of upstream DRM drivers need to map imported dma-bufs and
> never needed, then why do we need this?
> >>>
> >>> oh, tegra isn't using shmem helpers? I assumed it was. Well my point
> >>> was to make a more targeted fail on tegra, and a WARN_ON for everyone
> >>> else to make it clear that what they are doing is undefined behavior.
> >>> Because so far existing userspace (or well, panfrost and freedreno at
> >>> least, those are the two I know or checked) don't make special cases
> >>> for mmap'ing against the dmabuf fd against the d
Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs
On 8/11/22 02:19, Rob Clark wrote:
> On Wed, Aug 10, 2022 at 3:23 PM Dmitry Osipenko
> wrote:
>>
>> On 8/11/22 01:03, Rob Clark wrote:
>>> On Wed, Aug 10, 2022 at 12:26 PM Dmitry Osipenko
>>> wrote:
On 8/10/22 18:08, Rob Clark wrote:
> On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter wrote:
>>
>> On Wed, Jul 06, 2022 at 10:02:07AM +0300, Dmitry Osipenko wrote:
>>> On 7/6/22 00:48, Rob Clark wrote:
On Tue, Jul 5, 2022 at 4:51 AM Christian König
wrote:
>
> Am 01.07.22 um 11:02 schrieb Dmitry Osipenko:
>> Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers don't
>> handle imported dma-bufs properly, which results in mapping of
>> something
>> else than the imported dma-buf. On NVIDIA Tegra we get a hard lockup
>> when
>> userspace writes to the memory mapping of a dma-buf that was
>> imported into
>> Tegra's DRM GEM.
>>
>> Majority of DRM drivers prohibit mapping of the imported GEM objects.
>> Mapping of imported GEMs require special care from userspace since it
>> should sync dma-buf because mapping coherency of the exporter device
>> may
>> not match the DRM device. Let's prohibit the mapping for all DRM
>> drivers
>> for consistency.
>>
>> Suggested-by: Thomas Hellström
>> Signed-off-by: Dmitry Osipenko
>
> I'm pretty sure that this is the right approach, but it's certainly
> more
> than possible that somebody abused this already.
I suspect that this is abused if you run deqp cts on android.. ie. all
winsys buffers are dma-buf imports from gralloc. And then when you
hit readpix...
You might only hit this in scenarios with separate gpu and display (or
dGPU+iGPU) because self-imports are handled differently in
drm_gem_prime_import_dev().. and maybe not in cases where you end up
with a blit from tiled/compressed to linear.. maybe that narrows the
scope enough to just fix it in userspace?
>>>
>>> Given that that only drivers which use DRM-SHMEM potentially could've
>>> map imported dma-bufs (Panfrost, Lima) and they already don't allow to
>>> do that, I think we're good.
>>
>> So can I have an ack from Rob here or are there still questions that this
>> might go boom?
>>
>> Dmitry, since you have a bunch of patches merged now I think would also
>> be
>> good to get commit rights so you can drive this more yourself. I've asked
>> Daniel Stone to help you out with getting that.
>
> I *think* we'd be ok with this on msm, mostly just by dumb luck.
> Because the dma-buf's we import will be self-import. I'm less sure
> about panfrost (src/panfrost/lib/pan_bo.c doesn't seem to have a
> special path for imported dma-bufs either, and in that case they won't
> be self-imports.. but I guess no one has tried to run android cts on
> panfrost).
The last time I tried to mmap dma-buf imported to Panfrost didn't work
because Panfrost didn't implement something needed for that. I'll need
to take a look again because can't recall what it was.
Upd: I re-checked Panfrost using today's linux-next and mapping of
imported dma-buf works, I mapped imported buf from video decoder.
Apparently previously I had some local kernel change that broke the mapping.
> What about something less drastic to start, like (apologies for
> hand-edited patch):
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 86d670c71286..fc9ec42fa0ab 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1034,6 +1034,10 @@ int drm_gem_mmap_obj(struct drm_gem_object
> *obj, unsigned long obj_size,
> {
> int ret;
>
> + WARN_ON_ONCE(obj->import_attach);
This will hang NVIDIA Tegra, which is what this patch fixed initially.
If neither of upstream DRM drivers need to map imported dma-bufs and
never needed, then why do we need this?
>>>
>>> oh, tegra isn't using shmem helpers? I assumed it was. Well my point
>>> was to make a more targeted fail on tegra, and a WARN_ON for everyone
>>> else to make it clear that what they are doing is undefined behavior.
>>> Because so far existing userspace (or well, panfrost and freedreno at
>>> least, those are the two I know or checked) don't make special cases
>>> for mmap'ing against the dmabuf fd against the dmabuf fd instead of
>>> the drm device fd.
>>
>> It's not clear to me what bad Android does form yours comments. Does it
>> export dma-buf from GPU and then import it to GPU? If yes, then DRM core
>> has a check for the self-importing [1].
>>
>> [1]
>> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_prime.c#L918
>>
>> If you
Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs
On Wed, Aug 10, 2022 at 3:23 PM Dmitry Osipenko
wrote:
>
> On 8/11/22 01:03, Rob Clark wrote:
> > On Wed, Aug 10, 2022 at 12:26 PM Dmitry Osipenko
> > wrote:
> >>
> >> On 8/10/22 18:08, Rob Clark wrote:
> >>> On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter wrote:
>
> On Wed, Jul 06, 2022 at 10:02:07AM +0300, Dmitry Osipenko wrote:
> > On 7/6/22 00:48, Rob Clark wrote:
> >> On Tue, Jul 5, 2022 at 4:51 AM Christian König
> >> wrote:
> >>>
> >>> Am 01.07.22 um 11:02 schrieb Dmitry Osipenko:
> Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers don't
> handle imported dma-bufs properly, which results in mapping of
> something
> else than the imported dma-buf. On NVIDIA Tegra we get a hard lockup
> when
> userspace writes to the memory mapping of a dma-buf that was
> imported into
> Tegra's DRM GEM.
>
> Majority of DRM drivers prohibit mapping of the imported GEM objects.
> Mapping of imported GEMs require special care from userspace since it
> should sync dma-buf because mapping coherency of the exporter device
> may
> not match the DRM device. Let's prohibit the mapping for all DRM
> drivers
> for consistency.
>
> Suggested-by: Thomas Hellström
> Signed-off-by: Dmitry Osipenko
> >>>
> >>> I'm pretty sure that this is the right approach, but it's certainly
> >>> more
> >>> than possible that somebody abused this already.
> >>
> >> I suspect that this is abused if you run deqp cts on android.. ie. all
> >> winsys buffers are dma-buf imports from gralloc. And then when you
> >> hit readpix...
> >>
> >> You might only hit this in scenarios with separate gpu and display (or
> >> dGPU+iGPU) because self-imports are handled differently in
> >> drm_gem_prime_import_dev().. and maybe not in cases where you end up
> >> with a blit from tiled/compressed to linear.. maybe that narrows the
> >> scope enough to just fix it in userspace?
> >
> > Given that that only drivers which use DRM-SHMEM potentially could've
> > map imported dma-bufs (Panfrost, Lima) and they already don't allow to
> > do that, I think we're good.
>
> So can I have an ack from Rob here or are there still questions that this
> might go boom?
>
> Dmitry, since you have a bunch of patches merged now I think would also
> be
> good to get commit rights so you can drive this more yourself. I've asked
> Daniel Stone to help you out with getting that.
> >>>
> >>> I *think* we'd be ok with this on msm, mostly just by dumb luck.
> >>> Because the dma-buf's we import will be self-import. I'm less sure
> >>> about panfrost (src/panfrost/lib/pan_bo.c doesn't seem to have a
> >>> special path for imported dma-bufs either, and in that case they won't
> >>> be self-imports.. but I guess no one has tried to run android cts on
> >>> panfrost).
> >>
> >> The last time I tried to mmap dma-buf imported to Panfrost didn't work
> >> because Panfrost didn't implement something needed for that. I'll need
> >> to take a look again because can't recall what it was.
> >>
> >>> What about something less drastic to start, like (apologies for
> >>> hand-edited patch):
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> >>> index 86d670c71286..fc9ec42fa0ab 100644
> >>> --- a/drivers/gpu/drm/drm_gem.c
> >>> +++ b/drivers/gpu/drm/drm_gem.c
> >>> @@ -1034,6 +1034,10 @@ int drm_gem_mmap_obj(struct drm_gem_object
> >>> *obj, unsigned long obj_size,
> >>> {
> >>> int ret;
> >>>
> >>> + WARN_ON_ONCE(obj->import_attach);
> >>
> >> This will hang NVIDIA Tegra, which is what this patch fixed initially.
> >> If neither of upstream DRM drivers need to map imported dma-bufs and
> >> never needed, then why do we need this?
> >
> > oh, tegra isn't using shmem helpers? I assumed it was. Well my point
> > was to make a more targeted fail on tegra, and a WARN_ON for everyone
> > else to make it clear that what they are doing is undefined behavior.
> > Because so far existing userspace (or well, panfrost and freedreno at
> > least, those are the two I know or checked) don't make special cases
> > for mmap'ing against the dmabuf fd against the dmabuf fd instead of
> > the drm device fd.
>
> It's not clear to me what bad Android does form yours comments. Does it
> export dma-buf from GPU and then import it to GPU? If yes, then DRM core
> has a check for the self-importing [1].
>
> [1]
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_prime.c#L918
>
> If you're meaning something else, then please explain in a more details.
So, android/gralloc allocates buffers externally to the driver and
imports them into driver. (And that seems to not just be window
surfaces, but in cases random textures, etc)
In the nor
Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs
On 8/11/22 01:03, Rob Clark wrote:
> On Wed, Aug 10, 2022 at 12:26 PM Dmitry Osipenko
> wrote:
>>
>> On 8/10/22 18:08, Rob Clark wrote:
>>> On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter wrote:
On Wed, Jul 06, 2022 at 10:02:07AM +0300, Dmitry Osipenko wrote:
> On 7/6/22 00:48, Rob Clark wrote:
>> On Tue, Jul 5, 2022 at 4:51 AM Christian König
>> wrote:
>>>
>>> Am 01.07.22 um 11:02 schrieb Dmitry Osipenko:
Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers don't
handle imported dma-bufs properly, which results in mapping of
something
else than the imported dma-buf. On NVIDIA Tegra we get a hard lockup
when
userspace writes to the memory mapping of a dma-buf that was imported
into
Tegra's DRM GEM.
Majority of DRM drivers prohibit mapping of the imported GEM objects.
Mapping of imported GEMs require special care from userspace since it
should sync dma-buf because mapping coherency of the exporter device
may
not match the DRM device. Let's prohibit the mapping for all DRM
drivers
for consistency.
Suggested-by: Thomas Hellström
Signed-off-by: Dmitry Osipenko
>>>
>>> I'm pretty sure that this is the right approach, but it's certainly more
>>> than possible that somebody abused this already.
>>
>> I suspect that this is abused if you run deqp cts on android.. ie. all
>> winsys buffers are dma-buf imports from gralloc. And then when you
>> hit readpix...
>>
>> You might only hit this in scenarios with separate gpu and display (or
>> dGPU+iGPU) because self-imports are handled differently in
>> drm_gem_prime_import_dev().. and maybe not in cases where you end up
>> with a blit from tiled/compressed to linear.. maybe that narrows the
>> scope enough to just fix it in userspace?
>
> Given that that only drivers which use DRM-SHMEM potentially could've
> map imported dma-bufs (Panfrost, Lima) and they already don't allow to
> do that, I think we're good.
So can I have an ack from Rob here or are there still questions that this
might go boom?
Dmitry, since you have a bunch of patches merged now I think would also be
good to get commit rights so you can drive this more yourself. I've asked
Daniel Stone to help you out with getting that.
>>>
>>> I *think* we'd be ok with this on msm, mostly just by dumb luck.
>>> Because the dma-buf's we import will be self-import. I'm less sure
>>> about panfrost (src/panfrost/lib/pan_bo.c doesn't seem to have a
>>> special path for imported dma-bufs either, and in that case they won't
>>> be self-imports.. but I guess no one has tried to run android cts on
>>> panfrost).
>>
>> The last time I tried to mmap dma-buf imported to Panfrost didn't work
>> because Panfrost didn't implement something needed for that. I'll need
>> to take a look again because can't recall what it was.
>>
>>> What about something less drastic to start, like (apologies for
>>> hand-edited patch):
>>>
>>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
>>> index 86d670c71286..fc9ec42fa0ab 100644
>>> --- a/drivers/gpu/drm/drm_gem.c
>>> +++ b/drivers/gpu/drm/drm_gem.c
>>> @@ -1034,6 +1034,10 @@ int drm_gem_mmap_obj(struct drm_gem_object
>>> *obj, unsigned long obj_size,
>>> {
>>> int ret;
>>>
>>> + WARN_ON_ONCE(obj->import_attach);
>>
>> This will hang NVIDIA Tegra, which is what this patch fixed initially.
>> If neither of upstream DRM drivers need to map imported dma-bufs and
>> never needed, then why do we need this?
>
> oh, tegra isn't using shmem helpers? I assumed it was. Well my point
> was to make a more targeted fail on tegra, and a WARN_ON for everyone
> else to make it clear that what they are doing is undefined behavior.
> Because so far existing userspace (or well, panfrost and freedreno at
> least, those are the two I know or checked) don't make special cases
> for mmap'ing against the dmabuf fd against the dmabuf fd instead of
> the drm device fd.
It's not clear to me what bad Android does form yours comments. Does it
export dma-buf from GPU and then import it to GPU? If yes, then DRM core
has a check for the self-importing [1].
[1]
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_prime.c#L918
If you're meaning something else, then please explain in a more details.
> I *think* it should work out that we don't hit this path with
> freedreno but on android I can't really guarantee or prove it. So
> your patch would potentially break existing working userspace. Maybe
> it is userspace that isn't portable (but OTOH it isn't like you are
> going to be using freedreno on tegra). So why don't you go for a more
> targeted fix that only returns an error on hw where this is
> problematic?
That's what the first versions of the p
Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs
On Wed, Aug 10, 2022 at 12:26 PM Dmitry Osipenko
wrote:
>
> On 8/10/22 18:08, Rob Clark wrote:
> > On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter wrote:
> >>
> >> On Wed, Jul 06, 2022 at 10:02:07AM +0300, Dmitry Osipenko wrote:
> >>> On 7/6/22 00:48, Rob Clark wrote:
> On Tue, Jul 5, 2022 at 4:51 AM Christian König
> wrote:
> >
> > Am 01.07.22 um 11:02 schrieb Dmitry Osipenko:
> >> Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers don't
> >> handle imported dma-bufs properly, which results in mapping of
> >> something
> >> else than the imported dma-buf. On NVIDIA Tegra we get a hard lockup
> >> when
> >> userspace writes to the memory mapping of a dma-buf that was imported
> >> into
> >> Tegra's DRM GEM.
> >>
> >> Majority of DRM drivers prohibit mapping of the imported GEM objects.
> >> Mapping of imported GEMs require special care from userspace since it
> >> should sync dma-buf because mapping coherency of the exporter device
> >> may
> >> not match the DRM device. Let's prohibit the mapping for all DRM
> >> drivers
> >> for consistency.
> >>
> >> Suggested-by: Thomas Hellström
> >> Signed-off-by: Dmitry Osipenko
> >
> > I'm pretty sure that this is the right approach, but it's certainly more
> > than possible that somebody abused this already.
>
> I suspect that this is abused if you run deqp cts on android.. ie. all
> winsys buffers are dma-buf imports from gralloc. And then when you
> hit readpix...
>
> You might only hit this in scenarios with separate gpu and display (or
> dGPU+iGPU) because self-imports are handled differently in
> drm_gem_prime_import_dev().. and maybe not in cases where you end up
> with a blit from tiled/compressed to linear.. maybe that narrows the
> scope enough to just fix it in userspace?
> >>>
> >>> Given that that only drivers which use DRM-SHMEM potentially could've
> >>> map imported dma-bufs (Panfrost, Lima) and they already don't allow to
> >>> do that, I think we're good.
> >>
> >> So can I have an ack from Rob here or are there still questions that this
> >> might go boom?
> >>
> >> Dmitry, since you have a bunch of patches merged now I think would also be
> >> good to get commit rights so you can drive this more yourself. I've asked
> >> Daniel Stone to help you out with getting that.
> >
> > I *think* we'd be ok with this on msm, mostly just by dumb luck.
> > Because the dma-buf's we import will be self-import. I'm less sure
> > about panfrost (src/panfrost/lib/pan_bo.c doesn't seem to have a
> > special path for imported dma-bufs either, and in that case they won't
> > be self-imports.. but I guess no one has tried to run android cts on
> > panfrost).
>
> The last time I tried to mmap dma-buf imported to Panfrost didn't work
> because Panfrost didn't implement something needed for that. I'll need
> to take a look again because can't recall what it was.
>
> > What about something less drastic to start, like (apologies for
> > hand-edited patch):
> >
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index 86d670c71286..fc9ec42fa0ab 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -1034,6 +1034,10 @@ int drm_gem_mmap_obj(struct drm_gem_object
> > *obj, unsigned long obj_size,
> > {
> > int ret;
> >
> > + WARN_ON_ONCE(obj->import_attach);
>
> This will hang NVIDIA Tegra, which is what this patch fixed initially.
> If neither of upstream DRM drivers need to map imported dma-bufs and
> never needed, then why do we need this?
oh, tegra isn't using shmem helpers? I assumed it was. Well my point
was to make a more targeted fail on tegra, and a WARN_ON for everyone
else to make it clear that what they are doing is undefined behavior.
Because so far existing userspace (or well, panfrost and freedreno at
least, those are the two I know or checked) don't make special cases
for mmap'ing against the dmabuf fd against the dmabuf fd instead of
the drm device fd.
I *think* it should work out that we don't hit this path with
freedreno but on android I can't really guarantee or prove it. So
your patch would potentially break existing working userspace. Maybe
it is userspace that isn't portable (but OTOH it isn't like you are
going to be using freedreno on tegra). So why don't you go for a more
targeted fix that only returns an error on hw where this is
problematic?
BR,
-R
Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs
On 8/10/22 14:47, Daniel Vetter wrote: > Dmitry, since you have a bunch of patches merged now I think would also be > good to get commit rights so you can drive this more yourself. I've asked > Daniel Stone to help you out with getting that. > -Daniel Thank you! -- Best regards, Dmitry
Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs
On 8/10/22 18:08, Rob Clark wrote:
> On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter wrote:
>>
>> On Wed, Jul 06, 2022 at 10:02:07AM +0300, Dmitry Osipenko wrote:
>>> On 7/6/22 00:48, Rob Clark wrote:
On Tue, Jul 5, 2022 at 4:51 AM Christian König
wrote:
>
> Am 01.07.22 um 11:02 schrieb Dmitry Osipenko:
>> Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers don't
>> handle imported dma-bufs properly, which results in mapping of something
>> else than the imported dma-buf. On NVIDIA Tegra we get a hard lockup when
>> userspace writes to the memory mapping of a dma-buf that was imported
>> into
>> Tegra's DRM GEM.
>>
>> Majority of DRM drivers prohibit mapping of the imported GEM objects.
>> Mapping of imported GEMs require special care from userspace since it
>> should sync dma-buf because mapping coherency of the exporter device may
>> not match the DRM device. Let's prohibit the mapping for all DRM drivers
>> for consistency.
>>
>> Suggested-by: Thomas Hellström
>> Signed-off-by: Dmitry Osipenko
>
> I'm pretty sure that this is the right approach, but it's certainly more
> than possible that somebody abused this already.
I suspect that this is abused if you run deqp cts on android.. ie. all
winsys buffers are dma-buf imports from gralloc. And then when you
hit readpix...
You might only hit this in scenarios with separate gpu and display (or
dGPU+iGPU) because self-imports are handled differently in
drm_gem_prime_import_dev().. and maybe not in cases where you end up
with a blit from tiled/compressed to linear.. maybe that narrows the
scope enough to just fix it in userspace?
>>>
>>> Given that that only drivers which use DRM-SHMEM potentially could've
>>> map imported dma-bufs (Panfrost, Lima) and they already don't allow to
>>> do that, I think we're good.
>>
>> So can I have an ack from Rob here or are there still questions that this
>> might go boom?
>>
>> Dmitry, since you have a bunch of patches merged now I think would also be
>> good to get commit rights so you can drive this more yourself. I've asked
>> Daniel Stone to help you out with getting that.
>
> I *think* we'd be ok with this on msm, mostly just by dumb luck.
> Because the dma-buf's we import will be self-import. I'm less sure
> about panfrost (src/panfrost/lib/pan_bo.c doesn't seem to have a
> special path for imported dma-bufs either, and in that case they won't
> be self-imports.. but I guess no one has tried to run android cts on
> panfrost).
The last time I tried to mmap dma-buf imported to Panfrost didn't work
because Panfrost didn't implement something needed for that. I'll need
to take a look again because can't recall what it was.
> What about something less drastic to start, like (apologies for
> hand-edited patch):
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 86d670c71286..fc9ec42fa0ab 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1034,6 +1034,10 @@ int drm_gem_mmap_obj(struct drm_gem_object
> *obj, unsigned long obj_size,
> {
> int ret;
>
> + WARN_ON_ONCE(obj->import_attach);
This will hang NVIDIA Tegra, which is what this patch fixed initially.
If neither of upstream DRM drivers need to map imported dma-bufs and
never needed, then why do we need this?
--
Best regards,
Dmitry
Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs
On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter wrote:
>
> On Wed, Jul 06, 2022 at 10:02:07AM +0300, Dmitry Osipenko wrote:
> > On 7/6/22 00:48, Rob Clark wrote:
> > > On Tue, Jul 5, 2022 at 4:51 AM Christian König
> > > wrote:
> > >>
> > >> Am 01.07.22 um 11:02 schrieb Dmitry Osipenko:
> > >>> Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers don't
> > >>> handle imported dma-bufs properly, which results in mapping of something
> > >>> else than the imported dma-buf. On NVIDIA Tegra we get a hard lockup
> > >>> when
> > >>> userspace writes to the memory mapping of a dma-buf that was imported
> > >>> into
> > >>> Tegra's DRM GEM.
> > >>>
> > >>> Majority of DRM drivers prohibit mapping of the imported GEM objects.
> > >>> Mapping of imported GEMs require special care from userspace since it
> > >>> should sync dma-buf because mapping coherency of the exporter device may
> > >>> not match the DRM device. Let's prohibit the mapping for all DRM drivers
> > >>> for consistency.
> > >>>
> > >>> Suggested-by: Thomas Hellström
> > >>> Signed-off-by: Dmitry Osipenko
> > >>
> > >> I'm pretty sure that this is the right approach, but it's certainly more
> > >> than possible that somebody abused this already.
> > >
> > > I suspect that this is abused if you run deqp cts on android.. ie. all
> > > winsys buffers are dma-buf imports from gralloc. And then when you
> > > hit readpix...
> > >
> > > You might only hit this in scenarios with separate gpu and display (or
> > > dGPU+iGPU) because self-imports are handled differently in
> > > drm_gem_prime_import_dev().. and maybe not in cases where you end up
> > > with a blit from tiled/compressed to linear.. maybe that narrows the
> > > scope enough to just fix it in userspace?
> >
> > Given that that only drivers which use DRM-SHMEM potentially could've
> > map imported dma-bufs (Panfrost, Lima) and they already don't allow to
> > do that, I think we're good.
>
> So can I have an ack from Rob here or are there still questions that this
> might go boom?
>
> Dmitry, since you have a bunch of patches merged now I think would also be
> good to get commit rights so you can drive this more yourself. I've asked
> Daniel Stone to help you out with getting that.
I *think* we'd be ok with this on msm, mostly just by dumb luck.
Because the dma-buf's we import will be self-import. I'm less sure
about panfrost (src/panfrost/lib/pan_bo.c doesn't seem to have a
special path for imported dma-bufs either, and in that case they won't
be self-imports.. but I guess no one has tried to run android cts on
panfrost).
What about something less drastic to start, like (apologies for
hand-edited patch):
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 86d670c71286..fc9ec42fa0ab 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1034,6 +1034,10 @@ int drm_gem_mmap_obj(struct drm_gem_object
*obj, unsigned long obj_size,
{
int ret;
+ WARN_ON_ONCE(obj->import_attach);
+
/* Check for valid size. */
if (obj_size < vma->vm_end - vma->vm_start)
return -EINVAL;
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 8ad0e02991ca..6190f5018986 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -609,17 +609,8 @@ EXPORT_SYMBOL_GPL(drm_gem_shmem_vm_ops);
*/
int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct
vm_area_struct *vma)
{
- struct drm_gem_object *obj = &shmem->base;
int ret;
if (obj->import_attach) {
- /* Drop the reference drm_gem_mmap_obj() acquired.*/
- drm_gem_object_put(obj);
- vma->vm_private_data = NULL;
-
- return dma_buf_mmap(obj->dma_buf, vma, 0);
+ return -EINVAL;
}
ret = drm_gem_shmem_get_pages(shmem);
if (ret) {
drm_gem_vm_close(vma);
--
2.36.1
Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs
On Wed, Jul 06, 2022 at 10:02:07AM +0300, Dmitry Osipenko wrote: > On 7/6/22 00:48, Rob Clark wrote: > > On Tue, Jul 5, 2022 at 4:51 AM Christian König > > wrote: > >> > >> Am 01.07.22 um 11:02 schrieb Dmitry Osipenko: > >>> Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers don't > >>> handle imported dma-bufs properly, which results in mapping of something > >>> else than the imported dma-buf. On NVIDIA Tegra we get a hard lockup when > >>> userspace writes to the memory mapping of a dma-buf that was imported into > >>> Tegra's DRM GEM. > >>> > >>> Majority of DRM drivers prohibit mapping of the imported GEM objects. > >>> Mapping of imported GEMs require special care from userspace since it > >>> should sync dma-buf because mapping coherency of the exporter device may > >>> not match the DRM device. Let's prohibit the mapping for all DRM drivers > >>> for consistency. > >>> > >>> Suggested-by: Thomas Hellström > >>> Signed-off-by: Dmitry Osipenko > >> > >> I'm pretty sure that this is the right approach, but it's certainly more > >> than possible that somebody abused this already. > > > > I suspect that this is abused if you run deqp cts on android.. ie. all > > winsys buffers are dma-buf imports from gralloc. And then when you > > hit readpix... > > > > You might only hit this in scenarios with separate gpu and display (or > > dGPU+iGPU) because self-imports are handled differently in > > drm_gem_prime_import_dev().. and maybe not in cases where you end up > > with a blit from tiled/compressed to linear.. maybe that narrows the > > scope enough to just fix it in userspace? > > Given that that only drivers which use DRM-SHMEM potentially could've > map imported dma-bufs (Panfrost, Lima) and they already don't allow to > do that, I think we're good. So can I have an ack from Rob here or are there still questions that this might go boom? Dmitry, since you have a bunch of patches merged now I think would also be good to get commit rights so you can drive this more yourself. I've asked Daniel Stone to help you out with getting that. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs
On 7/6/22 00:48, Rob Clark wrote: > On Tue, Jul 5, 2022 at 4:51 AM Christian König > wrote: >> >> Am 01.07.22 um 11:02 schrieb Dmitry Osipenko: >>> Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers don't >>> handle imported dma-bufs properly, which results in mapping of something >>> else than the imported dma-buf. On NVIDIA Tegra we get a hard lockup when >>> userspace writes to the memory mapping of a dma-buf that was imported into >>> Tegra's DRM GEM. >>> >>> Majority of DRM drivers prohibit mapping of the imported GEM objects. >>> Mapping of imported GEMs require special care from userspace since it >>> should sync dma-buf because mapping coherency of the exporter device may >>> not match the DRM device. Let's prohibit the mapping for all DRM drivers >>> for consistency. >>> >>> Suggested-by: Thomas Hellström >>> Signed-off-by: Dmitry Osipenko >> >> I'm pretty sure that this is the right approach, but it's certainly more >> than possible that somebody abused this already. > > I suspect that this is abused if you run deqp cts on android.. ie. all > winsys buffers are dma-buf imports from gralloc. And then when you > hit readpix... > > You might only hit this in scenarios with separate gpu and display (or > dGPU+iGPU) because self-imports are handled differently in > drm_gem_prime_import_dev().. and maybe not in cases where you end up > with a blit from tiled/compressed to linear.. maybe that narrows the > scope enough to just fix it in userspace? Given that that only drivers which use DRM-SHMEM potentially could've map imported dma-bufs (Panfrost, Lima) and they already don't allow to do that, I think we're good. -- Best regards, Dmitry
Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs
On Tue, Jul 5, 2022 at 4:51 AM Christian König wrote:
>
> Am 01.07.22 um 11:02 schrieb Dmitry Osipenko:
> > Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers don't
> > handle imported dma-bufs properly, which results in mapping of something
> > else than the imported dma-buf. On NVIDIA Tegra we get a hard lockup when
> > userspace writes to the memory mapping of a dma-buf that was imported into
> > Tegra's DRM GEM.
> >
> > Majority of DRM drivers prohibit mapping of the imported GEM objects.
> > Mapping of imported GEMs require special care from userspace since it
> > should sync dma-buf because mapping coherency of the exporter device may
> > not match the DRM device. Let's prohibit the mapping for all DRM drivers
> > for consistency.
> >
> > Suggested-by: Thomas Hellström
> > Signed-off-by: Dmitry Osipenko
>
> I'm pretty sure that this is the right approach, but it's certainly more
> than possible that somebody abused this already.
I suspect that this is abused if you run deqp cts on android.. ie. all
winsys buffers are dma-buf imports from gralloc. And then when you
hit readpix...
You might only hit this in scenarios with separate gpu and display (or
dGPU+iGPU) because self-imports are handled differently in
drm_gem_prime_import_dev().. and maybe not in cases where you end up
with a blit from tiled/compressed to linear.. maybe that narrows the
scope enough to just fix it in userspace?
BR,
-R
> Anyway patch is Reviewed-by: Christian König
> since you are really fixing a major stability problem here.
>
> Regards,
> Christian.
>
> > ---
> > drivers/gpu/drm/drm_gem.c | 4
> > drivers/gpu/drm/drm_gem_shmem_helper.c | 9 -
> > 2 files changed, 4 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index 86d670c71286..fc9ec42fa0ab 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -1034,6 +1034,10 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj,
> > unsigned long obj_size,
> > {
> > int ret;
> >
> > + /* Don't allow imported objects to be mapped */
> > + if (obj->import_attach)
> > + return -EINVAL;
> > +
> > /* Check for valid size. */
> > if (obj_size < vma->vm_end - vma->vm_start)
> > return -EINVAL;
> > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
> > b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > index 8ad0e02991ca..6190f5018986 100644
> > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > @@ -609,17 +609,8 @@ EXPORT_SYMBOL_GPL(drm_gem_shmem_vm_ops);
> >*/
> > int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct
> > vm_area_struct *vma)
> > {
> > - struct drm_gem_object *obj = &shmem->base;
> > int ret;
> >
> > - if (obj->import_attach) {
> > - /* Drop the reference drm_gem_mmap_obj() acquired.*/
> > - drm_gem_object_put(obj);
> > - vma->vm_private_data = NULL;
> > -
> > - return dma_buf_mmap(obj->dma_buf, vma, 0);
> > - }
> > -
> > ret = drm_gem_shmem_get_pages(shmem);
> > if (ret) {
> > drm_gem_vm_close(vma);
>
Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs
Am 01.07.22 um 11:02 schrieb Dmitry Osipenko:
Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers don't
handle imported dma-bufs properly, which results in mapping of something
else than the imported dma-buf. On NVIDIA Tegra we get a hard lockup when
userspace writes to the memory mapping of a dma-buf that was imported into
Tegra's DRM GEM.
Majority of DRM drivers prohibit mapping of the imported GEM objects.
Mapping of imported GEMs require special care from userspace since it
should sync dma-buf because mapping coherency of the exporter device may
not match the DRM device. Let's prohibit the mapping for all DRM drivers
for consistency.
Suggested-by: Thomas Hellström
Signed-off-by: Dmitry Osipenko
I'm pretty sure that this is the right approach, but it's certainly more
than possible that somebody abused this already.
Anyway patch is Reviewed-by: Christian König
since you are really fixing a major stability problem here.
Regards,
Christian.
---
drivers/gpu/drm/drm_gem.c | 4
drivers/gpu/drm/drm_gem_shmem_helper.c | 9 -
2 files changed, 4 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 86d670c71286..fc9ec42fa0ab 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1034,6 +1034,10 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj,
unsigned long obj_size,
{
int ret;
+ /* Don't allow imported objects to be mapped */
+ if (obj->import_attach)
+ return -EINVAL;
+
/* Check for valid size. */
if (obj_size < vma->vm_end - vma->vm_start)
return -EINVAL;
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 8ad0e02991ca..6190f5018986 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -609,17 +609,8 @@ EXPORT_SYMBOL_GPL(drm_gem_shmem_vm_ops);
*/
int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct
vm_area_struct *vma)
{
- struct drm_gem_object *obj = &shmem->base;
int ret;
- if (obj->import_attach) {
- /* Drop the reference drm_gem_mmap_obj() acquired.*/
- drm_gem_object_put(obj);
- vma->vm_private_data = NULL;
-
- return dma_buf_mmap(obj->dma_buf, vma, 0);
- }
-
ret = drm_gem_shmem_get_pages(shmem);
if (ret) {
drm_gem_vm_close(vma);
[PATCH v8 2/2] drm/gem: Don't map imported GEMs
Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers don't
handle imported dma-bufs properly, which results in mapping of something
else than the imported dma-buf. On NVIDIA Tegra we get a hard lockup when
userspace writes to the memory mapping of a dma-buf that was imported into
Tegra's DRM GEM.
Majority of DRM drivers prohibit mapping of the imported GEM objects.
Mapping of imported GEMs require special care from userspace since it
should sync dma-buf because mapping coherency of the exporter device may
not match the DRM device. Let's prohibit the mapping for all DRM drivers
for consistency.
Suggested-by: Thomas Hellström
Signed-off-by: Dmitry Osipenko
---
drivers/gpu/drm/drm_gem.c | 4
drivers/gpu/drm/drm_gem_shmem_helper.c | 9 -
2 files changed, 4 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 86d670c71286..fc9ec42fa0ab 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1034,6 +1034,10 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj,
unsigned long obj_size,
{
int ret;
+ /* Don't allow imported objects to be mapped */
+ if (obj->import_attach)
+ return -EINVAL;
+
/* Check for valid size. */
if (obj_size < vma->vm_end - vma->vm_start)
return -EINVAL;
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 8ad0e02991ca..6190f5018986 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -609,17 +609,8 @@ EXPORT_SYMBOL_GPL(drm_gem_shmem_vm_ops);
*/
int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct
vm_area_struct *vma)
{
- struct drm_gem_object *obj = &shmem->base;
int ret;
- if (obj->import_attach) {
- /* Drop the reference drm_gem_mmap_obj() acquired.*/
- drm_gem_object_put(obj);
- vma->vm_private_data = NULL;
-
- return dma_buf_mmap(obj->dma_buf, vma, 0);
- }
-
ret = drm_gem_shmem_get_pages(shmem);
if (ret) {
drm_gem_vm_close(vma);
--
2.36.1
