Re: [Intel-gfx] [PATCH v5 02/10] drm/i915/guc: Add XE_LP registers for GuC error state capture.

2022-01-28 Thread Teres Alexis, Alan Previn
Facepalming myself - yes you're right - that's an easy fix...
move the device specific ext-list into the guc_capture_priv structure
which is allocated per gt. 

Thanks Jani. 

...alan

On Thu, 2022-01-27 at 11:30 +0200, Jani Nikula wrote:
> On Wed, 26 Jan 2022, "Teres Alexis, Alan Previn" 
>  wrote:
> > Thanks Jani for taking the time to review... 
> > 
> > 1. apologies on the const issue, this is my bad, i think it was
> > one of the comments from earlier rev not sure how i missed it.
> > Will fix this on next rev.
> > 
> > 2. I do have a question below on the const for one of specific types
> > of tables. Need your thoughts
> > 
> > ...alan
> > 
> > 
> > On Wed, 2022-01-26 at 20:13 +0200, Jani Nikula wrote:
> > > On Wed, 26 Jan 2022, Alan Previn  
> > > wrote:
> > > > Add device specific tables and register lists to cover different engines
> > > > class types for GuC error state capture for XE_LP products.
> > > > 
> > ...
> > 
> > > > +static struct __ext_steer_reg xelpd_extregs[] = {
> > > > +   {"GEN7_SAMPLER_INSTDONE", GEN7_SAMPLER_INSTDONE},
> > > > +   {"GEN7_ROW_INSTDONE", GEN7_ROW_INSTDONE}
> > > > +};
> > > 
> > > Either this needs to be const or, if it needs to be mutable, moved to
> > > device specific data.
> > > 
> > > Ditto for all such things all over the place.
> > > 
> > > BR,
> > > Jani.
> > 
> > I had a question though... the list of registers like the one above as well
> > as below shall be made const... however, the table-of-lists (see farther 
> > down), contains a pointer to "extended_regs"
> > that shall be allocated at startup - is it okay for that list to remain 
> > non-const
> > since the others with actual register offsets remain const?
> 
> A static mutable array like this is module or driver specific. Your
> allocation is device specific.
> 
> Sure, you have a check in there with /* already populated */ comment on
> the module specific data to avoid allocating it multiple times.
> 
> Now, consider probing two devices with different properties. The latter
> one will use the stuff you allocated for the first device. It will get
> really tricky really quickly.
> 
> Pretty much the rule is no static (or global) non-const data for
> anything. We do have to make some exceptions, but every one of them adds
> to the burden of checking if they're going to be a problem, maybe later
> on if not right now. So it's not so much about being const per se, it's
> about ensuring we don't screw up with device specific data.
> 
> 
> BR,
> Jani.
> 
> 
> > Alan: will add const for this and above tables:
> > static struct __guc_mmio_reg_descr xe_lpd_global_regs[] = {
> > COMMON_BASE_GLOBAL(),
> > COMMON_GEN9BASE_GLOBAL(),
> > COMMON_GEN12BASE_GLOBAL(),
> > };
> > 
> > Is this okay to not be const?:
> > static struct __guc_mmio_reg_descr_group default_lists[] = {
> > MAKE_REGLIST(default_global_regs, PF, GLOBAL, 0),
> > MAKE_REGLIST(default_rc_class_regs, PF, ENGINE_CLASS, 
> > GUC_RENDER_CLASS),
> > MAKE_REGLIST(xe_lpd_rc_inst_regs, PF, ENGINE_INSTANCE, 
> > GUC_RENDER_CLASS),
> > MAKE_REGLIST(empty_regs_list, PF, ENGINE_CLASS, 
> > GUC_VIDEO_CLASS),
> > MAKE_REGLIST(xe_lpd_vd_inst_regs, PF, ENGINE_INSTANCE, 
> > GUC_VIDEO_CLASS),
> > MAKE_REGLIST(empty_regs_list, PF, ENGINE_CLASS, 
> > GUC_VIDEOENHANCE_CLASS),
> > MAKE_REGLIST(xe_lpd_vec_inst_regs, PF, ENGINE_INSTANCE, 
> > GUC_VIDEOENHANCE_CLASS),
> > MAKE_REGLIST(empty_regs_list, PF, ENGINE_CLASS, 
> > GUC_BLITTER_CLASS),
> > MAKE_REGLIST(xe_lpd_blt_inst_regs, PF, ENGINE_INSTANCE, 
> > GUC_BLITTER_CLASS),
> > {}
> > };
> > 
> > 
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center



