[Intel-gfx] [RFC 3/7] drm/i915/guc: Populate XE_LP register lists for GuC error state capture.

[Teres Alexis, Alan Previn]

Thanks Michal for reviewing the code. I will get all of these fixed.

I still would like continue to have a first patch with a skeleton table of registers
as the patch that focuses on the infrastructure and another patch just for the registers.

That sad, to align with your review comments, i shall ensure the first patch starts
with one valid register that doesnt get removed and also move the ext-list
functions and macros into that first patch. But keep full register table population
in the 2nd patch..

...alan



On Tue, 2021-11-23 at 22:55 +0100, Michal Wajdeczko wrote:
>  
> >  /********************************* Gen12 LP  *********************************/
> >  /************** GLOBAL *************/
> >  struct __guc_mmio_reg_descr gen12lp_global_regs[] = {
> > -	{SWF_ILK(0),               0,      0, "SWF_ILK0"},
> 
> we should avoid adding/removing code in same series
> 
> > +struct __guc_mmio_reg_descr_group xe_lpd_lists[] = {
> > +	MAKE_GCAP_REGLIST_DESCR(gen12lp_global_regs, INDEX_PF, TYPE_GLOBAL, 0),
> > +	MAKE_GCAP_REGLIST_DESCR(gen12lp_rc_class_regs, INDEX_PF, TYPE_ENGINE_CLASS, GUC_RENDER_CLASS),
> > +	MAKE_GCAP_REGLIST_DESCR(gen12lp_rc_inst_regs, INDEX_PF, TYPE_ENGINE_INSTANCE, GUC_RENDER_CLASS),
> > +	MAKE_GCAP_REGLIST_DESCR(gen12lp_vd_class_regs, INDEX_PF, TYPE_ENGINE_CLASS, GUC_VIDEO_CLASS),
> > +	MAKE_GCAP_REGLIST_DESCR(gen12lp_vd_inst_regs, INDEX_PF, TYPE_ENGINE_INSTANCE, GUC_VIDEO_CLASS),
> > +	MAKE_GCAP_REGLIST_DESCR(gen12lp_vec_class_regs, INDEX_PF, TYPE_ENGINE_CLASS, GUC_VIDEOENHANCE_CLASS),
> > +	MAKE_GCAP_REGLIST_DESCR(gen12lp_vec_inst_regs, INDEX_PF, TYPE_ENGINE_INSTANCE, GUC_VIDEOENHANCE_CLASS),
> > +	MAKE_GCAP_REGLIST_DESCR(gen12lp_blt_class_regs, INDEX_PF, TYPE_ENGINE_CLASS, GUC_BLITTER_CLASS),
> > +	MAKE_GCAP_REGLIST_DESCR(gen12lp_blt_inst_regs, INDEX_PF, TYPE_ENGINE_INSTANCE, GUC_BLITTER_CLASS),
> 
> if you knew that you want to use macros, why not start with them in
> previous patch ?
> 
> >  	{NULL, 0, 0, 0, 0}
> >  };
> >  
> > -/************ FIXME: Populate tables for other devices in subsequent patch ************/
> > +/************* Populate additional registers / device tables *************/
> > +
> > +static inline struct __guc_mmio_reg_descr **
> > +guc_capture_get_ext_list_ptr(struct __guc_mmio_reg_descr_group * lists, u32 owner, u32 type, u32 class)
> > +{
> > +	while(lists->list){
> 
> please run checkpatch.pl
> 
> > +
> > +	sseu = &gt->info.sseu;
> > +	for_each_instdone_slice_subslice(i915, sseu, slice, subslice) {
> > +		num_tot_regs += 2; /* two registers of interest for now */
> > +	}
> > +	if (!num_tot_regs)
> > +		return;
> > +
> > +	*ext = kzalloc(2 * num_tot_regs * sizeof(struct __guc_mmio_reg_descr), GFP_KERNEL);
> 
> kcalloc ?
> 
> >  
> >  static struct __guc_mmio_reg_descr_group *
> >  guc_capture_get_device_reglist(struct drm_i915_private *dev_priv)
> >  {
> >  	if (IS_TIGERLAKE(dev_priv) || IS_ROCKETLAKE(dev_priv) ||
> >  	    IS_ALDERLAKE_S(dev_priv) || IS_ALDERLAKE_P(dev_priv)) {
> > -		return gen12lp_lists;
> 
> patch2: gen12lp_lists
> patch3: xe_lpd_lists
> 
> please be consistent across series
> 
> > +		/*
> > +		* For certain engine classes, there are slice and subslice
> > +		* level registers requiring steering. We allocate and populate
> > +		* these at init time based on hw config add it as an extension
> > +		* list at the end of the pre-populated render list.
> > +		*/
> > +		xelpd_alloc_steered_ext_list(dev_priv, xe_lpd_lists);
> > +		return xe_lpd_lists;
> >  	}
> >  
> >  	return NULL;
> > @@ -221,6 +346,7 @@ int intel_guc_capture_list_init(struct intel_guc *guc, u32 owner, u32 type, u32
> >  
> >  void intel_guc_capture_destroy(struct intel_guc *guc)
> >  {
> > +	guc_capture_clear_ext_regs(guc->capture.reglists);
> >  }
> 
> maybe whole function shall be introduced in this patch ?
> 
> -Michal
> 
> >