On 8/14/26 19:29, Alex Deucher wrote:
If we end up emitting a VM fence keep pipeline sync
associated with that fence. If not, emit them as
part of the IB fence.
v2: fix need_pipe_sync handling
Cc: David Rosca <[email protected]>
Fixes: cb1e657ccac8 ("drm/amdgpu: handle GDS and SPM without a VM fence")
Signed-off-by: Alex Deucher <[email protected]>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c | 6 +++++-
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 8 +++++---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 2 +-
3 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
index da4dc489e80bd..360e6f00cb7c0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
@@ -222,7 +222,7 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned
int num_ibs,
vm_af = job->hw_vm_fence;
/* VM sequence */
vm_af->ib_wptr = ring->wptr;
- amdgpu_vm_flush(ring, job, need_pipe_sync, &emit_spm_needed,
+ amdgpu_vm_flush(ring, job, &need_pipe_sync, &emit_spm_needed,
&emit_gds_needed);
vm_af->ib_dw_size =
amdgpu_ring_get_dw_distance(ring, vm_af->ib_wptr,
ring->wptr);
@@ -235,6 +235,10 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned
int num_ibs,
if (ring->funcs->insert_start)
ring->funcs->insert_start(ring);
+ /* this may have been handled by amdgpu_vm_flush */
+ if (need_pipe_sync)
+ amdgpu_ring_emit_pipeline_sync(ring);
+
if (emit_spm_needed)
adev->gfx.rlc.funcs->update_spm_vmid(adev, ring->xcc_id, ring,
job->vmid);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 71050a86bcc3a..b7d0461184d62 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -772,7 +772,7 @@ bool amdgpu_vm_need_pipeline_sync(struct amdgpu_ring *ring,
* Emit a VM flush when it is necessary.
*/
void amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
- bool need_pipe_sync, bool *emit_spm_needed,
+ bool *need_pipe_sync, bool *emit_spm_needed,
bool *emit_gds_needed)
{
struct amdgpu_device *adev = ring->adev;
@@ -827,7 +827,7 @@ void amdgpu_vm_flush(struct amdgpu_ring *ring, struct
amdgpu_job *job,
if (gds_switch_needed && emit_fence)
*emit_gds_needed = false;
- if (!vm_flush_needed && !gds_switch_needed && !need_pipe_sync &&
+ if (!vm_flush_needed && !gds_switch_needed && !(*need_pipe_sync) &&
The only time need_pipe_sync in this check makes a difference is when
only pasid_mapping_needed (which is missing from this condition, is that
intended?) is true and the rest *_needed are false. Then emit_fence is
true and pipeline_sync is emitted in this function which looks fine.
If pasid_mapping_needed and all other *_needed are false, then
emit_fence is false and the rest of the function effectively does
nothing. emit_pipeline_sync will be called from amdgpu_ib_schedule.
While this works, I think it would be better to return early here?
David
!cleaner_shader_needed && !spm_update_needed)
return;
@@ -847,8 +847,10 @@ void amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
patch = amdgpu_ring_init_cond_exec(ring,
ring->cond_exe_gpu_addr);
- if (need_pipe_sync)
+ if (emit_fence && *need_pipe_sync) {
amdgpu_ring_emit_pipeline_sync(ring);
+ *need_pipe_sync = false;
+ }
if (cleaner_shader_needed)
ring->funcs->emit_cleaner_shader(ring);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 7f2ba728e3ed3..d32183cd9e0fc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -512,7 +512,7 @@ int amdgpu_vm_validate(struct amdgpu_device *adev, struct
amdgpu_vm *vm,
int (*callback)(void *p, struct amdgpu_bo *bo),
void *param);
void amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
- bool need_pipe_sync, bool *emit_spm_needed,
+ bool *need_pipe_sync, bool *emit_spm_needed,
bool *emit_gds_needed);
int amdgpu_vm_update_pdes(struct amdgpu_device *adev,
struct amdgpu_vm *vm, bool immediate);
