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

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 14.05.2024 10:08, Piotr Piórkowski wrote:
> 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.

the idea was to mimic that this macro definition looks like actual
"case" statement - maybe with additional empty lines it will be better:

	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);

	/* generate above case statements here */
	MAKE_XE_GUC_KLV_THRESHOLDS_SET(...)
	}

> 
>> +	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
>>
>