Re: [PATCH] drm/amd: Fix MSB of SMU version printing

["Lazar, Lijo" <lijo.lazar@xxxxxxx>]

On 1/19/2022 8:34 AM, Mario Limonciello wrote:
> Yellow carp has been outputting versions like `1093.24.0`, but this
> is supposed to be 69.24.0. That is the MSB is being interpreted
> incorrectly.
>
> The MSB is not part of the major version, but has generally been
> treated that way thus far.  It's actually the program, and used to
> distinguish between two programs from a similar family but different
> codebase.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>

Reviewed-by: Lijo Lazar <lijo.lazar@xxxxxxx>

Thanks,
Lijo

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c        | 10 +++++-----
>   drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c | 10 +++++-----
>   drivers/gpu/drm/amd/pm/swsmu/smu12/smu_v12_0.c | 10 +++++-----
>   drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c | 14 +++++++-------
>   4 files changed, 22 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index f3c864223033..ae793c648f5d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -1431,8 +1431,7 @@ static int amdgpu_debugfs_firmware_info_show(struct seq_file *m, void *unused)
>   	struct drm_amdgpu_info_firmware fw_info;
>   	struct drm_amdgpu_query_fw query_fw;
>   	struct atom_context *ctx = adev->mode_info.atom_context;
> -	uint8_t smu_minor, smu_debug;
> -	uint16_t smu_major;
> +	uint8_t smu_program, smu_major, smu_minor, smu_debug;
>   	int ret, i;
>   
>   	static const char *ta_fw_name[TA_FW_TYPE_MAX_INDEX] = {
> @@ -1578,11 +1577,12 @@ static int amdgpu_debugfs_firmware_info_show(struct seq_file *m, void *unused)
>   	ret = amdgpu_firmware_info(&fw_info, &query_fw, adev);
>   	if (ret)
>   		return ret;
> -	smu_major = (fw_info.ver >> 16) & 0xffff;
> +	smu_program = (fw_info.ver >> 24) & 0xff;
> +	smu_major = (fw_info.ver >> 16) & 0xff;
>   	smu_minor = (fw_info.ver >> 8) & 0xff;
>   	smu_debug = (fw_info.ver >> 0) & 0xff;
> -	seq_printf(m, "SMC feature version: %u, firmware version: 0x%08x (%d.%d.%d)\n",
> -		   fw_info.feature, fw_info.ver, smu_major, smu_minor, smu_debug);
> +	seq_printf(m, "SMC feature version: %u, program: %d, firmware version: 0x%08x (%d.%d.%d)\n",
> +		   fw_info.feature, smu_program, fw_info.ver, smu_major, smu_minor, smu_debug);
>   
>   	/* SDMA */
>   	query_fw.fw_type = AMDGPU_INFO_FW_SDMA;
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> index 7029e5deb6b3..e94a400db669 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> @@ -225,15 +225,15 @@ int smu_v11_0_check_fw_version(struct smu_context *smu)
>   {
>   	struct amdgpu_device *adev = smu->adev;
>   	uint32_t if_version = 0xff, smu_version = 0xff;
> -	uint16_t smu_major;
> -	uint8_t smu_minor, smu_debug;
> +	uint8_t smu_program, smu_major, smu_minor, smu_debug;
>   	int ret = 0;
>   
>   	ret = smu_cmn_get_smc_version(smu, &if_version, &smu_version);
>   	if (ret)
>   		return ret;
>   
> -	smu_major = (smu_version >> 16) & 0xffff;
> +	smu_program = (smu_version >> 24) & 0xff;
> +	smu_major = (smu_version >> 16) & 0xff;
>   	smu_minor = (smu_version >> 8) & 0xff;
>   	smu_debug = (smu_version >> 0) & 0xff;
>   	if (smu->is_apu)
> @@ -287,9 +287,9 @@ int smu_v11_0_check_fw_version(struct smu_context *smu)
>   	 */
>   	if (if_version != smu->smc_driver_if_version) {
>   		dev_info(smu->adev->dev, "smu driver if version = 0x%08x, smu fw if version = 0x%08x, "
> -			"smu fw version = 0x%08x (%d.%d.%d)\n",
> +			"smu fw program = %d, version = 0x%08x (%d.%d.%d)\n",
>   			smu->smc_driver_if_version, if_version,
> -			smu_version, smu_major, smu_minor, smu_debug);
> +			smu_program, smu_version, smu_major, smu_minor, smu_debug);
>   		dev_warn(smu->adev->dev, "SMU driver if version not matched\n");
>   	}
>   
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu12/smu_v12_0.c b/drivers/gpu/drm/amd/pm/swsmu/smu12/smu_v12_0.c
> index 9c91e79c955f..56a02bc60cee 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu12/smu_v12_0.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu12/smu_v12_0.c
> @@ -74,15 +74,15 @@ int smu_v12_0_check_fw_version(struct smu_context *smu)
>   {
>   	struct amdgpu_device *adev = smu->adev;
>   	uint32_t if_version = 0xff, smu_version = 0xff;
> -	uint16_t smu_major;
> -	uint8_t smu_minor, smu_debug;
> +	uint8_t smu_program, smu_major, smu_minor, smu_debug;
>   	int ret = 0;
>   
>   	ret = smu_cmn_get_smc_version(smu, &if_version, &smu_version);
>   	if (ret)
>   		return ret;
>   
> -	smu_major = (smu_version >> 16) & 0xffff;
> +	smu_program = (smu_version >> 24) & 0xff;
> +	smu_major = (smu_version >> 16) & 0xff;
>   	smu_minor = (smu_version >> 8) & 0xff;
>   	smu_debug = (smu_version >> 0) & 0xff;
>   	if (smu->is_apu)
> @@ -98,9 +98,9 @@ int smu_v12_0_check_fw_version(struct smu_context *smu)
>   	 */
>   	if (if_version != smu->smc_driver_if_version) {
>   		dev_info(smu->adev->dev, "smu driver if version = 0x%08x, smu fw if version = 0x%08x, "
> -			"smu fw version = 0x%08x (%d.%d.%d)\n",
> +			"smu fw program = %d, smu fw version = 0x%08x (%d.%d.%d)\n",
>   			smu->smc_driver_if_version, if_version,
> -			smu_version, smu_major, smu_minor, smu_debug);
> +			smu_program, smu_version, smu_major, smu_minor, smu_debug);
>   		dev_warn(smu->adev->dev, "SMU driver if version not matched\n");
>   	}
>   
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
> index f66d8b9135ca..d9e3ebfeee73 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
> @@ -198,15 +198,15 @@ int smu_v13_0_check_fw_version(struct smu_context *smu)
>   {
>   	struct amdgpu_device *adev = smu->adev;
>   	uint32_t if_version = 0xff, smu_version = 0xff;
> -	uint16_t smu_major;
> -	uint8_t smu_minor, smu_debug;
> +	uint8_t smu_program, smu_major, smu_minor, smu_debug;
>   	int ret = 0;
>   
>   	ret = smu_cmn_get_smc_version(smu, &if_version, &smu_version);
>   	if (ret)
>   		return ret;
>   
> -	smu_major = (smu_version >> 16) & 0xffff;
> +	smu_program = (smu_version >> 24) & 0xff;
> +	smu_major = (smu_version >> 16) & 0xff;
>   	smu_minor = (smu_version >> 8) & 0xff;
>   	smu_debug = (smu_version >> 0) & 0xff;
>   	if (smu->is_apu)
> @@ -229,8 +229,8 @@ int smu_v13_0_check_fw_version(struct smu_context *smu)
>   
>   	/* only for dGPU w/ SMU13*/
>   	if (adev->pm.fw)
> -		dev_dbg(adev->dev, "smu fw reported version = 0x%08x (%d.%d.%d)\n",
> -			 smu_version, smu_major, smu_minor, smu_debug);
> +		dev_dbg(smu->adev->dev, "smu fw reported program %d, version = 0x%08x (%d.%d.%d)\n",
> +			 smu_program, smu_version, smu_major, smu_minor, smu_debug);
>   
>   	/*
>   	 * 1. if_version mismatch is not critical as our fw is designed
> @@ -242,9 +242,9 @@ int smu_v13_0_check_fw_version(struct smu_context *smu)
>   	 */
>   	if (if_version != smu->smc_driver_if_version) {
>   		dev_info(adev->dev, "smu driver if version = 0x%08x, smu fw if version = 0x%08x, "
> -			 "smu fw version = 0x%08x (%d.%d.%d)\n",
> +			 "smu fw program = %d, smu fw version = 0x%08x (%d.%d.%d)\n",
>   			 smu->smc_driver_if_version, if_version,
> -			 smu_version, smu_major, smu_minor, smu_debug);
> +			 smu_program, smu_version, smu_major, smu_minor, smu_debug);
>   		dev_warn(adev->dev, "SMU driver if version not matched\n");
>   	}
>