[Intel-xe] [PATCH 07/12] drm/xe/gsc: Implement WA 14015076503

Wed Nov 8 22:40:31 UTC 2023

On 11/8/2023 14:35, Daniele Ceraolo Spurio wrote:
> On 11/8/2023 2:22 PM, John Harrison wrote:
>> On 10/27/2023 15:29, Daniele Ceraolo Spurio wrote:
>>> When the GSC FW is loaded, we need to inform it when a GSCCS reset is
>>> coming and then wait 200ms for it to get ready to process the reset.
>>>
>>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>>> Cc: John Harrison <john.c.harrison at intel.com>
>>> ---
>>>   drivers/gpu/drm/xe/Makefile           |  2 +-
>>>   drivers/gpu/drm/xe/regs/xe_gsc_regs.h | 10 ++++++
>>>   drivers/gpu/drm/xe/xe_gt.c            | 44 
>>> +++++++++++++++++++++++++++
>>>   drivers/gpu/drm/xe/xe_wa_oob.rules    |  1 +
>>>   4 files changed, 56 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>>> index 474b6044d054..4a442dcf4d79 100644
>>> --- a/drivers/gpu/drm/xe/Makefile
>>> +++ b/drivers/gpu/drm/xe/Makefile
>>> @@ -37,7 +37,7 @@ quiet_cmd_wa_oob = GEN     $(notdir $(generated_oob))
>>>   $(generated_oob) &: $(obj)/xe_gen_wa_oob 
>>> $(srctree)/$(src)/xe_wa_oob.rules
>>>       $(call cmd,wa_oob)
>>>   -$(obj)/xe_guc.o $(obj)/xe_migrate.o $(obj)/xe_ring_ops.o 
>>> $(obj)/xe_vm.o $(obj)/xe_wa.o: $(generated_oob)
>>> +$(obj)/xe_gt.o $(obj)/xe_guc.o $(obj)/xe_migrate.o 
>>> $(obj)/xe_ring_ops.o $(obj)/xe_vm.o $(obj)/xe_wa.o: $(generated_oob)
>> Is it not worth splitting this into multiple lines?
>
> Not sure how readable it'd be if we have a rule on multiple lines. 
> Maybe instead I can just add a variable with the list of files and use 
> that in the rule. Not sure it belongs in this patch (or series) though.
Yeah, I guess.

