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

Mon Apr 20 15:32:42 UTC 2020

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 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);

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 at lists.freedesktop.org
> 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
> 