| Message ID | 20241024103917.3231206-1-nitin.r.gote@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] drm/i915/gt: Use ENGINE_TRACE for tracing. | expand |
Hi Nitin, 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. > > v2: Bit more specific in commit description (Andi) > > Signed-off-by: Nitin Gote <nitin.r.gote@intel.com> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> Thanks, Andi
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? Matt > > v2: Bit more specific in commit description (Andi) > > Signed-off-by: Nitin Gote <nitin.r.gote@intel.com> > --- > .../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 >
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@intel.com> >> --- >> .../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 >> >
On Fri, Oct 25, 2024 at 09:02:16AM +0100, Tvrtko Ursulin wrote: > > 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. I'm sorry, but I already applied the patch. I agree with it because most of the information provided are not really useful to the failure, but more to who is actually debugging the code. Would it make sense to add a gt_err() after or before the ENGINE_TRACE() so that we have the error printing along with debug information? Thanks, Andi
On 25/10/2024 12:21, Andi Shyti wrote: > On Fri, Oct 25, 2024 at 09:02:16AM +0100, Tvrtko Ursulin wrote: >> >> 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. > > I'm sorry, but I already applied the patch. > > I agree with it because most of the information provided are not > really useful to the failure, but more to who is actually > debugging the code. > > Would it make sense to add a gt_err() after or before the > ENGINE_TRACE() so that we have the error printing along with > debug information? I think so. Without that Matt's point that the patch makes harder to understand from the field bug report stands. Regards, Tvrtko
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; }
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. v2: Bit more specific in commit description (Andi) Signed-off-by: Nitin Gote <nitin.r.gote@intel.com> --- .../gpu/drm/i915/gt/intel_ring_submission.c | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)