[PATCH 4/8] drm/amdgpu: add helper function to init asd ucode

[Luben Tuikov]

On 2020-04-20 6:16 a.m., Hawking Zhang wrote:
> asd is unified ucode across asic. it is not necessary
> to keep its software structure to be ip specific one

Sentences usually start with a capital letter.
"ASD is unified uCode across ASICs."
The second sentence uses "it" twice and it is not clear
whose software structure?
ip --> IP and the sentence should end with a period.

> 
> Signed-off-by: Hawking Zhang <Hawking.Zhang at amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 36 +++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h |  2 ++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> index 7797065..3656068 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> @@ -1840,6 +1840,42 @@ int psp_ring_cmd_submit(struct psp_context *psp,
>  	return 0;
>  }
>  
> +int psp_init_asd_microcode(struct psp_context *psp,
> +			   const char *chip_name)
> +{
> +	struct amdgpu_device *adev = psp->adev;
> +	char fw_name[30];

I'm not sure that that length is going to be enough in all contingencies.
/lib/firmware and /usr/lib/firmware are indeed the same inode (hard link),
but if/when using /usr/lib/firmware as opposed to /lib/firmware, some names
for ASD firmware are at 40 or more characters:

$for F in /usr/lib/firmware/amdgpu/*asd*; do LL=`echo $F | wc -c`; echo $LL : $F ; done
42 : /usr/lib/firmware/amdgpu/arcturus_asd.bin
40 : /usr/lib/firmware/amdgpu/navi10_asd.bin
40 : /usr/lib/firmware/amdgpu/navi12_asd.bin
40 : /usr/lib/firmware/amdgpu/navi14_asd.bin
41 : /usr/lib/firmware/amdgpu/picasso_asd.bin
40 : /usr/lib/firmware/amdgpu/raven2_asd.bin
39 : /usr/lib/firmware/amdgpu/raven_asd.bin
40 : /usr/lib/firmware/amdgpu/renoir_asd.bin
40 : /usr/lib/firmware/amdgpu/vega10_asd.bin
40 : /usr/lib/firmware/amdgpu/vega12_asd.bin
40 : /usr/lib/firmware/amdgpu/vega20_asd.bin

And when using /lib/firmware, the above line lengths are less by 4 characters,
which leaves it too close for comfort given that ASIC names could vary.

So, I'd rather see the size of the path name be something larger,
say 50, or more.

> +	const struct psp_firmware_header_v1_0 *asd_hdr;
> +	int err = 0;
> +
> +	if (!chip_name) {
> +		dev_err(adev->dev, "invalid chip name for asd microcode\n");

Here, "chip_name" is not "invalid"--it's NULL. The message
printed in the kernel logs should be more clear:

"No chip name given for ASD microcode."

> +		return -EINVAL;
> +	}
> +
> +	snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_asd.bin", chip_name);
> +	err = request_firmware(&adev->psp.asd_fw, fw_name, adev->dev);
> +	if (err)
> +		goto out;
> +
> +	err = amdgpu_ucode_validate(adev->psp.asd_fw);
> +	if (err)
> +		goto out;
> +
> +	asd_hdr = (const struct psp_firmware_header_v1_0 *)adev->psp.asd_fw->data;
> +	adev->psp.asd_fw_version = le32_to_cpu(asd_hdr->header.ucode_version);
> +	adev->psp.asd_feature_version = le32_to_cpu(asd_hdr->ucode_feature_version);
> +	adev->psp.asd_ucode_size = le32_to_cpu(asd_hdr->header.ucode_size_bytes);
> +	adev->psp.asd_start_addr = (uint8_t *)asd_hdr +
> +				le32_to_cpu(asd_hdr->header.ucode_array_offset_bytes);
> +	return 0;
> +out:
> +	dev_err(adev->dev, "fail to initialize asd microcode\n");

This message is vague, in both counts: load and validate.
The rest of the driver prints something like "failed to load %s firmware", fw_name,
which is more descriptive an_ it shows the file which failed to be loaded
or validated (giving the user a visual check of the name).

For instance, see gfx_v10_0.v at the bottom of "..._init_microcode()" function.

Print something like,

	dev_err(adev->dev, "psp: Failed to load firmware \"%s\"\n", fw_name);

Regards,
Luben

> +	release_firmware(adev->psp.asd_fw);
> +	adev->psp.asd_fw = NULL;
> +	return err;
> +}
> +
>  static int psp_set_clockgating_state(void *handle,
>  				     enum amd_clockgating_state state)
>  {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
> index f8b1f03..a763148 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
> @@ -385,4 +385,6 @@ int psp_ring_cmd_submit(struct psp_context *psp,
>  			uint64_t cmd_buf_mc_addr,
>  			uint64_t fence_mc_addr,
>  			int index);
> +int psp_init_asd_microcode(struct psp_context *psp,
> +			   const char *chip_name);
>  #endif
>