Re: [Intel-gfx] [PATCH v5 02/10] drm/i915/guc: Add XE_LP registers for GuC error state capture.

2022-01-27 Thread Jani Nikula
On Wed, 26 Jan 2022, "Teres Alexis, Alan Previn" 
 wrote:
> Thanks Jani for taking the time to review... 
>
> 1. apologies on the const issue, this is my bad, i think it was
> one of the comments from earlier rev not sure how i missed it.
> Will fix this on next rev.
>
> 2. I do have a question below on the const for one of specific types
> of tables. Need your thoughts
>
> ...alan
>
>
> On Wed, 2022-01-26 at 20:13 +0200, Jani Nikula wrote:
>> On Wed, 26 Jan 2022, Alan Previn  wrote:
>> > Add device specific tables and register lists to cover different engines
>> > class types for GuC error state capture for XE_LP products.
>> > 
> ...
>
>> > +static struct __ext_steer_reg xelpd_extregs[] = {
>> > +  {"GEN7_SAMPLER_INSTDONE", GEN7_SAMPLER_INSTDONE},
>> > +  {"GEN7_ROW_INSTDONE", GEN7_ROW_INSTDONE}
>> > +};
>> 
>> Either this needs to be const or, if it needs to be mutable, moved to
>> device specific data.
>> 
>> Ditto for all such things all over the place.
>> 
>> BR,
>> Jani.
>
>
> I had a question though... the list of registers like the one above as well
> as below shall be made const... however, the table-of-lists (see farther 
> down), contains a pointer to "extended_regs"
> that shall be allocated at startup - is it okay for that list to remain 
> non-const
> since the others with actual register offsets remain const?

A static mutable array like this is module or driver specific. Your
allocation is device specific.

Sure, you have a check in there with /* already populated */ comment on
the module specific data to avoid allocating it multiple times.

Now, consider probing two devices with different properties. The latter
one will use the stuff you allocated for the first device. It will get
really tricky really quickly.

Pretty much the rule is no static (or global) non-const data for
anything. We do have to make some exceptions, but every one of them adds
to the burden of checking if they're going to be a problem, maybe later
on if not right now. So it's not so much about being const per se, it's
about ensuring we don't screw up with device specific data.


BR,
Jani.


>
> Alan: will add const for this and above tables:
>   static struct __guc_mmio_reg_descr xe_lpd_global_regs[] = {
>   COMMON_BASE_GLOBAL(),
>   COMMON_GEN9BASE_GLOBAL(),
>   COMMON_GEN12BASE_GLOBAL(),
>   };
>
> Is this okay to not be const?:
>   static struct __guc_mmio_reg_descr_group default_lists[] = {
>   MAKE_REGLIST(default_global_regs, PF, GLOBAL, 0),
>   MAKE_REGLIST(default_rc_class_regs, PF, ENGINE_CLASS, 
> GUC_RENDER_CLASS),
>   MAKE_REGLIST(xe_lpd_rc_inst_regs, PF, ENGINE_INSTANCE, 
> GUC_RENDER_CLASS),
>   MAKE_REGLIST(empty_regs_list, PF, ENGINE_CLASS, 
> GUC_VIDEO_CLASS),
>   MAKE_REGLIST(xe_lpd_vd_inst_regs, PF, ENGINE_INSTANCE, 
> GUC_VIDEO_CLASS),
>   MAKE_REGLIST(empty_regs_list, PF, ENGINE_CLASS, 
> GUC_VIDEOENHANCE_CLASS),
>   MAKE_REGLIST(xe_lpd_vec_inst_regs, PF, ENGINE_INSTANCE, 
> GUC_VIDEOENHANCE_CLASS),
>   MAKE_REGLIST(empty_regs_list, PF, ENGINE_CLASS, 
> GUC_BLITTER_CLASS),
>   MAKE_REGLIST(xe_lpd_blt_inst_regs, PF, ENGINE_INSTANCE, 
> GUC_BLITTER_CLASS),
>   {}
>   };
>
>

-- 
Jani Nikula, Intel Open Source Graphics Center


