Re: [PATCH 1/2] drm/xe/uc: Include patch version in expectations

["Welty, Brian" <brian.welty@intel.com>]

On 1/22/2024 1:37 PM, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> Patch level releases can be just as important as major level releases
> if they fix a critical bug. So include the patch version in the
> expectation check so the user is properly informed if they need to
> update.

Reviewed-by: Brian Welty <brian.welty@intel.com>

> 
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
>   drivers/gpu/drm/xe/xe_uc_fw.c | 49 +++++++++++++++++++----------------
>   1 file changed, 27 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_uc_fw.c b/drivers/gpu/drm/xe/xe_uc_fw.c
> index 9dff96dfe455..d3f0fe2101a7 100644
> --- a/drivers/gpu/drm/xe/xe_uc_fw.c
> +++ b/drivers/gpu/drm/xe/xe_uc_fw.c
> @@ -92,6 +92,7 @@ struct uc_fw_entry {
>   		const char *path;
>   		u16 major;
>   		u16 minor;
> +		u16 patch;
>   		bool full_ver_required;
>   	};
>   };
> @@ -102,14 +103,14 @@ struct fw_blobs_by_type {
>   };
>   
>   #define XE_GUC_FIRMWARE_DEFS(fw_def, mmp_ver, major_ver)			\
> -	fw_def(METEORLAKE,	major_ver(i915,	guc,	mtl,	70, 7))		\
> -	fw_def(DG2,		major_ver(i915,	guc,	dg2,	70, 5))		\
> -	fw_def(DG1,		major_ver(i915,	guc,	dg1,	70, 5))		\
> -	fw_def(ALDERLAKE_N,	major_ver(i915,	guc,	tgl,	70, 5))		\
> -	fw_def(ALDERLAKE_P,	major_ver(i915,	guc,	adlp,	70, 5))		\
> -	fw_def(ALDERLAKE_S,	major_ver(i915,	guc,	tgl,	70, 5))		\
> -	fw_def(ROCKETLAKE,	major_ver(i915,	guc,	tgl,	70, 5))		\
> -	fw_def(TIGERLAKE,	major_ver(i915,	guc,	tgl,	70, 5))
> +	fw_def(METEORLAKE,	major_ver(i915,	guc,	mtl,	70, 7, 0))	\
> +	fw_def(DG2,		major_ver(i915,	guc,	dg2,	70, 5, 0))	\
> +	fw_def(DG1,		major_ver(i915,	guc,	dg1,	70, 5, 0))	\
> +	fw_def(ALDERLAKE_N,	major_ver(i915,	guc,	tgl,	70, 5, 0))	\
> +	fw_def(ALDERLAKE_P,	major_ver(i915,	guc,	adlp,	70, 5, 0))	\
> +	fw_def(ALDERLAKE_S,	major_ver(i915,	guc,	tgl,	70, 5, 0))	\
> +	fw_def(ROCKETLAKE,	major_ver(i915,	guc,	tgl,	70, 5, 0))	\
> +	fw_def(TIGERLAKE,	major_ver(i915,	guc,	tgl,	70, 5, 0))
>   
>   #define XE_HUC_FIRMWARE_DEFS(fw_def, mmp_ver, no_ver)		\
>   	fw_def(METEORLAKE,	no_ver(i915,	huc_gsc,	mtl))		\
> @@ -121,24 +122,24 @@ struct fw_blobs_by_type {
>   
>   /* for the GSC FW we match the compatibility version and not the release one */
>   #define XE_GSC_FIRMWARE_DEFS(fw_def, major_ver)		\
> -	fw_def(METEORLAKE,	major_ver(i915,	gsc,	mtl,	1, 0))
> +	fw_def(METEORLAKE,	major_ver(i915,	gsc,	mtl,	1, 0, 0))
>   
>   #define MAKE_FW_PATH(dir__, uc__, shortname__, version__)			\
>   	__stringify(dir__) "/" __stringify(shortname__) "_" __stringify(uc__) version__ ".bin"
>   
>   #define fw_filename_mmp_ver(dir_, uc_, shortname_, a, b, c)			\
>   	MAKE_FW_PATH(dir_, uc_, shortname_, "_" __stringify(a ## . ## b ## . ## c))
> -#define fw_filename_major_ver(dir_, uc_, shortname_, a, b)			\
> +#define fw_filename_major_ver(dir_, uc_, shortname_, a, b, c)			\
>   	MAKE_FW_PATH(dir_, uc_, shortname_, "_" __stringify(a))
>   #define fw_filename_no_ver(dir_, uc_, shortname_)				\
>   	MAKE_FW_PATH(dir_, uc_, shortname_, "")
>   
>   #define uc_fw_entry_mmp_ver(dir_, uc_, shortname_, a, b, c)			\
>   	{ fw_filename_mmp_ver(dir_, uc_, shortname_, a, b, c),			\
> -	  a, b, true }
> -#define uc_fw_entry_major_ver(dir_, uc_, shortname_, a, b)			\
> -	{ fw_filename_major_ver(dir_, uc_, shortname_, a, b),			\
> -	  a, b }
> +	  a, b, c, true }
> +#define uc_fw_entry_major_ver(dir_, uc_, shortname_, a, b, c)			\
> +	{ fw_filename_major_ver(dir_, uc_, shortname_, a, b, c),		\
> +	  a, b, c }
>   #define uc_fw_entry_no_ver(dir_, uc_, shortname_)				\
>   	{ fw_filename_no_ver(dir_, uc_, shortname_),				\
>   	  0, 0 }
> @@ -221,6 +222,7 @@ uc_fw_auto_select(struct xe_device *xe, struct xe_uc_fw *uc_fw)
>   			uc_fw->path = entries[i].path;
>   			uc_fw->versions.wanted.major = entries[i].major;
>   			uc_fw->versions.wanted.minor = entries[i].minor;
> +			uc_fw->versions.wanted.patch = entries[i].patch;
>   			uc_fw->full_ver_required = entries[i].full_ver_required;
>   
>   			if (uc_fw->type == XE_UC_FW_TYPE_GSC)
> @@ -340,19 +342,22 @@ int xe_uc_fw_check_version_requirements(struct xe_uc_fw *uc_fw)
>   	 * Otherwise, at least the major version.
>   	 */
>   	if (wanted->major != found->major ||
> -	    (uc_fw->full_ver_required && wanted->minor != found->minor)) {
> -		drm_notice(&xe->drm, "%s firmware %s: unexpected version: %u.%u != %u.%u\n",
> +	    (uc_fw->full_ver_required &&
> +	     ((wanted->minor != found->minor) ||
> +	      (wanted->patch != found->patch)))) {
> +		drm_notice(&xe->drm, "%s firmware %s: unexpected version: %u.%u.%u != %u.%u.%u\n",
>   			   xe_uc_fw_type_repr(uc_fw->type), uc_fw->path,
> -			   found->major, found->minor,
> -			   wanted->major, wanted->minor);
> +			   found->major, found->minor, found->patch,
> +			   wanted->major, wanted->minor, wanted->patch);
>   		goto fail;
>   	}
>   
> -	if (wanted->minor > found->minor) {
> -		drm_notice(&xe->drm, "%s firmware (%u.%u) is recommended, but only (%u.%u) was found in %s\n",
> +	if (wanted->minor > found->minor ||
> +	    (wanted->minor == found->minor && wanted->patch > found->patch)) {
> +		drm_notice(&xe->drm, "%s firmware (%u.%u.%u) is recommended, but only (%u.%u.%u) was found in %s\n",
>   			   xe_uc_fw_type_repr(uc_fw->type),
> -			   wanted->major, wanted->minor,
> -			   found->major, found->minor,
> +			   wanted->major, wanted->minor, wanted->patch,
> +			   found->major, found->minor, found->patch,
>   			   uc_fw->path);
>   		drm_info(&xe->drm, "Consider updating your linux-firmware pkg or downloading from %s\n",
>   			 XE_UC_FIRMWARE_URL);