Re: [PATCH 2/7] drm/amdgpu: ACK the retry CAM after VM update finishes

["Timur Kristóf" <timur.kristof@gmail.com>]

On 2026. június 24., szerda 17:14:59 közép-európai nyári idő Tvrtko Ursulin 
wrote:
> On 24/06/2026 15:52, Timur Kristóf wrote:
> > On 2026. június 24., szerda 16:31:20 közép-európai nyári idő Tvrtko
> > Ursulin
> > 
> > wrote:
> >> On 29/05/2026 11:30, Timur Kristóf wrote:
> >>> Add a fence callback to the VM update and ACK the retry CAM
> >>> after the VM update is finished. Previously, we would ACK it
> >>> immediately after calling amdgpu_vm_handle_fault() which
> >>> caused a race condition that was likely to trigger the same
> >>> interrupt again, causing the same fault to be handled
> >>> multiple times.
> >>> 
> >>> Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> >>> ---
> >>> 
> >>>    drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c     | 28
> >>>    +++++++++++++++++++--
> >>>    drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h     |  8 ++++++
> >>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c      |  2 +-
> >>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c |  2 +-
> >>>    4 files changed, 36 insertions(+), 4 deletions(-)
> >>> 
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c index
> >>> 26aea960e2759..21c8d87477448 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> >>> @@ -545,6 +545,16 @@ void amdgpu_gmc_filter_faults_remove(struct
> >>> amdgpu_device *adev, uint64_t addr,>
> >>> 
> >>>    	} while (fault->timestamp < tmp);
> >>>    
> >>>    }
> >>> 
> >>> +static void amdgpu_gmc_retry_fault_handled(struct dma_fence *fence,
> >>> +					   struct dma_fence_cb
> > 
> > *cb)
> > 
> >>> +{
> >>> +	struct amdgpu_fence_cb *afc = container_of(cb, struct
> > 
> > amdgpu_fence_cb,
> > 
> >>> cb); +	struct amdgpu_device *adev = afc->adev;
> >>> +
> >>> +	/* CAM index is the array index of the current callback struct */
> >>> +	adev->irq.ih_funcs->retry_cam_ack(adev, afc - &adev-
> >> 
> >> gmc.retry_cb[0]);
> >> 
> >> Is the "afc - &adev->gmc.retry_cb[0]" part correct? It will be the index
> >> of the array element, while ->retry_cam_ack() expects the content of
> >> that element, no?
> > 
> > Like the comment says, the CAM index is the array index.
> > We just need the CAM index in order to tell the CAM to ACK the current
> > entry. The contents of the array are just there to make
> > dma_fence_add_callback() work with this callback function.
> 
> Ah you are right, I got confused. But it is also a bit bad, and I mean
> not just the array sizing dilema from lower in the email. But since the
> cam_index comes from the hardware and then below we blindly do:
> 
> 	if (dma_fence_add_callback(fence, &adev-
>gmc.retry_cb[cam_index].cb,
> amdgpu_gmc_retry_fault_handled))
> 
> Should hardware manage to send two faults with the same cam_index when
> the previous one hasn't been handled

The retry CAM exists to filter page fault interrupts and prevent sending 
multiple interrupts for the same fault. It won't send and interrupt with the 
same cam_index until we ACK the previous one.

> that is the very same callback is
> already installed and unsignaled (expect the unexpected), we have just
> upgraded the hardware bug to a kernel crash.
> 
> If I now understand it right, you want to "remember" the cam_index
> received so callback knows what to handle. Hmm.. Allocating memory does
> seem allowed if I follow correctly that amdgpu_vm_handle_fault() is
> calling dma_resv_reserve_fences(). So unless I am missing something
> perhaps kmalloc of struct amdgpu_fence_cb would be fine after all?

It may be fine, but I'd very much prefer to avoid it if possible.

> And
> if so you should also probably rename it to a less generic name along
> the lines of amgpud_retry_fault_cb or so. Workable?

I'm OK to rename it for sure.

Timur

> 
> >>> +}
> >>> +
> >>> 
> >>>    int amdgpu_gmc_handle_retry_fault(struct amdgpu_device *adev,
> >>>    
> >>>    				  struct amdgpu_iv_entry *entry,
> >>>    				  u64 addr,
> >>> 
> >>> @@ -552,6 +562,7 @@ int amdgpu_gmc_handle_retry_fault(struct
> >>> amdgpu_device
> >>> *adev,>
> >>> 
> >>>    				  u32 node_id,
> >>>    				  bool write_fault)
> >>>    
> >>>    {
> >>> 
> >>> +	struct dma_fence *fence = NULL;
> >>> 
> >>>    	int ret;
> >>>    	
> >>>    	if (adev->irq.retry_cam_enabled) {
> >>> 
> >>> @@ -564,8 +575,21 @@ int amdgpu_gmc_handle_retry_fault(struct
> >>> amdgpu_device *adev,>
> >>> 
> >>>    		}
> >>>    		
> >>>    		ret = amdgpu_vm_handle_fault(adev, entry->pasid,
> > 
> > entry->vmid, node_id,
> > 
> >>> -					     addr, entry-
> >> 
> >> timestamp, write_fault, NULL);
> >> 
> >>> -		adev->irq.ih_funcs->retry_cam_ack(adev, cam_index);
> >>> +					     addr, entry-
> >> 
> >> timestamp, write_fault, &fence);
> >> 
> >>> +
> >>> +		/* If the update is already done, ACK now, otherwise
> > 
> > when it's done. */
> > 
> >>> +		if (fence) {
> >>> +			adev->gmc.retry_cb[cam_index].adev = adev;
> >> 
> >> Why is 16 retry_cb elements enough? I see in the code cam_index extraced
> >> from the IV entry with a mask such as 0x3ff.
> > 
> > I think this came up in a conversation after I had already submitted the
> > patch. The maximum amount of CAM entries are specified by the
> > IH_RETRY_INT_CAM_CNTL.CAM_SIZE field.  The content of the field will need
> > to be interpreted as something like this:
> > ((CAM_SIZE + 1) * 64) = (15 + 1) * 64 = 1024
> > 
> > It is a good question whether we actually want to statically allocate that
> > many items. We should very much avoid doing dynamic allocation in the page
> > fault handler. I'm open to suggestions on how to move forward with this.
> > 
> >>> +
> >>> +			if (dma_fence_add_callback(fence, &adev-
> >> 
> >> gmc.retry_cb[cam_index].cb,
> >> 
> >>> +
> > 
> > amdgpu_gmc_retry_fault_handled))
> > 
> >>> +				adev->irq.ih_funcs-
> >> 
> >> retry_cam_ack(adev, cam_index);
> >> 
> >>> +
> >>> +			dma_fence_put(fence);
> >>> +		} else {
> >>> +			adev->irq.ih_funcs->retry_cam_ack(adev,
> > 
> > cam_index);
> > 
> >>> +		}
> >>> +
> >>> 
> >>>    		if (ret)
> >>>    		
> >>>    			return 1;
> >>>    	
> >>>    	} else {
> >>> 
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h index
> >>> 77eb153802845..3bfb06e011a86 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> >>> @@ -27,6 +27,7 @@
> >>> 
> >>>    #define __AMDGPU_GMC_H__
> >>>    
> >>>    #include <linux/types.h>
> >>> 
> >>> +#include <linux/dma-fence.h>
> >>> 
> >>>    #include "amdgpu_irq.h"
> >>>    #include "amdgpu_xgmi.h"
> >>> 
> >>> @@ -214,6 +215,11 @@ struct amdgpu_gmc_memrange {
> >>> 
> >>>    	int nid_mask;
> >>>    
> >>>    };
> >>> 
> >>> +struct amdgpu_fence_cb {
> >>> +	struct amdgpu_device *adev;
> >>> +	struct dma_fence_cb cb;
> >>> +};
> >>> +
> >>> 
> >>>    enum amdgpu_gart_placement {
> >>>    
> >>>    	AMDGPU_GART_PLACEMENT_BEST_FIT = 0,
> >>>    	AMDGPU_GART_PLACEMENT_HIGH,
> >>> 
> >>> @@ -305,6 +311,8 @@ struct amdgpu_gmc {
> >>> 
> >>>    	} fault_hash[AMDGPU_GMC_FAULT_HASH_SIZE];
> >>>    	uint64_t		last_fault:AMDGPU_GMC_FAULT_RING_ORDER;
> >>> 
> >>> +	struct amdgpu_fence_cb retry_cb[16];
> >>> +
> >>> 
> >>>    	bool tmz_enabled;
> >>>    	bool is_app_apu;
> >>> 
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index
> >>> 8c3ba7213eb22..f5e9b97e92a8c 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> >>> @@ -3035,7 +3035,7 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device
> >>> *adev, u32 pasid,>
> >>> 
> >>>    	r = amdgpu_vm_update_pdes(adev, vm, true);
> >>> 
> >>> -	*fence = vm->last_update;
> >>> +	*fence = dma_fence_get(vm->last_update);
> >> 
> >> Ah! But passing over since you said you are dropping that patch anyway.
> > 
> > That line should have gone to the previous patch and was added to this one
> > by mistake.
> > 
> >>>    error_unlock:
> >>>    	amdgpu_bo_unreserve(root);
> >>> 
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c index
> >>> 2eb64df6daa94..6e28f0e435bf5 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> >>> @@ -132,7 +132,7 @@ static int amdgpu_vm_sdma_commit(struct
> >>> amdgpu_vm_update_params *p,>
> >>> 
> >>>    				   DMA_RESV_USAGE_BOOKKEEP);
> >>>    	
> >>>    	}
> >>> 
> >>> -	if (fence && !p->immediate) {
> >>> +	if (fence) {
> >> 
> >> Is this deliberate and if so what it is about? Commit message should
> >> explain it as well.
> > 
> > The reason it is changed is because previously it wouldn't return a fence
> > in immediate mode. This line also should have gone to the previous patch
> > and was added to this one by mistake.
> > 
> > Thanks & best regards,
> > Timur