Re: [Intel-gfx] [PATCH 3/6] drm/i915/uc: Track patch level versions on reduced version firmware files

["Ceraolo Spurio, Daniele" <daniele.ceraolospurio@intel.com>]

On 4/20/2023 6:15 PM, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> When reduced version firmware files were added (matching major
> component being the only strict requirement), the minor version was
> still tracked and a notification reported if it was older. However,
> the patch version should really be tracked as well for the same
> reasons. The KMD can work without the change but if the effort has
> been taken to release a new firmware with the change then there must
> be a valid reason for doing so - important bug fix, security fix, etc.
> And in that case it would be good to alert the user if they are
> missing out on that new fix.
>
> v2: Use correct patch version number and drop redunant debug print
> (review by Daniele / CI results).
>
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Daniele

> ---
>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 30 +++++++++++++++---------
>   1 file changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> index a82a53dbbc86d..c9cd9bb47577f 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> @@ -80,14 +80,14 @@ void intel_uc_fw_change_status(struct intel_uc_fw *uc_fw,
>    */
>   #define INTEL_GUC_FIRMWARE_DEFS(fw_def, guc_maj, guc_mmp) \
>   	fw_def(METEORLAKE,   0, guc_mmp(mtl,  70, 6, 5)) \
> -	fw_def(DG2,          0, guc_maj(dg2,  70, 5)) \
> -	fw_def(ALDERLAKE_P,  0, guc_maj(adlp, 70, 5)) \
> +	fw_def(DG2,          0, guc_maj(dg2,  70, 5, 1)) \
> +	fw_def(ALDERLAKE_P,  0, guc_maj(adlp, 70, 5, 1)) \
>   	fw_def(ALDERLAKE_P,  0, guc_mmp(adlp, 70, 1, 1)) \
>   	fw_def(ALDERLAKE_P,  0, guc_mmp(adlp, 69, 0, 3)) \
> -	fw_def(ALDERLAKE_S,  0, guc_maj(tgl,  70, 5)) \
> +	fw_def(ALDERLAKE_S,  0, guc_maj(tgl,  70, 5, 1)) \
>   	fw_def(ALDERLAKE_S,  0, guc_mmp(tgl,  70, 1, 1)) \
>   	fw_def(ALDERLAKE_S,  0, guc_mmp(tgl,  69, 0, 3)) \
> -	fw_def(DG1,          0, guc_maj(dg1,  70, 5)) \
> +	fw_def(DG1,          0, guc_maj(dg1,  70, 5, 1)) \
>   	fw_def(ROCKETLAKE,   0, guc_mmp(tgl,  70, 1, 1)) \
>   	fw_def(TIGERLAKE,    0, guc_mmp(tgl,  70, 1, 1)) \
>   	fw_def(JASPERLAKE,   0, guc_mmp(ehl,  70, 1, 1)) \
> @@ -141,7 +141,7 @@ void intel_uc_fw_change_status(struct intel_uc_fw *uc_fw,
>   	__stringify(patch_) ".bin"
>   
>   /* Minor for internal driver use, not part of file name */
> -#define MAKE_GUC_FW_PATH_MAJOR(prefix_, major_, minor_) \
> +#define MAKE_GUC_FW_PATH_MAJOR(prefix_, major_, minor_, patch_) \
>   	__MAKE_UC_FW_PATH_MAJOR(prefix_, "guc", major_)
>   
>   #define MAKE_GUC_FW_PATH_MMP(prefix_, major_, minor_, patch_) \
> @@ -197,9 +197,9 @@ struct __packed uc_fw_blob {
>   	{ UC_FW_BLOB_BASE(major_, minor_, patch_, path_) \
>   	  .legacy = true }
>   
> -#define GUC_FW_BLOB(prefix_, major_, minor_) \
> -	UC_FW_BLOB_NEW(major_, minor_, 0, false, \
> -		       MAKE_GUC_FW_PATH_MAJOR(prefix_, major_, minor_))
> +#define GUC_FW_BLOB(prefix_, major_, minor_, patch_) \
> +	UC_FW_BLOB_NEW(major_, minor_, patch_, false, \
> +		       MAKE_GUC_FW_PATH_MAJOR(prefix_, major_, minor_, patch_))
>   
>   #define GUC_FW_BLOB_MMP(prefix_, major_, minor_, patch_) \
>   	UC_FW_BLOB_OLD(major_, minor_, patch_, \
> @@ -296,6 +296,7 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw)
>   		uc_fw->file_wanted.path = blob->path;
>   		uc_fw->file_wanted.ver.major = blob->major;
>   		uc_fw->file_wanted.ver.minor = blob->minor;
> +		uc_fw->file_wanted.ver.patch = blob->patch;
>   		uc_fw->loaded_via_gsc = blob->loaded_via_gsc;
>   		found = true;
>   		break;
> @@ -790,6 +791,9 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
>   		} else {
>   			if (uc_fw->file_selected.ver.minor < uc_fw->file_wanted.ver.minor)
>   				old_ver = true;
> +			else if ((uc_fw->file_selected.ver.minor == uc_fw->file_wanted.ver.minor) &&
> +				 (uc_fw->file_selected.ver.patch < uc_fw->file_wanted.ver.patch))
> +				old_ver = true;
>   		}
>   	}
>   
> @@ -797,12 +801,16 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
>   		/* Preserve the version that was really wanted */
>   		memcpy(&uc_fw->file_wanted, &file_ideal, sizeof(uc_fw->file_wanted));
>   
> -		gt_notice(gt, "%s firmware %s (%d.%d) is recommended, but only %s (%d.%d) was found\n",
> +		gt_notice(gt, "%s firmware %s (%d.%d.%d) is recommended, but only %s (%d.%d.%d) was found\n",
>   			  intel_uc_fw_type_repr(uc_fw->type),
>   			  uc_fw->file_wanted.path,
> -			  uc_fw->file_wanted.ver.major, uc_fw->file_wanted.ver.minor,
> +			  uc_fw->file_wanted.ver.major,
> +			  uc_fw->file_wanted.ver.minor,
> +			  uc_fw->file_wanted.ver.patch,
>   			  uc_fw->file_selected.path,
> -			  uc_fw->file_selected.ver.major, uc_fw->file_selected.ver.minor);
> +			  uc_fw->file_selected.ver.major,
> +			  uc_fw->file_selected.ver.minor,
> +			  uc_fw->file_selected.ver.patch);
>   		gt_info(gt, "Consider updating your linux-firmware pkg or downloading from %s\n",
>   			INTEL_UC_FIRMWARE_URL);
>   	}