Re: [PATCH 5/6] drm/amdgpu: Add logic for VF ipd and VF bios to init from dynamic crit_region offsets

["Christian König" <christian.koenig@amd.com>]

On 09.10.25 05:31, Ellen Pan wrote:
> 1. Added VF logic to init IP discovery using the offsets from dynamic(v2) critical regions;
> 2. Added VF logic to init bios image using the offsets from dynamic(v2) critical regions;
> 
> Signed-off-by: Ellen Pan <yunru.pan@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c      | 12 ++++-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c |  5 ++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c      | 47 +++++++++++++++++++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h      |  3 ++
>  4 files changed, 66 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
> index 00e96419fcda..2cbb24ede86e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
> @@ -114,7 +114,17 @@ static bool amdgpu_read_bios_from_vram(struct amdgpu_device *adev)
>  
>  	adev->bios = NULL;
>  	vram_base = pci_resource_start(adev->pdev, 0);
> -	bios = ioremap_wc(vram_base, size);
> +
> +	if (amdgpu_sriov_vf(adev) && adev->virt.init_data_done) {
> +		resource_size_t bios_offset;
> +
> +		if (amdgpu_virt_get_bios_info(adev, &bios_offset, &size))
> +			return false;
> +
> +		bios = ioremap_wc(vram_base + bios_offset, size);
> +	} else
> +		bios = ioremap_wc(vram_base, size);
> +

It would probably better to give offset as parameter to amdgpu_read_bios_from_vram().

There is already a comment saying "this is required for SR-IOV" where that code would make perfect sense.

>  	if (!bios)
>  		return false;
>  
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> index 73401f0aeb34..0dd9ff576cdc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> @@ -292,6 +292,11 @@ static int amdgpu_discovery_read_binary_from_mem(struct amdgpu_device *adev,
>  		}
>  	}
>  
> +	if ((adev->virt.req_init_data_ver == GPU_CRIT_REGION_V2) && adev->virt.init_data_done) {
> +		ret = amdgpu_virt_init_ip_discovery(adev, binary);
> +		return ret;
> +	}
> +

It is probably better to rework the code here and put all the SRIOV handling into the calling function.

And then call either amdgpu_discovery_read_binary_from_mem() or amdgpu_virt_init_ip_discovery() and not delegate the call to another function after calling the first one.

>  	vram_size = RREG32(mmRCC_CONFIG_MEMSIZE);
>  	if (!vram_size || vram_size == U32_MAX)
>  		sz_valid = false;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> index e9dbab53cb06..9181acef4e9b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> @@ -954,6 +954,53 @@ int amdgpu_virt_init_critical_region(struct amdgpu_device *adev)
>  	return r;
>  }
>  
> +int amdgpu_virt_init_ip_discovery(struct amdgpu_device *adev, uint8_t *binary)
> +{
> +	uint32_t ip_discovery_offset =
> +		adev->virt.crit_region_offsets[AMD_SRIOV_MSG_IPD_TABLE_ID];
> +	uint32_t ip_discovery_size =
> +		adev->virt.crit_region_sizes_kb[AMD_SRIOV_MSG_IPD_TABLE_ID] << 10;
> +	uint64_t pos = 0;
> +
> +	dev_info(adev->dev, "use ip discovery information copied from dynamic crit_region_table at offset 0x%x with size of 0x%x bytes.\n", ip_discovery_offset, ip_discovery_size);

That line is to long, please use checkpatch.pl.

> +
> +	if (!IS_ALIGNED(ip_discovery_offset, 4) || !IS_ALIGNED(ip_discovery_size, 4)) {
> +		DRM_ERROR("IP discovery data not aligned to 4 bytes\n");
> +		return -EINVAL;
> +	}
> +
> +	if (ip_discovery_size > DISCOVERY_TMR_SIZE) {
> +		DRM_ERROR("Invalid IP discovery size: 0x%x\n", ip_discovery_size);
> +		return -EINVAL;
> +	}
> +
> +	pos = (uint64_t)ip_discovery_offset;
> +	amdgpu_device_vram_access(adev, pos, (uint32_t *)binary,
> +					  ip_discovery_size, false);
> +
> +	return 0;
> +}
> +
> +int amdgpu_virt_get_bios_info(struct amdgpu_device *adev,
> +		resource_size_t *bios_offset, resource_size_t *bios_size)
> +{
> +	uint32_t vbios_offset = adev->virt.crit_region_offsets[AMD_SRIOV_MSG_VBIOS_IMG_TABLE_ID];
> +	uint32_t vbios_size =
> +		adev->virt.crit_region_sizes_kb[AMD_SRIOV_MSG_VBIOS_IMG_TABLE_ID] << 10;
> +
> +	dev_info(adev->dev, "use bios information copied from dynamic crit_region_table at offset 0x%x with size of 0x%x bytes.\n", vbios_offset, vbios_size);

Same here.

Regards,
Christian.

> +
> +	if (vbios_size > *bios_size) {
> +		DRM_ERROR("Invalid vbios size: 0x%x\n", vbios_size);
> +		return -EINVAL;
> +	}
> +
> +	*bios_offset = vbios_offset;
> +	*bios_size = vbios_size;
> +
> +	return 0;
> +}
> +
>  void amdgpu_virt_init(struct amdgpu_device *adev)
>  {
>  	bool is_sriov = false;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h
> index 5f6014b2f349..d122347ff666 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h
> @@ -434,6 +434,9 @@ void amdgpu_virt_fini_data_exchange(struct amdgpu_device *adev);
>  void amdgpu_virt_init(struct amdgpu_device *adev);
>  
>  int amdgpu_virt_init_critical_region(struct amdgpu_device *adev);
> +int amdgpu_virt_init_ip_discovery(struct amdgpu_device *adev, uint8_t *binary);
> +int amdgpu_virt_get_bios_info(struct amdgpu_device *adev,
> +					  resource_size_t *bios_offset, resource_size_t *bios_size);
>  
>  bool amdgpu_virt_can_access_debugfs(struct amdgpu_device *adev);
>  int amdgpu_virt_enable_access_debugfs(struct amdgpu_device *adev);