Re: [PATCH 2/8] drm/xe/guc: Introduce GuC KLV thresholds set

["Piotr Piórkowski" <piotr.piorkowski@intel.com>]

Michal Wajdeczko <michal.wajdeczko@intel.com> wrote on pon [2024-maj-06 15:38:08 +0200]:
> The GuC firmware monitors VF's activity and notifies the PF driver
> once any configured threshold related to such activity is exceeded.
> 
> The available thresholds are defined in the GuC ABI as part of the
> GuC VF Configuration KLVs.  Threshold configurations performed by
> the PF driver and notifications sent by the GuC rely on the KLV keys,
> which are not zero-based and might not guarantee continuity.
> 
> To simplify the driver code and eliminate the need to repeat very
> similar code for each threshold, introduce the threshold set macro
> that allows to generate required code based on unique threshold tag.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> ---
>  .../gpu/drm/xe/xe_guc_klv_thresholds_set.h    | 58 +++++++++++++++++
>  .../drm/xe/xe_guc_klv_thresholds_set_types.h  | 65 +++++++++++++++++++
>  2 files changed, 123 insertions(+)
>  create mode 100644 drivers/gpu/drm/xe/xe_guc_klv_thresholds_set.h
>  create mode 100644 drivers/gpu/drm/xe/xe_guc_klv_thresholds_set_types.h
> 
> diff --git a/drivers/gpu/drm/xe/xe_guc_klv_thresholds_set.h b/drivers/gpu/drm/xe/xe_guc_klv_thresholds_set.h
> new file mode 100644
> index 000000000000..da9ae10c72b8
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_guc_klv_thresholds_set.h
> @@ -0,0 +1,58 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2024 Intel Corporation
> + */
> +
> +#ifndef _XE_GUC_KLV_THRESHOLDS_SET_H_
> +#define _XE_GUC_KLV_THRESHOLDS_SET_H_
> +
> +#include "abi/guc_klvs_abi.h"
> +#include "xe_guc_klv_helpers.h"
> +#include "xe_guc_klv_thresholds_set_types.h"
> +
> +/**
> + * MAKE_GUC_KLV_VF_CFG_THRESHOLD_KEY - Prepare the name of the KLV key constant.
> + * @TAG: unique tag of the GuC threshold KLV key.
> + */
> +#define MAKE_GUC_KLV_VF_CFG_THRESHOLD_KEY(TAG) \
> +	MAKE_GUC_KLV_KEY(CONCATENATE(VF_CFG_THRESHOLD_, TAG))
> +
> +/**
> + * xe_guc_klv_threshold_key_to_index - Find index of the tracked GuC threshold.
> + * @key: GuC threshold KLV key.
> + *
> + * This translation is automatically generated using &MAKE_XE_GUC_KLV_THRESHOLDS_SET.
> + * Return: index of the GuC threshold KLV or -1 if not found.
> + */
> +static inline int xe_guc_klv_threshold_key_to_index(u32 key)
> +{
> +	switch (key) {
> +#define define_xe_guc_klv_threshold_key_to_index_case(TAG, ...)		\
> +	case MAKE_GUC_KLV_VF_CFG_THRESHOLD_KEY(TAG):			\
> +		return MAKE_XE_GUC_KLV_THRESHOLD_INDEX(TAG);

NIT: In my opinion, putting these definitions inside a switch (or in another case,
inside an enum) worsens the readability of the code, which is already unreadable by the fact
that these are macros - which, by the way, are very interesting in itself.

> +	MAKE_XE_GUC_KLV_THRESHOLDS_SET(define_xe_guc_klv_threshold_key_to_index_case)
> +	}
> +	return -1;
> +#undef define_xe_guc_klv_threshold_key_to_index_case
> +}
> +
> +/**
> + * xe_guc_klv_threshold_index_to_key - Get tracked GuC threshold KLV key.
> + * @index: GuC threshold KLV index.
> + *
> + * This translation is automatically generated using &MAKE_XE_GUC_KLV_THRESHOLDS_SET.
> + * Return: key of the GuC threshold KLV or 0 on malformed index.
> + */
> +static inline u32 xe_guc_klv_threshold_index_to_key(enum xe_guc_klv_threshold_index index)
> +{
> +	switch (index) {
> +#define define_xe_guc_klv_threshold_index_to_key_case(TAG, ...)		\
> +	case MAKE_XE_GUC_KLV_THRESHOLD_INDEX(TAG):			\
> +		return MAKE_GUC_KLV_VF_CFG_THRESHOLD_KEY(TAG);
> +	MAKE_XE_GUC_KLV_THRESHOLDS_SET(define_xe_guc_klv_threshold_index_to_key_case)
> +	}
> +	return 0; /* unreachable */
> +#undef define_xe_guc_klv_threshold_index_to_key_case
> +}
> +
> +#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
> new file mode 100644
> index 000000000000..6a48d76924a4
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_guc_klv_thresholds_set_types.h
> @@ -0,0 +1,65 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2024 Intel Corporation
> + */
> +
> +#ifndef _XE_GUC_KLV_THRESHOLDS_SET_TYPES_H_
> +#define _XE_GUC_KLV_THRESHOLDS_SET_TYPES_H_
> +
> +#include "xe_args.h"
> +
> +/**
> + * MAKE_XE_GUC_KLV_THRESHOLDS_SET - Generate various GuC thresholds definitions.
> + * @define: name of the inner macro to expand.
> + *
> + * The GuC firmware is able to monitor VF's adverse activity and will notify the
> + * PF driver once any threshold is exceeded.
> + *
> + * This super macro allows various conversions between the GuC adverse event
> + * threshold KLV definitions and the driver code without repeating similar code
> + * or risking missing some cases.
> + *
> + * For each GuC threshold definition, the inner macro &define will be provided
> + * with the &TAG, that corresponds to the GuC threshold KLV key name defined by
> + * ABI and the associated &NAME, that may be used in code or debugfs/sysfs::
> + *
> + *	define(TAG, NAME)
> + */
> +#define MAKE_XE_GUC_KLV_THRESHOLDS_SET(define)		\
> +	define(CAT_ERR, cat_error_count)		\
> +	define(ENGINE_RESET, engine_reset_count)	\
> +	define(PAGE_FAULT, page_fault_count)		\
> +	define(H2G_STORM, guc_time_us)			\
> +	define(IRQ_STORM, irq_time_us)			\
> +	define(DOORBELL_STORM, doorbell_time_us)	\
> +	/* end */
> +
> +/**
> + * XE_GUC_KLV_NUM_THRESHOLDS - Number of GuC thresholds KLVs.
> + *
> + * Calculated automatically using &MAKE_XE_GUC_KLV_THRESHOLDS_SET.
> + */
> +#define XE_GUC_KLV_NUM_THRESHOLDS \
> +	(CALL_ARGS(COUNT_ARGS, MAKE_XE_GUC_KLV_THRESHOLDS_SET(ARGS_SEP_COMMA)) - 1)
> +
> +/**
> + * MAKE_XE_GUC_KLV_THRESHOLD_INDEX - Create enumerator name.
> + * @TAG: unique TAG of the enum xe_guc_klv_threshold_index.
> + */
> +#define MAKE_XE_GUC_KLV_THRESHOLD_INDEX(TAG) \
> +	CONCATENATE(XE_GUC_KLV_THRESHOLD_INDEX_, TAG)
> +
> +/**
> + * enum xe_guc_klv_threshold_index - Index of the tracked GuC threshold.
> + *
> + * This enum is automatically generated using &MAKE_XE_GUC_KLV_THRESHOLDS_SET.
> + * All these generated enumerators will only be used by the also generated code.
> + */
> +enum xe_guc_klv_threshold_index {
> +#define define_xe_guc_klv_threshold_index_enum(TAG, ...) \
> +	MAKE_XE_GUC_KLV_THRESHOLD_INDEX(TAG),
> +	MAKE_XE_GUC_KLV_THRESHOLDS_SET(define_xe_guc_klv_threshold_index_enum)
> +#undef define_xe_guc_klv_threshold_index_enum
> +};
> +
> +#endif

One comment above, however, the code functionally seems ok:
Reviewed-by: Piotr Piórkowski <piotr.piorkowski@intel.com>

> -- 
> 2.43.0
> 

--