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

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

On 09.04.2024 17:45, Ghimiray, Himal Prasad wrote:
> 
> On 09-04-2024 15:05, Michal Wajdeczko wrote:
>>
>> On 08.04.2024 22:23, Ghimiray, Himal Prasad wrote:
>>> On 08-04-2024 23:44, 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)))
>>> is this macro necessary, it's being called in one place ?
>>>
>>> ((u64)hi << 32) | lo  should be enough.
>> IMO it is better to have a macro (with name) than a magic formula
>> besides, 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/
> Thanks for the info.
>>
>>>> +
>>>> +/**
>>>> + * 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)";
>>>
>>> Have seen warning/errors if default is not defined in switch-case.
>> likely compiler was complaining because of enum being used in switch
>> statement, but here it is u16, so there shouldn't be any warnings
>>
>> but can add 'default' case if you consider that as a blocker
> I have no strong objection on this, if compiler is not complaining.
>>
>>>> +    }
>>>> +    return "(unknown)";
>>> remove.
>>>> +}
>>>> +
>>>> +/**
>>>> + * 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]),
>>>> +                   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.
>> see here ^^^
>>
>>>> + */
>>>> +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;
>>> while loop can be terminated only if num_dwords < len + GUC_KLV_LEN_MIN
>>> or num_dwords is 0. Therefore return num_klvs; should be fine.
>> but then we won't catch the case with truncated buffer (which indicates
>> bug somewhere in code or firmware) and it is one of the function goal
>> and return value
> 
> I fail to understand in current implementation, even if there is bug
> somewhere in code or firmware
> 
> how return num_dwords ? -ENODATA : num_klvs; can return -ENODATA ?
> 
> num_dwords is u32 so always >= 0 and while loop keeps running unless
> num_dwords < len + GUC_KLV_LEN_MIN

loop keeps running unless (num_dwords < GUC_KLV_LEN_MIN)
there is no 'len' in this condition

> 
> or num_dwords is 0. 

that's true but only because you've looked at the value of
GUC_KLV_LEN_MIN which happen to be 1

> In case of num_dwords < len + GUC_KLV_LEN_MIN it
> will return -ENODATA in above loop itself.

this early return inside the loop was for cases where the buffer was
truncated in such a way that it contains the KLV header but doesn't hold
full value, while the final return was to catch the case where the
buffer was truncated at the KLV header itself

so yes, with GUC_KLV_LEN_MIN=1 the num_dwords outside the loop will be
always 0, making this final condition questionable, but that requires
knowledge of the GUC_KLV_LEN_MIN

maybe best option to have generic solution would be to just break the
loop on (num_dwords < len + GUC_KLV_LEN_MIN) instead doing early return

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