Hi Jani,
On Wed, Dec 04, 2024 at 12:51:56PM +0200, Jani Nikula wrote:
> On Wed, 04 Dec 2024, Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx> wrote:
> > Hi Michal,
> >
> >> > + if (rq && !i915_request_started(rq)) {
> >> > + /*
> >> > + * We want to know also what is the gcu_id of the context,
> >>
> >> typo: guc_id
> >>
> >> > + * but if we don't have the context reference, then skip
> >> > + * printing it.
> >> > + */
> >>
> >> but IMO this comment is redundant as it's quite obvious that without
> >> context pointer you can't print guc_id member
> >
> > I recommended to add a comment because there is some code
> > redundancy that, I think, needs some explanation; someone might
> > not spot immediately the need for ce, unless goes a the end of
> > the drm_info parameter's list.
> >
> >> > + if (ce)
> >> > + drm_info(&engine->gt->i915->drm,
> >> > + "Got hung context on %s with active request %lld:%lld [0x%04X] not yet started\n",
> >> > + engine->name, rq->fence.context, rq->fence.seqno, ce->guc_id.id);
> >> > + else
> >> > + drm_info(&engine->gt->i915->drm,
> >> > + "Got hung context on %s with active request %lld:%lld not yet started\n",
> >> > + engine->name, rq->fence.context, rq->fence.seqno);
> >>
> >> since you are touching drm_info() where we use engine->gt then maybe
> >> it's good time to switch to gt_info() to get better per-GT message?
> >
> > I think the original reason for using drm_info is because we are
> > outside the GT. But, because the engine belongs to the GT, it
> > makes also sense to use gt_info(), I don't oppose.
> >
> > It would make more sense to move this function completely into
> > gt/.
>
> Can we converge on the patch instead of diverge, please?
>
> It's a Cc: stable null pointer dereference fix.
>
> It's already been iterated for two weeks to reach v6.
>
> Fix the comment typo while applying, but there's nothing inherently
> wrong here AFAICT. Merge it and move on.
Thanks for the feedback, will go ahead and merge.
All other gt/ adjustments can be done later.
Andi
