Re: [PATCH v2 1/5] drm/amdgpu: Split amdgpu_ucode_init/fini_bo into two functions
On Tue, Oct 9, 2018 at 11:36 AM Rex Zhu wrote:
>
> 1. one is for create/free bo when init/fini
> 2. one is for fill the bo before fw loading
>
> the ucode bo only need to be created when load driver
> and free when driver unload.
>
> when resume/reset, driver only need to re-fill the bo
> if the bo is allocated in vram.
>
> Suggested by Christian.
>
> v2: Return error when bo create failed.
>
> Signed-off-by: Rex Zhu
Reviewed-by: Alex Deucher
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 4 +++
> drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c | 58
> +++---
> drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h | 3 ++
> 3 files changed, 36 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 94c92f5..680df05 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -1581,6 +1581,9 @@ static int amdgpu_device_ip_init(struct amdgpu_device
> *adev)
> }
> }
>
> + r = amdgpu_ucode_create_bo(adev); /* create ucode bo when sw_init
> complete*/
> + if (r)
> + return r;
> for (i = 0; i < adev->num_ip_blocks; i++) {
> if (!adev->ip_blocks[i].status.sw)
> continue;
> @@ -1803,6 +1806,7 @@ static int amdgpu_device_ip_fini(struct amdgpu_device
> *adev)
> continue;
>
> if (adev->ip_blocks[i].version->type ==
> AMD_IP_BLOCK_TYPE_GMC) {
> + amdgpu_ucode_free_bo(adev);
> amdgpu_free_static_csa(adev);
> amdgpu_device_wb_fini(adev);
> amdgpu_device_vram_scratch_fini(adev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> index adfeb93..57ed384 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> @@ -422,32 +422,42 @@ static int amdgpu_ucode_patch_jt(struct
> amdgpu_firmware_info *ucode,
> return 0;
> }
>
> +int amdgpu_ucode_create_bo(struct amdgpu_device *adev)
> +{
> + if (adev->firmware.load_type != AMDGPU_FW_LOAD_DIRECT) {
> + amdgpu_bo_create_kernel(adev, adev->firmware.fw_size,
> PAGE_SIZE,
> + amdgpu_sriov_vf(adev) ? AMDGPU_GEM_DOMAIN_VRAM :
> AMDGPU_GEM_DOMAIN_GTT,
> + >firmware.fw_buf,
> + >firmware.fw_buf_mc,
> + >firmware.fw_buf_ptr);
> + if (!adev->firmware.fw_buf) {
> + dev_err(adev->dev, "failed to create kernel buffer
> for firmware.fw_buf\n");
> + return -ENOMEM;
> + } else if (amdgpu_sriov_vf(adev)) {
> + memset(adev->firmware.fw_buf_ptr, 0,
> adev->firmware.fw_size);
> + }
> + }
> + return 0;
> +}
> +
> +void amdgpu_ucode_free_bo(struct amdgpu_device *adev)
> +{
> + if (adev->firmware.load_type != AMDGPU_FW_LOAD_DIRECT)
> + amdgpu_bo_free_kernel(>firmware.fw_buf,
> + >firmware.fw_buf_mc,
> + >firmware.fw_buf_ptr);
> +}
> +
> int amdgpu_ucode_init_bo(struct amdgpu_device *adev)
> {
> uint64_t fw_offset = 0;
> - int i, err;
> + int i;
> struct amdgpu_firmware_info *ucode = NULL;
> const struct common_firmware_header *header = NULL;
>
> - if (!adev->firmware.fw_size) {
> - dev_warn(adev->dev, "No ip firmware need to load\n");
> + /* for baremetal, the ucode is allocated in gtt, so don't need to fill the
> bo when reset/suspend */
> + if (!amdgpu_sriov_vf(adev) && (adev->in_gpu_reset ||
> adev->in_suspend))
> return 0;
> - }
> -
> - if (!adev->in_gpu_reset && !adev->in_suspend) {
> - err = amdgpu_bo_create_kernel(adev, adev->firmware.fw_size,
> PAGE_SIZE,
> - amdgpu_sriov_vf(adev) ?
> AMDGPU_GEM_DOMAIN_VRAM : AMDGPU_GEM_DOMAIN_GTT,
> - >firmware.fw_buf,
> - >firmware.fw_buf_mc,
> - >firmware.fw_buf_ptr);
> - if (err) {
> - dev_err(adev->dev, "failed to create kernel buffer
> for firmware.fw_buf\n");
> - goto failed;
> - }
> - }
> -
> - memset(adev->firmware.fw_buf_ptr, 0, adev->firmware.fw_size);
> -
> /*
> * if SMU loaded firmware, it needn't add SMC, UVD, and VCE
> * ucode info here
> @@ -479,12 +489,6 @@ int amdgpu_ucode_init_bo(struct amdgpu_device *adev)
> }
> }
> return 0;
> -
> -failed:
> - if (err)
> - adev->firmware.load_type = AMDGPU_FW_LOAD_DIRECT;
> -
> - return err;
> }
>
>
[PATCH v2 1/5] drm/amdgpu: Split amdgpu_ucode_init/fini_bo into two functions
1. one is for create/free bo when init/fini
2. one is for fill the bo before fw loading
the ucode bo only need to be created when load driver
and free when driver unload.
when resume/reset, driver only need to re-fill the bo
if the bo is allocated in vram.
Suggested by Christian.
v2: Return error when bo create failed.
Signed-off-by: Rex Zhu
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 4 +++
drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c | 58 +++---
drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h | 3 ++
3 files changed, 36 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 94c92f5..680df05 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1581,6 +1581,9 @@ static int amdgpu_device_ip_init(struct amdgpu_device
*adev)
}
}
+ r = amdgpu_ucode_create_bo(adev); /* create ucode bo when sw_init
complete*/
+ if (r)
+ return r;
for (i = 0; i < adev->num_ip_blocks; i++) {
if (!adev->ip_blocks[i].status.sw)
continue;
@@ -1803,6 +1806,7 @@ static int amdgpu_device_ip_fini(struct amdgpu_device
*adev)
continue;
if (adev->ip_blocks[i].version->type == AMD_IP_BLOCK_TYPE_GMC) {
+ amdgpu_ucode_free_bo(adev);
amdgpu_free_static_csa(adev);
amdgpu_device_wb_fini(adev);
amdgpu_device_vram_scratch_fini(adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
index adfeb93..57ed384 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
@@ -422,32 +422,42 @@ static int amdgpu_ucode_patch_jt(struct
amdgpu_firmware_info *ucode,
return 0;
}
+int amdgpu_ucode_create_bo(struct amdgpu_device *adev)
+{
+ if (adev->firmware.load_type != AMDGPU_FW_LOAD_DIRECT) {
+ amdgpu_bo_create_kernel(adev, adev->firmware.fw_size, PAGE_SIZE,
+ amdgpu_sriov_vf(adev) ? AMDGPU_GEM_DOMAIN_VRAM :
AMDGPU_GEM_DOMAIN_GTT,
+ >firmware.fw_buf,
+ >firmware.fw_buf_mc,
+ >firmware.fw_buf_ptr);
+ if (!adev->firmware.fw_buf) {
+ dev_err(adev->dev, "failed to create kernel buffer for
firmware.fw_buf\n");
+ return -ENOMEM;
+ } else if (amdgpu_sriov_vf(adev)) {
+ memset(adev->firmware.fw_buf_ptr, 0,
adev->firmware.fw_size);
+ }
+ }
+ return 0;
+}
+
+void amdgpu_ucode_free_bo(struct amdgpu_device *adev)
+{
+ if (adev->firmware.load_type != AMDGPU_FW_LOAD_DIRECT)
+ amdgpu_bo_free_kernel(>firmware.fw_buf,
+ >firmware.fw_buf_mc,
+ >firmware.fw_buf_ptr);
+}
+
int amdgpu_ucode_init_bo(struct amdgpu_device *adev)
{
uint64_t fw_offset = 0;
- int i, err;
+ int i;
struct amdgpu_firmware_info *ucode = NULL;
const struct common_firmware_header *header = NULL;
- if (!adev->firmware.fw_size) {
- dev_warn(adev->dev, "No ip firmware need to load\n");
+ /* for baremetal, the ucode is allocated in gtt, so don't need to fill the bo
when reset/suspend */
+ if (!amdgpu_sriov_vf(adev) && (adev->in_gpu_reset || adev->in_suspend))
return 0;
- }
-
- if (!adev->in_gpu_reset && !adev->in_suspend) {
- err = amdgpu_bo_create_kernel(adev, adev->firmware.fw_size,
PAGE_SIZE,
- amdgpu_sriov_vf(adev) ?
AMDGPU_GEM_DOMAIN_VRAM : AMDGPU_GEM_DOMAIN_GTT,
- >firmware.fw_buf,
- >firmware.fw_buf_mc,
- >firmware.fw_buf_ptr);
- if (err) {
- dev_err(adev->dev, "failed to create kernel buffer for
firmware.fw_buf\n");
- goto failed;
- }
- }
-
- memset(adev->firmware.fw_buf_ptr, 0, adev->firmware.fw_size);
-
/*
* if SMU loaded firmware, it needn't add SMC, UVD, and VCE
* ucode info here
@@ -479,12 +489,6 @@ int amdgpu_ucode_init_bo(struct amdgpu_device *adev)
}
}
return 0;
-
-failed:
- if (err)
- adev->firmware.load_type = AMDGPU_FW_LOAD_DIRECT;
-
- return err;
}
int amdgpu_ucode_fini_bo(struct amdgpu_device *adev)
@@ -503,9 +507,5 @@ int amdgpu_ucode_fini_bo(struct amdgpu_device *adev)
}
}
- amdgpu_bo_free_kernel(>firmware.fw_buf,
- >firmware.fw_buf_mc,
- >firmware.fw_buf_ptr);
-
return 0;
}
diff
