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 = >->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
>
> >
