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

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

On 25.03.2024 19:56, John Harrison wrote:
> On 3/25/2024 08:32, 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 ?
> By reading the spec.
> 
>> as this is the ABI header, IMO we should define that here along the KEY
> I strongly disagree with copying the entire GuC API spec into the source
> code just for the sake of copying the spec. The implementation of this
> w/a (and a whole bunch more that will follow) has no use for extra
> defines about sizes or fields. And creating defines that will never be
> used is a pointless exercise.

but without those defines in ABI we are using plain numbers in code:

+	u32 klv_entry[] = {
+		/* 16:16 key/length */
+		FIELD_PREP(GUC_KLV_0_KEY, klv_id) |
+		FIELD_PREP(GUC_KLV_0_LEN, 0),	<<=== hardcoded 0 len
+		/* 0 dwords data */		<<===
+	};

> 
>>
>>> +};
>>> +
>>>   #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);
>>
>>> +
>>> +    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
>>
>>> +    } 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
> It looks the same to me. It has simply optimised the the offset/remain
> calculation to be done inside the helper rather than outside it to avoid
> replication when all the other 'simple' w/a's are added. The actual
> behaviour is identical.

still you can move this 'optimization' to the comment in patch 1/2

alternatively reconsider the need for this comment in patch 1/2 if you
drop it immediately in next patch 2/2

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