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

["Nilawar, Badal" <badal.nilawar@intel.com>]

On 26-03-2024 16:51, Michal Wajdeczko wrote:
> 
> 
> 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
Sure, will remove calculation part. Will just add /* Add workarounds 
here */.
> 
>>
>> 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)
>>