FW: [PATCH] drm/amdgpu: add vram reservation logic based on vram_usagebyfirmware_v2_2
Christian König
christian.koenig at amd.com
Tue Nov 8 18:13:30 UTC 2022
Am 08.11.22 um 18:30 schrieb Zhang, Bokun:
> [AMD Official Use Only - General]
>
> Please find below patch after passing code style check
> Thanks!
I'm not sure which coding style check you use, but when I use
checkpatch.pl it points out tons of problems with this patch.
>
> -----Original Message-----
> From: Bokun Zhang <bokun.zhang at amd.com>
> Sent: Tuesday, November 8, 2022 12:29 PM
> To: amd-gfx at lists.freedesktop.org
> Cc: Zhang, Bokun <Bokun.Zhang at amd.com>; Liu01, Tong (Esther) <Tong.Liu01 at amd.com>
> Subject: [PATCH] drm/amdgpu: add vram reservation logic based on vram_usagebyfirmware_v2_2
>
> - Move TMR region from top of FB to 2MB for FFBM, so we need to
> reserve TMR region firstly to make sure TMR can be allocated at 2MB
>
> Signed-off-by: Tong Liu01 <Tong.Liu01 at amd.com>
> Signed-off-by: Bokun Zhang <bokun.zhang at amd.com>
> ---
> .../gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c | 106 ++++++++++++++----
> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 50 +++++++++
> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h | 5 +
> drivers/gpu/drm/amd/include/atomfirmware.h | 68 +++++++++--
> 4 files changed, 198 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
> index b81b77a9efa6..032dc2678d7c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
> @@ -101,39 +101,99 @@ void amdgpu_atomfirmware_scratch_regs_init(struct amdgpu_device *adev)
> }
> }
>
> +static int amdgpu_atomfirmware_allocate_fb_v2_1(struct amdgpu_device *adev,
> + struct vram_usagebyfirmware_v2_1 *firmware_usage_v2_1,
> + int *usage_bytes)
> +{
> + uint32_t start_addr, fw_size, drv_size;
> +
> + start_addr = le32_to_cpu(firmware_usage_v2_1->start_address_in_kb);
> + fw_size = le16_to_cpu(firmware_usage_v2_1->used_by_firmware_in_kb);
> + drv_size = le16_to_cpu(firmware_usage_v2_1->used_by_driver_in_kb);
> +
> + DRM_DEBUG("atom firmware v2_1 requested %08x %dkb fw %dkb drv\n",
> + start_addr,
> + fw_size,
> + drv_size);
> +
> + if ((start_addr & ATOM_VRAM_OPERATION_FLAGS_MASK) ==
> + (uint32_t)(ATOM_VRAM_BLOCK_SRIOV_MSG_SHARE_RESERVATION <<
> + ATOM_VRAM_OPERATION_FLAGS_SHIFT)) {
This still doesn't looks like correct coding style. Please align the
second line under the ( of the first line.
> + /* Firmware request VRAM reservation for SR-IOV */
> + adev->mman.fw_vram_usage_start_offset = (start_addr &
> + (~ATOM_VRAM_OPERATION_FLAGS_MASK)) << 10;
> + adev->mman.fw_vram_usage_size = fw_size << 10;
> + /* Use the default scratch size */
> + *usage_bytes = 0;
> + } else {
> + *usage_bytes = drv_size << 10;
> + }
> + return 0;
> +}
> +
> +static int amdgpu_atomfirmware_allocate_fb_v2_2(struct amdgpu_device *adev,
> + struct vram_usagebyfirmware_v2_2 *firmware_usage_v2_2,
> + int *usage_bytes)
Same here, the parameters are not correctly indented.
> +{
> + uint32_t fw_start_addr, fw_size, drv_start_addr, drv_size;
> +
> + fw_start_addr = le32_to_cpu(firmware_usage_v2_2->fw_region_start_address_in_kb);
> + fw_size = le16_to_cpu(firmware_usage_v2_2->used_by_firmware_in_kb);
> +
> + drv_start_addr = le32_to_cpu(firmware_usage_v2_2->driver_region0_start_address_in_kb);
> + drv_size =
> +le32_to_cpu(firmware_usage_v2_2->used_by_driver_region0_in_kb);
> +
> + DRM_DEBUG("atom requested fw start at %08x %dkb and drv start at %08x %dkb\n",
> + fw_start_addr,
> + fw_size,
> + drv_start_addr,
> + drv_size);
> +
> + if ((fw_start_addr & (ATOM_VRAM_BLOCK_NEEDS_NO_RESERVATION << 30)) == 0) {
> + /* Firmware request VRAM reservation for SR-IOV */
> + adev->mman.fw_vram_usage_start_offset = (fw_start_addr &
> + (~ATOM_VRAM_OPERATION_FLAGS_MASK)) << 10;
> + adev->mman.fw_vram_usage_size = fw_size << 10;
> + }
> +
> + if ((drv_start_addr & (ATOM_VRAM_BLOCK_NEEDS_NO_RESERVATION << 30)) == 0) {
> + /* driver request VRAM reservation for SR-IOV */
> + adev->mman.drv_vram_usage_start_offset = (drv_start_addr &
> + (~ATOM_VRAM_OPERATION_FLAGS_MASK)) << 10;
> + adev->mman.drv_vram_usage_size = drv_size << 10;
> + }
> +
> + *usage_bytes = 0;
> + return 0;
> +}
> +
> int amdgpu_atomfirmware_allocate_fb_scratch(struct amdgpu_device *adev) {
> struct atom_context *ctx = adev->mode_info.atom_context;
> int index = get_index_into_master_table(atom_master_list_of_data_tables_v2_1,
> vram_usagebyfirmware);
> - struct vram_usagebyfirmware_v2_1 *firmware_usage;
> - uint32_t start_addr, size;
> + struct vram_usagebyfirmware_v2_1 *firmware_usage_v2_1;
> + struct vram_usagebyfirmware_v2_2 *firmware_usage_v2_2;
> uint16_t data_offset;
> + uint8_t frev, crev;
> int usage_bytes = 0;
>
> - if (amdgpu_atom_parse_data_header(ctx, index, NULL, NULL, NULL, &data_offset)) {
> - firmware_usage = (struct vram_usagebyfirmware_v2_1 *)(ctx->bios + data_offset);
> - DRM_DEBUG("atom firmware requested %08x %dkb fw %dkb drv\n",
> - le32_to_cpu(firmware_usage->start_address_in_kb),
> - le16_to_cpu(firmware_usage->used_by_firmware_in_kb),
> - le16_to_cpu(firmware_usage->used_by_driver_in_kb));
> -
> - start_addr = le32_to_cpu(firmware_usage->start_address_in_kb);
> - size = le16_to_cpu(firmware_usage->used_by_firmware_in_kb);
> -
> - if ((uint32_t)(start_addr & ATOM_VRAM_OPERATION_FLAGS_MASK) ==
> - (uint32_t)(ATOM_VRAM_BLOCK_SRIOV_MSG_SHARE_RESERVATION <<
> - ATOM_VRAM_OPERATION_FLAGS_SHIFT)) {
> - /* Firmware request VRAM reservation for SR-IOV */
> - adev->mman.fw_vram_usage_start_offset = (start_addr &
> - (~ATOM_VRAM_OPERATION_FLAGS_MASK)) << 10;
> - adev->mman.fw_vram_usage_size = size << 10;
> - /* Use the default scratch size */
> - usage_bytes = 0;
> - } else {
> - usage_bytes = le16_to_cpu(firmware_usage->used_by_driver_in_kb) << 10;
> + if (amdgpu_atom_parse_data_header(ctx, index, NULL, &frev, &crev, &data_offset)) {
> + if (frev == 2 && crev == 1) {
> + firmware_usage_v2_1 =
> + (struct vram_usagebyfirmware_v2_1 *)(ctx->bios + data_offset);
> + amdgpu_atomfirmware_allocate_fb_v2_1(adev,
> + firmware_usage_v2_1,
> + &usage_bytes);
> + } else if (frev >= 2 && crev >= 2) {
> + firmware_usage_v2_2 =
> + (struct vram_usagebyfirmware_v2_2 *)(ctx->bios + data_offset);
> + amdgpu_atomfirmware_allocate_fb_v2_2(adev,
> + firmware_usage_v2_2,
> + &usage_bytes);
> }
> }
> +
> ctx->scratch_size_bytes = 0;
> if (usage_bytes == 0)
> usage_bytes = 20 * 1024;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 76a8ebfc9e71..06673a756231 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -1579,6 +1579,22 @@ static void amdgpu_ttm_fw_reserve_vram_fini(struct amdgpu_device *adev)
> NULL, &adev->mman.fw_vram_usage_va);
> }
>
> +/*
> + * Driver Reservation functions
> + */
> +/**
> + * amdgpu_ttm_drv_reserve_vram_fini - free drv reserved vram
> + *
> + * @adev: amdgpu_device pointer
> + *
> + * free drv reserved vram if it has been reserved.
> + */
Please no kernel doc for static functions, except when you need to
explain what they do.
> +static void amdgpu_ttm_drv_reserve_vram_fini(struct amdgpu_device
> +*adev) {
The variable name should always be on the same line as the type.
When this becomes to long you can write it like this:
static void
amdgpu_ttm_drv_reserve_vram_fini(struct amdgpu_device *adev)
{
And BTW the { should always be on a new line.
Regards,
Christian.
> + amdgpu_bo_free_kernel(&adev->mman.drv_vram_usage_reserved_bo,
> + NULL, NULL);
> +}
> +
> /**
> * amdgpu_ttm_fw_reserve_vram_init - create bo vram reservation from fw
> *
> @@ -1605,6 +1621,31 @@ static int amdgpu_ttm_fw_reserve_vram_init(struct amdgpu_device *adev)
> &adev->mman.fw_vram_usage_va);
> }
>
> +/**
> + * amdgpu_ttm_drv_reserve_vram_init - create bo vram reservation from
> +driver
> + *
> + * @adev: amdgpu_device pointer
> + *
> + * create bo vram reservation from drv.
> + */
> +static int amdgpu_ttm_drv_reserve_vram_init(struct amdgpu_device *adev)
> +{
> + uint64_t vram_size = adev->gmc.visible_vram_size;
> +
> + adev->mman.drv_vram_usage_reserved_bo = NULL;
> +
> + if (adev->mman.drv_vram_usage_size == 0 ||
> + adev->mman.drv_vram_usage_size > vram_size)
> + return 0;
> +
> + return amdgpu_bo_create_kernel_at(adev,
> + adev->mman.drv_vram_usage_start_offset,
> + adev->mman.drv_vram_usage_size,
> + AMDGPU_GEM_DOMAIN_VRAM,
> + &adev->mman.drv_vram_usage_reserved_bo,
> + NULL);
> +}
> +
> /*
> * Memoy training reservation functions
> */
> @@ -1772,6 +1813,14 @@ int amdgpu_ttm_init(struct amdgpu_device *adev)
> return r;
> }
>
> + /*
> + *The reserved vram for driver must be pinned to the specified
> + *place on the VRAM, so reserve it early.
> + */
> + r = amdgpu_ttm_drv_reserve_vram_init(adev);
> + if (r)
> + return r;
> +
> /*
> * only NAVI10 and onwards ASIC support for IP discovery.
> * If IP discovery enabled, a block of memory should be @@ -1897,6 +1946,7 @@ void amdgpu_ttm_fini(struct amdgpu_device *adev)
> amdgpu_bo_free_kernel(&adev->mman.sdma_access_bo, NULL,
> &adev->mman.sdma_access_ptr);
> amdgpu_ttm_fw_reserve_vram_fini(adev);
> + amdgpu_ttm_drv_reserve_vram_fini(adev);
>
> if (drm_dev_enter(adev_to_drm(adev), &idx)) {
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> index 6a70818039dd..7c38843f411e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> @@ -84,6 +84,11 @@ struct amdgpu_mman {
> struct amdgpu_bo *fw_vram_usage_reserved_bo;
> void *fw_vram_usage_va;
>
> + /* driver VRAM reservation */
> + u64 drv_vram_usage_start_offset;
> + u64 drv_vram_usage_size;
> + struct amdgpu_bo *drv_vram_usage_reserved_bo;
> +
> /* PAGE_SIZE'd BO for process memory r/w over SDMA. */
> struct amdgpu_bo *sdma_access_bo;
> void *sdma_access_ptr;
> diff --git a/drivers/gpu/drm/amd/include/atomfirmware.h b/drivers/gpu/drm/amd/include/atomfirmware.h
> index ff855cb21d3f..68a86b14dff4 100644
> --- a/drivers/gpu/drm/amd/include/atomfirmware.h
> +++ b/drivers/gpu/drm/amd/include/atomfirmware.h
> @@ -705,20 +705,72 @@ struct atom_gpio_pin_lut_v2_1 };
>
>
> -/*
> - ***************************************************************************
> - Data Table vram_usagebyfirmware structure
> - ***************************************************************************
> +/*
> + * VBIOS/PRE-OS always reserve a FB region at the top of frame buffer.
> + * driver should not write access that region.
> + * driver can allocate their own reservation region as long as it does
> +not overlap firwmare's
> + * reservation region.
> + * if( atom data table firmwareInfoTable version < 3.3) { //( pre-NV1X
> +)
> + * in this case, atom data table vram_usagebyfirmwareTable version
> +always <= 2.1
> + * if( VBIOS/UEFI GOP is posted ) {
> + * VBIOS/UEFIGOP update used_by_firmware_in_kb = total reserved size by VBIOS
> + * update start_address_in_kb = total_mem_size_in_kb - used_by_firmware_in_kb;
> + * Where: ( total_mem_size_in_kb = reg(CONFIG_MEMSIZE)<<10)
> + * driver can allocate driver reservation region under firmware reservation,
> + * used_by_driver_in_kb = driver reservation size
> + * driver reservation start address = (start_address_in_kb - used_by_driver_in_kb)
> + * Comment1[hchan]:
> + * There is only one reservation at the beginning of the FB reserved by Host driver.
> + * Host driver would overwrite the table with the following
> + * used_by_firmware_in_kb = total reserved size for pf-vf info
> +exchange and
> + * set SRIOV_MSG_SHARE_RESERVATION mask start_address_in_kb = 0
> + * } else {
> + * there is no VBIOS reservation region
> + * driver must allocate driver reservation region at top of FB.
> + * driver set used_by_driver_in_kb = driver reservation size
> + * driver reservation start address = (total_mem_size_in_kb - used_by_driver_in_kb)
> + * same as Comment1
> + * }
> + * } else { //( NV1X and after)
> + * if( VBIOS/UEFI GOP is posted ) {
> + * VBIOS/UEFIGOP update used_by_firmware_in_kb = atom_firmware_Info_v3_3.fw_reserved_size_in_kb;
> + * update start_address_in_kb = total_mem_size_in_kb - used_by_firmware_in_kb;
> + * Where: ( total_mem_size_in_kb = reg(CONFIG_MEMSIZE)<<10 )
> + * }
> + * if( vram_usagebyfirmwareTable version <= 2.1 ) {
> + * driver can allocate driver reservation region under firmware reservation,
> + * driver set used_by_driver_in_kb = driver reservation size
> + * driver reservation start address = (start_address_in_kb - used_by_driver_in_kb)
> + * same as Comment1
> + * } else {
> + * dirver can allocate it reservation any place as long as
> + * it does overlap pre-OS FW reservation area
> + * driver set used_by_driver_region0_in_kb = driver reservation size
> + * driver set driver_region0_start_address_in_kb = driver reservation region start address
> + * Comment2[hchan]: Host driver can set used_by_firmware_in_kb and
> +start_address_in_kb to zero
> + * as the reservation for VF as it doesn’t exist. And Host driver
> +should also
> + * update atom_firmware_Info table to remove the same VBIOS reservation as well.
> + * }
> + * }
> */
>
> struct vram_usagebyfirmware_v2_1
> {
> - struct atom_common_table_header table_header;
> - uint32_t start_address_in_kb;
> - uint16_t used_by_firmware_in_kb;
> - uint16_t used_by_driver_in_kb;
> + struct atom_common_table_header table_header;
> + uint32_t start_address_in_kb;
> + uint16_t used_by_firmware_in_kb;
> + uint16_t used_by_driver_in_kb;
> };
>
> +struct vram_usagebyfirmware_v2_2 {
> + struct atom_common_table_header table_header;
> + uint32_t fw_region_start_address_in_kb;
> + uint16_t used_by_firmware_in_kb;
> + uint16_t reserved;
> + uint32_t driver_region0_start_address_in_kb;
> + uint32_t used_by_driver_region0_in_kb;
> + uint32_t reserved32[7];
> +};
>
> /*
> ***************************************************************************
> --
> 2.25.1
More information about the amd-gfx
mailing list