Re: [PATCH 1/2] drm/xe/guc: Add helpers for GuC KLVs

[John Harrison <john.c.harrison@intel.com>]

On 4/8/2024 11:14, Michal Wajdeczko wrote:
> Many of the GuC actions use KLVs to pass additional parameters or
> configuration data. Add few helper functions for better reporting
> any information related to KLVs.
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> ---
>   drivers/gpu/drm/xe/Makefile             |   1 +
>   drivers/gpu/drm/xe/xe_guc_klv_helpers.c | 133 ++++++++++++++++++++++++
>   drivers/gpu/drm/xe/xe_guc_klv_helpers.h |  51 +++++++++
>   3 files changed, 185 insertions(+)
>   create mode 100644 drivers/gpu/drm/xe/xe_guc_klv_helpers.c
>   create mode 100644 drivers/gpu/drm/xe/xe_guc_klv_helpers.h
>
> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> index e5b1715f721e..ec0d1fb49b1e 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -98,6 +98,7 @@ xe-y += xe_bb.o \
>   	xe_guc_debugfs.o \
>   	xe_guc_hwconfig.o \
>   	xe_guc_id_mgr.o \
> +	xe_guc_klv_helpers.o \
>   	xe_guc_log.o \
>   	xe_guc_pc.o \
>   	xe_guc_submit.o \
> diff --git a/drivers/gpu/drm/xe/xe_guc_klv_helpers.c b/drivers/gpu/drm/xe/xe_guc_klv_helpers.c
> new file mode 100644
> index 000000000000..c39fda08afcb
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_guc_klv_helpers.c
> @@ -0,0 +1,133 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2024 Intel Corporation
> + */
> +
> +#include <linux/bitfield.h>
> +#include <drm/drm_print.h>
> +
> +#include "abi/guc_klvs_abi.h"
> +#include "xe_guc_klv_helpers.h"
> +
> +#define make_u64(hi, lo) ((u64)((u64)(u32)(hi) << 32 | (u32)(lo)))
> +
> +/**
> + * xe_guc_klv_key_to_string - Convert KLV key into friendly name.
> + * @key: the `GuC KLV`_ key
> + *
> + * Return: name of the KLV key.
> + */
> +const char *xe_guc_klv_key_to_string(u16 key)
> +{
> +	switch (key) {
> +	/* VGT POLICY keys */
> +	case GUC_KLV_VGT_POLICY_SCHED_IF_IDLE_KEY:
> +		return "sched_if_idle";
> +	case GUC_KLV_VGT_POLICY_ADVERSE_SAMPLE_PERIOD_KEY:
> +		return "sample_period";
> +	case GUC_KLV_VGT_POLICY_RESET_AFTER_VF_SWITCH_KEY:
> +		return "reset_engine";
> +	/* VF CFG keys */
> +	case GUC_KLV_VF_CFG_GGTT_START_KEY:
> +		return "ggtt_start";
> +	case GUC_KLV_VF_CFG_GGTT_SIZE_KEY:
> +		return "ggtt_size";
> +	case GUC_KLV_VF_CFG_LMEM_SIZE_KEY:
> +		return "lmem_size";
> +	case GUC_KLV_VF_CFG_NUM_CONTEXTS_KEY:
> +		return "num_contexts";
> +	case GUC_KLV_VF_CFG_TILE_MASK_KEY:
> +		return "tile_mask";
> +	case GUC_KLV_VF_CFG_NUM_DOORBELLS_KEY:
> +		return "num_doorbells";
> +	case GUC_KLV_VF_CFG_EXEC_QUANTUM_KEY:
> +		return "exec_quantum";
> +	case GUC_KLV_VF_CFG_PREEMPT_TIMEOUT_KEY:
> +		return "preempt_timeout";
> +	case GUC_KLV_VF_CFG_BEGIN_DOORBELL_ID_KEY:
> +		return "begin_db_id";
> +	case GUC_KLV_VF_CFG_BEGIN_CONTEXT_ID_KEY:
> +		return "begin_ctx_id";
> +	}
> +	return "(unknown)";
> +}
> +
> +/**
> + * xe_guc_klv_print - Print content of the buffer with `GuC KLV`_.
> + * @klvs: the buffer with KLVs
> + * @num_dwords: number of dwords (u32) available in the buffer
> + * @p: the &drm_printer
> + *
> + * The buffer may contain more than one KLV.
> + */
> +void xe_guc_klv_print(const u32 *klvs, u32 num_dwords, struct drm_printer *p)
> +{
> +	while (num_dwords >= GUC_KLV_LEN_MIN) {
> +		u32 key = FIELD_GET(GUC_KLV_0_KEY, klvs[0]);
> +		u32 len = FIELD_GET(GUC_KLV_0_LEN, klvs[0]);
> +
> +		klvs += GUC_KLV_LEN_MIN;
> +		num_dwords -= GUC_KLV_LEN_MIN;
> +
> +		if (num_dwords < len) {
> +			drm_printf(p, "{ key %#06x : truncated %zu of %zu bytes %*ph } # %s\n",
> +				   key, num_dwords * sizeof(u32), len * sizeof(u32),
> +				   (int)(num_dwords * sizeof(u32)), klvs,
> +				   xe_guc_klv_key_to_string(key));
> +			return;
> +		}
> +
> +		switch (len) {
> +		case 0:
> +			drm_printf(p, "{ key %#06x : no value } # %s\n",
> +				   key, xe_guc_klv_key_to_string(key));
> +			break;
> +		case 1:
> +			drm_printf(p, "{ key %#06x : 32b value %u } # %s\n",
> +				   key, klvs[0], xe_guc_klv_key_to_string(key));
> +			break;
> +		case 2:
> +			drm_printf(p, "{ key %#06x : 64b value %#llx } # %s\n",
> +				   key, make_u64(klvs[1], klvs[0]),
Or maybe it is actually two separate 32bit words? Why bother forcing it 
to a u64 with a brand new custom helper? Seems simpler to just print it 
as a pair of 32s, the reader can trivially combine it to a single 64 in 
their head if printing as hex anyway.

Also, there is no code to actually use any of these helpers yet? I 
thought adding unused code was not allowed. You have to have a purpose, 
at least in the same series if not the same patch.

John.

> +				   xe_guc_klv_key_to_string(key));
> +			break;
> +		default:
> +			drm_printf(p, "{ key %#06x : %zu bytes %*ph } # %s\n",
> +				   key, len * sizeof(u32), (int)(len * sizeof(u32)),
> +				   klvs, xe_guc_klv_key_to_string(key));
> +			break;
> +		}
> +
> +		klvs += len;
> +		num_dwords -= len;
> +	}
> +
> +	/* we don't expect any leftovers, fix if KLV header is ever changed */
> +	BUILD_BUG_ON(GUC_KLV_LEN_MIN > 1);
> +}
> +
> +/**
> + * xe_guc_klv_count - Count KLVs present in the buffer.
> + * @klvs: the buffer with KLVs
> + * @num_dwords: number of dwords (u32) in the buffer
> + *
> + * Return: number of recognized KLVs or
> + *          a negative error code if KLV buffer is truncated.
> + */
> +int xe_guc_klv_count(const u32 *klvs, u32 num_dwords)
> +{
> +	int num_klvs = 0;
> +
> +	while (num_dwords >= GUC_KLV_LEN_MIN) {
> +		u32 len = FIELD_GET(GUC_KLV_0_LEN, klvs[0]);
> +
> +		if (num_dwords < len + GUC_KLV_LEN_MIN)
> +			return -ENODATA;
> +
> +		klvs += GUC_KLV_LEN_MIN + len;
> +		num_dwords -= GUC_KLV_LEN_MIN + len;
> +		num_klvs++;
> +	}
> +
> +	return num_dwords ? -ENODATA : num_klvs;
> +}
> diff --git a/drivers/gpu/drm/xe/xe_guc_klv_helpers.h b/drivers/gpu/drm/xe/xe_guc_klv_helpers.h
> new file mode 100644
> index 000000000000..b835e0ebe6db
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_guc_klv_helpers.h
> @@ -0,0 +1,51 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2024 Intel Corporation
> + */
> +
> +#ifndef _XE_GUC_KLV_HELPERS_H_
> +#define _XE_GUC_KLV_HELPERS_H_
> +
> +#include <linux/types.h>
> +
> +struct drm_printer;
> +
> +const char *xe_guc_klv_key_to_string(u16 key);
> +
> +void xe_guc_klv_print(const u32 *klvs, u32 num_dwords, struct drm_printer *p);
> +int xe_guc_klv_count(const u32 *klvs, u32 num_dwords);
> +
> +/**
> + * PREP_GUC_KLV - Prepare KLV header value based on provided key and len.
> + * @key: KLV key
> + * @len: KLV length
> + *
> + * Return: value of the KLV header (u32).
> + */
> +#define PREP_GUC_KLV(key, len) \
> +	(FIELD_PREP(GUC_KLV_0_KEY, (key)) | \
> +	 FIELD_PREP(GUC_KLV_0_LEN, (len)))
> +
> +/**
> + * PREP_GUC_KLV_CONST - Prepare KLV header value based on const key and len.
> + * @key: const KLV key
> + * @len: const KLV length
> + *
> + * Return: value of the KLV header (u32).
> + */
> +#define PREP_GUC_KLV_CONST(key, len) \
> +	(FIELD_PREP_CONST(GUC_KLV_0_KEY, (key)) | \
> +	 FIELD_PREP_CONST(GUC_KLV_0_LEN, (len)))
> +
> +/**
> + * PREP_GUC_KLV_TAG - Prepare KLV header value based on unique KLV definition tag.
> + * @TAG: unique tag of the KLV definition
> + *
> + * Combine separate KEY and LEN definitions of the KLV identified by the TAG.
> + *
> + * Return: value of the KLV header (u32).
> + */
> +#define PREP_GUC_KLV_TAG(TAG) \
> +	PREP_GUC_KLV_CONST(GUC_KLV_##TAG##_KEY, GUC_KLV_##TAG##_LEN)
> +
> +#endif