[PATCH] drm/amdgpu: always emit the job vm fence

[PATCH] drm/amdgpu: always emit the job vm fence

[Alex Deucher]

We need the fence to reemit the gds switch or spm update
after a queue reset.

Fixes: a17ef941212b ("drm/amdgpu: rework ring reset backup and reemit v9")
Cc: timur.kristof@gmail.com
Cc: christian.koenig@amd.com
Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 2f3470208829e..7e0e2281719b1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -853,12 +853,10 @@ void amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
 					    job->oa_size);
 	}
 
-	if (vm_flush_needed || pasid_mapping_needed || cleaner_shader_needed) {
-		amdgpu_fence_emit(ring, job->hw_vm_fence, 0);
-		fence = &job->hw_vm_fence->base;
-		/* get a ref for the job */
-		dma_fence_get(fence);
-	}
+	amdgpu_fence_emit(ring, job->hw_vm_fence, 0);
+	fence = &job->hw_vm_fence->base;
+	/* get a ref for the job */
+	dma_fence_get(fence);
 
 	if (vm_flush_needed) {
 		mutex_lock(&id_mgr->lock);
-- 
2.54.0

Re: [PATCH] drm/amdgpu: always emit the job vm fence

[Timur Kristóf]

Hi Alex,

On 2026. június 3., szerda 21:45:35 közép-európai nyári idő Alex Deucher 
wrote:
> We need the fence to reemit the gds switch or spm update
> after a queue reset.
> 
> Fixes: a17ef941212b ("drm/amdgpu: rework ring reset backup and reemit v9")
> Cc: timur.kristof@gmail.com
> Cc: christian.koenig@amd.com
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>

The patch makes good sense to me. Thanks!

Reviewed-by: Timur Kristóf <timur.kristof@gmail.com>

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index 2f3470208829e..7e0e2281719b1
> 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -853,12 +853,10 @@ void amdgpu_vm_flush(struct amdgpu_ring *ring, struct
> amdgpu_job *job, job->oa_size);
>  	}
> 
> -	if (vm_flush_needed || pasid_mapping_needed || 
cleaner_shader_needed) {
> -		amdgpu_fence_emit(ring, job->hw_vm_fence, 0);
> -		fence = &job->hw_vm_fence->base;
> -		/* get a ref for the job */
> -		dma_fence_get(fence);
> -	}
> +	amdgpu_fence_emit(ring, job->hw_vm_fence, 0);
> +	fence = &job->hw_vm_fence->base;
> +	/* get a ref for the job */
> +	dma_fence_get(fence);
> 
>  	if (vm_flush_needed) {
>  		mutex_lock(&id_mgr->lock);

Re: [PATCH] drm/amdgpu: always emit the job vm fence

[Christian König]

On 6/3/26 21:45, Alex Deucher wrote:
> We need the fence to reemit the gds switch or spm update
> after a queue reset.
> 
> Fixes: a17ef941212b ("drm/amdgpu: rework ring reset backup and reemit v9")
> Cc: timur.kristof@gmail.com
> Cc: christian.koenig@amd.com
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>

That was avoided because it means another entry in the EOP ring buffer which can be bad for performance.

But correctness is obviously more important, just to keep in mind when we suddenly see 1% fps decrease and don't know where it's coming from.

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 2f3470208829e..7e0e2281719b1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -853,12 +853,10 @@ void amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
>  					    job->oa_size);
>  	}
>  
> -	if (vm_flush_needed || pasid_mapping_needed || cleaner_shader_needed) {
> -		amdgpu_fence_emit(ring, job->hw_vm_fence, 0);
> -		fence = &job->hw_vm_fence->base;
> -		/* get a ref for the job */
> -		dma_fence_get(fence);
> -	}
> +	amdgpu_fence_emit(ring, job->hw_vm_fence, 0);
> +	fence = &job->hw_vm_fence->base;
> +	/* get a ref for the job */
> +	dma_fence_get(fence);
>  
>  	if (vm_flush_needed) {
>  		mutex_lock(&id_mgr->lock);

Re: [PATCH] drm/amdgpu: always emit the job vm fence

[Timur Kristóf]

On 2026. június 5., péntek 11:24:45 közép-európai nyári idő Christian König 
wrote:
> On 6/3/26 21:45, Alex Deucher wrote:
> > We need the fence to reemit the gds switch or spm update
> > after a queue reset.
> > 
> > Fixes: a17ef941212b ("drm/amdgpu: rework ring reset backup and reemit v9")
> > Cc: timur.kristof@gmail.com
> > Cc: christian.koenig@amd.com
> > Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> 
> That was avoided because it means another entry in the EOP ring buffer which
> can be bad for performance.
> 
> But correctness is obviously more important, just to keep in mind when we
> suddenly see 1% fps decrease and don't know where it's coming from.
> 
> Reviewed-by: Christian König <christian.koenig@amd.com>

Thanks Christian, that's a valid point.
Do you think we should worry about an actual perf impact here?

If we want to avoid adding an extra fence, then an alternative solution could 
be to include the emitted commands in the ib_wptr of the job's own fence when 
a VM fence was not emitted.

What do you guys think?

Thanks,
Timur

> 
> > ---
> > 
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 10 ++++------
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index
> > 2f3470208829e..7e0e2281719b1 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > @@ -853,12 +853,10 @@ void amdgpu_vm_flush(struct amdgpu_ring *ring,
> > struct amdgpu_job *job,> 
> >  					    job->oa_size);
> >  	
> >  	}
> > 
> > -	if (vm_flush_needed || pasid_mapping_needed || 
cleaner_shader_needed) {
> > -		amdgpu_fence_emit(ring, job->hw_vm_fence, 0);
> > -		fence = &job->hw_vm_fence->base;
> > -		/* get a ref for the job */
> > -		dma_fence_get(fence);
> > -	}
> > +	amdgpu_fence_emit(ring, job->hw_vm_fence, 0);
> > +	fence = &job->hw_vm_fence->base;
> > +	/* get a ref for the job */
> > +	dma_fence_get(fence);
> > 
> >  	if (vm_flush_needed) {
> >  	
> >  		mutex_lock(&id_mgr->lock);

Re: [PATCH] drm/amdgpu: always emit the job vm fence

[Alex Deucher]

On Fri, Jun 5, 2026 at 6:11 AM Timur Kristóf <timur.kristof@gmail.com> wrote:
>
> On 2026. június 5., péntek 11:24:45 közép-európai nyári idő Christian König
> wrote:
> > On 6/3/26 21:45, Alex Deucher wrote:
> > > We need the fence to reemit the gds switch or spm update
> > > after a queue reset.
> > >
> > > Fixes: a17ef941212b ("drm/amdgpu: rework ring reset backup and reemit v9")
> > > Cc: timur.kristof@gmail.com
> > > Cc: christian.koenig@amd.com
> > > Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> >
> > That was avoided because it means another entry in the EOP ring buffer which
> > can be bad for performance.
> >
> > But correctness is obviously more important, just to keep in mind when we
> > suddenly see 1% fps decrease and don't know where it's coming from.
> >
> > Reviewed-by: Christian König <christian.koenig@amd.com>
>
> Thanks Christian, that's a valid point.
> Do you think we should worry about an actual perf impact here?
>
> If we want to avoid adding an extra fence, then an alternative solution could
> be to include the emitted commands in the ib_wptr of the job's own fence when
> a VM fence was not emitted.
>
> What do you guys think?

@Christian Koenig Is there a reason we need to emit gds and spm before
the vm fence if the vm fence is required?  I've sent out a series to
retain that behavior, but if it doesn't matter, we could simplify
things more and move gds and spm into the ib fence.

Alex

>
> Thanks,
> Timur
>
> >
> > > ---
> > >
> > >  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 10 ++++------
> > >  1 file changed, 4 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index
> > > 2f3470208829e..7e0e2281719b1 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > > @@ -853,12 +853,10 @@ void amdgpu_vm_flush(struct amdgpu_ring *ring,
> > > struct amdgpu_job *job,>
> > >                                         job->oa_size);
> > >
> > >     }
> > >
> > > -   if (vm_flush_needed || pasid_mapping_needed ||
> cleaner_shader_needed) {
> > > -           amdgpu_fence_emit(ring, job->hw_vm_fence, 0);
> > > -           fence = &job->hw_vm_fence->base;
> > > -           /* get a ref for the job */
> > > -           dma_fence_get(fence);
> > > -   }
> > > +   amdgpu_fence_emit(ring, job->hw_vm_fence, 0);
> > > +   fence = &job->hw_vm_fence->base;
> > > +   /* get a ref for the job */
> > > +   dma_fence_get(fence);
> > >
> > >     if (vm_flush_needed) {
> > >
> > >             mutex_lock(&id_mgr->lock);
>
>
>
>