Re: [PATCH v27 6/6] drm/xe/guc: Save manual engine capture into capture list

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

On Fri, 2024-10-04 at 10:38 -0700, Teres Alexis, Alan Previn wrote:
> As per offline conversation just now, since our last thorough review in rev18,
> we notice many trivial changes that are not required at all that got accidentally
> included as a result of ongoing offline debugging work that you forgot to remove.
> I'll highlight them below as part of this review. Looking forward for next rev.
> 
> 
> 
> On Thu, 2024-10-03 at 20:24 -0700, Zhanjun Dong wrote:
> > Save manual engine capture into capture list.
> > This removes duplicate register definitions across manual-capture vs
> > guc-err-capture.
> > 
> > 
> > -void xe_guc_capture_free(struct xe_guc *guc)
> > +void xe_guc_capture_put_matched_nodes(struct xe_guc *guc)
> >  {
> >         struct xe_device *xe = guc_to_xe(guc);
> >         struct xe_devcoredump *devcoredump = &xe->devcoredump;
> > -       struct __guc_capture_parsed_output *n = devcoredump->snapshot.matched_node;
> alan: all the changes in this function doesnt change the implementation, usage or behavior
> since Patch #5 - so we can just move these changes directly into patch #5. The name change is
> the one thing i did want to see since "free" is the opposite of "alloc" but we are not
> freeing content, we are returning a node we had "get" previously.
> >  
> > -       if (n) {
> > -               guc_capture_remove_stale_matches_from_list(guc->capture, n);
> > -               guc_capture_free_outlist_node(guc->capture, n);
> > +       if (devcoredump->snapshot.matched_node) {
> > +               guc_capture_remove_stale_matches_from_list(guc->capture,
> > +                                                          devcoredump->snapshot.matched_node);
> > +               guc_capture_free_outlist_node(guc->capture, devcoredump->snapshot.matched_node);
> > +
> > +               devcoredump->snapshot.matched_node = NULL;
> >         }
> > -       devcoredump->snapshot.matched_node = NULL;
alan: WRT to bringing this hunk into Pach #5, i am okay with you retaining the RB in Patch #5 if not other changes.
(same as the earlier comment in my last reply of this patch)
> >  }
> >  
> >  /*
> alan:snip