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

[Michal Wajdeczko]

On 16.05.2024 16:23, Lucas De Marchi wrote:
> 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

it was added in v2 on purpose to make the macro looks more like real
code inside switch statement and thus improve readability (same done
also in few other places)

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

the trick is that while VERY_LONG_NAME_FOO names are already defined as
part of the GuC ABI, we never have to use them directly in the code and
just treat them in generic way as a set of VERY_LONG_NAMEs identified
only by the TAG suffix

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

Thanks!