Re: [Intel-gfx] [PATCH v5 02/10] drm/i915/guc: Add XE_LP registers for GuC error state capture.
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.
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.
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.
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.
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(), }; /*