Am 21.08.2017 um 11:35 schrieb Quan, Evan: > Hi Christian, > > On the 1st run, it goes for amdgpu/%s_mec2_2.bin. Then it will goes for amdgpu/%s_mec2.bin if failed. > So, i did not see any problem with it. Ah, now I see. The naming of the firmware files is a bit confusing, but should indeed work. > Actually i talked with firmware guys. There is no change for the firmware loading way. > The new firmwares depends on a critical feature(unhalt MEC) which we lost before. We added that feature. > But considering the possible use case: old kernel + new firmwares, we decide to give the new firmwares other names. Makes sense. In this case the patch is Acked-by: Christian König <christian.koenig at amd.com>. Regards, Christian. > > Regards, > Evan >> -----Original Message----- >> From: Christian König [mailto:deathsimple at vodafone.de] >> Sent: Monday, August 21, 2017 4:18 PM >> To: Quan, Evan <Evan.Quan at amd.com>; amd-gfx at lists.freedesktop.org >> Subject: Re: [PATCH 2/2] drm/amdgpu: support polaris10/11/12 new cp firmwares >> >> Am 21.08.2017 um 03:39 schrieb Evan Quan: >>> Newer versions of the CP firmware require changes in how the driver >>> initializes the hw block. >>> Change the firmware name for new firmware to maintain compatibility with >>> older kernels. >>> >>> Change-Id: I32e3382768a2d9ff1e2978bcadb3bd44afb3db01 >>> Signed-off-by: Evan Quan <evan.quan at amd.com> >>> --- >>> drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c | 65 +++++++++++++++++++++++++++++- >> ----- >>> 1 file changed, 55 insertions(+), 10 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c >> b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c >>> index 38f70a1..6f2901a 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c >>> @@ -918,8 +918,17 @@ static int gfx_v8_0_init_microcode(struct amdgpu_device *adev) >>> BUG(); >>> } >>> >>> - snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_pfp.bin", chip_name); >>> - err = request_firmware(&adev->gfx.pfp_fw, fw_name, adev->dev); >>> + if (adev->asic_type >= CHIP_POLARIS10 && adev->asic_type <= CHIP_POLARIS12) >> { >>> + snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_pfp_2.bin", chip_name); >>> + err = request_firmware(&adev->gfx.pfp_fw, fw_name, adev->dev); >>> + if (err == -ENOENT) { >>> + snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_pfp.bin", >> chip_name); >>> + err = request_firmware(&adev->gfx.pfp_fw, fw_name, adev->dev); >>> + } >>> + } else { >>> + snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_pfp.bin", chip_name); >>> + err = request_firmware(&adev->gfx.pfp_fw, fw_name, adev->dev); >>> + } >>> if (err) >>> goto out; >>> err = amdgpu_ucode_validate(adev->gfx.pfp_fw); >>> @@ -929,8 +938,17 @@ static int gfx_v8_0_init_microcode(struct amdgpu_device *adev) >>> adev->gfx.pfp_fw_version = le32_to_cpu(cp_hdr->header.ucode_version); >>> adev->gfx.pfp_feature_version = le32_to_cpu(cp_hdr->ucode_feature_version); >>> >>> - snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_me.bin", chip_name); >>> - err = request_firmware(&adev->gfx.me_fw, fw_name, adev->dev); >>> + if (adev->asic_type >= CHIP_POLARIS10 && adev->asic_type <= CHIP_POLARIS12) >> { >>> + snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_me_2.bin", chip_name); >>> + err = request_firmware(&adev->gfx.me_fw, fw_name, adev->dev); >>> + if (err == -ENOENT) { >>> + snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_me.bin", >> chip_name); >>> + err = request_firmware(&adev->gfx.me_fw, fw_name, adev->dev); >>> + } >>> + } else { >>> + snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_me.bin", chip_name); >>> + err = request_firmware(&adev->gfx.me_fw, fw_name, adev->dev); >>> + } >>> if (err) >>> goto out; >>> err = amdgpu_ucode_validate(adev->gfx.me_fw); >>> @@ -941,8 +959,17 @@ static int gfx_v8_0_init_microcode(struct amdgpu_device *adev) >>> >>> adev->gfx.me_feature_version = le32_to_cpu(cp_hdr->ucode_feature_version); >>> >>> - snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_ce.bin", chip_name); >>> - err = request_firmware(&adev->gfx.ce_fw, fw_name, adev->dev); >>> + if (adev->asic_type >= CHIP_POLARIS10 && adev->asic_type <= CHIP_POLARIS12) >> { >>> + snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_ce_2.bin", chip_name); >>> + err = request_firmware(&adev->gfx.ce_fw, fw_name, adev->dev); >>> + if (err == -ENOENT) { >>> + snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_ce.bin", >> chip_name); >>> + err = request_firmware(&adev->gfx.ce_fw, fw_name, adev->dev); >>> + } >>> + } else { >>> + snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_ce.bin", chip_name); >>> + err = request_firmware(&adev->gfx.ce_fw, fw_name, adev->dev); >>> + } >>> if (err) >>> goto out; >>> err = amdgpu_ucode_validate(adev->gfx.ce_fw); >>> @@ -1012,8 +1039,17 @@ static int gfx_v8_0_init_microcode(struct amdgpu_device >> *adev) >>> for (i = 0 ; i < (rlc_hdr->reg_list_size_bytes >> 2); i++) >>> adev->gfx.rlc.register_restore[i] = le32_to_cpu(tmp[i]); >>> >>> - snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_mec.bin", chip_name); >>> - err = request_firmware(&adev->gfx.mec_fw, fw_name, adev->dev); >>> + if (adev->asic_type >= CHIP_POLARIS10 && adev->asic_type <= CHIP_POLARIS12) >> { >>> + snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_mec_2.bin", chip_name); >>> + err = request_firmware(&adev->gfx.mec_fw, fw_name, adev->dev); >>> + if (err == -ENOENT) { >>> + snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_mec.bin", >> chip_name); >>> + err = request_firmware(&adev->gfx.mec_fw, fw_name, adev->dev); >>> + } >>> + } else { >>> + snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_mec.bin", chip_name); >>> + err = request_firmware(&adev->gfx.mec_fw, fw_name, adev->dev); >>> + } >>> if (err) >>> goto out; >>> err = amdgpu_ucode_validate(adev->gfx.mec_fw); >>> @@ -1025,8 +1061,17 @@ static int gfx_v8_0_init_microcode(struct amdgpu_device >> *adev) >>> if ((adev->asic_type != CHIP_STONEY) && >>> (adev->asic_type != CHIP_TOPAZ)) { >>> - snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_mec2.bin", chip_name); >>> - err = request_firmware(&adev->gfx.mec2_fw, fw_name, adev->dev); >>> + if (adev->asic_type >= CHIP_POLARIS10 && adev->asic_type <= >> CHIP_POLARIS12) { >>> + snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_mec2_2.bin", >> chip_name); >>> + err = request_firmware(&adev->gfx.mec2_fw, fw_name, adev->dev); >>> + if (err == -ENOENT) { >>> + snprintf(fw_name, sizeof(fw_name), >> "amdgpu/%s_mec2.bin", chip_name); >>> + err = request_firmware(&adev->gfx.mec2_fw, fw_name, >> adev->dev); >> >> That chunk above looks incorrect to me. You just query >> "amdgpu/%s_mec2.bin" twice. >> >> Apart from that do we really need that? Couldn't we just push back on >> the firmware guys to start the MEC after loading like in older versions? >> >> Regards, >> Christian. >> >>> + } >>> + } else { >>> + snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_mec2.bin", >> chip_name); >>> + err = request_firmware(&adev->gfx.mec2_fw, fw_name, adev->dev); >>> + } >>> if (!err) { >>> err = amdgpu_ucode_validate(adev->gfx.mec2_fw); >>> if (err)
