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@xxxxxxx>
> ---
> 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
>
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
