Re: [PATCH 2/3] drm/xe/pf: Prepare for new threshold KLVs

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

[-- Attachment #1: Type: text/plain, Size: 7561 bytes --]



On 12/12/2025 9:34 AM, Michal Wajdeczko wrote:
> We want to extend our macro-based KLV list definitions with new
> information about the version from which given KLV is supported.
> Prepare our code generators to emit dedicated version check if
> a KLV was defined with the version information.
>
> Signed-off-by: Michal Wajdeczko<michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio<daniele.ceraolospurio@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c    | 20 ++++++++++++-------
>   drivers/gpu/drm/xe/xe_gt_sriov_pf_debugfs.c   |  9 ++++++---
>   drivers/gpu/drm/xe/xe_guc.h                   | 18 +++++++++++++++++
>   .../drm/xe/xe_guc_klv_thresholds_set_types.h  |  5 +++++
>   4 files changed, 42 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c b/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
> index 59c5c6b4d994..2d039f67c99d 100644
> --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
> @@ -269,7 +269,8 @@ static u32 encode_config_ggtt(u32 *cfg, const struct xe_gt_sriov_config *config,
>   }
>   
>   /* Return: number of configuration dwords written */
> -static u32 encode_config(u32 *cfg, const struct xe_gt_sriov_config *config, bool details)
> +static u32 encode_config(struct xe_gt *gt, u32 *cfg, const struct xe_gt_sriov_config *config,
> +			 bool details)
>   {
>   	u32 n = 0;
>   
> @@ -303,9 +304,11 @@ static u32 encode_config(u32 *cfg, const struct xe_gt_sriov_config *config, bool
>   	cfg[n++] = PREP_GUC_KLV_TAG(VF_CFG_PREEMPT_TIMEOUT);
>   	cfg[n++] = config->preempt_timeout;
>   
> -#define encode_threshold_config(TAG, ...) ({					\
> -	cfg[n++] = PREP_GUC_KLV_TAG(VF_CFG_THRESHOLD_##TAG);			\
> -	cfg[n++] = config->thresholds[MAKE_XE_GUC_KLV_THRESHOLD_INDEX(TAG)];	\
> +#define encode_threshold_config(TAG, NAME, VER...) ({						\
> +	if (IF_ARGS(xe_guc_supports_fw_version(&gt->uc.guc, MAKE_GUC_VER_ARGS(VER)), true, VER)) { \
> +		cfg[n++] = PREP_GUC_KLV_TAG(VF_CFG_THRESHOLD_##TAG);				\
> +		cfg[n++] = config->thresholds[MAKE_XE_GUC_KLV_THRESHOLD_INDEX(TAG)];		\
> +	}											\
>   });
>   
>   	MAKE_XE_GUC_KLV_THRESHOLDS_SET(encode_threshold_config);
> @@ -328,7 +331,7 @@ static int pf_push_full_vf_config(struct xe_gt *gt, unsigned int vfid)
>   		return -ENOBUFS;
>   
>   	cfg = xe_guc_buf_cpu_ptr(buf);
> -	num_dwords = encode_config(cfg, config, true);
> +	num_dwords = encode_config(gt, cfg, config, true);
>   	xe_gt_assert(gt, num_dwords<= max_cfg_dwords); if (xe_gt_is_media_type(gt)) { @@ -2518,7 +2521,7 
> @@ ssize_t xe_gt_sriov_pf_config_save(struct xe_gt *gt, unsigned int 
> vfid, void *bu ret = -ENOBUFS; } else { config = pf_pick_vf_config(gt, 
> vfid); - ret = encode_config(buf, config, false) * sizeof(u32); + ret 
> = encode_config(gt, buf, config, false) * sizeof(u32); } } 
> mutex_unlock(xe_gt_sriov_pf_master_mutex(gt)); @@ -2551,11 +2554,14 @@ 
> static int pf_restore_vf_config_klv(struct xe_gt *gt, unsigned int 
> vfid, return pf_provision_preempt_timeout(gt, vfid, value[0]); /* 
> auto-generate case statements */ -#define 
> define_threshold_key_to_provision_case(TAG, ...) \ +#define 
> define_threshold_key_to_provision_case(TAG, NAME, VER...) \ case 
> MAKE_GUC_KLV_VF_CFG_THRESHOLD_KEY(TAG): \ 
> BUILD_BUG_ON(MAKE_GUC_KLV_VF_CFG_THRESHOLD_LEN(TAG) != 1u); \ if (len 
> != MAKE_GUC_KLV_VF_CFG_THRESHOLD_LEN(TAG)) \ return -EBADMSG; \ + if 
> (IF_ARGS(!xe_guc_supports_fw_version(&gt->uc.guc, MAKE_GUC_VER_ARGS(VER)), \
> +			    false, VER))						\
> +			return -EKEYREJECTED;						\
>   		return pf_provision_threshold(gt, vfid,					\
>   					      MAKE_XE_GUC_KLV_THRESHOLD_INDEX(TAG),	\
>   					      value[0]);
> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_pf_debugfs.c b/drivers/gpu/drm/xe/xe_gt_sriov_pf_debugfs.c
> index 0fd863609848..5362c791a698 100644
> --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf_debugfs.c
> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf_debugfs.c
> @@ -21,6 +21,7 @@
>   #include "xe_gt_sriov_pf_monitor.h"
>   #include "xe_gt_sriov_pf_policy.h"
>   #include "xe_gt_sriov_pf_service.h"
> +#include "xe_guc.h"
>   #include "xe_pm.h"
>   #include "xe_sriov_pf.h"
>   #include "xe_sriov_pf_provision.h" @@ -301,9 +302,11 @@ static void pf_add_config_attrs(struct xe_gt 
> *gt, struct dentry *parent, unsigne &sched_priority_fops); /* register 
> all threshold attributes */ -#define register_threshold_attribute(TAG, 
> NAME, ...) \ - debugfs_create_file_unsafe("threshold_" #NAME, 0644, parent, parent, \
> -				   &NAME##_fops);
> +#define register_threshold_attribute(TAG, NAME, VER...) ({					\
> +	if (IF_ARGS(xe_guc_supports_fw_version(&gt->uc.guc, MAKE_GUC_VER_ARGS(VER)), true, VER))\
> +		debugfs_create_file_unsafe("threshold_" #NAME, 0644, parent, parent,		\
> +					   &NAME##_fops);					\
> +});
>   	MAKE_XE_GUC_KLV_THRESHOLDS_SET(register_threshold_attribute)
>   #undef register_threshold_attribute
>   }
> diff --git a/drivers/gpu/drm/xe/xe_guc.h b/drivers/gpu/drm/xe/xe_guc.h
> index fdb08658d05a..2a083d5c5865 100644
> --- a/drivers/gpu/drm/xe/xe_guc.h
> +++ b/drivers/gpu/drm/xe/xe_guc.h
> @@ -18,6 +18,9 @@
>    */
>   #define MAKE_GUC_VER(maj, min, pat)	(((maj) << 16) | ((min) << 8) | (pat))
>   #define MAKE_GUC_VER_STRUCT(ver)	MAKE_GUC_VER((ver).major, (ver).minor, (ver).patch)
> +#define MAKE_GUC_VER_ARGS(ver...) \
> +	MAKE_GUC_VER(PICK_ARG1(ver), PICK_ARG2(ver), IF_ARGS(PICK_ARG3(ver), 0, PICK_ARG3(ver)))
> +
>   #define GUC_SUBMIT_VER(guc) \
>   	MAKE_GUC_VER_STRUCT((guc)->fw.versions.found[XE_UC_FW_VER_COMPATIBILITY])
>   #define GUC_FIRMWARE_VER(guc) \
> @@ -96,4 +99,19 @@ static inline struct drm_device *guc_to_drm(struct xe_guc *guc)
>   	return &guc_to_xe(guc)->drm;
>   }
>   
> +/**
> + * xe_guc_supports_fw_version() - Check if GuC supports given version.

I don't think "supports_fw_version" is the correct name here, because 
the GuC has a given version, not support for multiple of them. Maybe use 
xe_guc_fw_version_at_least or something like that?

> + * @guc: the &xe_guc
> + * @ver: the version to check
> + *
> + * The @ver should be prepared using MAKE_GUC_VER(major, minor, patch).
> + *
> + * Return: true if loaded GuC firmware is at least of given version,
> + *         false otherwise.
> + */
> +static inline bool xe_guc_supports_fw_version(struct xe_guc *guc, u32 ver)
> +{
> +	return GUC_FIRMWARE_VER(guc) >= ver;
> +}