Re: [Intel-gfx] [PATCH v5 02/10] drm/i915/guc: Add XE_LP registers for GuC error state capture.

2022-01-26 Thread Teres Alexis, Alan Previn

Thanks Jani for taking the time to review... 

1. apologies on the const issue, this is my bad, i think it was
one of the comments from earlier rev not sure how i missed it.
Will fix this on next rev.

2. I do have a question below on the const for one of specific types
of tables. Need your thoughts

...alan


On Wed, 2022-01-26 at 20:13 +0200, Jani Nikula wrote:
> On Wed, 26 Jan 2022, Alan Previn  wrote:
> > Add device specific tables and register lists to cover different engines
> > class types for GuC error state capture for XE_LP products.
> > 
...

> > +static struct __ext_steer_reg xelpd_extregs[] = {
> > +   {"GEN7_SAMPLER_INSTDONE", GEN7_SAMPLER_INSTDONE},
> > +   {"GEN7_ROW_INSTDONE", GEN7_ROW_INSTDONE}
> > +};
> 
> Either this needs to be const or, if it needs to be mutable, moved to
> device specific data.
> 
> Ditto for all such things all over the place.
> 
> BR,
> Jani.


I had a question though... the list of registers like the one above as well
as below shall be made const... however, the table-of-lists (see farther down), 
contains a pointer to "extended_regs"
that shall be allocated at startup - is it okay for that list to remain 
non-const
since the others with actual register offsets remain const?

Alan: will add const for this and above tables:
static struct __guc_mmio_reg_descr xe_lpd_global_regs[] = {
COMMON_BASE_GLOBAL(),
COMMON_GEN9BASE_GLOBAL(),
COMMON_GEN12BASE_GLOBAL(),
};

Is this okay to not be const?:
static struct __guc_mmio_reg_descr_group default_lists[] = {
MAKE_REGLIST(default_global_regs, PF, GLOBAL, 0),
MAKE_REGLIST(default_rc_class_regs, PF, ENGINE_CLASS, 
GUC_RENDER_CLASS),
MAKE_REGLIST(xe_lpd_rc_inst_regs, PF, ENGINE_INSTANCE, 
GUC_RENDER_CLASS),
MAKE_REGLIST(empty_regs_list, PF, ENGINE_CLASS, 
GUC_VIDEO_CLASS),
MAKE_REGLIST(xe_lpd_vd_inst_regs, PF, ENGINE_INSTANCE, 
GUC_VIDEO_CLASS),
MAKE_REGLIST(empty_regs_list, PF, ENGINE_CLASS, 
GUC_VIDEOENHANCE_CLASS),
MAKE_REGLIST(xe_lpd_vec_inst_regs, PF, ENGINE_INSTANCE, 
GUC_VIDEOENHANCE_CLASS),
MAKE_REGLIST(empty_regs_list, PF, ENGINE_CLASS, 
GUC_BLITTER_CLASS),
MAKE_REGLIST(xe_lpd_blt_inst_regs, PF, ENGINE_INSTANCE, 
GUC_BLITTER_CLASS),
{}
};




Re: [Intel-gfx] [PATCH v5 02/10] drm/i915/guc: Add XE_LP registers for GuC error state capture.

