[Intel-xe] [PATCH 07/12] drm/xe/gsc: Implement WA 14015076503
Daniele Ceraolo Spurio
daniele.ceraolospurio at intel.com
Wed Nov 8 22:35:35 UTC 2023
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.
>
>> # 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-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.
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)
>
More information about the Intel-xe
mailing list