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

[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" <alan.previn.teres.alexis at intel.com> 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 <alan.previn.teres.alexis at intel.com> 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