I wonder if it is worth macroing this up to avoid the MAKE_GUC_VER in 
the caller? something like:

GUC_VER_AT_LEAST(guc_, ver_...) \
     (GUC_FIRMWARE_VER(guc_) >= MAKE_GUC_VER_ARGS(ver_))

or even further:

GUC_VER_AT_LEAST(guc_, ver_...) \
IF_ARGS(GUC_FIRMWARE_VER(guc_) >= MAKE_GUC_VER_ARGS(ver_), true, ver_))

Just a thought, not a blocker.

With the rename:

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

Daniele

> +
>   #endif
> diff --git a/drivers/gpu/drm/xe/xe_guc_klv_thresholds_set_types.h b/drivers/gpu/drm/xe/xe_guc_klv_thresholds_set_types.h
> index 0a028c94756d..5f84da3d10d3 100644
> --- a/drivers/gpu/drm/xe/xe_guc_klv_thresholds_set_types.h
> +++ b/drivers/gpu/drm/xe/xe_guc_klv_thresholds_set_types.h
> @@ -24,6 +24,11 @@
>    * ABI and the associated &NAME, that may be used in code or debugfs/sysfs::
>    *
>    *	define(TAG, NAME)
> + *
> + * If required, KLVs can be labeled with GuC firmware version that added them::
> + *
> + *	define(TAG, NAME, MAJOR, MINOR)
> + *	define(TAG, NAME, MAJOR, MINOR, PATCH)
>    */
>   #define MAKE_XE_GUC_KLV_THRESHOLDS_SET(define)		\
>   	define(CAT_ERR, cat_error_count)		\

[-- Attachment #2: Type: text/html, Size: 9842 bytes --]