Re: [PATCH v6] drm/i915: Fix NULL pointer dereference in capture_engine

[Andi Shyti <andi.shyti@linux.intel.com>]

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/.

Thanks,
Andi