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