Re: [PATCHv2 1/8] drm/amdgpu: Add UALink loopback infrastructure

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

On 2026-10-27 15:54, Mukul Joshi wrote:
> Add the infrastructure for UALink loopback mode, where a single GPU
> plays both the exporter and the importer role under two fixed
> accelerator IDs:
>
> - Add struct amdgpu_ualink_loopback to track the loopback state, along
>    with the fixed exporter and importer accel IDs.
>
> - Replace ualink_accel_id() with ualink_exp_accel_id() and
>    ualink_imp_accel_id(), which return the exporter and importer accel
>    ID in loopback mode.
>
> - Add amdgpu_ualink_loopback_init(), which allocates the ordered
>    workqueue used to emulate exporter->importer messages, and destroy
>    it in amdgpu_ualink_object_fini().
>
> Loopback is not enabled yet, and outside loopback both helpers return
> ppod.accel_id, so there is no functional change.
>
> Signed-off-by: Mukul Joshi <mukul.joshi@amd.com>
> ---
> v1-v2:
> - Drop the change in find_peer_adev() as its not needed (Lijo).
>
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c | 99 +++++++++++++++++-----
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.h | 29 +++++++
>   2 files changed, 109 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c
> index ca6432610f71..2cd6dc71ab9f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c
> @@ -89,8 +89,34 @@ static int amdgpu_ualink_drm_client_create(struct amdgpu_device *adev)
>   	return 0;
>   }
>   
> +/*
> + * amdgpu_ualink_loopback_init - set up UALink loopback mode
> + *
> + * Called once loopback mode has been detected. Allocates the ordered
> + * workqueue used to emulate exporter->importer interrupts in SW.
> + */
> +static int amdgpu_ualink_loopback_init(struct amdgpu_device *adev)
> +{
> +	adev->ualink.loopback.wq =
> +		alloc_ordered_workqueue("amdgpu_ualink_loopback", 0);
> +	if (!adev->ualink.loopback.wq)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
>   static void amdgpu_ualink_object_fini(struct amdgpu_device *adev)
>   {
> +	/* Tear down the loopback self-interrupt workqueue allocated in
> +	 * amdgpu_ualink_loopback_init(). destroy_workqueue() drains any
> +	 * pending self-interrupt work first.
> +	 */
> +	if (adev->ualink.loopback.wq) {
> +		destroy_workqueue(adev->ualink.loopback.wq);
> +		adev->ualink.loopback.wq = NULL;
> +		adev->ualink.loopback.enabled = false;
> +	}
> +
>   	if (!adev->ualink.info)
>   		return;
>   
> @@ -472,6 +498,14 @@ int ualink_ip_late_init(struct amdgpu_ip_block *ip_block)
>   		__amdgpu_ualink_activate_vpod_locked(adev);
>   	}
>   
> +	if (adev->ualink.loopback.enabled) {
> +		r = amdgpu_ualink_loopback_init(adev);
> +		if (r) {
> +			dev_err(adev->dev, "Failed to set up UALink loopback: %d\n", r);
> +			goto error;
> +		}
> +	}
> +
>   	r = amdgpu_ualink_drm_client_create(adev);
>   	if (r) {
>   		dev_err(adev->dev, "Failed to create UALink DRM client: %d\n",
> @@ -1028,6 +1062,31 @@ static struct amdgpu_device *find_peer_adev(unsigned int accel_id)
>   	return NULL;
>   }
>   
> +/*
> + * Physical accelerator roles.
> + *
> + * Outside loopback a GPU owns a single accelerator, so both helpers return its
> + * ppod.accel_id and the exporter/importer distinction collapses. In loopback
> + * the one physical GPU plays both roles: the exporter owns the MP nHT and
> + * receives all HW traffic, while the importer is the (virtual) HW sender for
> + * the importer -> exporter path.
> + */
> +static inline u32 ualink_exp_accel_id(struct amdgpu_device *adev)
> +{
> +	if (adev->ualink.loopback.enabled)
> +		return adev->ualink.loopback.exporter_accel_id;
> +
> +	return adev->ualink.info->ppod.accel_id;
> +}
> +
> +static inline u32 ualink_imp_accel_id(struct amdgpu_device *adev)
> +{
> +	if (adev->ualink.loopback.enabled)
> +		return adev->ualink.loopback.importer_accel_id;
> +
> +	return adev->ualink.info->ppod.accel_id;
> +}
> +
>   static bool amdgpu_ualink_is_local_accel(struct amdgpu_device *adev,
>   					 u32 accel_id)
>   {
> @@ -4487,11 +4546,6 @@ static bool amdgpu_ualink_vpod_membership_changed(struct amdgpu_device *adev)
>   			     AMDGPU_UALINK_ACCEL_MAX);
>   }
>   
> -static inline u32 ualink_accel_id(struct amdgpu_device *adev)
> -{
> -	return adev->ualink.info->ppod.accel_id;
> -}
> -
>   static inline enum amdgpu_ualink_addr_mode ualink_addr_mode(struct amdgpu_device *adev)
>   {
>   	return adev->ualink.info->vpod.addr_mode;
> @@ -4837,7 +4891,7 @@ static void amdgpu_ualink_metadata_npa_unmapping(struct amdgpu_device *adev)
>   {
>   	struct amdgpu_ualink_remote *remote = to_remote(adev);
>   	u64 timeout = msecs_to_jiffies(2000);
> -	u32 dst_accel_id = ualink_accel_id(adev);
> +	u32 dst_accel_id = ualink_exp_accel_id(adev);
>   	struct amdgpu_bo *bos[2];
>   	u32 n_bos;
>   	struct dma_fence *fence;
> @@ -4956,7 +5010,7 @@ static void amdgpu_ualink_metadata_npa_unmapping(struct amdgpu_device *adev)
>   static int amdgpu_ualink_metadata_npa_mapping(struct amdgpu_device *adev)
>   {
>   	struct amdgpu_ualink_remote *remote = to_remote(adev);
> -	u32 dst_accel_id = ualink_accel_id(adev);
> +	u32 dst_accel_id = ualink_exp_accel_id(adev);
>   	u64 timeout = msecs_to_jiffies(2000);
>   	struct amdgpu_bo *bos[2];
>   	u32 n_bos;
> @@ -5241,7 +5295,7 @@ static void amdgpu_ualink_metadata_fini(struct amdgpu_device *adev)
>   {
>   	struct amdgpu_ualink_remote *remote = to_remote(adev);
>   
> -	dev_dbg(adev->dev, "accel_id %u\n", ualink_accel_id(adev));
> +	dev_dbg(adev->dev, "accel_id %u\n", ualink_exp_accel_id(adev));
>   	amdgpu_bo_free_kernel(&remote->rptr_bo, &remote->rptr_gpu_addr,
>   				      &remote->rptr_cpu_addr);
>   	amdgpu_bo_free_kernel(&remote->ring_bo, &remote->rb_gpu_addr,
> @@ -5288,7 +5342,7 @@ static int amdgpu_ualink_metadata_init(struct amdgpu_device *adev)
>   	}
>   
>   	dev_dbg(adev->dev, "accel_id %u addr_mode %d fw status 0x%x\n",
> -		ualink_accel_id(adev), ualink_addr_mode(adev), status);
> +		ualink_exp_accel_id(adev), ualink_addr_mode(adev), status);
>   
>   	/* Alloc metadata structure for all GPUs */
>   	metadata_size = sizeof(struct amdgpu_ualink_metadata) * AMDGPU_UALINK_ACCEL_MAX;
> @@ -5361,12 +5415,12 @@ static int amdgpu_ualink_metadata_init(struct amdgpu_device *adev)
>   	}
>   
>   	if (ualink_addr_mode(adev) == AMDGPU_UALINK_ADDR_MODE_SOURCE_ALIAS)
> -		wptr_gpu_addr = remote->rb_gpu_addr + rb_size * ualink_accel_id(adev);
> +		wptr_gpu_addr = remote->rb_gpu_addr + rb_size * ualink_exp_accel_id(adev);
>   
>   	for_each_set_bit(accel_id, remote->active_accel_bits, AMDGPU_UALINK_ACCEL_MAX) {
>   		dev_dbg(adev->dev, "init for accel_id %u\n", accel_id);
>   
> -		if (accel_id == ualink_accel_id(adev))
> +		if (accel_id == ualink_exp_accel_id(adev))
>   			continue;
>   
>   		rb_gpu_addr = remote->rb_gpu_addr + rb_size * accel_id;
> @@ -5392,7 +5446,7 @@ static int amdgpu_ualink_metadata_init(struct amdgpu_device *adev)
>   						     ualink_tlb_wptr_offset(adev, accel_id);
>   
>   		dev_dbg(adev->dev, "init from accel_id %u to accel_id %u, rb_size 0x%x\n",
> -			accel_id, ualink_accel_id(adev), metadata[accel_id].rb_size);
> +			accel_id, ualink_exp_accel_id(adev), metadata[accel_id].rb_size);
>   		dev_dbg(adev->dev, "rb 0x%llx tlb rb 0x%llx\n",
>   			metadata[accel_id].ri_rb, metadata[accel_id].tlb_inv_rb);
>   		dev_dbg(adev->dev, "rb wptr at 0x%llx tlb wptr at 0x%llx\n",
> @@ -5806,7 +5860,14 @@ static void amdgpu_ualink_get_wb_addr(struct amdgpu_device *adev,
>   				      u64 *wb_npa, u32 type)
>   {
>   	struct amdgpu_ualink_remote *remote = to_remote(adev);
> -	u32 accel_id = ualink_accel_id(adev);
> +	/* The GPUID encoded in the write-back NPA address is the routing target
> +	 * the remote writes the rptr back to, i.e. THIS GPU. It must be the
> +	 * physically-owned accel_id: outside loopback that is our own accel_id,
> +	 * and in loopback it is the exporter (the only id the single GPU owns
> +	 * and can receive on). The importer is only a logical source id for the
> +	 * forward path and must never appear as an NPA GPUID.
> +	 */
> +	u32 accel_id = ualink_exp_accel_id(adev);
>   	u32 rb_size = AMDGPU_GPU_PAGE_ALIGN(2 * AMDGPU_UALINK_RB_SIZE);
>   	uintptr_t wb;
>   	u64 npa;
> @@ -5981,19 +6042,19 @@ static int amdgpu_ualink_ring_resync_ptrs(struct amdgpu_device *adev,
>   						   &peer->shootdown, &tlb_wptr);
>   	if (r) {
>   		dev_dbg(adev->dev, "accel_id %u read remote %u wptr failed %d\n",
> -			ualink_accel_id(adev), remote_accel_id, r);
> +			ualink_exp_accel_id(adev), remote_accel_id, r);
>   		return r;
>   	}
>   
>   	if (peer->interrupt.wptr == int_wptr && peer->shootdown.wptr == tlb_wptr) {
>   		dev_dbg(adev->dev, "accel_id %u wptr 0x%llx 0x%llx is same as remote %u\n",
> -			ualink_accel_id(adev), int_wptr, tlb_wptr, remote_accel_id);
> +			ualink_exp_accel_id(adev), int_wptr, tlb_wptr, remote_accel_id);
>   		return 0;
>   	}
>   
>   	dev_dbg(adev->dev,
>   		"accel_id %u wptr 0x%llx 0x%llx sync to remote %u wptr 0x%llx 0x%llx\n",
> -		ualink_accel_id(adev), peer->interrupt.wptr, peer->shootdown.wptr,
> +		ualink_exp_accel_id(adev), peer->interrupt.wptr, peer->shootdown.wptr,
>   		remote_accel_id, int_wptr, tlb_wptr);
>   
>   	amdgpu_ualink_get_wb_addr(adev, remote_accel_id, &int_wb, NULL,
> @@ -6164,7 +6225,7 @@ static int amdgpu_ualink_peer_remote_init(struct amdgpu_device *adev)
>   	struct amdgpu_ualink_peer *peer;
>   	struct amdgpu_ualink_ring *interrupt;
>   	struct amdgpu_ualink_ring *shootdown;
> -	u32 src_accel_id = ualink_accel_id(adev);
> +	u32 src_accel_id = ualink_exp_accel_id(adev);

This is changed to ualink_imp_accel_id in patch 5. I think we should use 
the correct function here, where it's introduced.


>   	u32 dst_accel_id;
>   	u64 npa, flags;
>   	int r = 0;
> @@ -6287,7 +6348,7 @@ static void amdgpu_ualink_peer_remote_fini(struct amdgpu_device *adev)
>   {
>   	u32 rb_pages = AMDGPU_UALINK_RB_SIZE >> AMDGPU_GPU_PAGE_SHIFT;
>   	struct amdgpu_ualink_remote *remote = to_remote(adev);
> -	u32 src_accel_id = ualink_accel_id(adev);
> +	u32 src_accel_id = ualink_exp_accel_id(adev);

This is changed to ualink_imp_accel_id in patch 5. I think we should use 
the correct function here, where it's introduced.

Regards,
   Felix


>   	u32 dst_accel_id;
>   	struct amdgpu_ualink_peer *peer;
>   	struct amdgpu_ualink_ring *ring;
> @@ -6374,7 +6435,7 @@ int amdgpu_ualink_sw_init(struct amdgpu_device *adev)
>    */
>   void amdgpu_ualink_sw_fini(struct amdgpu_device *adev)
>   {
> -	dev_dbg(adev->dev, "halt accel_id %u addr_mode %d\n", ualink_accel_id(adev),
> +	dev_dbg(adev->dev, "halt accel_id %u addr_mode %d\n", ualink_exp_accel_id(adev),
>   		ualink_addr_mode(adev));
>   
>   	adev->ualink.msg_ctl->send_halt(adev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.h
> index 85c9a9634a4b..0b1e8dd38666 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.h
> @@ -97,6 +97,17 @@ enum AMDGPU_UALINK_PROTOCOL_MESSAGES {
>   #define AMDGPU_UALINK_LIGHTWEIGHT_TLB_SHOOTDOWN		0x1
>   #define AMDGPU_UALINK_HEAVYWEIGHT_TLB_SHOOTDOWN		0x2
>   
> +/* UALink loopback support.
> + *
> + * In loopback mode a single GPU loops UALink requests back to itself for
> + * manufacturing testing. The GPU exposes two fixed accelerator IDs: it acts
> + * as the exporter on one and the importer on the other. Loopback is enabled
> + * with the amdgpu.ualink_loopback module parameter.
> + */
> +/* Fixed accelerator IDs used by a loopback GPU. */
> +#define AMDGPU_UALINK_LOOPBACK_EXPORTER_ACCID		1
> +#define AMDGPU_UALINK_LOOPBACK_IMPORTER_ACCID		0
> +
>   enum amdgpu_ualink_conn_state {
>   	AMDGPU_UALINK_CONN_NOT_READY			= 0,
>   	AMDGPU_UALINK_CONN_IN_PROGRESS			= 1,
> @@ -319,6 +330,21 @@ struct amdgpu_ualink_msg_ctl {
>   	int (*send_halt)(struct amdgpu_device *adev);
>   };
>   
> +/* UALink loopback mode state.
> + *
> + * When @enabled is true this single GPU acts as both exporter
> + * (@exporter_accel_id) and importer (@importer_accel_id). There is no second
> + * physical GPU, so exporter->importer messages cannot be delivered by the
> + * (single) MP nHT and are instead emulated in SW by feeding them back into
> + * our own IRQ handler via @wq.
> + */
> +struct amdgpu_ualink_loopback {
> +	bool enabled;
> +	u32 exporter_accel_id;
> +	u32 importer_accel_id;
> +	struct workqueue_struct *wq;
> +};
> +
>   struct amdgpu_ualink_mgr {
>   	u64 npa_size;
>   	u32 psp_if_ver;
> @@ -333,6 +359,9 @@ struct amdgpu_ualink_mgr {
>   	/* For remote interrupt and shootdown */
>   	struct amdgpu_ualink_remote *remote;
>   
> +	/* UALink loopback mode state (see struct amdgpu_ualink_loopback) */
> +	struct amdgpu_ualink_loopback loopback;
> +
>   	/* handle irq from ualink client of remote GPUs */
>   	struct amdgpu_irq_src irq;
>