* [PATCH 1/2] drm/amdgpu: revert "always sync the GFX pipe on ctx switch"
@ 2025-05-08 12:05 Christian König
2025-05-08 12:05 ` [PATCH 2/2] drm/amdgpu: cleanup amdgpu_vm_flush v7 Christian König
0 siblings, 1 reply; 4+ messages in thread
From: Christian König @ 2025-05-08 12:05 UTC (permalink / raw)
To: srinivasan.shanmugam, amd-gfx
This reverts commit c2cc3648ba517a6c270500b5447d5a1efdad5936.
Not needed any more with the updated cleaner shader code.
Signed-off-by: Christian König <christian.koenig@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c | 4 ++--
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
index 802743efa3b3..5eab1c1a380c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
@@ -191,8 +191,8 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned int num_ibs,
need_ctx_switch = ring->current_ctx != fence_ctx;
if (ring->funcs->emit_pipeline_sync && job &&
((tmp = amdgpu_sync_get_fence(&job->explicit_sync)) ||
- need_ctx_switch || amdgpu_vm_need_pipeline_sync(ring, job))) {
-
+ (amdgpu_sriov_vf(adev) && need_ctx_switch) ||
+ amdgpu_vm_need_pipeline_sync(ring, job))) {
need_pipe_sync = true;
if (tmp)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 3911c78f8282..0a80c011e678 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -801,7 +801,7 @@ int 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 (need_pipe_sync || cleaner_shader_needed)
amdgpu_ring_emit_pipeline_sync(ring);
if (cleaner_shader_needed)
--
2.34.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/2] drm/amdgpu: cleanup amdgpu_vm_flush v7
2025-05-08 12:05 [PATCH 1/2] drm/amdgpu: revert "always sync the GFX pipe on ctx switch" Christian König
@ 2025-05-08 12:05 ` Christian König
2025-05-08 18:33 ` Alex Deucher
0 siblings, 1 reply; 4+ messages in thread
From: Christian König @ 2025-05-08 12:05 UTC (permalink / raw)
To: srinivasan.shanmugam, amd-gfx
Check if the cleaner shader should run directly.
While at it remove amdgpu_vm_need_pipeline_sync(), we also check again
if the VMID has seen a GPU reset since last use and the gds switch
setiing can be handled more simply as well.
Also remove some duplicate checks and re-order and document the code.
v2: restructure the while function
v3: fix logic error pointed out by Srini
v4: fix typo in comment, fix crash caused by incorrect check
v5: once more fix the logic
v6: separate cleaner shader checks as suggested by Srini
v7: re-order incorrect check
v8: separate the revert
Signed-off-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c | 6 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 106 ++++++++++---------------
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 5 +-
3 files changed, 46 insertions(+), 71 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
index 5eab1c1a380c..30b58772598c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
@@ -189,10 +189,8 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned int num_ibs,
}
need_ctx_switch = ring->current_ctx != fence_ctx;
- if (ring->funcs->emit_pipeline_sync && job &&
- ((tmp = amdgpu_sync_get_fence(&job->explicit_sync)) ||
- (amdgpu_sriov_vf(adev) && need_ctx_switch) ||
- amdgpu_vm_need_pipeline_sync(ring, job))) {
+ if ((job && (tmp = amdgpu_sync_get_fence(&job->explicit_sync))) ||
+ (amdgpu_sriov_vf(adev) && need_ctx_switch)) {
need_pipe_sync = true;
if (tmp)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 0a80c011e678..31c423663b54 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -707,37 +707,6 @@ void amdgpu_vm_check_compute_bug(struct amdgpu_device *adev)
}
}
-/**
- * amdgpu_vm_need_pipeline_sync - Check if pipe sync is needed for job.
- *
- * @ring: ring on which the job will be submitted
- * @job: job to submit
- *
- * Returns:
- * True if sync is needed.
- */
-bool amdgpu_vm_need_pipeline_sync(struct amdgpu_ring *ring,
- struct amdgpu_job *job)
-{
- struct amdgpu_device *adev = ring->adev;
- unsigned vmhub = ring->vm_hub;
- struct amdgpu_vmid_mgr *id_mgr = &adev->vm_manager.id_mgr[vmhub];
-
- if (job->vmid == 0)
- return false;
-
- if (job->vm_needs_flush || ring->has_compute_vm_bug)
- return true;
-
- if (ring->funcs->emit_gds_switch && job->gds_switch_needed)
- return true;
-
- if (amdgpu_vmid_had_gpu_reset(adev, &id_mgr->ids[job->vmid]))
- return true;
-
- return false;
-}
-
/**
* amdgpu_vm_flush - hardware flush the vm
*
@@ -758,44 +727,52 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
unsigned vmhub = ring->vm_hub;
struct amdgpu_vmid_mgr *id_mgr = &adev->vm_manager.id_mgr[vmhub];
struct amdgpu_vmid *id = &id_mgr->ids[job->vmid];
- bool spm_update_needed = job->spm_update_needed;
- bool gds_switch_needed = ring->funcs->emit_gds_switch &&
- job->gds_switch_needed;
- bool vm_flush_needed = job->vm_needs_flush;
- bool cleaner_shader_needed = false;
- bool pasid_mapping_needed = false;
- struct dma_fence *fence = NULL;
+ bool gds_switch_needed, vm_flush_needed, spm_update_needed,
+ cleaner_shader_needed, pasid_mapping_needed;
+ struct dma_fence *fence;
unsigned int patch;
int r;
+ /* First of all figure out what needs to be done */
if (amdgpu_vmid_had_gpu_reset(adev, id)) {
+ need_pipe_sync = true;
gds_switch_needed = true;
vm_flush_needed = true;
pasid_mapping_needed = true;
spm_update_needed = true;
+ cleaner_shader_needed = true;
+ } else {
+ gds_switch_needed = job->gds_switch_needed;
+ vm_flush_needed = job->vm_needs_flush;
+ mutex_lock(&id_mgr->lock);
+ pasid_mapping_needed = id->pasid != job->pasid ||
+ !id->pasid_mapping ||
+ !dma_fence_is_signaled(id->pasid_mapping);
+ mutex_unlock(&id_mgr->lock);
+ spm_update_needed = job->spm_update_needed;
+ cleaner_shader_needed = job->run_cleaner_shader &&
+ job->base.s_fence && &job->base.s_fence->scheduled ==
+ isolation->spearhead;
+ need_pipe_sync |= ring->has_compute_vm_bug || vm_flush_needed ||
+ cleaner_shader_needed || gds_switch_needed;
}
- mutex_lock(&id_mgr->lock);
- if (id->pasid != job->pasid || !id->pasid_mapping ||
- !dma_fence_is_signaled(id->pasid_mapping))
- pasid_mapping_needed = true;
- mutex_unlock(&id_mgr->lock);
-
+ /* Then check the pre-requisites */
+ need_pipe_sync &= !!ring->funcs->emit_pipeline_sync;
gds_switch_needed &= !!ring->funcs->emit_gds_switch;
vm_flush_needed &= !!ring->funcs->emit_vm_flush &&
job->vm_pd_addr != AMDGPU_BO_INVALID_OFFSET;
pasid_mapping_needed &= adev->gmc.gmc_funcs->emit_pasid_mapping &&
ring->funcs->emit_wreg;
-
- cleaner_shader_needed = job->run_cleaner_shader &&
- adev->gfx.enable_cleaner_shader &&
- ring->funcs->emit_cleaner_shader && job->base.s_fence &&
- &job->base.s_fence->scheduled == isolation->spearhead;
+ spm_update_needed &= !!adev->gfx.rlc.funcs->update_spm_vmid;
+ cleaner_shader_needed &= adev->gfx.enable_cleaner_shader &&
+ ring->funcs->emit_cleaner_shader;
if (!vm_flush_needed && !gds_switch_needed && !need_pipe_sync &&
- !cleaner_shader_needed)
+ !cleaner_shader_needed && !spm_update_needed)
return 0;
+ /* Then actually prepare the submission frame */
amdgpu_ring_ib_begin(ring);
if (ring->funcs->init_cond_exec)
patch = amdgpu_ring_init_cond_exec(ring,
@@ -815,23 +792,34 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
if (pasid_mapping_needed)
amdgpu_gmc_emit_pasid_mapping(ring, job->vmid, job->pasid);
- if (spm_update_needed && adev->gfx.rlc.funcs->update_spm_vmid)
+ if (spm_update_needed)
adev->gfx.rlc.funcs->update_spm_vmid(adev, ring, job->vmid);
- if (ring->funcs->emit_gds_switch &&
- gds_switch_needed) {
+ if (gds_switch_needed)
amdgpu_ring_emit_gds_switch(ring, job->vmid, job->gds_base,
job->gds_size, job->gws_base,
job->gws_size, job->oa_base,
job->oa_size);
- }
if (vm_flush_needed || pasid_mapping_needed || cleaner_shader_needed) {
r = amdgpu_fence_emit(ring, &fence, NULL, 0);
if (r)
return r;
+ } else {
+ fence = NULL;
+ }
+
+ amdgpu_ring_patch_cond_exec(ring, patch);
+
+ /* the double SWITCH_BUFFER here *cannot* be skipped by COND_EXEC */
+ if (ring->funcs->emit_switch_buffer) {
+ amdgpu_ring_emit_switch_buffer(ring);
+ amdgpu_ring_emit_switch_buffer(ring);
}
+ amdgpu_ring_ib_end(ring);
+
+ /* And finally remember what the ring has executed */
if (vm_flush_needed) {
mutex_lock(&id_mgr->lock);
dma_fence_put(id->last_flush);
@@ -861,16 +849,6 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
mutex_unlock(&adev->enforce_isolation_mutex);
}
dma_fence_put(fence);
-
- amdgpu_ring_patch_cond_exec(ring, patch);
-
- /* the double SWITCH_BUFFER here *cannot* be skipped by COND_EXEC */
- if (ring->funcs->emit_switch_buffer) {
- amdgpu_ring_emit_switch_buffer(ring);
- amdgpu_ring_emit_switch_buffer(ring);
- }
-
- amdgpu_ring_ib_end(ring);
return 0;
}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index f3ad687125ad..c9578b7f670c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -498,7 +498,8 @@ int amdgpu_vm_validate(struct amdgpu_device *adev, struct amdgpu_vm *vm,
struct ww_acquire_ctx *ticket,
int (*callback)(void *p, struct amdgpu_bo *bo),
void *param);
-int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job, bool need_pipe_sync);
+int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
+ bool need_pipe_sync);
int amdgpu_vm_update_pdes(struct amdgpu_device *adev,
struct amdgpu_vm *vm, bool immediate);
int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
@@ -559,8 +560,6 @@ void amdgpu_vm_adjust_size(struct amdgpu_device *adev, uint32_t min_vm_size,
uint32_t fragment_size_default, unsigned max_level,
unsigned max_bits);
int amdgpu_vm_ioctl(struct drm_device *dev, void *data, struct drm_file *filp);
-bool amdgpu_vm_need_pipeline_sync(struct amdgpu_ring *ring,
- struct amdgpu_job *job);
void amdgpu_vm_check_compute_bug(struct amdgpu_device *adev);
struct amdgpu_task_info *
--
2.34.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] drm/amdgpu: cleanup amdgpu_vm_flush v7
2025-05-08 12:05 ` [PATCH 2/2] drm/amdgpu: cleanup amdgpu_vm_flush v7 Christian König
@ 2025-05-08 18:33 ` Alex Deucher
2025-05-15 7:41 ` SRINIVASAN SHANMUGAM
0 siblings, 1 reply; 4+ messages in thread
From: Alex Deucher @ 2025-05-08 18:33 UTC (permalink / raw)
To: Christian König; +Cc: srinivasan.shanmugam, amd-gfx
On Thu, May 8, 2025 at 8:05 AM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> Check if the cleaner shader should run directly.
>
> While at it remove amdgpu_vm_need_pipeline_sync(), we also check again
> if the VMID has seen a GPU reset since last use and the gds switch
> setiing can be handled more simply as well.
>
> Also remove some duplicate checks and re-order and document the code.
>
> v2: restructure the while function
> v3: fix logic error pointed out by Srini
> v4: fix typo in comment, fix crash caused by incorrect check
> v5: once more fix the logic
> v6: separate cleaner shader checks as suggested by Srini
> v7: re-order incorrect check
> v8: separate the revert
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Reviewed-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c | 6 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 106 ++++++++++---------------
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 5 +-
> 3 files changed, 46 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> index 5eab1c1a380c..30b58772598c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> @@ -189,10 +189,8 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned int num_ibs,
> }
>
> need_ctx_switch = ring->current_ctx != fence_ctx;
> - if (ring->funcs->emit_pipeline_sync && job &&
> - ((tmp = amdgpu_sync_get_fence(&job->explicit_sync)) ||
> - (amdgpu_sriov_vf(adev) && need_ctx_switch) ||
> - amdgpu_vm_need_pipeline_sync(ring, job))) {
> + if ((job && (tmp = amdgpu_sync_get_fence(&job->explicit_sync))) ||
> + (amdgpu_sriov_vf(adev) && need_ctx_switch)) {
> need_pipe_sync = true;
>
> if (tmp)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 0a80c011e678..31c423663b54 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -707,37 +707,6 @@ void amdgpu_vm_check_compute_bug(struct amdgpu_device *adev)
> }
> }
>
> -/**
> - * amdgpu_vm_need_pipeline_sync - Check if pipe sync is needed for job.
> - *
> - * @ring: ring on which the job will be submitted
> - * @job: job to submit
> - *
> - * Returns:
> - * True if sync is needed.
> - */
> -bool amdgpu_vm_need_pipeline_sync(struct amdgpu_ring *ring,
> - struct amdgpu_job *job)
> -{
> - struct amdgpu_device *adev = ring->adev;
> - unsigned vmhub = ring->vm_hub;
> - struct amdgpu_vmid_mgr *id_mgr = &adev->vm_manager.id_mgr[vmhub];
> -
> - if (job->vmid == 0)
> - return false;
> -
> - if (job->vm_needs_flush || ring->has_compute_vm_bug)
> - return true;
> -
> - if (ring->funcs->emit_gds_switch && job->gds_switch_needed)
> - return true;
> -
> - if (amdgpu_vmid_had_gpu_reset(adev, &id_mgr->ids[job->vmid]))
> - return true;
> -
> - return false;
> -}
> -
> /**
> * amdgpu_vm_flush - hardware flush the vm
> *
> @@ -758,44 +727,52 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
> unsigned vmhub = ring->vm_hub;
> struct amdgpu_vmid_mgr *id_mgr = &adev->vm_manager.id_mgr[vmhub];
> struct amdgpu_vmid *id = &id_mgr->ids[job->vmid];
> - bool spm_update_needed = job->spm_update_needed;
> - bool gds_switch_needed = ring->funcs->emit_gds_switch &&
> - job->gds_switch_needed;
> - bool vm_flush_needed = job->vm_needs_flush;
> - bool cleaner_shader_needed = false;
> - bool pasid_mapping_needed = false;
> - struct dma_fence *fence = NULL;
> + bool gds_switch_needed, vm_flush_needed, spm_update_needed,
> + cleaner_shader_needed, pasid_mapping_needed;
Would be good to document what all of these flags are used for. E.g
/* need_pipe_sync - if set, we wait for the last fence on the ring to
signal before executing more commands
* cleaner_shader_needed - if set we emit the cleaner shader to clean
up GPRs and LDS before a new command is submitted
* etc.
*/
> + struct dma_fence *fence;
> unsigned int patch;
> int r;
>
> + /* First of all figure out what needs to be done */
> if (amdgpu_vmid_had_gpu_reset(adev, id)) {
Please add a comment here to explain why we set all of these to true
if we had a GPU reset.
> + need_pipe_sync = true;
> gds_switch_needed = true;
> vm_flush_needed = true;
> pasid_mapping_needed = true;
> spm_update_needed = true;
> + cleaner_shader_needed = true;
> + } else {
Would be good to document all of these cases as well.
> + gds_switch_needed = job->gds_switch_needed;
> + vm_flush_needed = job->vm_needs_flush;
> + mutex_lock(&id_mgr->lock);
> + pasid_mapping_needed = id->pasid != job->pasid ||
> + !id->pasid_mapping ||
> + !dma_fence_is_signaled(id->pasid_mapping);
> + mutex_unlock(&id_mgr->lock);
> + spm_update_needed = job->spm_update_needed;
E.g.:
/* The spearhead marks the first submission from a new client. We
need to run the cleaner shader
* if it is requested by the job and we have a new spearhead so that
we clean up before it runs.
*/
> + cleaner_shader_needed = job->run_cleaner_shader &&
> + job->base.s_fence && &job->base.s_fence->scheduled ==
> + isolation->spearhead;
E.g.,
/* This will cause the queue to wait for the current fence on the ring
to signal before new work executes (wait for idle).
* This is needed as a workaround for some hardware
(ring->has_compute_vm_bug), if we are updating
* the vmid or page tables (vm_flush_needed), if we need to emit the
cleaner shader (which must execute while the
* queue is idle), or if the job uses gds and we need to update the
gds mappings (gds_switch_needed).
*/
> + need_pipe_sync |= ring->has_compute_vm_bug || vm_flush_needed ||
> + cleaner_shader_needed || gds_switch_needed;
> }
>
> - mutex_lock(&id_mgr->lock);
> - if (id->pasid != job->pasid || !id->pasid_mapping ||
> - !dma_fence_is_signaled(id->pasid_mapping))
> - pasid_mapping_needed = true;
> - mutex_unlock(&id_mgr->lock);
> -
> + /* Then check the pre-requisites */
> + need_pipe_sync &= !!ring->funcs->emit_pipeline_sync;
> gds_switch_needed &= !!ring->funcs->emit_gds_switch;
> vm_flush_needed &= !!ring->funcs->emit_vm_flush &&
> job->vm_pd_addr != AMDGPU_BO_INVALID_OFFSET;
> pasid_mapping_needed &= adev->gmc.gmc_funcs->emit_pasid_mapping &&
> ring->funcs->emit_wreg;
> -
> - cleaner_shader_needed = job->run_cleaner_shader &&
> - adev->gfx.enable_cleaner_shader &&
> - ring->funcs->emit_cleaner_shader && job->base.s_fence &&
> - &job->base.s_fence->scheduled == isolation->spearhead;
> + spm_update_needed &= !!adev->gfx.rlc.funcs->update_spm_vmid;
> + cleaner_shader_needed &= adev->gfx.enable_cleaner_shader &&
> + ring->funcs->emit_cleaner_shader;
>
> if (!vm_flush_needed && !gds_switch_needed && !need_pipe_sync &&
> - !cleaner_shader_needed)
> + !cleaner_shader_needed && !spm_update_needed)
> return 0;
>
> + /* Then actually prepare the submission frame */
> amdgpu_ring_ib_begin(ring);
> if (ring->funcs->init_cond_exec)
> patch = amdgpu_ring_init_cond_exec(ring,
> @@ -815,23 +792,34 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
> if (pasid_mapping_needed)
> amdgpu_gmc_emit_pasid_mapping(ring, job->vmid, job->pasid);
>
> - if (spm_update_needed && adev->gfx.rlc.funcs->update_spm_vmid)
> + if (spm_update_needed)
> adev->gfx.rlc.funcs->update_spm_vmid(adev, ring, job->vmid);
>
> - if (ring->funcs->emit_gds_switch &&
> - gds_switch_needed) {
> + if (gds_switch_needed)
> amdgpu_ring_emit_gds_switch(ring, job->vmid, job->gds_base,
> job->gds_size, job->gws_base,
> job->gws_size, job->oa_base,
> job->oa_size);
> - }
>
> if (vm_flush_needed || pasid_mapping_needed || cleaner_shader_needed) {
> r = amdgpu_fence_emit(ring, &fence, NULL, 0);
> if (r)
> return r;
> + } else {
> + fence = NULL;
> + }
> +
> + amdgpu_ring_patch_cond_exec(ring, patch);
> +
> + /* the double SWITCH_BUFFER here *cannot* be skipped by COND_EXEC */
> + if (ring->funcs->emit_switch_buffer) {
> + amdgpu_ring_emit_switch_buffer(ring);
> + amdgpu_ring_emit_switch_buffer(ring);
> }
>
> + amdgpu_ring_ib_end(ring);
> +
> + /* And finally remember what the ring has executed */
> if (vm_flush_needed) {
> mutex_lock(&id_mgr->lock);
> dma_fence_put(id->last_flush);
> @@ -861,16 +849,6 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
> mutex_unlock(&adev->enforce_isolation_mutex);
> }
> dma_fence_put(fence);
> -
> - amdgpu_ring_patch_cond_exec(ring, patch);
> -
> - /* the double SWITCH_BUFFER here *cannot* be skipped by COND_EXEC */
> - if (ring->funcs->emit_switch_buffer) {
> - amdgpu_ring_emit_switch_buffer(ring);
> - amdgpu_ring_emit_switch_buffer(ring);
> - }
> -
> - amdgpu_ring_ib_end(ring);
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index f3ad687125ad..c9578b7f670c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -498,7 +498,8 @@ int amdgpu_vm_validate(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> struct ww_acquire_ctx *ticket,
> int (*callback)(void *p, struct amdgpu_bo *bo),
> void *param);
> -int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job, bool need_pipe_sync);
> +int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
> + bool need_pipe_sync);
> int amdgpu_vm_update_pdes(struct amdgpu_device *adev,
> struct amdgpu_vm *vm, bool immediate);
> int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
> @@ -559,8 +560,6 @@ void amdgpu_vm_adjust_size(struct amdgpu_device *adev, uint32_t min_vm_size,
> uint32_t fragment_size_default, unsigned max_level,
> unsigned max_bits);
> int amdgpu_vm_ioctl(struct drm_device *dev, void *data, struct drm_file *filp);
> -bool amdgpu_vm_need_pipeline_sync(struct amdgpu_ring *ring,
> - struct amdgpu_job *job);
> void amdgpu_vm_check_compute_bug(struct amdgpu_device *adev);
>
> struct amdgpu_task_info *
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] drm/amdgpu: cleanup amdgpu_vm_flush v7
2025-05-08 18:33 ` Alex Deucher
@ 2025-05-15 7:41 ` SRINIVASAN SHANMUGAM
0 siblings, 0 replies; 4+ messages in thread
From: SRINIVASAN SHANMUGAM @ 2025-05-15 7:41 UTC (permalink / raw)
To: Alex Deucher, Christian König; +Cc: amd-gfx
On 5/9/2025 12:03 AM, Alex Deucher wrote:
> On Thu, May 8, 2025 at 8:05 AM Christian König
> <ckoenig.leichtzumerken@gmail.com> wrote:
>> Check if the cleaner shader should run directly.
>>
>> While at it remove amdgpu_vm_need_pipeline_sync(), we also check again
>> if the VMID has seen a GPU reset since last use and the gds switch
>> setiing can be handled more simply as well.
>>
>> Also remove some duplicate checks and re-order and document the code.
>>
>> v2: restructure the while function
>> v3: fix logic error pointed out by Srini
>> v4: fix typo in comment, fix crash caused by incorrect check
>> v5: once more fix the logic
>> v6: separate cleaner shader checks as suggested by Srini
>> v7: re-order incorrect check
>> v8: separate the revert
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> Reviewed-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c | 6 +-
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 106 ++++++++++---------------
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 5 +-
>> 3 files changed, 46 insertions(+), 71 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>> index 5eab1c1a380c..30b58772598c 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>> @@ -189,10 +189,8 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned int num_ibs,
>> }
>>
>> need_ctx_switch = ring->current_ctx != fence_ctx;
>> - if (ring->funcs->emit_pipeline_sync && job &&
>> - ((tmp = amdgpu_sync_get_fence(&job->explicit_sync)) ||
>> - (amdgpu_sriov_vf(adev) && need_ctx_switch) ||
>> - amdgpu_vm_need_pipeline_sync(ring, job))) {
>> + if ((job && (tmp = amdgpu_sync_get_fence(&job->explicit_sync))) ||
>> + (amdgpu_sriov_vf(adev) && need_ctx_switch)) {
>> need_pipe_sync = true;
>>
>> if (tmp)
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 0a80c011e678..31c423663b54 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -707,37 +707,6 @@ void amdgpu_vm_check_compute_bug(struct amdgpu_device *adev)
>> }
>> }
>>
>> -/**
>> - * amdgpu_vm_need_pipeline_sync - Check if pipe sync is needed for job.
>> - *
>> - * @ring: ring on which the job will be submitted
>> - * @job: job to submit
>> - *
>> - * Returns:
>> - * True if sync is needed.
>> - */
>> -bool amdgpu_vm_need_pipeline_sync(struct amdgpu_ring *ring,
>> - struct amdgpu_job *job)
>> -{
>> - struct amdgpu_device *adev = ring->adev;
>> - unsigned vmhub = ring->vm_hub;
>> - struct amdgpu_vmid_mgr *id_mgr = &adev->vm_manager.id_mgr[vmhub];
>> -
>> - if (job->vmid == 0)
>> - return false;
>> -
>> - if (job->vm_needs_flush || ring->has_compute_vm_bug)
>> - return true;
>> -
>> - if (ring->funcs->emit_gds_switch && job->gds_switch_needed)
>> - return true;
>> -
>> - if (amdgpu_vmid_had_gpu_reset(adev, &id_mgr->ids[job->vmid]))
>> - return true;
>> -
>> - return false;
>> -}
>> -
>> /**
>> * amdgpu_vm_flush - hardware flush the vm
>> *
>> @@ -758,44 +727,52 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
>> unsigned vmhub = ring->vm_hub;
>> struct amdgpu_vmid_mgr *id_mgr = &adev->vm_manager.id_mgr[vmhub];
>> struct amdgpu_vmid *id = &id_mgr->ids[job->vmid];
>> - bool spm_update_needed = job->spm_update_needed;
>> - bool gds_switch_needed = ring->funcs->emit_gds_switch &&
>> - job->gds_switch_needed;
>> - bool vm_flush_needed = job->vm_needs_flush;
>> - bool cleaner_shader_needed = false;
>> - bool pasid_mapping_needed = false;
>> - struct dma_fence *fence = NULL;
>> + bool gds_switch_needed, vm_flush_needed, spm_update_needed,
>> + cleaner_shader_needed, pasid_mapping_needed;
> Would be good to document what all of these flags are used for. E.g
>
> /* need_pipe_sync - if set, we wait for the last fence on the ring to
> signal before executing more commands
> * cleaner_shader_needed - if set we emit the cleaner shader to clean
> up GPRs and LDS before a new command is submitted
> * etc.
> */
>
>> + struct dma_fence *fence;
>> unsigned int patch;
>> int r;
>>
>> + /* First of all figure out what needs to be done */
>> if (amdgpu_vmid_had_gpu_reset(adev, id)) {
> Please add a comment here to explain why we set all of these to true
> if we had a GPU reset.
>
>> + need_pipe_sync = true;
>> gds_switch_needed = true;
>> vm_flush_needed = true;
>> pasid_mapping_needed = true;
>> spm_update_needed = true;
>> + cleaner_shader_needed = true;
Hi Christian,
may I please have your thoughts onto this. ie., "cleaner_shader_needed =
true;" wrt GPU reset
ie., why do we expect "cleaner_shader_needed" to be emitted during "GPU
reset" process?,
because even in normal bootup case, currently we go and enable
process_isolation/trigger cleaner shader manually, only as and when
needed.
Best regards,
Srini
>> + } else {
> Would be good to document all of these cases as well.
>
>> + gds_switch_needed = job->gds_switch_needed;
>> + vm_flush_needed = job->vm_needs_flush;
>> + mutex_lock(&id_mgr->lock);
>> + pasid_mapping_needed = id->pasid != job->pasid ||
>> + !id->pasid_mapping ||
>> + !dma_fence_is_signaled(id->pasid_mapping);
>> + mutex_unlock(&id_mgr->lock);
>> + spm_update_needed = job->spm_update_needed;
> E.g.:
> /* The spearhead marks the first submission from a new client. We
> need to run the cleaner shader
> * if it is requested by the job and we have a new spearhead so that
> we clean up before it runs.
> */
>
>> + cleaner_shader_needed = job->run_cleaner_shader &&
>> + job->base.s_fence && &job->base.s_fence->scheduled ==
>> + isolation->spearhead;
> E.g.,
> /* This will cause the queue to wait for the current fence on the ring
> to signal before new work executes (wait for idle).
> * This is needed as a workaround for some hardware
> (ring->has_compute_vm_bug), if we are updating
> * the vmid or page tables (vm_flush_needed), if we need to emit the
> cleaner shader (which must execute while the
> * queue is idle), or if the job uses gds and we need to update the
> gds mappings (gds_switch_needed).
> */
>
>> + need_pipe_sync |= ring->has_compute_vm_bug || vm_flush_needed ||
>> + cleaner_shader_needed || gds_switch_needed;
>> }
>>
>> - mutex_lock(&id_mgr->lock);
>> - if (id->pasid != job->pasid || !id->pasid_mapping ||
>> - !dma_fence_is_signaled(id->pasid_mapping))
>> - pasid_mapping_needed = true;
>> - mutex_unlock(&id_mgr->lock);
>> -
>> + /* Then check the pre-requisites */
>> + need_pipe_sync &= !!ring->funcs->emit_pipeline_sync;
>> gds_switch_needed &= !!ring->funcs->emit_gds_switch;
>> vm_flush_needed &= !!ring->funcs->emit_vm_flush &&
>> job->vm_pd_addr != AMDGPU_BO_INVALID_OFFSET;
>> pasid_mapping_needed &= adev->gmc.gmc_funcs->emit_pasid_mapping &&
>> ring->funcs->emit_wreg;
>> -
>> - cleaner_shader_needed = job->run_cleaner_shader &&
>> - adev->gfx.enable_cleaner_shader &&
>> - ring->funcs->emit_cleaner_shader && job->base.s_fence &&
>> - &job->base.s_fence->scheduled == isolation->spearhead;
>> + spm_update_needed &= !!adev->gfx.rlc.funcs->update_spm_vmid;
>> + cleaner_shader_needed &= adev->gfx.enable_cleaner_shader &&
>> + ring->funcs->emit_cleaner_shader;
>>
>> if (!vm_flush_needed && !gds_switch_needed && !need_pipe_sync &&
>> - !cleaner_shader_needed)
>> + !cleaner_shader_needed && !spm_update_needed)
>> return 0;
>>
>> + /* Then actually prepare the submission frame */
>> amdgpu_ring_ib_begin(ring);
>> if (ring->funcs->init_cond_exec)
>> patch = amdgpu_ring_init_cond_exec(ring,
>> @@ -815,23 +792,34 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
>> if (pasid_mapping_needed)
>> amdgpu_gmc_emit_pasid_mapping(ring, job->vmid, job->pasid);
>>
>> - if (spm_update_needed && adev->gfx.rlc.funcs->update_spm_vmid)
>> + if (spm_update_needed)
>> adev->gfx.rlc.funcs->update_spm_vmid(adev, ring, job->vmid);
>>
>> - if (ring->funcs->emit_gds_switch &&
>> - gds_switch_needed) {
>> + if (gds_switch_needed)
>> amdgpu_ring_emit_gds_switch(ring, job->vmid, job->gds_base,
>> job->gds_size, job->gws_base,
>> job->gws_size, job->oa_base,
>> job->oa_size);
>> - }
>>
>> if (vm_flush_needed || pasid_mapping_needed || cleaner_shader_needed) {
>> r = amdgpu_fence_emit(ring, &fence, NULL, 0);
>> if (r)
>> return r;
>> + } else {
>> + fence = NULL;
>> + }
>> +
>> + amdgpu_ring_patch_cond_exec(ring, patch);
>> +
>> + /* the double SWITCH_BUFFER here *cannot* be skipped by COND_EXEC */
>> + if (ring->funcs->emit_switch_buffer) {
>> + amdgpu_ring_emit_switch_buffer(ring);
>> + amdgpu_ring_emit_switch_buffer(ring);
>> }
>>
>> + amdgpu_ring_ib_end(ring);
>> +
>> + /* And finally remember what the ring has executed */
>> if (vm_flush_needed) {
>> mutex_lock(&id_mgr->lock);
>> dma_fence_put(id->last_flush);
>> @@ -861,16 +849,6 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
>> mutex_unlock(&adev->enforce_isolation_mutex);
>> }
>> dma_fence_put(fence);
>> -
>> - amdgpu_ring_patch_cond_exec(ring, patch);
>> -
>> - /* the double SWITCH_BUFFER here *cannot* be skipped by COND_EXEC */
>> - if (ring->funcs->emit_switch_buffer) {
>> - amdgpu_ring_emit_switch_buffer(ring);
>> - amdgpu_ring_emit_switch_buffer(ring);
>> - }
>> -
>> - amdgpu_ring_ib_end(ring);
>> return 0;
>> }
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> index f3ad687125ad..c9578b7f670c 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> @@ -498,7 +498,8 @@ int amdgpu_vm_validate(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>> struct ww_acquire_ctx *ticket,
>> int (*callback)(void *p, struct amdgpu_bo *bo),
>> void *param);
>> -int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job, bool need_pipe_sync);
>> +int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
>> + bool need_pipe_sync);
>> int amdgpu_vm_update_pdes(struct amdgpu_device *adev,
>> struct amdgpu_vm *vm, bool immediate);
>> int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
>> @@ -559,8 +560,6 @@ void amdgpu_vm_adjust_size(struct amdgpu_device *adev, uint32_t min_vm_size,
>> uint32_t fragment_size_default, unsigned max_level,
>> unsigned max_bits);
>> int amdgpu_vm_ioctl(struct drm_device *dev, void *data, struct drm_file *filp);
>> -bool amdgpu_vm_need_pipeline_sync(struct amdgpu_ring *ring,
>> - struct amdgpu_job *job);
>> void amdgpu_vm_check_compute_bug(struct amdgpu_device *adev);
>>
>> struct amdgpu_task_info *
>> --
>> 2.34.1
>>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-05-15 7:41 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-08 12:05 [PATCH 1/2] drm/amdgpu: revert "always sync the GFX pipe on ctx switch" Christian König
2025-05-08 12:05 ` [PATCH 2/2] drm/amdgpu: cleanup amdgpu_vm_flush v7 Christian König
2025-05-08 18:33 ` Alex Deucher
2025-05-15 7:41 ` SRINIVASAN SHANMUGAM
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.