Re: [PATCH v3 2/2] drm/xe/lnl: Enable GuC Wa_14019882105

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

On 3/25/2024 11:09, Nilawar, Badal wrote:
> On 25-03-2024 21:02, Michal Wajdeczko wrote:
>> On 25.03.2024 16:04, Badal Nilawar wrote:
>>> Enable GuC Wa_14019882105 to block interrupts during C6 flow
>>> when the memory path has been blocked
>>>
>>> v2: Make helper function generic and name it as
>>>      guc_waklv_enable_simple (John Harrison)
>>> v3: Make warning descriptive (John Harrison)
>>>
>>> Cc: John Harrison <john.harrison@intel.com>
>>> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
>>> ---
>>>   drivers/gpu/drm/xe/abi/guc_klvs_abi.h |  7 +++++
>>>   drivers/gpu/drm/xe/xe_guc_ads.c       | 41 
>>> +++++++++++++++++++++------
>>>   drivers/gpu/drm/xe/xe_wa_oob.rules    |  1 +
>>>   3 files changed, 40 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/abi/guc_klvs_abi.h 
>>> b/drivers/gpu/drm/xe/abi/guc_klvs_abi.h
>>> index 0400bc0fccdc..5dd45e06f0b6 100644
>>> --- a/drivers/gpu/drm/xe/abi/guc_klvs_abi.h
>>> +++ b/drivers/gpu/drm/xe/abi/guc_klvs_abi.h
>>> @@ -319,4 +319,11 @@ enum  {
>>>   #define GUC_KLV_VF_CFG_BEGIN_CONTEXT_ID_KEY    0x8a0b
>>>   #define GUC_KLV_VF_CFG_BEGIN_CONTEXT_ID_LEN    1u
>>>   +/*
>>> + * Workaround keys:
>>> + */
>>> +enum xe_guc_klv_ids {
>>> + GUC_WORKAROUND_KLV_BLOCK_INTERRUPTS_WHEN_MGSR_BLOCKED = 0x9002,
>>
>> how should we know the LEN of the particular W/A KLV ?
>> as this is the ABI header, IMO we should define that here along the KEY
> W/A KLV are alternative to flags to enable workaround. So length for 
> W/A KLV will always be 0. Should I still add length? May be I can 
> define #define GUC_WORKAROUND_KLV_LENGTH 0 which will be common for 
> all W/A KLVs.
Not true. Key 0x9008 in the latest GuC release takes a data field.

But where/how would you use the GUC_WORKAROUND_KLV_LENGTH define? The 
whole point of the guc_waklv_enable_simple function is that it adds a 
KLV which takes no data. Are you going to add a test to every w/a for 
it's length to be defined as zero and then call _simple() or _fancy() as 
appropriate? That is pointless complication. The size is static and is 
known to be zero for all current keys other than 0x9008. So there is 
zero point in adding code to check the size of those keys.

John.

>>
>>> +};
>>> +
>>>   #endif
>>> diff --git a/drivers/gpu/drm/xe/xe_guc_ads.c 
>>> b/drivers/gpu/drm/xe/xe_guc_ads.c
>>> index a98344a0ff4b..633e5fd9c738 100644
>>> --- a/drivers/gpu/drm/xe/xe_guc_ads.c
>>> +++ b/drivers/gpu/drm/xe/xe_guc_ads.c
>>> @@ -7,6 +7,8 @@
>>>     #include <drm/drm_managed.h>
>>>   +#include <generated/xe_wa_oob.h>
>>> +
>>>   #include "regs/xe_engine_regs.h"
>>>   #include "regs/xe_gt_regs.h"
>>>   #include "regs/xe_guc_regs.h"
>>> @@ -19,6 +21,7 @@
>>>   #include "xe_map.h"
>>>   #include "xe_mmio.h"
>>>   #include "xe_platform_types.h"
>>> +#include "xe_wa.h"
>>>     /* Slack of a few additional entries per engine */
>>>   #define ADS_REGSET_EXTRA_MAX    8
>>> @@ -279,23 +282,43 @@ static size_t calculate_golden_lrc_size(struct 
>>> xe_guc_ads *ads)
>>>       return total_size;
>>>   }
>>>   +static void guc_waklv_enable_simple(struct xe_guc_ads *ads,
>>> +                    enum xe_guc_klv_ids klv_id, u32 *offset, u32 
>>> *remain)
>>> +{
>>> +    u32 size;
>>> +    u32 klv_entry[] = {
>>> +        /* 16:16 key/length */
>>
>> drop this comment, code is self explanatory
>>
>>> +        FIELD_PREP(GUC_KLV_0_KEY, klv_id) |
>>> +        FIELD_PREP(GUC_KLV_0_LEN, 0),
>>> +        /* 0 dwords data */
>>> +    };
>>
>> you can define size here:
>>
>>         u32 size = sizeof(klv_entry);
> Sure
>>
>>> +
>>> +    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);
>>
>> this looks like our programming error so xe_gt_assert() should be
>> sufficient as we don't expect this ever happen in production
>>
>> but if you want to keep the WARN then use xe_gt_WARN() instead
> Ok
>>
>>> +    } 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_init(struct xe_guc_ads *ads)
>>>   {
>>> +    struct xe_gt *gt = ads_to_gt(ads);
>>>       u64 addr_ggtt;
>>>       u32 offset, remain, size;
>>>         offset = guc_ads_waklv_offset(ads);
>>>       remain = guc_ads_waklv_size(ads);
>>>   -    /*
>>> -     * Add workarounds here:
>>> -     *
>>> -     * if (want_wa_<name>) {
>>> -     *      size = guc_waklv_<name>(guc, offset, remain);
>>> -     *      offset += size;
>>> -     *      remain -= size;
>>> -     * }
>>> -     */
>>> +    if (XE_WA(gt, 14019882105))
>>> +        guc_waklv_enable_simple(ads,
>>> + GUC_WORKAROUND_KLV_BLOCK_INTERRUPTS_WHEN_MGSR_BLOCKED,
>>> +                    &offset, &remain);
>>
>> hmm, it looks that your implementation here in patch 2/2 is different
>> than suggested one in patch 1/2
> I wrapped up adding W/A KLV entry inside the function. I will refine 
> comment in patch 1/2 to align with patch 2/2.
>
> Regards,
> Badal
>>
>>>         size = guc_ads_waklv_size(ads) - remain;
>>>       if (!size)
>>> diff --git a/drivers/gpu/drm/xe/xe_wa_oob.rules 
>>> b/drivers/gpu/drm/xe/xe_wa_oob.rules
>>> index 48cdba1cbf95..a8d15f004b6c 100644
>>> --- a/drivers/gpu/drm/xe/xe_wa_oob.rules
>>> +++ b/drivers/gpu/drm/xe/xe_wa_oob.rules
>>> @@ -19,3 +19,4 @@
>>>           GRAPHICS_VERSION_RANGE(1270, 1274)
>>>           MEDIA_VERSION(1300)
>>>           PLATFORM(DG2)
>>> +14019882105    GRAPHICS_VERSION(2004), GRAPHICS_STEP(A0, B0)