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

[Aradhya Bhatia]

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?

> 
>> +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