[PATCH v2 2/2] drm/xe/oa: Refactor Wa_18013179988 and Wa_14015568240

[Lucas De Marchi]

On Fri, Jan 17, 2025 at 02:19:52PM +0530, Aradhya Bhatia wrote:
>Hi Lucas,
>
>Thank you for reviewing the patches, and catching the obvious range
>mistakes. They should have been correct from my end in the first place.
>
>I agree with all but one of them. More details inline.
>
>On 16-01-2025 20:31, Lucas De Marchi wrote:
>> On Thu, Jan 16, 2025 at 02:40:04PM +0530, Aradhya Bhatia wrote:
>>> Refactor Wa_18013179988 and Wa_14015568240, to use the cleaner and
>>> newer workaround-check implementation, and drop the use of the platform
>>> based switch selection.
>>>
>>> While at it, fix the lineage number for the MTL.
>>>
>>> Signed-off-by: Aradhya Bhatia <aradhya.bhatia at intel.com>
>>> ---
>>> drivers/gpu/drm/xe/xe_oa.c         | 14 ++++++--------
>>> drivers/gpu/drm/xe/xe_wa_oob.rules |  3 +++
>>> 2 files changed, 9 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
>>> index eeb96b5f49e2..3736e4cd10f7 100644
>>> --- a/drivers/gpu/drm/xe/xe_oa.c
>>> +++ b/drivers/gpu/drm/xe/xe_oa.c
>>> @@ -12,6 +12,8 @@
>>> #include <drm/drm_managed.h>
>>> #include <uapi/drm/xe_drm.h>
>>>
>>> +#include <generated/xe_wa_oob.h>
>>> +
>>> #include "abi/guc_actions_slpc_abi.h"
>>> #include "instructions/xe_mi_commands.h"
>>> #include "regs/xe_engine_regs.h"
>>> @@ -35,6 +37,7 @@
>>> #include "xe_sched_job.h"
>>> #include "xe_sriov.h"
>>> #include "xe_sync.h"
>>> +#include "xe_wa.h"
>>>
>>> #define DEFAULT_POLL_FREQUENCY_HZ 200
>>> #define DEFAULT_POLL_PERIOD_NS (NSEC_PER_SEC / DEFAULT_POLL_FREQUENCY_HZ)
>>> @@ -1854,22 +1857,17 @@ u32 xe_oa_timestamp_frequency(struct xe_gt *gt)
>>>     u32 reg, shift;
>>>
>>>     /*
>>> -     * Wa_18013179988:dg2
>>> +     * Wa_18013179988:dg2/mtl
>>>      * Wa_14015568240:pvc
>>> -     * Wa_14015846243:mtl
>>
>> this comment should just be dropped so it doesn't get out of sync again
>> with what the implementation is doing.
>
>Okay!
>
>>
>>>      */
>>> -    switch (gt_to_xe(gt)->info.platform) {
>>> -    case XE_DG2:
>>> -    case XE_PVC:
>>> -    case XE_METEORLAKE:
>>> +    if (XE_WA(gt, 18013179988) || XE_WA(gt, 14015568240)) {
>>>         xe_pm_runtime_get(gt_to_xe(gt));
>>>         reg = xe_mmio_read32(&gt->mmio, RPM_CONFIG0);
>>>         xe_pm_runtime_put(gt_to_xe(gt));
>>>
>>>         shift = REG_FIELD_GET(RPM_CONFIG0_CTC_SHIFT_PARAMETER_MASK, reg);
>>>         return gt->info.reference_clock << (3 - shift);
>>> -
>>> -    default:
>>> +    } else {
>>>         return gt->info.reference_clock;
>>>     }
>>> }
>>> diff --git a/drivers/gpu/drm/xe/xe_wa_oob.rules b/drivers/gpu/drm/xe/
>>> xe_wa_oob.rules
>>> index 536b91cc4d3f..b79b78d5feba 100644
>>> --- a/drivers/gpu/drm/xe/xe_wa_oob.rules
>>> +++ b/drivers/gpu/drm/xe/xe_wa_oob.rules
>>> @@ -44,3 +44,6 @@ no_media_l3    MEDIA_VERSION(3000)
>>>         MEDIA_VERSION(3000), MEDIA_STEP(A0, B0)
>>> 16021333562    GRAPHICS_VERSION_RANGE(1200, 1300)
>>> 14016712196    GRAPHICS_VERSION_RANGE(1255, 1274)
>>> +14015568240    GRAPHICS_VERSION_RANGE(1255, 1260)
>>
>> why not 1274 as upper end?
>
>This is the one I couldn't follow. The wa db does not have 1274 filed in
>for this lineage number at all. Am I missing something else?

no, 1260 is right... I guess I looked in the wrong place while
reviewing.

Lucas De Marchi

>
>>
>>> +18013179988    GRAPHICS_VERSION(1255)
>>> +        GRAPHICS_VERSION_RANGE(1270, 1300)
>>
>> humn... I think you are missing the fact that the graphics/media
>> versions use **inclusive** ranges:
>>
>> /**                                                                                       
>>  * XE_RTP_RULE_GRAPHICS_VERSION_RANGE - Create rule matching a range of graphics version
>>  * @ver_start__: First graphics IP version to match
>>  * @ver_end__: Last graphics IP version to match
>>  *
>>  * Note that the range matching this rule is [ @ver_start__,
>> @ver_end__ ], i.e.
>>  * inclusive on both sides
>>  *
>>  * Refer to XE_RTP_RULES() for expected usage.
>>  */
>>
>> So assuming we want to drop 1260, this would be:
>>
>>
>> 18013179988    GRAPHICS_VERSION(1255)
>>         GRAPHICS_VERSION_RANGE(1270, 1274)
>
>Right, yes! This would indeed be the correct way. Will fix it up in v3.
>
>
>Regards
>Aradhya
>
>