[PATCH v3 2/2] drm/xe/lnl: Enable GuC Wa_14019882105
John Harrison
john.c.harrison at intel.com
Thu Mar 28 00:18:41 UTC 2024
On 3/26/2024 04:21, 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 */ <<===
> + };
Which is in a common helper for a whole bunch of different KLV ids that
all take no data. So how are you going to work in the use of each
individual KLV id's data length definition? Either it must be generic
code that is doing unnecessary things such as taking a pointer to a data
block that is always empty, or it is coded specifically for no data and
does not need an extra define to tell it that there is no data.
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);
>>>
>>>> +
>>>> + 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)
More information about the Intel-xe
mailing list