Re: [PATCH v3 2/4] drm/xe/guc: Release GuC v70.29.2 for Xe as minimum required version

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

I'd reword the title a bit, maybe just something like:

"Set GuC v70.29.2 as minimum required version"

On 8/1/2024 3:42 PM, Julia Filipchuk wrote:
> The VF API version for this release is 1.13.4
>
> Bumped recommended versions for all platforms. Check for release version
> v70.29.2 as minimum suported.

This sounds like an inversion of order (i.e. the bumping of the 
recommended version is the consequence, not the cause). Also, you need 
to explain why we're doing this. Maybe reword it to something like:

"Bumping minimum required GuC version to v70.29.2 ahead of the 
force_probe removal; this way we're guaranteed that we'll never load any 
GuC older than that and therefore won't have to maintain the required 
support in the code. The recommended GuC version is also bumped to match."

>
> Add comparable version macro to xe_uc_fw.
>
> Signed-off-by: Julia Filipchuk <julia.filipchuk@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_guc.h   |  3 +++
>   drivers/gpu/drm/xe/xe_uc_fw.c | 26 ++++++++++++++------------
>   drivers/gpu/drm/xe/xe_uc_fw.h |  6 ++++++
>   3 files changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_guc.h b/drivers/gpu/drm/xe/xe_guc.h
> index e0bbf98f849d..9a0b9123dce0 100644
> --- a/drivers/gpu/drm/xe/xe_guc.h
> +++ b/drivers/gpu/drm/xe/xe_guc.h
> @@ -11,6 +11,9 @@
>   #include "xe_hw_engine_types.h"
>   #include "xe_macros.h"
>   
> +#define GUC_SUBMIT_VER(guc)   MAKE_VER_STRUCT((guc)->fw.versions.found[XE_UC_FW_VER_COMPATIBILITY])
> +#define GUC_FIRMWARE_VER(guc) MAKE_VER_STRUCT((guc)->fw.versions.found[XE_UC_FW_VER_RELEASE])
> +
>   struct drm_printer;
>   
>   void xe_guc_comm_init_early(struct xe_guc *guc);
> diff --git a/drivers/gpu/drm/xe/xe_uc_fw.c b/drivers/gpu/drm/xe/xe_uc_fw.c
> index 5b70d23724c4..fe1b97d3d2a0 100644
> --- a/drivers/gpu/drm/xe/xe_uc_fw.c
> +++ b/drivers/gpu/drm/xe/xe_uc_fw.c
> @@ -15,6 +15,7 @@
>   #include "xe_gsc.h"
>   #include "xe_gt.h"
>   #include "xe_gt_printk.h"
> +#include "xe_guc.h"
>   #include "xe_map.h"
>   #include "xe_mmio.h"
>   #include "xe_module.h"
> @@ -105,15 +106,15 @@ struct fw_blobs_by_type {
>   };
>   
>   #define XE_GUC_FIRMWARE_DEFS(fw_def, mmp_ver, major_ver)			\
> -	fw_def(LUNARLAKE,	major_ver(xe,	guc,	lnl,	70, 19, 2))	\
> -	fw_def(METEORLAKE,	major_ver(i915,	guc,	mtl,	70, 19, 2))	\
> -	fw_def(DG2,		major_ver(i915,	guc,	dg2,	70, 19, 2))	\
> -	fw_def(DG1,		major_ver(i915,	guc,	dg1,	70, 19, 2))	\
> -	fw_def(ALDERLAKE_N,	major_ver(i915,	guc,	tgl,	70, 19, 2))	\
> -	fw_def(ALDERLAKE_P,	major_ver(i915,	guc,	adlp,	70, 19, 2))	\
> -	fw_def(ALDERLAKE_S,	major_ver(i915,	guc,	tgl,	70, 19, 2))	\
> -	fw_def(ROCKETLAKE,	major_ver(i915,	guc,	tgl,	70, 19, 2))	\
> -	fw_def(TIGERLAKE,	major_ver(i915,	guc,	tgl,	70, 19, 2))
> +	fw_def(LUNARLAKE,	major_ver(xe,	guc,	lnl,	70, 29, 2))	\
> +	fw_def(METEORLAKE,	major_ver(i915,	guc,	mtl,	70, 29, 2))	\
> +	fw_def(DG2,		major_ver(i915,	guc,	dg2,	70, 29, 2))	\
> +	fw_def(DG1,		major_ver(i915,	guc,	dg1,	70, 29, 2))	\
> +	fw_def(ALDERLAKE_N,	major_ver(i915,	guc,	tgl,	70, 29, 2))	\
> +	fw_def(ALDERLAKE_P,	major_ver(i915,	guc,	adlp,	70, 29, 2))	\
> +	fw_def(ALDERLAKE_S,	major_ver(i915,	guc,	tgl,	70, 29, 2))	\
> +	fw_def(ROCKETLAKE,	major_ver(i915,	guc,	tgl,	70, 29, 2))	\
> +	fw_def(TIGERLAKE,	major_ver(i915,	guc,	tgl,	70, 29, 2))
>   
>   #define XE_HUC_FIRMWARE_DEFS(fw_def, mmp_ver, no_ver)		\
>   	fw_def(BATTLEMAGE,	no_ver(xe,	huc,		bmg))		\
> @@ -304,14 +305,15 @@ static void uc_fw_fini(struct drm_device *drm, void *arg)
>   static int guc_read_css_info(struct xe_uc_fw *uc_fw, struct uc_css_header *css)
>   {
>   	struct xe_gt *gt = uc_fw_to_gt(uc_fw);
> +	struct xe_guc *guc = container_of(uc_fw, struct xe_guc, fw);
>   	struct xe_uc_fw_version *release = &uc_fw->versions.found[XE_UC_FW_VER_RELEASE];
>   	struct xe_uc_fw_version *compatibility = &uc_fw->versions.found[XE_UC_FW_VER_COMPATIBILITY];
>   
>   	xe_gt_assert(gt, uc_fw->type == XE_UC_FW_TYPE_GUC);
>   
> -	/* We don't support GuC releases older than 70.19 */
> -	if (release->major < 70 || (release->major == 70 && release->minor < 19)) {
> -		xe_gt_err(gt, "Unsupported GuC v%u.%u! v70.19 or newer is required\n",
> +	/* We don't support GuC releases older than 70.29.2 */
> +	if (GUC_FIRMWARE_VER(guc) < MAKE_VER(70, 29, 2)) {

Here you could use MAKE_VER_STRUCT(release) instead of 
GUC_FIRMWARE_VER(guc) and avoid having to get the GUC variable

> +		xe_gt_err(gt, "Unsupported GuC v%u.%u! v70.29.2 or newer is required\n",
>   			  release->major, release->minor);

Should we print release->patch here as well, now that we're comparing 
down to the patch level?

>   		return -EINVAL;
>   	}
> diff --git a/drivers/gpu/drm/xe/xe_uc_fw.h b/drivers/gpu/drm/xe/xe_uc_fw.h
> index c108e9d08e70..1b1fdd103b9c 100644
> --- a/drivers/gpu/drm/xe/xe_uc_fw.h
> +++ b/drivers/gpu/drm/xe/xe_uc_fw.h
> @@ -12,6 +12,12 @@
>   #include "xe_uc_fw_abi.h"
>   #include "xe_uc_fw_types.h"
>   
> +/* Create a comparable u64 version number. Prevent truncation from smaller types. */
> +#define MAKE_VER(maj, min, pat) \
> +	(((u64)(maj) << 32) | ((u64)(min) << 16) | (pat))
> +
> +#define MAKE_VER_STRUCT(ver) MAKE_VER((ver).major, (ver).minor, (ver).patch)
> +

IIRC we had some issue with a similar macro in i915 when using u64 and 
doing a 32b build, but the casting you have should cover it.
However, IMO this macro shouldn't be here anyway because different FWs 
have different ways of comparing versions, so we can't have a generic 
MAKE_VER() that applies to all. I think it'd be better to have a 
GuC-specific macro, at which point we can use a u32 because the GuC 
versions is guaranteed to fit in that.

Also, I suggest to use less generic names to avoid a define clash.

Daniele

>   struct drm_printer;
>   
>   int xe_uc_fw_init(struct xe_uc_fw *uc_fw);