[PATCH v3 2/2] drm/xe/lnl: Enable GuC Wa_14019882105
Nilawar, Badal
badal.nilawar at intel.com
Wed Mar 27 04:51:45 UTC 2024
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 at intel.com>
>>>> Signed-off-by: Badal Nilawar <badal.nilawar at 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)
>>
More information about the Intel-xe
mailing list