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

[Lucas De Marchi]

On Tue, May 14, 2024 at 09:00:09PM GMT, Michal Wajdeczko wrote:
>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.
>
>Reviewed-by: Piotr Piórkowski <piotr.piorkowski at intel.com>
>Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
>---
>v2: improve readability of generated switch/enum (Piotr)
>    silence kernel-doc warning (CI.hooks)
>---
> .../gpu/drm/xe/xe_guc_klv_thresholds_set.h    | 64 +++++++++++++++++
> .../drm/xe/xe_guc_klv_thresholds_set_types.h  | 68 +++++++++++++++++++
> 2 files changed, 132 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..da0fedbbdbaf
>--- /dev/null
>+++ b/drivers/gpu/drm/xe/xe_guc_klv_thresholds_set.h
>@@ -0,0 +1,64 @@
>+/* 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, ...)		\
>+									\

something went wrong with this newline above

I checked only until here in the patch series. As I said offline, I
don't really like much this multiple indirections on creating
symbols/enums.

we end up having things like:

	xe_guc_klv_threshold_key_to_index(VERY_LONG_NAME_FOO)

and then when we are grepping the code to find VERY_LONG_NAME_FOO it's
impossible to quickly find it. From a quick look it seems the cases here
are like:

	static int pf_handle_vf_threshold_event(struct xe_gt *gt, u32 vfid, u32 threshold)
	{
		char origin[8];
		int e;

		e = xe_guc_klv_threshold_key_to_index(threshold);
		...

and it does have the error handling.  So, ack on the approach. Let's
just not go overboard with these macros please.

Acked-by: Lucas De Marchi <lucas.demarchi at intel.com>

Lucas De Marchi