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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 26 Jan 2022, "Teres Alexis, Alan Previn" <alan.previn.teres.alexis@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@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