On 9/9/26 11:46, Mike Lothian wrote:
> amdgpu_dma_buf_map() adds VRAM to the allowed domains for a peer2peer
> attachment, so ttm_bo_validate() can migrate the buffer from GTT into
> VRAM. While the exporting device is runtime suspended its SDMA rings
> are down and the move fails:
>
> amdgpu: Move buffer fallback to memcpy unavailable
>
> An importer on a second GPU reaches this holding no runtime PM
> reference on the exporter, e.g. a compositor on the APU submitting a
> frame that references a buffer exported by an idle dGPU:
>
> amdgpu_cs_ioctl -> amdgpu_cs_parser_bos -> amdgpu_cs_bo_validate
> -> ttm_bo_validate -> amdgpu_bo_move -> dma_buf_map_attachment
> -> amdgpu_dma_buf_map -> ttm_bo_validate -> amdgpu_bo_move
>
> Only migrate into VRAM while holding the exporter awake.
> pm_runtime_get_if_active() takes a reference only when the device is
> already active and never resumes it, so it cannot deadlock against the
> reservation held across these callbacks. That deadlock is why
> commit 030631e97b20 ("drm/amdgpu: revert "take runtime pm reference
> when we attach a buffer" v2") removed the pm_runtime_get_sync() from
> the attach callback.
>
> If the device is suspended or suspending the buffer stays in GTT, which
> remains accessible while the GPU is powered down. A negative return
> means runtime PM is disabled, so the device cannot suspend and VRAM
> stays usable.
That is a good catch, but your bug explanation as well as the solution still
look a bit questionable to me.
> Fixes: 030631e97b20 ("drm/amdgpu: revert "take runtime pm reference when we
> attach a buffer" v2")
> Cc: [email protected]
> Signed-off-by: Mike Lothian <[email protected]>
> Assisted-by: Claude:Opus-5 [Claude Code]
> ---
>
> v2: hold the exporter with pm_runtime_get_if_active() across the
> validate instead of testing adev->mman.buffer_funcs_enabled. The
> v1 check was racy - the device could suspend between the test and
> ttm_bo_validate(), so amdgpu_bo_move() could still see the rings
> torn down. Reported by Sashiko AI review.
>
> Is this the failure that commit c52feb436539 ("drm/amdgpu: Disable
> runtime PM for externally attached dGPUs") was working around? That
> commit explains how to detect external attachment but not what breaks,
> so I can't tell which.
>
> If it is the same thing, could the pci_is_thunderbolt_attached() ||
> dev_is_removable() check there be narrowed or dropped on top of this,
> so eGPU users keep runtime PM?
>
> I can't test that here: this box hits the bug by missing that check.
> The dGPU is on an oculink port off a native AMD root port, so
> pci_is_thunderbolt_attached() is false, dev_is_removable() is empty,
> and runtime PM stays enabled.
>
> Reproduced on a HawkPoint APU [1002:1900] driving the display with a
> Navi 48 [Radeon AI PRO R9700] [1002:7551] on oculink for render
> offload. Without the patch kwin_wayland hits the call chain above
> within a minute of the dGPU autosuspending and the desktop stops
> repainting until it resumes.
That sounds like there is also a bug in kwin as well.
The GPU can only go into suspend when the rendering application closes it
driver connection and that usually only happens when it terminates.
So question is here why is kwin still having that imported DMA-buf as necessary
resource for the rendering?
But we still need to fix this properly in the kernel anyway to avoid having a
deny of service.
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 16 ++++++++++++++--
> 1 file changed, 14 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 b33c300e26e2..c89846f266d3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> @@ -43,6 +43,7 @@
> #include <linux/dma-buf.h>
> #include <linux/dma-fence-array.h>
> #include <linux/pci-p2pdma.h>
> +#include <linux/pm_runtime.h>
>
> static const struct dma_buf_attach_ops amdgpu_dma_buf_attach_ops;
>
> @@ -189,14 +190,25 @@ static struct sg_table *amdgpu_dma_buf_map(struct
> dma_buf_attachment *attach,
> /* move buffer into GTT or VRAM */
> struct ttm_operation_ctx ctx = { false, false };
> unsigned int domains = AMDGPU_GEM_DOMAIN_GTT;
> + int pm_ref = 0;
>
> if (bo->preferred_domains & AMDGPU_GEM_DOMAIN_VRAM &&
> attach->peer2peer) {
> - bo->flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
> - domains |= AMDGPU_GEM_DOMAIN_VRAM;
> + /*
> + * Only migrate into VRAM while the exporter is held
> + * awake. A negative return means runtime PM is
> + * disabled, so it cannot suspend either.
> + */
Setting AMDGPU_GEM_DOMAIN_VRAM doesn't automatically migrate the BO, it just
sets this as possible placement.
BO migration is only triggered if the BO is swapped out or similar. What most
likely happens instead is that we suspend while something is still ongoing.
But anyway the problem goes deeper than just the amdgpu_dma_buf_map() callback.
We have picked up pinning DMA-buf to VRAM for RDMA without ODP (e.g. exactly
the feature I mention in the commit message of c52feb436539), but failed to
correctly fix the PM handling.
So we really need to call pm_runtime_get_if_active() in amdgpu_dma_buf_attach()
and fail to let some other driver attach if the device is already suspended.
Regards,
Christian.
> + pm_ref =
> pm_runtime_get_if_active(adev_to_drm(adev)->dev);
> + if (pm_ref) {
> + bo->flags |=
> AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
> + domains |= AMDGPU_GEM_DOMAIN_VRAM;
> + }
> }
> amdgpu_bo_placement_from_domain(bo, domains);
> r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
> + if (pm_ref > 0)
> + pm_runtime_put_autosuspend(adev_to_drm(adev)->dev);
> if (r)
> return ERR_PTR(r);
> }
