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

["Dong, Zhanjun" <zhanjun.dong@intel.com>]

On 2024-05-10 8:17 p.m., Teres Alexis, Alan Previn wrote:
> 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.
> 
in Xe folder, egrep "&gt_to_xe\(.*\)->drm" *.c
we have many

>> +
>> +       /* 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.
Typo, will changed to =
> 
> alan:snip
>