Re: [PATCH v8 2/6] drm/xe/guc: Add XE_LP steered register lists

["Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>]

only a couple of nits, so, 
Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com>

On Mon, 2024-05-06 at 18:47 -0700, Zhanjun Dong wrote:
> Add the ability for runtime allocation and freeing of
> steered register list extentions that depend on the
> detected HW config fuses.
> 
alan:snip
> +static void
> +guc_capture_alloc_steered_lists(struct xe_guc *guc, const struct __guc_mmio_reg_descr_group *lists)
> +{
> +       struct xe_gt *gt = guc_to_gt(guc);
> +       u16 slice, subslice;
> +       int iter, i, num_steer_regs, num_tot_regs = 0;
> +       const struct __guc_mmio_reg_descr_group *list;
> +       struct __guc_mmio_reg_descr_group *extlists;
> +       struct __guc_mmio_reg_descr *extarray;
> +       bool has_xehpg_extregs;
> +       struct drm_device *drm = &gt_to_xe(gt)->drm;
alan: nit: order vars as reverse christmas tree. I'm not sure if this is a hard rule,
I've seen many review comments about this but cant find a hard rule in kernel doc.
I wasnt paying attention to this earlier and realize that Patch #1 has at least 4 functions
with this issue. I'm going to leave at your discretion since i assume this is a nit.

> +
> +       /* steered registers currently only exist for the render-class */
> +       list = guc_capture_get_one_list(lists, GUC_CAPTURE_LIST_INDEX_PF,
> +                                       GUC_CAPTURE_LIST_TYPE_ENGINE_CLASS,
> +                                       GUC_CAPTURE_LIST_CLASS_RENDER_COMPUTE);
> +       /* skip if extlists was previously allocated */
> +       if (!list || guc->capture->extlists)
> +               return;
> +
> +       has_xehpg_extregs = GRAPHICS_VERx100(gt_to_xe(gt)) >= 1255;
> +
> +       num_steer_regs = ARRAY_SIZE(xe_extregs);
> +       if (has_xehpg_extregs)
> +               num_steer_regs += ARRAY_SIZE(xehpg_extregs);
> +
> +       num_tot_regs += num_steer_regs * bitmap_weight(gt->fuse_topo.g_dss_mask,
> +                                                      sizeof(gt->fuse_topo.g_dss_mask) * 8);
alan: nit: a bit confusing we are using "num_tot_regs +=" when its the first use of this
variable after init to zero, so just "num_tot_regs =" is fine here.

alan:snip