Re: [PATCH 1/3] drm/xe/guc: move KLV helpers to common file

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

On 28.02.2025 00:59, Daniele Ceraolo Spurio wrote:
> 
> 
> On 2/27/2025 3:29 PM, Michal Wajdeczko wrote:
>>
>> On 27.02.2025 02:05, Daniele Ceraolo Spurio wrote:
>>> The GuC uses the same format for all its KLVs, so having the functions
>>> to set them in a common file will allow us to re-use it for different
>>> types of KLVs and not just the WAs in ADS.
>>> The next patch will make use of these functions for a new H2G command.
>>>
>>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>> Cc: John Harrison <John.C.Harrison@Intel.com>
>>> ---
>>>   drivers/gpu/drm/xe/xe_guc_ads.c         | 89 ++++++-------------------
>>>   drivers/gpu/drm/xe/xe_guc_klv_helpers.c | 62 +++++++++++++++++
>>>   drivers/gpu/drm/xe/xe_guc_klv_helpers.h |  7 ++
>>>   3 files changed, 89 insertions(+), 69 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_guc_ads.c b/drivers/gpu/drm/xe/
>>> xe_guc_ads.c
>>> index fab259adc380..ed3304a11812 100644
>>> --- a/drivers/gpu/drm/xe/xe_guc_ads.c
>>> +++ b/drivers/gpu/drm/xe/xe_guc_ads.c
>>> @@ -22,6 +22,7 @@
>>>   #include "xe_guc.h"
>>>   #include "xe_guc_capture.h"
>>>   #include "xe_guc_ct.h"
>>> +#include "xe_guc_klv_helpers.h"
>>>   #include "xe_hw_engine.h"
>>>   #include "xe_lrc.h"
>>>   #include "xe_map.h"
>>> @@ -283,56 +284,6 @@ static size_t calculate_golden_lrc_size(struct
>>> xe_guc_ads *ads)
>>>       return total_size;
>>>   }
>>>   -static void guc_waklv_enable_one_word(struct xe_guc_ads *ads,
>>> -                      enum xe_guc_klv_ids klv_id,
>>> -                      u32 value,
>>> -                      u32 *offset, u32 *remain)
>>> -{
>>> -    u32 size;
>>> -    u32 klv_entry[] = {
>>> -        /* 16:16 key/length */
>>> -        FIELD_PREP(GUC_KLV_0_KEY, klv_id) |
>>> -        FIELD_PREP(GUC_KLV_0_LEN, 1),
>>> -        value,
>>> -        /* 1 dword data */
>>> -    };
>>> -
>>> -    size = sizeof(klv_entry);
>>> -
>>> -    if (*remain < size) {
>>> -        drm_warn(&ads_to_xe(ads)->drm,
>>> -             "w/a klv buffer too small to add klv id %d\n", klv_id);
>>> -    } else {
>>> -        xe_map_memcpy_to(ads_to_xe(ads), ads_to_map(ads), *offset,
>>> -                 klv_entry, size);
>>> -        *offset += size;
>>> -        *remain -= size;
>>> -    }
>>> -}
>>> -
>>> -static void guc_waklv_enable_simple(struct xe_guc_ads *ads,
>>> -                    enum xe_guc_klv_ids klv_id, u32 *offset, u32
>>> *remain)
>>> -{
>>> -    u32 klv_entry[] = {
>>> -        /* 16:16 key/length */
>>> -        FIELD_PREP(GUC_KLV_0_KEY, klv_id) |
>>> -        FIELD_PREP(GUC_KLV_0_LEN, 0),
>>> -        /* 0 dwords data */
>>> -    };
>>> -    u32 size;
>>> -
>>> -    size = sizeof(klv_entry);
>>> -
>>> -    if (xe_gt_WARN(ads_to_gt(ads), *remain < size,
>>> -               "w/a klv buffer too small to add klv id %d\n", klv_id))
>>> -        return;
>>> -
>>> -    xe_map_memcpy_to(ads_to_xe(ads), ads_to_map(ads), *offset,
>>> -             klv_entry, size);
>>> -    *offset += size;
>>> -    *remain -= size;
>>> -}
>>> -
>>>   static void guc_waklv_init(struct xe_guc_ads *ads)
>>>   {
>>>       struct xe_gt *gt = ads_to_gt(ads);
>>> @@ -343,22 +294,22 @@ static void guc_waklv_init(struct xe_guc_ads *ads)
>>>       remain = guc_ads_waklv_size(ads);
>>>         if (XE_WA(gt, 14019882105))
>>> -        guc_waklv_enable_simple(ads,
>>> -                   
>>> GUC_WORKAROUND_KLV_BLOCK_INTERRUPTS_WHEN_MGSR_BLOCKED,
>>> -                    &offset, &remain);
>>> +        xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
>>> +                    
>>> GUC_WORKAROUND_KLV_BLOCK_INTERRUPTS_WHEN_MGSR_BLOCKED,
>>> +                     &offset, &remain);
>>>       if (XE_WA(gt, 18024947630))
>>> -        guc_waklv_enable_simple(ads,
>>> -                    GUC_WORKAROUND_KLV_ID_GAM_PFQ_SHADOW_TAIL_POLLING,
>>> -                    &offset, &remain);
>>> +        xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
>>> +                     GUC_WORKAROUND_KLV_ID_GAM_PFQ_SHADOW_TAIL_POLLING,
>>> +                     &offset, &remain);
>>>       if (XE_WA(gt, 16022287689))
>>> -        guc_waklv_enable_simple(ads,
>>> -                   
>>> GUC_WORKAROUND_KLV_ID_DISABLE_MTP_DURING_ASYNC_COMPUTE,
>>> -                    &offset, &remain);
>>> +        xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
>>> +                    
>>> GUC_WORKAROUND_KLV_ID_DISABLE_MTP_DURING_ASYNC_COMPUTE,
>>> +                     &offset, &remain);
>>>         if (XE_WA(gt, 14022866841))
>>> -        guc_waklv_enable_simple(ads,
>>> -                    GUC_WA_KLV_WAKE_POWER_DOMAINS_FOR_OUTBOUND_MMIO,
>>> -                    &offset, &remain);
>>> +        xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
>>> +                     GUC_WA_KLV_WAKE_POWER_DOMAINS_FOR_OUTBOUND_MMIO,
>>> +                     &offset, &remain);
>>>         /*
>>>        * On RC6 exit, GuC will write register 0xB04 with the default
>>> value provided. As of now,
>>> @@ -366,15 +317,15 @@ static void guc_waklv_init(struct xe_guc_ads *ads)
>>>        * future, so GuC depends on KMD to send it the correct value.
>>>        */
>>>       if (XE_WA(gt, 13011645652))
>>> -        guc_waklv_enable_one_word(ads,
>>> -                     
>>> GUC_WA_KLV_NP_RD_WRITE_TO_CLEAR_RCSM_AT_CGP_LATE_RESTORE,
>>> -                      0xC40,
>>> -                      &offset, &remain);
>>> +        xe_guc_klv_enable_one_word(ads_to_guc(ads), ads_to_map(ads),
>>> +                      
>>> GUC_WA_KLV_NP_RD_WRITE_TO_CLEAR_RCSM_AT_CGP_LATE_RESTORE,
>>> +                       0xC40,
>>> +                       &offset, &remain);
>>>         if (XE_WA(gt, 14022293748) || XE_WA(gt, 22019794406))
>>> -        guc_waklv_enable_simple(ads,
>>> -                   
>>> GUC_WORKAROUND_KLV_ID_BACK_TO_BACK_RCS_ENGINE_RESET,
>>> -                    &offset, &remain);
>>> +        xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
>>> +                    
>>> GUC_WORKAROUND_KLV_ID_BACK_TO_BACK_RCS_ENGINE_RESET,
>>> +                     &offset, &remain);
>>>         size = guc_ads_waklv_size(ads) - remain;
>>>       if (!size)
>>> diff --git a/drivers/gpu/drm/xe/xe_guc_klv_helpers.c b/drivers/gpu/
>>> drm/xe/xe_guc_klv_helpers.c
>>> index 146a6eda9e06..50eab2086ee3 100644
>>> --- a/drivers/gpu/drm/xe/xe_guc_klv_helpers.c
>>> +++ b/drivers/gpu/drm/xe/xe_guc_klv_helpers.c
>>> @@ -7,8 +7,11 @@
>>>   #include <drm/drm_print.h>
>>>     #include "abi/guc_klvs_abi.h"
>>> +#include "xe_gt_printk.h"
>>> +#include "xe_guc.h"
>>>   #include "xe_guc_klv_helpers.h"
>>>   #include "xe_guc_klv_thresholds_set.h"
>>> +#include "xe_map.h"
>>>     #define make_u64(hi, lo) ((u64)((u64)(u32)(hi) << 32 | (u32)(lo)))
>>>   @@ -146,3 +149,62 @@ int xe_guc_klv_count(const u32 *klvs, u32
>>> num_dwords)
>>>         return num_dwords ? -ENODATA : num_klvs;
>>>   }
>>> +
>>> +static void emit_klv(struct xe_guc *guc, struct iosys_map *map, u32
>>> *klv_entry,
>>> +             u32 size, u32 *offset, u32 *remain)
>> s/klv_entry/klv
>>
>> s/size/num_dwords
>>
>> KLVs are always u32 aligned
>> 'size/offset/remain' all should be in dwords
> 
> That would mean having to convert back and forth between u8 and u32:
> 
> - allocation is in u8 aligned
> - params to this function would be u32 aligned

