On 2020-04-20 11:29 a.m., Luben Tuikov wrote:
> 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);
Ah, I see you're only printing "amdgpu/%s_asd.bin", so I guess 30 chars
should fit.
Regards,
Luben
>> + 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cluben.tuikov%40amd.com%7C3bf4ce6df8a348e735a608d7e53fada3%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637229933975152085&sdata=Vlq1qsHb8ygT46RJgf9fHamzUGAFuWB3%2B1xKaI06H2o%3D&reserved=0
>
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
