Re: [PATCHv3 5/8] drm/amdgpu: Set up UALink rings and metadata for loopback

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

On 2026-11-04 15:20, Mukul Joshi wrote:
> On 11/3/2026 6:15 PM, Felix Kuehling wrote:
>> On 2026-11-02 10:47, Mukul Joshi wrote:
>>> In UALink loopback mode a single GPU acts as both the exporter and the
>>> importer, and the only path the HW serves is importer -> exporter.
>>> Adjust the per-peer setup accordingly:
>>>
>>> - The importer's doorbell reaches the MP nHT stamped with the GPU ID of
>>>    the exporter, and the FW looks up the metadata entry by that ID. So
>>>    publish the importer's rings and tail pointers in metadata[exporter].
>>>
>>> - Skip the ring pointer resync. Both ends of every ring are on this
>>>    GPU, so there is no remote peer that can reboot and leave the
>>>    pointers stale.
>>>
>>> - Do not take the local dma-buf shortcut on import. It would bypass
>>>    the NPA path that loopback is meant to exercise.
>>>
>>> Signed-off-by: Mukul Joshi <mukul.joshi@amd.com>
>>> ---
>>> v2->v3:
>>> - Move updating src_accel_id in peer_remote_init and fini to patch 1.
>>>
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c | 54 ++++++++++++++++------
>>>   1 file changed, 41 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c
>>> index 8558dc1a9e84..74bc8a907f5b 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c
>>> @@ -3976,7 +3976,13 @@ int amdgpu_ualink_import_handle(struct drm_device *dev,
>>>   		return -EINVAL;
>>>   	}
>>>   
>>> -	if (amdgpu_ualink_is_local_accel(adev, remote_acc_id))
>>> +	/* In loopback mode the exporter and importer are the same GPU, but the
>>> +	 * import must still go through the full cross-domain NPA path (the
>>> +	 * dma_buf shortcut in amdgpu_ualink_local_import() bypasses NPA). Only
>>> +	 * take the local shortcut for genuine same-pod local accelerators.
>>> +	 */
>>> +	if (!adev->ualink.loopback.enabled &&
>>> +	    amdgpu_ualink_is_local_accel(adev, remote_acc_id))
>>>   		return amdgpu_ualink_local_import(adev, remote_acc_id,
>>>   						  &handle, fd_out);
>>>   
>>> @@ -5452,11 +5458,28 @@ static int amdgpu_ualink_metadata_init(struct amdgpu_device *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) {
>>> +		struct amdgpu_ualink_metadata *md;
>>> +
>>>   		dev_dbg(adev->dev, "init for accel_id %u\n", accel_id);
>>>   
>>>   		if (accel_id == ualink_exp_accel_id(adev))
>>>   			continue;
>> I think you could drop the condition just below if you change this to
>>
>> 		if (accel_id == ualink_imp_accel_id(adev))
>> 			continue;
>>
>> It makes no difference if loopback is disabled. And with loopback
>> enabled it's the more obvious way to only initialize the ring for the
>> exporter ID. Then all the other changes in this function are unnecessary.
>>
> Unfortunately, this won't work.
>
> In the normal (non-loopback) case, when AccId 0 sends a message to AccId
> 1, it writes the message to the ring buffer and wptr NPA addresses that
> AccId 1 set up for source AccId 0, and then rings the doorbell.
>
> The remote interrupt the MPNHT on AccId 1 receives carries AccId 0 as
> the source, and the MPNHT FW uses that to look up metadata[0], which
> holds the ring buffer and wptr addresses for AccId 0.
>
> With loopback it's slightly different. AccId 0 (the importer) still
> writes to the ring buffer and wptr NPA addresses for source AccId 0 and
> rings the doorbell.
>
> However, the interrupt the MPNHT gets reports the source as AccId 1 (the
> exporter).
>
> So the MPNHT FW looks up metadata[1], and for it to find the right
> buffers we have to program the importer's (AccId 0) ring buffer and wptr
> addresses at index 1.
>
> If we skipped the importer and ran the loop for the exporter instead,
> metadata[1] would be filled with AccId 1's own ring buffer and wptr
> addresses, which the importer never writes to.
>
> That's why the patch builds the entry from the importer's addresses but
> stores it at the exporter's index.
>
>
> Regards,
>
> Mukul
>
>> Regards,
>>    Felix
>>
>>
>>>   
>>> +		/* In loopback the single MP nHT services the exporter half:
>>> +		 * the only messages delivered over HW are importer -> exporter
>>> +		 * (source = importer accel_id), while the exporter -> importer
>>> +		 * direction is emulated in SW and never reaches the MP nHT.
>>> +		 *
>>> +		 * The importer's doorbell reaches the MP nHT stamped with the
>>> +		 * GPU ID of the single physical station, the exporter, so the
>>> +		 * FW looks up metadata[exporter]. Publish the importer's rings
>>> +		 * and tail pointers in that entry.
>>> +		 */
>>> +		if (adev->ualink.loopback.enabled)
>>> +			md = &metadata[ualink_exp_accel_id(adev)];
>>> +		else
>>> +			md = &metadata[accel_id];
>>> +
>>>   		rb_gpu_addr = remote->rb_gpu_addr + rb_size * accel_id;
>>>   
>>>   		if (ualink_addr_mode(adev) == AMDGPU_UALINK_ADDR_MODE_SOURCE_IDENT) {
>>> @@ -5464,27 +5487,26 @@ static int amdgpu_ualink_metadata_init(struct amdgpu_device *adev)
>>>   			wptr_gpu_addr += wptr_size * accel_id;
>>>   		}
>>>   
>>> -		metadata[accel_id].header = AMDGPU_UALINK_METADATA_HEADER;
>>> -		metadata[accel_id].rb_size = fls(AMDGPU_UALINK_RB_SIZE / 64) - 1;
>>> +		md->header = AMDGPU_UALINK_METADATA_HEADER;
>>> +		md->rb_size = fls(AMDGPU_UALINK_RB_SIZE / 64) - 1;
>>>   
>>>   		/*
>>>   		 * remote shootdown type is 0, remote command type is 1
>>>   		 * with 2MB ring buffer mapping, remote shootdown ring NPA is before interrupt
>>>   		 */
>>> -		metadata[accel_id].ri_rb = rb_gpu_addr + AMDGPU_UALINK_RB_SIZE;
>>> -		metadata[accel_id].tlb_inv_rb = rb_gpu_addr;
>>> +		md->ri_rb = rb_gpu_addr + AMDGPU_UALINK_RB_SIZE;
>>> +		md->tlb_inv_rb = rb_gpu_addr;
>>>   
>>> -		metadata[accel_id].tailptr_ri = wptr_gpu_addr +
>>> -						ualink_wptr_offset(adev, accel_id);
>>> -		metadata[accel_id].tailptr_tlb_inv = wptr_gpu_addr +
>>> -						     ualink_tlb_wptr_offset(adev, accel_id);
>>> +		md->tailptr_ri = wptr_gpu_addr + ualink_wptr_offset(adev, accel_id);
>>> +		md->tailptr_tlb_inv = wptr_gpu_addr +
>>> +				      ualink_tlb_wptr_offset(adev, accel_id);

OK, I missed that it's still using the accel_id here to calculate the 
offsets. So what I proposed wouldn't work as needed. The comment above 
explains it too. The patch is

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


>>>   
>>> -		dev_dbg(adev->dev, "init from accel_id %u to accel_id %u, rb_size 0x%x\n",
>>> -			accel_id, ualink_exp_accel_id(adev), metadata[accel_id].rb_size);
>>> +		dev_dbg(adev->dev, "init from accel_id %u to accel_id %u, metadata entry %td, rb_size 0x%x\n",
>>> +			accel_id, ualink_exp_accel_id(adev), md - metadata, md->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);
>>> +			md->ri_rb, md->tlb_inv_rb);
>>>   		dev_dbg(adev->dev, "rb wptr at 0x%llx tlb wptr at 0x%llx\n",
>>> -			metadata[accel_id].tailptr_ri, metadata[accel_id].tailptr_tlb_inv);
>>> +			md->tailptr_ri, md->tailptr_tlb_inv);
>>>   	}
>>>   
>>>   	r = adev->ualink.msg_ctl->send_metadata(adev,
>>> @@ -6069,6 +6091,12 @@ static int amdgpu_ualink_ring_resync_ptrs(struct amdgpu_device *adev,
>>>   	u64 int_wptr, tlb_wptr;
>>>   	int r;
>>>   
>>> +	/* UALink loopback: both ends of every ring live on this GPU, so there
>>> +	 * is no remote peer that can reboot and leave the pointers stale.
>>> +	 */
>>> +	if (adev->ualink.loopback.enabled)
>>> +		return 0;
>>> +
>>>   	/* ring->wptr and ring->rptr are owned by the send path. */
>>>   	guard(mutex)(&peer->lock);
>>>