but for generic KLV functions it doesn't make sense to pass u8 sizes
and we should aim to define function signatures that enforce proper logic

> - xe_map_memcpy_to below needs u8 alignment, so convert back
> - the GuC expects a u8 aligned size, so need to convert remain back to u8

that looks like a mistake done in this action definition, since all
initial actions that introduced KLV concept (like UPDATE_VGT_POLICY,
UPDATE_VF_CFG) were defined to operate on dwords as size

> 
> Just using u8 everywhere seems simpler instead of converting back and
> forth.
> 
>>
>> this will also match rest of functions in this file
>>
>>> +{
>>> +    if (xe_gt_WARN(guc_to_gt(guc), *remain < size,
>> do we need xe_gt_WARN in production?
>> maybe xe_gt_assert will suffice?
> 
> I've kept this the same as what was in the original function. Not sure
> why it was xe_gt_WARN instead of assert.
> 
>>
>>> +               "klv buffer too small to add klv id 0x%04x\n",
>> s/klv/KLV
>>
>>> +               FIELD_GET(GUC_KLV_0_KEY, klv_entry[0])))
>>> +        return;
>>> +
>>> +    xe_map_memcpy_to(guc_to_xe(guc), map, *offset, klv_entry, size);
>>> +    *offset += size;
>>> +    *remain -= size;
>>> +}
>>> +
>>> +/**
>>> + * xe_guc_klv_enable_simple - Emits a KLV with no data to an iosys
>>> mapped buffer
>>> + * @guc: the guc structure
>>> + * @map: the iosys_map to write to
>>> + * @klv_id: the KLV to enable
>> key ? there is no Len nor Value
>>
>> this will follow other functions in this file
> 
> ok, will rename.
> 
>>
>>> + * @offset: pointer to a variable holding the offset to write to
>>> + * @remain: pointer to a variable holding the remaining writing
>>> space available
>>> + *
>>> + * The function checks if there is enough space to emit a KLV with
>>> no data and
>>> + * if so writes it to memory. After the write, the offset and remain
>>> variables
>>> + * are respectively increased and decreased of the written size to
>>> be ready for
>>> + * the next emission.
>>> + */
>>> +void xe_guc_klv_enable_simple(struct xe_guc *guc, struct iosys_map
>>> *map,
>>> +                  u16 klv_id, u32 *offset, u32 *remain)
>>> +{
>>> +    u32 klv_entry = PREP_GUC_KLV(klv_id, 0);
>>     u32 klv[] = { PREP_GUC_KLV(key, 0) };
>>
>>     emit(... ARRAY_SIZE(klv)
> 
> Why? this function is specifically targeted to writing a single u32, why
> write it as a u32[1]?
> 
>>> +
>>> +    emit_klv(guc, map, &klv_entry, sizeof(klv_entry), offset, remain);
>>> +}
>>> +
>>> +/**
>>> + * xe_guc_klv_enable_one_word - Emits a KLV with 1 DW data to an
>>> iosys mapped buffer
>>> + * @guc: the guc structure
>>> + * @map: the iosys_map to write to
>>> + * @klv_id: the KLV to enable
>> s/klv_id/key
>>
>>> + * @value: the data associated with the KLV
>>> + * @offset: pointer to a variable holding the offset to write to
>>> + * @remain: pointer to a variable holding the remaining writing
>>> space available
>>> + *
>>> + * The function checks if there is enough space to emit a KLV with 1
>>> DW data and
>>> + * if so writes it to memory. After the write, the offset and remain
>>> variables
>>> + * are respectively increased and decreased of the written size to
>>> be ready for
>>> + * the next emission.
>>> + */
>>> +void xe_guc_klv_enable_one_word(struct xe_guc *guc, struct iosys_map
>>> *map,
>>> +                u16 klv_id, u32 value, u32 *offset, u32 *remain)
>>> +{
>>> +    u32 klv_entry[] = {
>>> +        PREP_GUC_KLV(klv_id, 1),
>>> +        value,
>>> +    };
>>> +
>>> +    emit_klv(guc, map, klv_entry, sizeof(klv_entry), offset, remain);
>>> +}
>>> diff --git a/drivers/gpu/drm/xe/xe_guc_klv_helpers.h b/drivers/gpu/
>>> drm/xe/xe_guc_klv_helpers.h
>>> index c676d21c173b..231f7c4b0947 100644
>>> --- a/drivers/gpu/drm/xe/xe_guc_klv_helpers.h
>>> +++ b/drivers/gpu/drm/xe/xe_guc_klv_helpers.h
>>> @@ -10,6 +10,8 @@
>>>   #include <linux/types.h>
>>>     struct drm_printer;
>>> +struct iosys_map;
>>> +struct xe_guc;
>>>     const char *xe_guc_klv_key_to_string(u16 key);
>>>   @@ -61,4 +63,9 @@ int xe_guc_klv_count(const u32 *klvs, u32
>>> num_dwords);
>>>   #define PREP_GUC_KLV_TAG(TAG) \
>>>       PREP_GUC_KLV_CONST(MAKE_GUC_KLV_KEY(TAG), MAKE_GUC_KLV_LEN(TAG))
>>>   +void xe_guc_klv_enable_simple(struct xe_guc *guc, struct iosys_map
>>> *map,
>>> +                  u16 klv_id, u32 *offset, u32 *remain);
>>> +void xe_guc_klv_enable_one_word(struct xe_guc *guc, struct iosys_map
>>> *map,
>>> +                u16 klv_id, u32 value, u32 *offset, u32 *remain);
>> hmm, "enable" does not fit here, maybe:
>>
>> xe_guc_klv_emit_empty / no_data / no_value
>> xe_guc_klv_emit_dword
> 
> emit_nodata should work.
> 
>>
>> and those should be inlines that use:
>>
>> void xe_guc_klv_emit_simple(
>>     struct xe_guc *guc,
>>     struct iosys_map *map,
>>     u16 key, u16 len, const u32 *klv,
>>     u32 *offset, u32 *remain);
>>
>> like:
>>
>> inline xe_guc_klv_emit_simple_empty(.. key ..)
>> {
>>     xe_guc_klv_emit_simple(.. key, 0, NULL, ..);
>> }
>>
>>
>> inline xe_guc_klv_emit_simple_dword(.. key, value ..)
>> {
>>     xe_guc_klv_emit_simple(.. key, 1, &value, ..);
>> }
> 
> This seems to just make things harder. xe_guc_klv_emit_simple() would
> need to handle the case where we have data differently from the case
> where we don't, potentially with 2 separate memcpys (one always done for

this isn't on the hot path so I would prefer avoid code duplication over
perf optimization

> the key and one optional for the data). I really don't see the benefit
> when the current approach still makes most of the code common in
> emit_klv() and only has separate arrays. Also, that wouldn't be a
> "simple" emission anymore since it'd handle all the possible cases, so
> the naming also wouldn't fit.

but the goal is to have helper function to 'emit KLVs', so the 'simple'
here means help in building and emit of the proper KLV-header based on
the provided key/len params, followed by emit of value (as dwords)

and while your current case is only KLV with 0 or 1 dword, nothing
prevents us from implementing generic helper, and satisfying your need
with trivial wrappers for 0/1 case

> 
> Daniele
> 
>>
>>> +
>>>   #endif
>