Re: [PATCH 4/5] drm/xe/guc: Add helpers for GuC KLVs

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

On 10.04.2024 17:30, Piotr Piórkowski wrote:
> Michal Wajdeczko <michal.wajdeczko@intel.com> wrote on śro [2024-kwi-10 14:31:24 +0200]:
>> 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>
>> Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>> ---
>> v2: review comments from Himal
>> ---
>>  drivers/gpu/drm/xe/Makefile             |   1 +
>>  drivers/gpu/drm/xe/xe_guc_klv_helpers.c | 134 ++++++++++++++++++++++++
>>  drivers/gpu/drm/xe/xe_guc_klv_helpers.h |  51 +++++++++
>>  3 files changed, 186 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 8d79df05b84f..ad238f48cf51 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..8927374f55ff
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_guc_klv_helpers.c
>> @@ -0,0 +1,134 @@
>> +// 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)))
>> +
> 
> This is the second such macro in Xe - maybe it would be worth moving
> it to a common header somewhere ?

it's complicated

this macro here is (I hope) just temporary as either:
  - it will be promoted to xe_macros.h [1], or
  - something similar will be available in wordpart.h [2]

[1] https://patchwork.freedesktop.org/series/129854/

[2]
https://lore.kernel.org/lkml/20240214214654.1700-1-michal.wajdeczko@intel.com/

> 
> 
>> +/**
>> + * 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";
>> +	default:
>> +		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]);
> 
> u16 is probably sufficient in this case,
> especially since you use u16 in xe_guc_klv_key_to_string() also for the
> same key.

but base KLV unit is u32 so IMO better to start with that
and for the compiler likely it doesn't matter

>> +
>> +		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]),
>> +				   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.
> 
> ident too large by one space - I do not know if it is not allowed, but it catches my eye :)

good catch, had to double check to see it
will fix while pushing

> 
>> + */
>> +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)
>> +			break;
>> +
>> +		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
> 
> Only minor comments, besides LGTM:
> Reviewed-by: Piotr Piórkowski <piotr.piorkowski@intel.com>

thanks,
Michal

> 
>> -- 
>> 2.43.0
>>
>