2022-01-26 Thread Jani Nikula
On Wed, 26 Jan 2022, Alan Previn  wrote:
> Add device specific tables and register lists to cover different engines
> class types for GuC error state capture for XE_LP products.
>
> Also, add runtime allocation and freeing of extended register lists
> for registers that need steering identifiers that depend on
> the detected HW config.
>
> Signed-off-by: Alan Previn 
> ---
>  drivers/gpu/drm/i915/gt/uc/guc_capture_fwif.h |   2 +
>  .../gpu/drm/i915/gt/uc/intel_guc_capture.c| 207 +++---
>  drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h   |   4 +-
>  3 files changed, 180 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/guc_capture_fwif.h 
> b/drivers/gpu/drm/i915/gt/uc/guc_capture_fwif.h
> index 15b8c02b8a76..a2f97d04ff18 100644
> --- a/drivers/gpu/drm/i915/gt/uc/guc_capture_fwif.h
> +++ b/drivers/gpu/drm/i915/gt/uc/guc_capture_fwif.h
> @@ -24,6 +24,8 @@ struct __guc_mmio_reg_descr_group {
>   u32 owner; /* see enum guc_capture_owner */
>   u32 type; /* see enum guc_capture_type */
>   u32 engine; /* as per MAX_ENGINE_CLASS */
> + int num_ext;
> + struct __guc_mmio_reg_descr *ext;
>  };
>  
>  struct __guc_state_capture_priv {
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c 
> b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> index 06873d617b8b..b6882074fc8d 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> @@ -19,40 +19,101 @@
>   * NOTE: For engine-registers, GuC only needs the register offsets
>   *   from the engine-mmio-base
>   */
> +#define COMMON_GEN12BASE_GLOBAL() \
> + {GEN12_FAULT_TLB_DATA0,0,  0, "GEN12_FAULT_TLB_DATA0"}, \
> + {GEN12_FAULT_TLB_DATA1,0,  0, "GEN12_FAULT_TLB_DATA1"}, \
> + {FORCEWAKE_MT, 0,  0, "FORCEWAKE_MT"}, \
> + {GEN12_AUX_ERR_DBG,0,  0, "GEN12_AUX_ERR_DBG"}, \
> + {GEN12_GAM_DONE,   0,  0, "GEN12_GAM_DONE"}, \
> + {GEN12_RING_FAULT_REG, 0,  0, "GEN12_RING_FAULT_REG"}
> +
> +#define COMMON_GEN12BASE_ENGINE_INSTANCE() \
> + {RING_PSMI_CTL(0), 0,  0, "RING_PSMI_CTL"}, \
> + {RING_ESR(0),  0,  0, "RING_ESR"}, \
> + {RING_DMA_FADD(0), 0,  0, "RING_DMA_FADD_LOW32"}, \
> + {RING_DMA_FADD_UDW(0), 0,  0, "RING_DMA_FADD_UP32"}, \
> + {RING_IPEIR(0),0,  0, "RING_IPEIR"}, \
> + {RING_IPEHR(0),0,  0, "RING_IPEHR"}, \
> + {RING_INSTPS(0),   0,  0, "RING_INSTPS"}, \
> + {RING_BBADDR(0),   0,  0, "RING_BBADDR_LOW32"}, \
> + {RING_BBADDR_UDW(0),   0,  0, "RING_BBADDR_UP32"}, \
> + {RING_BBSTATE(0),  0,  0, "RING_BBSTATE"}, \
> + {CCID(0),  0,  0, "CCID"}, \
> + {RING_ACTHD(0),0,  0, "RING_ACTHD_LOW32"}, \
> + {RING_ACTHD_UDW(0),0,  0, "RING_ACTHD_UP32"}, \
> + {RING_INSTPM(0),   0,  0, "RING_INSTPM"}, \
> + {RING_NOPID(0),0,  0, "RING_NOPID"}, \
> + {RING_START(0),0,  0, "RING_START"}, \
> + {RING_HEAD(0), 0,  0, "RING_HEAD"}, \
> + {RING_TAIL(0), 0,  0, "RING_TAIL"}, \
> + {RING_CTL(0),  0,  0, "RING_CTL"}, \
> + {RING_MI_MODE(0),  0,  0, "RING_MI_MODE"}, \
> + {RING_CONTEXT_CONTROL(0),  0,  0, "RING_CONTEXT_CONTROL"}, \
> + {RING_INSTDONE(0), 0,  0, "RING_INSTDONE"}, \
> + {RING_HWS_PGA(0),  0,  0, "RING_HWS_PGA"}, \
> + {RING_MODE_GEN7(0),0,  0, "RING_MODE_GEN7"}, \
> + {GEN8_RING_PDP_LDW(0, 0),  0,  0, "GEN8_RING_PDP0_LDW"}, \
> + {GEN8_RING_PDP_UDW(0, 0),  0,  0, "GEN8_RING_PDP0_UDW"}, \
> + {GEN8_RING_PDP_LDW(0, 1),  0,  0, "GEN8_RING_PDP1_LDW"}, \
> + {GEN8_RING_PDP_UDW(0, 1),  0,  0, "GEN8_RING_PDP1_UDW"}, \
> + {GEN8_RING_PDP_LDW(0, 2),  0,  0, "GEN8_RING_PDP2_LDW"}, \
> + {GEN8_RING_PDP_UDW(0, 2),  0,  0, "GEN8_RING_PDP2_UDW"}, \
> + {GEN8_RING_PDP_LDW(0, 3),  0,  0, "GEN8_RING_PDP3_LDW"}, \
> + {GEN8_RING_PDP_UDW(0, 3),  0,  0, "GEN8_RING_PDP3_UDW"}
> +
> +#define COMMON_GEN12BASE_HAS_EU() \
> + {EIR,  0,  0, "EIR"}
> +
> +#define COMMON_GEN12BASE_RENDER() \
> + {GEN7_SC_INSTDONE, 0,  0, "GEN7_SC_INSTDONE"}, \
> + {GEN12_SC_INSTDONE_EXTRA,  0,  0, "GEN12_SC_INSTDONE_EXTRA"}, \
> + {GEN12_SC_INSTDONE_EXTRA2, 0,  0, "GEN12_SC_INSTDONE_EXTRA2"}
> +
> +#define COMMON_GEN12BASE_VEC() \
> + {GEN12_SFC_DONE(0),0,  0, "GEN12_SFC_DONE0"}, \
> + {GEN12_SFC_DONE(1),0,  0, "GEN12_SFC_DONE1"}, \
> + {GEN12_SFC_DONE(2),0,  0, "GEN12_SFC_DONE2"}, \
> + {GEN12_SFC_DONE(3),0,  0, "GEN12_SFC_DONE3"}
> +
>  /* XE_LPD - Global */
>  static struct __guc_mmio_reg_descr 

[Intel-gfx] [PATCH v5 02/10] drm/i915/guc: Add XE_LP registers for GuC error state capture.

2022-01-26 Thread Alan Previn
Add device specific tables and register lists to cover different engines
class types for GuC error state capture for XE_LP products.

Also, add runtime allocation and freeing of extended register lists
for registers that need steering identifiers that depend on
the detected HW config.

Signed-off-by: Alan Previn 
---
 drivers/gpu/drm/i915/gt/uc/guc_capture_fwif.h |   2 +
 .../gpu/drm/i915/gt/uc/intel_guc_capture.c| 207 +++---
 drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h   |   4 +-
 3 files changed, 180 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/guc_capture_fwif.h 
b/drivers/gpu/drm/i915/gt/uc/guc_capture_fwif.h
index 15b8c02b8a76..a2f97d04ff18 100644
--- a/drivers/gpu/drm/i915/gt/uc/guc_capture_fwif.h
+++ b/drivers/gpu/drm/i915/gt/uc/guc_capture_fwif.h
@@ -24,6 +24,8 @@ struct __guc_mmio_reg_descr_group {
u32 owner; /* see enum guc_capture_owner */
u32 type; /* see enum guc_capture_type */
u32 engine; /* as per MAX_ENGINE_CLASS */
+   int num_ext;
+   struct __guc_mmio_reg_descr *ext;
 };
 
 struct __guc_state_capture_priv {
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c 
b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
index 06873d617b8b..b6882074fc8d 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
@@ -19,40 +19,101 @@
  * NOTE: For engine-registers, GuC only needs the register offsets
  *   from the engine-mmio-base
  */
+#define COMMON_GEN12BASE_GLOBAL() \
+   {GEN12_FAULT_TLB_DATA0,0,  0, "GEN12_FAULT_TLB_DATA0"}, \
+   {GEN12_FAULT_TLB_DATA1,0,  0, "GEN12_FAULT_TLB_DATA1"}, \
+   {FORCEWAKE_MT, 0,  0, "FORCEWAKE_MT"}, \
+   {GEN12_AUX_ERR_DBG,0,  0, "GEN12_AUX_ERR_DBG"}, \
+   {GEN12_GAM_DONE,   0,  0, "GEN12_GAM_DONE"}, \
+   {GEN12_RING_FAULT_REG, 0,  0, "GEN12_RING_FAULT_REG"}
+
+#define COMMON_GEN12BASE_ENGINE_INSTANCE() \
+   {RING_PSMI_CTL(0), 0,  0, "RING_PSMI_CTL"}, \
+   {RING_ESR(0),  0,  0, "RING_ESR"}, \
+   {RING_DMA_FADD(0), 0,  0, "RING_DMA_FADD_LOW32"}, \
+   {RING_DMA_FADD_UDW(0), 0,  0, "RING_DMA_FADD_UP32"}, \
+   {RING_IPEIR(0),0,  0, "RING_IPEIR"}, \
+   {RING_IPEHR(0),0,  0, "RING_IPEHR"}, \
+   {RING_INSTPS(0),   0,  0, "RING_INSTPS"}, \
+   {RING_BBADDR(0),   0,  0, "RING_BBADDR_LOW32"}, \
+   {RING_BBADDR_UDW(0),   0,  0, "RING_BBADDR_UP32"}, \
+   {RING_BBSTATE(0),  0,  0, "RING_BBSTATE"}, \
+   {CCID(0),  0,  0, "CCID"}, \
+   {RING_ACTHD(0),0,  0, "RING_ACTHD_LOW32"}, \
+   {RING_ACTHD_UDW(0),0,  0, "RING_ACTHD_UP32"}, \
+   {RING_INSTPM(0),   0,  0, "RING_INSTPM"}, \
+   {RING_NOPID(0),0,  0, "RING_NOPID"}, \
+   {RING_START(0),0,  0, "RING_START"}, \
+   {RING_HEAD(0), 0,  0, "RING_HEAD"}, \
+   {RING_TAIL(0), 0,  0, "RING_TAIL"}, \
+   {RING_CTL(0),  0,  0, "RING_CTL"}, \
+   {RING_MI_MODE(0),  0,  0, "RING_MI_MODE"}, \
+   {RING_CONTEXT_CONTROL(0),  0,  0, "RING_CONTEXT_CONTROL"}, \
+   {RING_INSTDONE(0), 0,  0, "RING_INSTDONE"}, \
+   {RING_HWS_PGA(0),  0,  0, "RING_HWS_PGA"}, \
+   {RING_MODE_GEN7(0),0,  0, "RING_MODE_GEN7"}, \
+   {GEN8_RING_PDP_LDW(0, 0),  0,  0, "GEN8_RING_PDP0_LDW"}, \
+   {GEN8_RING_PDP_UDW(0, 0),  0,  0, "GEN8_RING_PDP0_UDW"}, \
+   {GEN8_RING_PDP_LDW(0, 1),  0,  0, "GEN8_RING_PDP1_LDW"}, \
+   {GEN8_RING_PDP_UDW(0, 1),  0,  0, "GEN8_RING_PDP1_UDW"}, \
+   {GEN8_RING_PDP_LDW(0, 2),  0,  0, "GEN8_RING_PDP2_LDW"}, \
+   {GEN8_RING_PDP_UDW(0, 2),  0,  0, "GEN8_RING_PDP2_UDW"}, \
+   {GEN8_RING_PDP_LDW(0, 3),  0,  0, "GEN8_RING_PDP3_LDW"}, \
+   {GEN8_RING_PDP_UDW(0, 3),  0,  0, "GEN8_RING_PDP3_UDW"}
+
+#define COMMON_GEN12BASE_HAS_EU() \
+   {EIR,  0,  0, "EIR"}
+
+#define COMMON_GEN12BASE_RENDER() \
+   {GEN7_SC_INSTDONE, 0,  0, "GEN7_SC_INSTDONE"}, \
+   {GEN12_SC_INSTDONE_EXTRA,  0,  0, "GEN12_SC_INSTDONE_EXTRA"}, \
+   {GEN12_SC_INSTDONE_EXTRA2, 0,  0, "GEN12_SC_INSTDONE_EXTRA2"}
+
+#define COMMON_GEN12BASE_VEC() \
+   {GEN12_SFC_DONE(0),0,  0, "GEN12_SFC_DONE0"}, \
+   {GEN12_SFC_DONE(1),0,  0, "GEN12_SFC_DONE1"}, \
+   {GEN12_SFC_DONE(2),0,  0, "GEN12_SFC_DONE2"}, \
+   {GEN12_SFC_DONE(3),0,  0, "GEN12_SFC_DONE3"}
+
 /* XE_LPD - Global */
 static struct __guc_mmio_reg_descr xe_lpd_global_regs[] = {
-   {GEN12_RING_FAULT_REG, 0,  0, "GEN12_RING_FAULT_REG"}
+   COMMON_GEN12BASE_GLOBAL(),
 };
 
 /*