>
>>
>>>     # Please keep these build lists sorted!
>>>   diff --git a/drivers/gpu/drm/xe/regs/xe_gsc_regs.h 
>>> b/drivers/gpu/drm/xe/regs/xe_gsc_regs.h
>>> index 22d2ad9cb64d..9a84b55d66ee 100644
>>> --- a/drivers/gpu/drm/xe/regs/xe_gsc_regs.h
>>> +++ b/drivers/gpu/drm/xe/regs/xe_gsc_regs.h
>>> @@ -16,6 +16,13 @@
>>>   #define MTL_GSC_HECI1_BASE    0x00116000
>>>   #define MTL_GSC_HECI2_BASE    0x00117000
>>>   +#define HECI_H_CSR(base)    XE_REG((base) + 0x4)
>>> +#define   HECI_H_CSR_IE        REG_BIT(0)
>>> +#define   HECI_H_CSR_IS        REG_BIT(1)
>>> +#define   HECI_H_CSR_IG        REG_BIT(2)
>>> +#define   HECI_H_CSR_RDY    REG_BIT(3)
>>> +#define   HECI_H_CSR_RST    REG_BIT(4)
>>> +
>>>   /*
>>>    * The FWSTS register values are FW defined and can be different 
>>> between
>>>    * HECI1 and HECI2
>>> @@ -26,4 +33,7 @@
>>>   #define   HECI1_FWSTS1_PROXY_STATE_NORMAL        5
>>>   #define   HECI1_FWSTS1_INIT_COMPLETE            REG_BIT(9)
>>>   +#define HECI_H_GS1(base)    XE_REG((base) + 0xc4c)
>>> +#define   HECI_H_GS1_ER_PREP    REG_BIT(0)
>>> +
>>>   #endif
>>> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
>>> index 73c090762771..0e8e24bded1c 100644
>>> --- a/drivers/gpu/drm/xe/xe_gt.c
>>> +++ b/drivers/gpu/drm/xe/xe_gt.c
>>> @@ -10,8 +10,10 @@
>>>   #include <drm/drm_managed.h>
>>>   #include <drm/xe_drm.h>
>>>   +#include "generated/xe_wa_oob.h"
>>>   #include "instructions/xe_gfxpipe_commands.h"
>>>   #include "instructions/xe_mi_commands.h"
>>> +#include "regs/xe_gsc_regs.h"
>>>   #include "regs/xe_gt_regs.h"
>>>   #include "xe_assert.h"
>>>   #include "xe_bb.h"
>>> @@ -504,16 +506,58 @@ int xe_gt_init(struct xe_gt *gt)
>>>       return 0;
>>>   }
>>>   +static int gsc_fw_is_loaded(struct xe_gt *gt)
>>> +{
>>> +    return xe_mmio_read32(gt, HECI_FWSTS1(MTL_GSC_HECI1_BASE)) &
>>> +                  HECI1_FWSTS1_INIT_COMPLETE;
>>> +}
>> This function is duplicated exactly in the GSC source file. That 
>> seems bad. Either it should be exported and shared or the usage here 
>> should be moved to the GSC specific file.
>
> This was on purpose. We do have a bit of duplication for simpler 
> functions in the Xe codebase; e.g., guc_to_gt() is re-implemented in 
> every single file that uses it (I count 6 copies). I can see that this 
> is pushing it a bit tough, since it's a GSC internal detail in a non-
Seriously? Ew! Surely that has to be worth an xe_guc_accessor.h or 
something!

> GSC file, so I'll export it as you suggested. I might also instead 
> export the WA code from the GSC file to hide all the GSC internals in 
> there, will look at both options and see what looks better.

Keeping all the GSC internals in the GSC file seems better to me. Given 
that this is all firmware specific 'registers' rather than bspec defined 
hardware registers. But either way works.

John.

>
> Daniele
>
>>
>> John.
>>
>>> +
>>> +static void wa_14015076503_start(struct xe_gt *gt)
>>> +{
>>> +    /* WA only applies if the GSC is loaded */
>>> +    if (!XE_WA(gt, 14015076503) || !gsc_fw_is_loaded(gt))
>>> +        return;
>>> +
>>> +    /*
>>> +     * wa_14015076503: if the GSC FW is loaded, we need to alert it 
>>> that
>>> +     * we're going to do a GSC engine reset and then wait for 200ms 
>>> for the
>>> +     * FW to get ready for it.
>>> +     */
>>> +    xe_mmio_rmw32(gt, HECI_H_GS1(MTL_GSC_HECI2_BASE),
>>> +              0, HECI_H_GS1_ER_PREP);
>>> +
>>> +    /* make sure the reset bit is clear when writing the CSR reg */
>>> +    xe_mmio_rmw32(gt, HECI_H_CSR(MTL_GSC_HECI2_BASE),
>>> +              HECI_H_CSR_RST, HECI_H_CSR_IG);
>>> +    msleep(200);
>>> +
>>> +    return;
>>> +}
>>> +
>>> +static void
>>> +wa_14015076503_end(struct xe_gt *gt)
>>> +{
>>> +    if (!XE_WA(gt, 14015076503))
>>> +        return;
>>> +
>>> +    xe_mmio_rmw32(gt, HECI_H_GS1(MTL_GSC_HECI2_BASE),
>>> +              HECI_H_GS1_ER_PREP, 0);
>>> +}
>>> +
>>>   static int do_gt_reset(struct xe_gt *gt)
>>>   {
>>>       int err;
>>>   +    wa_14015076503_start(gt);
>>> +
>>>       xe_mmio_write32(gt, GDRST, GRDOM_FULL);
>>>       err = xe_mmio_wait32(gt, GDRST, GRDOM_FULL, 0, 5000, NULL, 
>>> false);
>>>       if (err)
>>>           xe_gt_err(gt, "failed to clear GEN11_GRDOM_FULL (%pe)\n",
>>>                 ERR_PTR(err));
>>>   +    wa_14015076503_end(gt);
>>> +
>>>       return err;
>>>   }
>>>   diff --git a/drivers/gpu/drm/xe/xe_wa_oob.rules 
>>> b/drivers/gpu/drm/xe/xe_wa_oob.rules
>>> index f3ff774dc4aa..132c35497f74 100644
>>> --- a/drivers/gpu/drm/xe/xe_wa_oob.rules
>>> +++ b/drivers/gpu/drm/xe/xe_wa_oob.rules
>>> @@ -19,3 +19,4 @@
>>>           SUBPLATFORM(DG2, G12)
>>>   16017236439    PLATFORM(PVC)
>>>   22010954014    PLATFORM(DG2)
>>> +14015076503    MEDIA_VERSION(1300)
>>
>