Re: [PATCH v2] drm/amdgpu: Flush the pasid on all XCCs in parallel

["Kuehling, Felix" <felix.kuehling@amd.com>]

On 2026-09-28 14:52, David Yat Sin wrote:
> amdgpu_vm_flush_compute_tlb() walks the XCCs of the compute partition one
> at a time, and amdgpu_gmc_flush_gpu_tlb_pasid() submits the invalidation
> to one XCC's KIQ and then busy-waits for its fence before the caller can
> move on to the next. On a partition that owns every XCC of the device
> that is num_xcc round trips back to back.
>
> The XCCs do not depend on each other here. Each has its own KIQ ring,
> ring lock and fence sequence, the page tables are already updated before
> any invalidation is issued, and no invalidation needs another XCC to have
> finished first.
>
> Split the submit out of amdgpu_gmc_flush_gpu_tlb_pasid() and add
> amdgpu_gmc_flush_gpu_tlb_pasid_xccs(), which queues the invalidation on
> every XCC in the mask before collecting any of the fences, so the round
> trips overlap. amdgpu_gmc_flush_gpu_tlb_pasid() becomes a one bit mask
> call into it, so both entry points share the reset-domain handling and
> the KIQ dispatch.
>
> Note that amdgpu_vm_flush_compute_tlb() no longer stops at the first XCC
> that fails. Every XCC in the mask is flushed regardless and the first
> error is returned, which is a deliberate behaviour change for callers:
> one XCC failing to queue its invalidation no longer leaves the remaining
> XCCs unflushed.
>
> Measured on gfx950 with all 8 XCCs in one SPX partition, revoking host
> memory access for a pasid mapped on every XCC. Traced with ftrace,
> amdgpu_vm_flush_compute_tlb() drops from 141 us to 65 us, against 23 us
> for the slowest individual XCC flush. The KFD SVM ioctl carrying that
> flush drops from 0.14 ms to 0.05 ms at p50.
>
> Assisted-by: Cursor:claude-opus-5
> Signed-off-by: David Yat Sin <David.YatSin@amd.com>

Reviewed-by: Felix Kuehling <felix.kuehling@amd.com>


> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c | 233 +++++++++++++++++-------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h |   8 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  |  10 +-
>   3 files changed, 173 insertions(+), 78 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> index 1bf2a42fa63c..167f1344f8f4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> @@ -782,98 +782,197 @@ void amdgpu_gmc_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,
>   	dev_err(adev->dev, "Error flushing GPU TLB using the SDMA (%d)!\n", r);
>   }
>   
> -int amdgpu_gmc_flush_gpu_tlb_pasid(struct amdgpu_device *adev, uint16_t pasid,
> -				   uint32_t flush_type, bool all_hub,
> -				   uint32_t inst)
> +static void amdgpu_gmc_flush_pasid_regs(struct amdgpu_device *adev, u16 pasid,
> +					u32 flush_type, bool all_hub,
> +					u32 inst)
> +{
> +	if (!adev->gmc.gmc_funcs->flush_gpu_tlb_pasid)
> +		return;
> +
> +	if (adev->gmc.flush_tlb_needs_extra_type_2)
> +		adev->gmc.gmc_funcs->flush_gpu_tlb_pasid(adev, pasid, 2, all_hub,
> +							 inst);
> +
> +	if (adev->gmc.flush_tlb_needs_extra_type_0 && flush_type == 2)
> +		adev->gmc.gmc_funcs->flush_gpu_tlb_pasid(adev, pasid, 0, all_hub,
> +							 inst);
> +
> +	adev->gmc.gmc_funcs->flush_gpu_tlb_pasid(adev, pasid, flush_type, all_hub,
> +						 inst);
> +}
> +
> +/*
> + * Queue the invalidation on one XCC and return its fence without waiting, so
> + * that a caller flushing several XCCs can have them in flight together.  The
> + * KIQ ring, its lock and its fence sequence are per XCC, so the submissions do
> + * not interfere with each other.
> + */
> +static int amdgpu_gmc_flush_pasid_kiq_submit(struct amdgpu_device *adev,
> +					     u16 pasid, u32 flush_type,
> +					     bool all_hub, u32 inst,
> +					     u32 *seq)
>   {
> -	struct amdgpu_ring *ring = &adev->gfx.kiq[inst].ring;
>   	struct amdgpu_kiq *kiq = &adev->gfx.kiq[inst];
> +	struct amdgpu_ring *ring = &kiq->ring;
>   	unsigned int ndw;
> -	int r, cnt = 0;
> -	uint32_t seq;
> +	int r;
>   
> -	/*
> -	 * A GPU reset should flush all TLBs anyway, so no need to do
> -	 * this while one is ongoing.
> -	 */
> -	if (!down_read_trylock(&adev->reset_domain->sem))
> -		return 0;
> +	/* one flush + 8 dwords fence */
> +	ndw = kiq->pmf->invalidate_tlbs_size + 8;
>   
> -	if (!adev->gmc.flush_pasid_uses_kiq || !ring->sched.ready) {
> +	if (adev->gmc.flush_tlb_needs_extra_type_2)
> +		ndw += kiq->pmf->invalidate_tlbs_size;
>   
> -		if (!adev->gmc.gmc_funcs->flush_gpu_tlb_pasid) {
> -			r = 0;
> -			goto error_unlock_reset;
> -		}
> +	if (adev->gmc.flush_tlb_needs_extra_type_0 && flush_type == 2)
> +		ndw += kiq->pmf->invalidate_tlbs_size;
>   
> -		if (adev->gmc.flush_tlb_needs_extra_type_2)
> -			adev->gmc.gmc_funcs->flush_gpu_tlb_pasid(adev, pasid,
> -								 2, all_hub,
> -								 inst);
> +	spin_lock(&kiq->ring_lock);
> +	r = amdgpu_ring_alloc(ring, ndw);
> +	if (r) {
> +		spin_unlock(&kiq->ring_lock);
> +		return r;
> +	}
>   
> -		if (adev->gmc.flush_tlb_needs_extra_type_0 && flush_type == 2)
> -			adev->gmc.gmc_funcs->flush_gpu_tlb_pasid(adev, pasid,
> -								 0, all_hub,
> -								 inst);
> +	if (adev->gmc.flush_tlb_needs_extra_type_2)
> +		kiq->pmf->kiq_invalidate_tlbs(ring, pasid, 2, all_hub);
>   
> -		adev->gmc.gmc_funcs->flush_gpu_tlb_pasid(adev, pasid,
> -							 flush_type, all_hub,
> -							 inst);
> -		r = 0;
> -	} else {
> -		/* 2 dwords flush + 8 dwords fence */
> -		ndw = kiq->pmf->invalidate_tlbs_size + 8;
> +	if (flush_type == 2 && adev->gmc.flush_tlb_needs_extra_type_0)
> +		kiq->pmf->kiq_invalidate_tlbs(ring, pasid, 0, all_hub);
>   
> -		if (adev->gmc.flush_tlb_needs_extra_type_2)
> -			ndw += kiq->pmf->invalidate_tlbs_size;
> -
> -		if (adev->gmc.flush_tlb_needs_extra_type_0)
> -			ndw += kiq->pmf->invalidate_tlbs_size;
> +	kiq->pmf->kiq_invalidate_tlbs(ring, pasid, flush_type, all_hub);
> +	r = amdgpu_fence_emit_polling(ring, seq, MAX_KIQ_REG_WAIT);
> +	if (r) {
> +		amdgpu_ring_undo(ring);
> +		spin_unlock(&kiq->ring_lock);
> +		return r;
> +	}
>   
> -		spin_lock(&adev->gfx.kiq[inst].ring_lock);
> -		r = amdgpu_ring_alloc(ring, ndw);
> -		if (r) {
> -			spin_unlock(&adev->gfx.kiq[inst].ring_lock);
> -			goto error_unlock_reset;
> -		}
> -		if (adev->gmc.flush_tlb_needs_extra_type_2)
> -			kiq->pmf->kiq_invalidate_tlbs(ring, pasid, 2, all_hub);
> +	amdgpu_ring_commit(ring);
> +	spin_unlock(&kiq->ring_lock);
>   
> -		if (flush_type == 2 && adev->gmc.flush_tlb_needs_extra_type_0)
> -			kiq->pmf->kiq_invalidate_tlbs(ring, pasid, 0, all_hub);
> +	return 0;
> +}
>   
> -		kiq->pmf->kiq_invalidate_tlbs(ring, pasid, flush_type, all_hub);
> -		r = amdgpu_fence_emit_polling(ring, &seq, MAX_KIQ_REG_WAIT);
> -		if (r) {
> -			amdgpu_ring_undo(ring);
> -			spin_unlock(&adev->gfx.kiq[inst].ring_lock);
> -			goto error_unlock_reset;
> -		}
> +/*
> + * Wait for an invalidation queued by amdgpu_gmc_flush_pasid_kiq_submit().
> + *
> + * Bailing out because a reset became pending is reported as success: the reset
> + * flushes all TLBs anyway, so the invalidation no longer has to complete.  Only
> + * running out of tries is an error.
> + */
> +static int amdgpu_gmc_flush_pasid_kiq_wait(struct amdgpu_device *adev,
> +					   u32 inst, u32 seq)
> +{
> +	struct amdgpu_ring *ring = &adev->gfx.kiq[inst].ring;
> +	int cnt = 0;
> +	signed long r;
>   
> -		amdgpu_ring_commit(ring);
> -		spin_unlock(&adev->gfx.kiq[inst].ring_lock);
> +	r = amdgpu_fence_wait_polling(ring, seq, MAX_KIQ_REG_WAIT);
>   
> +	might_sleep();
> +	while (r < 1 && cnt++ < MAX_KIQ_REG_TRY &&
> +	       !amdgpu_reset_pending(adev->reset_domain)) {
> +		msleep(MAX_KIQ_REG_BAILOUT_INTERVAL);
>   		r = amdgpu_fence_wait_polling(ring, seq, MAX_KIQ_REG_WAIT);
> +	}
> +
> +	if (cnt > MAX_KIQ_REG_TRY) {
> +		dev_err(adev->dev, "timeout waiting for kiq fence\n");
> +		return -ETIME;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * amdgpu_gmc_flush_gpu_tlb_pasid_xccs - flush a pasid on several XCCs at once
> + *
> + * @adev: amdgpu_device pointer
> + * @pasid: pasid to be flushed
> + * @flush_type: the flush type
> + * @all_hub: flush all hubs
> + * @xcc_mask: mask of the XCCs to flush
> + *
> + * Submits the invalidation to every XCC in @xcc_mask before waiting for any of
> + * them, so the round trips overlap instead of running back to back.  Flushing
> + * one XCC at a time costs num_xcc times the latency of a single one, which on
> + * a partition holding every XCC of the device is most of the cost of a compute
> + * TLB flush.
> + *
> + * The XCCs are independent of each other here: the page tables are already
> + * updated before any invalidation is issued, and nothing in an invalidation
> + * depends on another XCC having completed its own.
> + *
> + * Returns:
> + * 0 for success, the first error otherwise.  Every XCC is flushed even if one
> + * of them fails.
> + */
> +int amdgpu_gmc_flush_gpu_tlb_pasid_xccs(struct amdgpu_device *adev, u16 pasid,
> +					u32 flush_type, bool all_hub,
> +					u32 xcc_mask)
> +{
> +	u32 seq[AMDGPU_MAX_GC_INSTANCES];
> +	unsigned long pending = 0;
> +	int xcc, r = 0, err;
>   
> -		might_sleep();
> -		while (r < 1 && cnt++ < MAX_KIQ_REG_TRY &&
> -		       !amdgpu_reset_pending(adev->reset_domain)) {
> -			msleep(MAX_KIQ_REG_BAILOUT_INTERVAL);
> -			r = amdgpu_fence_wait_polling(ring, seq, MAX_KIQ_REG_WAIT);
> +	/*
> +	 * A GPU reset should flush all TLBs anyway, so no need to do
> +	 * this while one is ongoing.
> +	 *
> +	 * Unlike the one XCC at a time flush this holds the read side across
> +	 * every submit and every wait, so a reset's down_write() can only get
> +	 * in once the whole mask is done.  The waits are sequential, which
> +	 * bounds that at num_xcc * MAX_KIQ_REG_TRY * MAX_KIQ_REG_BAILOUT_INTERVAL
> +	 * in the worst case, and amdgpu_gmc_flush_pasid_kiq_wait() cuts each
> +	 * wait short as soon as a reset becomes pending.
> +	 */
> +	if (!down_read_trylock(&adev->reset_domain->sem))
> +		return 0;
> +
> +	for_each_inst(xcc, xcc_mask) {
> +		struct amdgpu_ring *ring;
> +
> +		if (WARN_ON_ONCE(xcc >= AMDGPU_MAX_GC_INSTANCES)) {
> +			if (!r)
> +				r = -EINVAL;
> +			break;
>   		}
>   
> -		if (cnt > MAX_KIQ_REG_TRY) {
> -			dev_err(adev->dev, "timeout waiting for kiq fence\n");
> -			r = -ETIME;
> -		} else
> -			r = 0;
> +		ring = &adev->gfx.kiq[xcc].ring;
> +		if (!adev->gmc.flush_pasid_uses_kiq || !ring->sched.ready) {
> +			amdgpu_gmc_flush_pasid_regs(adev, pasid, flush_type,
> +						    all_hub, xcc);
> +			continue;
> +		}
> +
> +		err = amdgpu_gmc_flush_pasid_kiq_submit(adev, pasid, flush_type,
> +							all_hub, xcc, &seq[xcc]);
> +		if (err) {
> +			if (!r)
> +				r = err;
> +			continue;
> +		}
> +
> +		pending |= BIT(xcc);
> +	}
> +
> +	for_each_set_bit(xcc, &pending, AMDGPU_MAX_GC_INSTANCES) {
> +		err = amdgpu_gmc_flush_pasid_kiq_wait(adev, xcc, seq[xcc]);
> +		if (err && !r)
> +			r = err;
>   	}
>   
> -error_unlock_reset:
>   	up_read(&adev->reset_domain->sem);
>   	return r;
>   }
>   
> +int amdgpu_gmc_flush_gpu_tlb_pasid(struct amdgpu_device *adev, u16 pasid,
> +				   u32 flush_type, bool all_hub, u32 inst)
> +{
> +	return amdgpu_gmc_flush_gpu_tlb_pasid_xccs(adev, pasid, flush_type,
> +						   all_hub, BIT(inst));
> +}
> +
>   void amdgpu_gmc_fw_reg_write_reg_wait(struct amdgpu_device *adev,
>   				      uint32_t reg0, uint32_t reg1,
>   				      uint32_t ref, uint32_t mask,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> index cffd5ca3fc66..76e6f1f96913 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> @@ -444,9 +444,11 @@ int amdgpu_gmc_ras_sw_init(struct amdgpu_device *adev);
>   int amdgpu_gmc_allocate_vm_inv_eng(struct amdgpu_device *adev);
>   void amdgpu_gmc_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,
>   			      uint32_t vmhub, uint32_t flush_type);
> -int amdgpu_gmc_flush_gpu_tlb_pasid(struct amdgpu_device *adev, uint16_t pasid,
> -				   uint32_t flush_type, bool all_hub,
> -				   uint32_t inst);
> +int amdgpu_gmc_flush_gpu_tlb_pasid(struct amdgpu_device *adev, u16 pasid,
> +				   u32 flush_type, bool all_hub, u32 inst);
> +int amdgpu_gmc_flush_gpu_tlb_pasid_xccs(struct amdgpu_device *adev, u16 pasid,
> +					u32 flush_type, bool all_hub,
> +					u32 xcc_mask);
>   void amdgpu_gmc_fw_reg_write_reg_wait(struct amdgpu_device *adev,
>   				      uint32_t reg0, uint32_t reg1,
>   				      uint32_t ref, uint32_t mask,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 29a66e39f3d6..25a74d497255 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1723,7 +1723,6 @@ int amdgpu_vm_flush_compute_tlb(struct amdgpu_device *adev,
>   {
>   	uint64_t tlb_seq = amdgpu_vm_tlb_seq(vm);
>   	bool all_hub = false;
> -	int xcc = 0, r = 0;
>   
>   	WARN_ON_ONCE(!vm->is_compute_context);
>   
> @@ -1739,13 +1738,8 @@ int amdgpu_vm_flush_compute_tlb(struct amdgpu_device *adev,
>   	    adev->family == AMDGPU_FAMILY_RV)
>   		all_hub = true;
>   
> -	for_each_inst(xcc, xcc_mask) {
> -		r = amdgpu_gmc_flush_gpu_tlb_pasid(adev, vm->pasid, flush_type,
> -						   all_hub, xcc);
> -		if (r)
> -			break;
> -	}
> -	return r;
> +	return amdgpu_gmc_flush_gpu_tlb_pasid_xccs(adev, vm->pasid, flush_type,
> +						   all_hub, xcc_mask);
>   }
>   
>   /**