Re: [PATCH 1/2] drm/xe/lnl: Apply Wa_22019338487

["Belgaumkar, Vinay" <vinay.belgaumkar@intel.com>]

On 4/23/2024 10:43 AM, Lucas De Marchi wrote:
> On Mon, Apr 22, 2024 at 11:10:43AM GMT, Belgaumkar, Vinay wrote:
>>
>> On 4/22/2024 11:09 AM, Lucas De Marchi wrote:
>>> On Fri, Apr 19, 2024 at 01:13:43PM GMT, Vinay Belgaumkar wrote:
>>>> This WA requires us to limit media GT frequency requests to a certain
>>>> cap value during driver load as well as after driver unload. Freq 
>>>> limits
>>>> are restored after driver load completes, so perf will not be
>>>> affected during normal operations.
>>>>
>>>> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
>>>> ---
>>>> drivers/gpu/drm/xe/xe_device.c |  6 +++++
>>>> drivers/gpu/drm/xe/xe_gsc.c    |  8 +++++++
>>>> drivers/gpu/drm/xe/xe_guc_pc.c | 40 ++++++++++++++++++++++++++++++++--
>>>> drivers/gpu/drm/xe/xe_guc_pc.h |  3 +++
>>>> 4 files changed, 55 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/xe/xe_device.c 
>>>> b/drivers/gpu/drm/xe/xe_device.c
>>>> index d85a2ba0a057..e29c152a6c4e 100644
>>>> --- a/drivers/gpu/drm/xe/xe_device.c
>>>> +++ b/drivers/gpu/drm/xe/xe_device.c
>>>> @@ -30,6 +30,7 @@
>>>> #include "xe_gsc_proxy.h"
>>>> #include "xe_gt.h"
>>>> #include "xe_gt_mcr.h"
>>>> +#include "xe_guc_pc.h"
>>>> #include "xe_hwmon.h"
>>>> #include "xe_irq.h"
>>>> #include "xe_memirq.h"
>>>> @@ -336,6 +337,7 @@ static void xe_driver_flr(struct xe_device *xe)
>>>> {
>>>>     const unsigned int flr_timeout = 3 * MICRO; /* specs recommend 
>>>> a 3s wait */
>>>>     struct xe_gt *gt = xe_root_mmio_gt(xe);
>>>> +    struct xe_guc_pc *pc = &gt->uc.guc.pc;
>>>>     int ret;
>>>>
>>>>     if (xe_mmio_read32(gt, GU_CNTL_PROTECTED) & DRIVERINT_FLR_DIS) {
>>>> @@ -343,6 +345,10 @@ static void xe_driver_flr(struct xe_device *xe)
>>>>         return;
>>>>     }
>>>>
>>>> +    /* Set requested freq to mert_freq_cap before FLR */
>>>> +    if (xe_guc_pc_needs_wa_22019338487(pc))
>>>> +        pc_set_cur_freq(pc, min(xe_guc_pc_mert_freq_cap(pc), 
>>>> pc->rpe_freq));
>>>> +
>>>>     drm_dbg(&xe->drm, "Triggering Driver-FLR\n");
>>>>
>>>>     /*
>>>> diff --git a/drivers/gpu/drm/xe/xe_gsc.c b/drivers/gpu/drm/xe/xe_gsc.c
>>>> index 60202b903687..556e73fca813 100644
>>>> --- a/drivers/gpu/drm/xe/xe_gsc.c
>>>> +++ b/drivers/gpu/drm/xe/xe_gsc.c
>>>> @@ -19,6 +19,7 @@
>>>> #include "xe_gt.h"
>>>> #include "xe_gt_mcr.h"
>>>> #include "xe_gt_printk.h"
>>>> +#include "xe_guc_pc.h"
>>>> #include "xe_huc.h"
>>>> #include "xe_map.h"
>>>> #include "xe_mmio.h"
>>>> @@ -339,6 +340,7 @@ static void gsc_work(struct work_struct *work)
>>>>     struct xe_gsc *gsc = container_of(work, typeof(*gsc), work);
>>>>     struct xe_gt *gt = gsc_to_gt(gsc);
>>>>     struct xe_device *xe = gt_to_xe(gt);
>>>> +    struct xe_guc_pc *pc = &gt->uc.guc.pc;
>>>>     u32 actions;
>>>>     int ret;
>>>>
>>>> @@ -367,6 +369,12 @@ static void gsc_work(struct work_struct *work)
>>>>     if (actions & GSC_ACTION_SW_PROXY)
>>>>         xe_gsc_proxy_request_handler(gsc);
>>>>
>>>> +    /* Revert the min/max freq limits as we're done with 
>>>> GSC/driver load */
>>>> +    if (xe_guc_pc_needs_wa_22019338487(pc)) {
>>>> +        xe_guc_pc_set_max_freq(pc, pc->rp0_freq);
>>>> +        xe_guc_pc_set_min_freq(pc, pc->rpe_freq);
>>>> +    }
>>>> +
>>>> out:
>>>>     xe_force_wake_put(gt_to_fw(gt), XE_FW_GSC);
>>>>     xe_pm_runtime_put(xe);
>>>> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c 
>>>> b/drivers/gpu/drm/xe/xe_guc_pc.c
>>>> index 521ae24f2314..7f82b6c2ad3c 100644
>>>> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
>>>> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
>>>> @@ -40,6 +40,8 @@
>>>> #define GT_FREQUENCY_MULTIPLIER    50
>>>> #define GT_FREQUENCY_SCALER    3
>>>>
>>>> +#define LNL_MERT_FREQ_CAP    800
>>>> +
>>>> /**
>>>>  * DOC: GuC Power Conservation (PC)
>>>>  *
>>>> @@ -237,7 +239,7 @@ static void pc_set_manual_rp_ctrl(struct 
>>>> xe_guc_pc *pc, bool enable)
>>>>     xe_mmio_write32(gt, RP_CONTROL, state);
>>>> }
>>>>
>>>> -static void pc_set_cur_freq(struct xe_guc_pc *pc, u32 freq)
>>>> +void pc_set_cur_freq(struct xe_guc_pc *pc, u32 freq)
>>>> {
>>>>     struct xe_gt *gt = pc_to_gt(pc);
>>>>     u32 rpnswreq;
>>>> @@ -673,6 +675,25 @@ static void pc_init_fused_rp_values(struct 
>>>> xe_guc_pc *pc)
>>>>         tgl_init_fused_rp_values(pc);
>>>> }
>>>>
>>>> +bool xe_guc_pc_needs_wa_22019338487(struct xe_guc_pc *pc)
>>>> +{
>>>> +    struct xe_gt *gt = pc_to_gt(pc);
>>>> +    struct xe_device *xe = gt_to_xe(gt);
>>>> +
>>>> +    if (MEDIA_VERx100(xe) == 2000 && xe_gt_is_media_type(gt))
>>>
>>> that's what xe_wa_oob.rules is for. Why are you adding it as a separate
>>> function that won't allow debugfs/.../workarounds to match reality?
>>
>> The workarounds are not initialized at this point, this is early in 
>> the driver load process.
>
> ugh... the _early() variant was supposed to be the sw part with
> minimal hw. Now we added a few super_early(),
> no_I_mean_earlier_than_super_early() variants... but with other names.
>
> How do we call xe_force_wake_init_gt() and xe_pcode_init() passing the
> gt without the gt even be initialized? Are we trying to create a house
> of cards? This init order is in an awful state :(. We should stop
> pretending we are getting the engine info from hwconfig and just fix the
> ordering so we can call xe_gt_init_early() and it really happens before
> things like loading guc.

Yup, doesn't look like an easy fix here. the GMDID is populated 
somewhere in xe_gt_init() which comes way after the early_init(). Cannot 
check for WAs unless we have that.

Thanks,

Vinay.

>
> Lucas De Marchi