Re: [PATCH v2] drm/i915/gt: Use ENGINE_TRACE for tracing.

[Tvrtko Ursulin <tursulin@xxxxxxxxxxx>]

On 24/10/2024 19:58, Matt Roper wrote:
> On Thu, Oct 24, 2024 at 04:09:17PM +0530, Nitin Gote wrote:
> > There is ENGINE_TRACE() macro which introduce engine name
> > with GEM tracing in i915. So, it will be good to use ENGINE_TRACE()
> > over drm_err() drm_device based logging for engine debug log.
>
> Doesn't this just eliminate the logging completely if the driver is
> built without CONFIG_DRM_I915_TRACE_GEM?  That means that most users
> will probably get no dmesg output at all on failure now, which could
> make it harder for us to understand and debug user-reported bugs.
>
> Of course intel_ring_submission.c only gets used for the old
> pre-execlist platforms (HSW and older) so maybe there are few enough of
> those in active usage that we don't really get too many new bug reports
> anyway?

Yeah, plus, the justification about engine name does not appear to hold, 
since the old message was printing it already. So if there is something 
more happening here under the covers please do explain it properly in 
the commit message.

Regards,

Tvrtko


> Matt
>
> > v2: Bit more specific in commit description (Andi)
> >
> > Signed-off-by: Nitin Gote <nitin.r.gote@xxxxxxxxx>
> > ---
> >   .../gpu/drm/i915/gt/intel_ring_submission.c   | 20 +++++++++----------
> >   1 file changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_ring_submission.c b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> > index 32f3b52a183a..74d6a2b703ac 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> > @@ -282,16 +282,16 @@ static int xcs_resume(struct intel_engine_cs *engine)
> >   	return 0;
> >   
> >   err:
> > -	drm_err(&engine->i915->drm,
> > -		"%s initialization failed; "
> > -		"ctl %08x (valid? %d) head %08x [%08x] tail %08x [%08x] start %08x [expected %08x]\n",
> > -		engine->name,
> > -		ENGINE_READ(engine, RING_CTL),
> > -		ENGINE_READ(engine, RING_CTL) & RING_VALID,
> > -		ENGINE_READ(engine, RING_HEAD), ring->head,
> > -		ENGINE_READ(engine, RING_TAIL), ring->tail,
> > -		ENGINE_READ(engine, RING_START),
> > -		i915_ggtt_offset(ring->vma));
> > +	ENGINE_TRACE(engine,
> > +		     "initialization failed; "
> > +		     "ctl %08x (valid? %d) head %08x [%08x] tail %08x [%08x] start %08x [expected %08x]\n",
> > +		     ENGINE_READ(engine, RING_CTL),
> > +		     ENGINE_READ(engine, RING_CTL) & RING_VALID,
> > +		     ENGINE_READ(engine, RING_HEAD), ring->head,
> > +		     ENGINE_READ(engine, RING_TAIL), ring->tail,
> > +		     ENGINE_READ(engine, RING_START),
> > +		     i915_ggtt_offset(ring->vma));
> > +	GEM_TRACE_DUMP();
> >   	return -EIO;
> >   }
> >   
> > --
> > 2.25.1