Re: [PATCH 1/5] drm/amdgpu: Split amdgpu_ucode_init/fini_bo into two functions

[Christian König <ckoenig.leichtzumerken@xxxxxxxxx>]

Am 09.10.2018 um 14:43 schrieb Rex Zhu:
> 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.
>
> Signed-off-by: Rex Zhu <Rex.Zhu@xxxxxxx>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  3 ++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c  | 57 +++++++++++++++---------------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h  |  3 ++
>   3 files changed, 34 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..4787571 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -1581,6 +1581,8 @@ static int amdgpu_device_ip_init(struct amdgpu_device *adev)
>   		}
>   	}
>   
> +	amdgpu_ucode_create_bo(adev); /* create ucode bo when sw_init complete*/
> +
>   	for (i = 0; i < adev->num_ip_blocks; i++) {
>   		if (!adev->ip_blocks[i].status.sw)
>   			continue;
> @@ -1803,6 +1805,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..7b6b2f4c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> @@ -422,32 +422,41 @@ static int amdgpu_ucode_patch_jt(struct amdgpu_firmware_info *ucode,
>   	return 0;
>   }
>   
> +void amdgpu_ucode_create_bo(struct amdgpu_device *adev)
> +{
> +	if (adev->firmware.load_type != AMDGPU_FW_LOAD_DIRECT) {

Better use early bailout here, e.g. in this case a return statement.

Apart from that looks good to me,
Christian.

> +		amdgpu_bo_create_kernel(adev, adev->firmware.fw_size, PAGE_SIZE,
> +			amdgpu_sriov_vf(adev) ? AMDGPU_GEM_DOMAIN_VRAM : AMDGPU_GEM_DOMAIN_GTT,
> +			&adev->firmware.fw_buf,
> +			&adev->firmware.fw_buf_mc,
> +			&adev->firmware.fw_buf_ptr);
> +		if (!adev->firmware.fw_buf) {
> +			dev_err(adev->dev, "failed to create kernel buffer for firmware.fw_buf\n");
> +			adev->firmware.load_type = AMDGPU_FW_LOAD_DIRECT;
> +		} else if (amdgpu_sriov_vf(adev)) {
> +			memset(adev->firmware.fw_buf_ptr, 0, adev->firmware.fw_size);
> +		}
> +	}
> +}
> +
> +void amdgpu_ucode_free_bo(struct amdgpu_device *adev)
> +{
> +	if (adev->firmware.load_type != AMDGPU_FW_LOAD_DIRECT)
> +		amdgpu_bo_free_kernel(&adev->firmware.fw_buf,
> +		&adev->firmware.fw_buf_mc,
> +		&adev->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,
> -					&adev->firmware.fw_buf,
> -					&adev->firmware.fw_buf_mc,
> -					&adev->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 +488,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 +506,5 @@ int amdgpu_ucode_fini_bo(struct amdgpu_device *adev)
>   		}
>   	}
>   
> -	amdgpu_bo_free_kernel(&adev->firmware.fw_buf,
> -				&adev->firmware.fw_buf_mc,
> -				&adev->firmware.fw_buf_ptr);
> -
>   	return 0;
>   }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h
> index 8f3f111..4c0e5be 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h
> @@ -279,6 +279,9 @@ bool amdgpu_ucode_hdr_version(union amdgpu_firmware_header *hdr,
>   int amdgpu_ucode_init_bo(struct amdgpu_device *adev);
>   int amdgpu_ucode_fini_bo(struct amdgpu_device *adev);
>   
> +void amdgpu_ucode_create_bo(struct amdgpu_device *adev);
> +void amdgpu_ucode_free_bo(struct amdgpu_device *adev);
> +
>   enum amdgpu_firmware_load_type
>   amdgpu_ucode_get_load_type(struct amdgpu_device *adev, int load_type);
>   

_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx