On Thu, Aug 21, 2025 at 10:10 AM Thomas Zimmermann <tzimmerm...@suse.de> wrote:
>
> Hi
>
> Am 21.08.25 um 15:13 schrieb Alex Deucher:
> > On Thu, Aug 21, 2025 at 4:52 AM Christian König
> > <christian.koe...@amd.com> wrote:
> >>
> >>
> >> On 21.08.25 08:40, Thomas Zimmermann wrote:
> >>> Current dma-buf vmap semantics require that the mapped buffer remains
> >>> in place until the corresponding vunmap has completed.
> >>>
> >>> For GEM-SHMEM, this used to be guaranteed by a pin operation while
> >>> creating
> >>> an S/G table in import. GEM-SHMEN can now import dma-buf objects without
> >>> creating the S/G table, so the pin is missing. Leads to page-fault errors,
> >>> such as the one shown below.
> >>>
> >>> [ 102.101726] BUG: unable to handle page fault for address:
> >>> ffffc90127000000
> >>> [...]
> >>> [ 102.157102] RIP: 0010:udl_compress_hline16+0x219/0x940 [udl]
> >>> [...]
> >>> [ 102.243250] Call Trace:
> >>> [ 102.245695] <TASK>
> >>> [ 102.2477V95] ? validate_chain+0x24e/0x5e0
> >>> [ 102.251805] ? __lock_acquire+0x568/0xae0
> >>> [ 102.255807] udl_render_hline+0x165/0x341 [udl]
> >>> [ 102.260338] ? __pfx_udl_render_hline+0x10/0x10 [udl]
> >>> [ 102.265379] ? local_clock_noinstr+0xb/0x100
> >>> [ 102.269642] ? __lock_release.isra.0+0x16c/0x2e0
> >>> [ 102.274246] ? mark_held_locks+0x40/0x70
> >>> [ 102.278177] udl_primary_plane_helper_atomic_update+0x43e/0x680 [udl]
> >>> [ 102.284606] ? __pfx_udl_primary_plane_helper_atomic_update+0x10/0x10
> >>> [udl]
> >>> [ 102.291551] ? lockdep_hardirqs_on_prepare.part.0+0x92/0x170
> >>> [ 102.297208] ? lockdep_hardirqs_on+0x88/0x130
> >>> [ 102.301554] ? _raw_spin_unlock_irq+0x24/0x50
> >>> [ 102.305901] ? wait_for_completion_timeout+0x2bb/0x3a0
> >>> [ 102.311028] ?
> >>> drm_atomic_helper_calc_timestamping_constants+0x141/0x200
> >>> [ 102.317714] ? drm_atomic_helper_commit_planes+0x3b6/0x1030
> >>> [ 102.323279] drm_atomic_helper_commit_planes+0x3b6/0x1030
> >>> [ 102.328664] drm_atomic_helper_commit_tail+0x41/0xb0
> >>> [ 102.333622] commit_tail+0x204/0x330
> >>> [...]
> >>> [ 102.529946] ---[ end trace 0000000000000000 ]---
> >>> [ 102.651980] RIP: 0010:udl_compress_hline16+0x219/0x940 [udl]
> >>>
> >>> In this stack strace, udl (based on GEM-SHMEM) imported and vmap'ed a
> >>> dma-buf from amdgpu. Amdgpu relocated the buffer, thereby invalidating the
> >>> mapping.
> >>>
> >>> Provide a custom dma-buf vmap method in amdgpu that pins the object before
> >>> mapping it's buffer's pages into kernel address space. Do the opposite in
> >>> vunmap.
> >>>
> >>> Note that dma-buf vmap differs from GEM vmap in how it handles relocation.
> >>> While dma-buf vmap keeps the buffer in place, GEM vmap requires the caller
> >>> to keep the buffer in place. Hence, this fix is in amdgpu's dma-buf code
> >>> instead of its GEM code.
> >>>
> >>> A discussion of various approaches to solving the problem is available
> >>> at [1].
> >>>
> >>> v3:
> >>> - try (GTT | VRAM); drop CPU domain (Christian)
> >>> v2:
> >>> - only use mapable domains (Christian)
> >>> - try pinning to domains in preferred order
> >>>
> >>> Signed-off-by: Thomas Zimmermann <tzimmerm...@suse.de>
> >>> Fixes: 660cd44659a0 ("drm/shmem-helper: Import dmabuf without mapping its
> >>> sg_table")
> >>> Reported-by: Thomas Zimmermann <tzimmerm...@suse.de>
> >>> Closes:
> >>> https://lore.kernel.org/dri-devel/ba1bdfb8-dbf7-4372-bdcb-df7e0511c...@suse.de/
> >>> Cc: Shixiong Ou <oushixi...@kylinos.cn>
> >>> Cc: Thomas Zimmermann <tzimmerm...@suse.de>
> >>> Cc: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
> >>> Cc: Maxime Ripard <mrip...@kernel.org>
> >>> Cc: David Airlie <airl...@gmail.com>
> >>> Cc: Simona Vetter <sim...@ffwll.ch>
> >>> Cc: Sumit Semwal <sumit.sem...@linaro.org>
> >>> Cc: "Christian König" <christian.koe...@amd.com>
> >>> Cc: dri-de...@lists.freedesktop.org
> >>> Cc: linux-me...@vger.kernel.org
> >>> Cc: linaro-mm-...@lists.linaro.org
> >>> Link:
> >>> https://lore.kernel.org/dri-devel/9792c6c3-a2b8-4b2b-b5ba-fba19b153...@suse.de/
> >>> # [1]
> >> Reviewed-by: Christian König <christian.koe...@amd.com>
> > Thomas did you want to take this through drm-misc or do you want me to
> > pick this up?
>
> If you haven't send out this week's fixes for amdgpu, you're welcome to
> pick it up. Otherwise I can merge it via drm-misc-fixes next week.
Please go ahead. I've already sent out my -fixes PR this week.
Alex
>
> Best regards
> Thomas
>
> >
> > Thanks,
> >
> > Alex
> >
> >>> ---
> >>> drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 34 +++++++++++++++++++--
> >>> 1 file changed, 32 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> >>> index 5743ebb2f1b7..ce27cb5bb05e 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> >>> @@ -285,6 +285,36 @@ static int amdgpu_dma_buf_begin_cpu_access(struct
> >>> dma_buf *dma_buf,
> >>> return ret;
> >>> }
> >>>
> >>> +static int amdgpu_dma_buf_vmap(struct dma_buf *dma_buf, struct iosys_map
> >>> *map)
> >>> +{
> >>> + struct drm_gem_object *obj = dma_buf->priv;
> >>> + struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
> >>> + int ret;
> >>> +
> >>> + /*
> >>> + * Pin to keep buffer in place while it's vmap'ed. The actual
> >>> + * domain is not that important as long as it's mapable. Using
> >>> + * GTT and VRAM should be compatible with most use cases.
> >>> + */
> >>> + ret = amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT |
> >>> AMDGPU_GEM_DOMAIN_VRAM);
> >>> + if (ret)
> >>> + return ret;
> >>> + ret = drm_gem_dmabuf_vmap(dma_buf, map);
> >>> + if (ret)
> >>> + amdgpu_bo_unpin(bo);
> >>> +
> >>> + return ret;
> >>> +}
> >>> +
> >>> +static void amdgpu_dma_buf_vunmap(struct dma_buf *dma_buf, struct
> >>> iosys_map *map)
> >>> +{
> >>> + struct drm_gem_object *obj = dma_buf->priv;
> >>> + struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
> >>> +
> >>> + drm_gem_dmabuf_vunmap(dma_buf, map);
> >>> + amdgpu_bo_unpin(bo);
> >>> +}
> >>> +
> >>> const struct dma_buf_ops amdgpu_dmabuf_ops = {
> >>> .attach = amdgpu_dma_buf_attach,
> >>> .pin = amdgpu_dma_buf_pin,
> >>> @@ -294,8 +324,8 @@ const struct dma_buf_ops amdgpu_dmabuf_ops = {
> >>> .release = drm_gem_dmabuf_release,
> >>> .begin_cpu_access = amdgpu_dma_buf_begin_cpu_access,
> >>> .mmap = drm_gem_dmabuf_mmap,
> >>> - .vmap = drm_gem_dmabuf_vmap,
> >>> - .vunmap = drm_gem_dmabuf_vunmap,
> >>> + .vmap = amdgpu_dma_buf_vmap,
> >>> + .vunmap = amdgpu_dma_buf_vunmap,
> >>> };
> >>>
> >>> /**
> >> _______________________________________________
> >> Linaro-mm-sig mailing list -- linaro-mm-...@lists.linaro.org
> >> To unsubscribe send an email to linaro-mm-sig-le...@lists.linaro.org
>
> --
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Frankenstrasse 146, 90461 Nuernberg, Germany
> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
> HRB 36809 (AG Nuernberg)
>
>
