Re: [PATCH v4 07/27] drm/amd: Convert SDMA to use `amdgpu_ucode_ip_version_decode`

["Lazar, Lijo" <lijo.lazar@xxxxxxx>]

On 1/4/2023 3:48 AM, Mario Limonciello wrote:
> Simplifies the code so that all SDMA versions will get the firmware
> name from `amdgpu_ucode_ip_version_decode`.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> ---
> v3->v4:
>   * Move out of IP discovery and instead simplify early_init
> v2->v3:
>   * Fix dGPU naming scheme
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c |  7 ++-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h |  4 +-
>   drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c   | 47 +-------------------
>   drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c   | 30 +------------
>   drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c   | 55 +-----------------------
>   drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c   | 25 +----------
>   6 files changed, 13 insertions(+), 155 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c
> index 9e85a078d918..83e8f0dae647 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c
> @@ -200,15 +200,18 @@ void amdgpu_sdma_destroy_inst_ctx(struct amdgpu_device *adev,
>   }
>   
>   int amdgpu_sdma_init_microcode(struct amdgpu_device *adev,
> -			       char *fw_name, u32 instance,
> -			       bool duplicate)
> +			       u32 instance, bool duplicate)
>   {
>   	struct amdgpu_firmware_info *info = NULL;
>   	const struct common_firmware_header *header = NULL;
>   	int err = 0, i;
>   	const struct sdma_firmware_header_v2_0 *sdma_hdr;
>   	uint16_t version_major;
> +	char ucode_prefix[30];
> +	char fw_name[40];
>   
> +	amdgpu_ucode_ip_version_decode(adev, SDMA0_HWIP, ucode_prefix, sizeof(ucode_prefix));
> +	snprintf(fw_name, sizeof(fw_name), "amdgpu/%s%s.bin", ucode_prefix, !instance ? "" : "1");

It is safer to keep the original logic with instance number as suffix 
rather than hardcoding to 1.

Thanks,
Lijo

>   	err = amdgpu_ucode_load(adev, &adev->sdma.instance[instance].fw, fw_name);
>   	if (err)
>   		goto out;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h
> index 7d99205c2e01..2d16e6d36728 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h
> @@ -124,8 +124,8 @@ int amdgpu_sdma_process_ras_data_cb(struct amdgpu_device *adev,
>   int amdgpu_sdma_process_ecc_irq(struct amdgpu_device *adev,
>   				      struct amdgpu_irq_src *source,
>   				      struct amdgpu_iv_entry *entry);
> -int amdgpu_sdma_init_microcode(struct amdgpu_device *adev,
> -        char *fw_name, u32 instance, bool duplicate);
> +int amdgpu_sdma_init_microcode(struct amdgpu_device *adev, u32 instance,
> +			       bool duplicate);
>   void amdgpu_sdma_destroy_inst_ctx(struct amdgpu_device *adev,
>           bool duplicate);
>   void amdgpu_sdma_unset_buffer_funcs_helper(struct amdgpu_device *adev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> index 4d780e4430e7..017ae298558e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> @@ -575,60 +575,17 @@ static void sdma_v4_0_setup_ulv(struct amdgpu_device *adev)
>   // vega10 real chip need to use PSP to load firmware
>   static int sdma_v4_0_init_microcode(struct amdgpu_device *adev)
>   {
> -	const char *chip_name;
> -	char fw_name[30];
>   	int ret, i;
>   
> -	DRM_DEBUG("\n");
> -
> -	switch (adev->ip_versions[SDMA0_HWIP][0]) {
> -	case IP_VERSION(4, 0, 0):
> -		chip_name = "vega10";
> -		break;
> -	case IP_VERSION(4, 0, 1):
> -		chip_name = "vega12";
> -		break;
> -	case IP_VERSION(4, 2, 0):
> -		chip_name = "vega20";
> -		break;
> -	case IP_VERSION(4, 1, 0):
> -	case IP_VERSION(4, 1, 1):
> -		if (adev->apu_flags & AMD_APU_IS_RAVEN2)
> -			chip_name = "raven2";
> -		else if (adev->apu_flags & AMD_APU_IS_PICASSO)
> -			chip_name = "picasso";
> -		else
> -			chip_name = "raven";
> -		break;
> -	case IP_VERSION(4, 2, 2):
> -		chip_name = "arcturus";
> -		break;
> -	case IP_VERSION(4, 1, 2):
> -		if (adev->apu_flags & AMD_APU_IS_RENOIR)
> -			chip_name = "renoir";
> -		else
> -			chip_name = "green_sardine";
> -		break;
> -	case IP_VERSION(4, 4, 0):
> -		chip_name = "aldebaran";
> -		break;
> -	default:
> -		BUG();
> -	}
> -
>   	for (i = 0; i < adev->sdma.num_instances; i++) {
> -		if (i == 0)
> -			snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_sdma.bin", chip_name);
> -		else
> -			snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_sdma%d.bin", chip_name, i);
>   		if (adev->ip_versions[SDMA0_HWIP][0] == IP_VERSION(4, 2, 2) ||
>                       adev->ip_versions[SDMA0_HWIP][0] == IP_VERSION(4, 4, 0)) {
>   			/* Acturus & Aldebaran will leverage the same FW memory
>   			   for every SDMA instance */
> -			ret = amdgpu_sdma_init_microcode(adev, fw_name, 0, true);
> +			ret = amdgpu_sdma_init_microcode(adev, 0, true);
>   			break;
>   		} else {
> -			ret = amdgpu_sdma_init_microcode(adev, fw_name, i, false);
> +			ret = amdgpu_sdma_init_microcode(adev, i, false);
>   			if (ret)
>   				return ret;
>   		}
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c
> index d4d9f196db83..1941b3b7c5d9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c
> @@ -237,39 +237,13 @@ static void sdma_v5_0_init_golden_registers(struct amdgpu_device *adev)
>   // emulation only, won't work on real chip
>   // navi10 real chip need to use PSP to load firmware
>   static int sdma_v5_0_init_microcode(struct amdgpu_device *adev)
> -{
> -	const char *chip_name;
> -	char fw_name[40];
> -	int ret, i;
> +{	int ret, i;
>   
>   	if (amdgpu_sriov_vf(adev) && (adev->ip_versions[SDMA0_HWIP][0] == IP_VERSION(5, 0, 5)))
>   		return 0;
>   
> -	DRM_DEBUG("\n");
> -
> -	switch (adev->ip_versions[SDMA0_HWIP][0]) {
> -	case IP_VERSION(5, 0, 0):
> -		chip_name = "navi10";
> -		break;
> -	case IP_VERSION(5, 0, 2):
> -		chip_name = "navi14";
> -		break;
> -	case IP_VERSION(5, 0, 5):
> -		chip_name = "navi12";
> -		break;
> -	case IP_VERSION(5, 0, 1):
> -		chip_name = "cyan_skillfish2";
> -		break;
> -	default:
> -		BUG();
> -	}
> -
>   	for (i = 0; i < adev->sdma.num_instances; i++) {
> -		if (i == 0)
> -			snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_sdma.bin", chip_name);
> -		else
> -			snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_sdma1.bin", chip_name);
> -		ret = amdgpu_sdma_init_microcode(adev, fw_name, i, false);
> +		ret = amdgpu_sdma_init_microcode(adev, i, false);
>   		if (ret)
>   			return ret;
>   	}
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
> index 65e7a710298d..8e445eb9dd49 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
> @@ -89,59 +89,6 @@ static u32 sdma_v5_2_get_reg_offset(struct amdgpu_device *adev, u32 instance, u3
>   	return base + internal_offset;
>   }
>   
> -/**
> - * sdma_v5_2_init_microcode - load ucode images from disk
> - *
> - * @adev: amdgpu_device pointer
> - *
> - * Use the firmware interface to load the ucode images into
> - * the driver (not loaded into hw).
> - * Returns 0 on success, error on failure.
> - */
> -
> -// emulation only, won't work on real chip
> -// navi10 real chip need to use PSP to load firmware
> -static int sdma_v5_2_init_microcode(struct amdgpu_device *adev)
> -{
> -	const char *chip_name;
> -	char fw_name[40];
> -
> -	DRM_DEBUG("\n");
> -
> -	switch (adev->ip_versions[SDMA0_HWIP][0]) {
> -	case IP_VERSION(5, 2, 0):
> -		chip_name = "sienna_cichlid_sdma";
> -		break;
> -	case IP_VERSION(5, 2, 2):
> -		chip_name = "navy_flounder_sdma";
> -		break;
> -	case IP_VERSION(5, 2, 1):
> -		chip_name = "vangogh_sdma";
> -		break;
> -	case IP_VERSION(5, 2, 4):
> -		chip_name = "dimgrey_cavefish_sdma";
> -		break;
> -	case IP_VERSION(5, 2, 5):
> -		chip_name = "beige_goby_sdma";
> -		break;
> -	case IP_VERSION(5, 2, 3):
> -		chip_name = "yellow_carp_sdma";
> -		break;
> -	case IP_VERSION(5, 2, 6):
> -		chip_name = "sdma_5_2_6";
> -		break;
> -	case IP_VERSION(5, 2, 7):
> -		chip_name = "sdma_5_2_7";
> -		break;
> -	default:
> -		BUG();
> -	}
> -
> -	snprintf(fw_name, sizeof(fw_name), "amdgpu/%s.bin", chip_name);
> -
> -	return amdgpu_sdma_init_microcode(adev, fw_name, 0, true);
> -}
> -
>   static unsigned sdma_v5_2_ring_init_cond_exec(struct amdgpu_ring *ring)
>   {
>   	unsigned ret;
> @@ -1288,7 +1235,7 @@ static int sdma_v5_2_sw_init(void *handle)
>   			return r;
>   	}
>   
> -	r = sdma_v5_2_init_microcode(adev);
> +	r = amdgpu_sdma_init_microcode(adev, 0, true);
>   	if (r) {
>   		DRM_ERROR("Failed to load sdma firmware!\n");
>   		return r;
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c
> index 049c26a45d85..bf1fa5e8d2f9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c
> @@ -78,29 +78,6 @@ static u32 sdma_v6_0_get_reg_offset(struct amdgpu_device *adev, u32 instance, u3
>   	return base + internal_offset;
>   }
>   
> -/**
> - * sdma_v6_0_init_microcode - load ucode images from disk
> - *
> - * @adev: amdgpu_device pointer
> - *
> - * Use the firmware interface to load the ucode images into
> - * the driver (not loaded into hw).
> - * Returns 0 on success, error on failure.
> - */
> -static int sdma_v6_0_init_microcode(struct amdgpu_device *adev)
> -{
> -	char fw_name[30];
> -	char ucode_prefix[30];
> -
> -	DRM_DEBUG("\n");
> -
> -	amdgpu_ucode_ip_version_decode(adev, SDMA0_HWIP, ucode_prefix, sizeof(ucode_prefix));
> -
> -	snprintf(fw_name, sizeof(fw_name), "amdgpu/%s.bin", ucode_prefix);
> -
> -	return amdgpu_sdma_init_microcode(adev, fw_name, 0, true);
> -}
> -
>   static unsigned sdma_v6_0_ring_init_cond_exec(struct amdgpu_ring *ring)
>   {
>   	unsigned ret;
> @@ -1260,7 +1237,7 @@ static int sdma_v6_0_sw_init(void *handle)
>   	if (r)
>   		return r;
>   
> -	r = sdma_v6_0_init_microcode(adev);
> +	r = amdgpu_sdma_init_microcode(adev, 0, true);
>   	if (r) {
>   		DRM_ERROR("Failed to load sdma firmware!\n");
>   		return r;