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 <[email protected]> > --- > 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 [email protected] https://lists.freedesktop.org/mailman/listinfo/amd-gfx
