On Fri, 17 Mar 2023 16:16:39 -0700, Umesh Nerlige Ramappa wrote:
>
Hi Umesh,
> +static void oa_init_groups(struct intel_gt *gt)
> +{
> + int i, num_groups = gt->perf.num_perf_groups;
> +
> + for (i = 0; i < num_groups; i++) {
> + struct i915_perf_group *g = >->perf.group[i];
> +
> + /* Fused off engines can result in a group with num_engines == 0 */
> + if (g->num_engines == 0)
> + continue;
> +
> + if (i == PERF_GROUP_OAG && gt->type != GT_MEDIA) {
> + g->regs = __oag_regs();
> + g->type = TYPE_OAG;
> + } else if (GRAPHICS_VER_FULL(gt->i915) >= IP_VER(12, 70)) {
> + g->regs = __oam_regs(mtl_oa_base[i]);
> + g->type = TYPE_OAM;
> + }
This if/else statement, rather the whole business of gt->perf.group array
with PERF_GROUP_OAG and PERF_GROUP_OAM_SAMEDIA_0 both having the same value
0 is definitely a little klunky. Not too sure but a idr based approach
might have been cleaner (so not allocate an array at all but just kmalloc
each i915_perf_group entry independently and associate with an idr). It
might also have been good to just drop num_perf_groups for now and use a
value 1 (that is get rid of any num_perf_groups loops) and introduce
num_perf_groups later when we had a platform with num_perf_groups > 1.
Anyway since I said earlier let's keep it, let's do that and merge this
first (there is already too much code here) and revisit afterwards and see
if we can improve anything.
Rest looks good now:
Reviewed-by: Ashutosh Dixit <ashutosh.dixit@xxxxxxxxx>
