Re: [PATCH 27/34] drm/i915: Remove the intel_engine_notify tracepoint

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

On 23/01/2019 12:54, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-01-22 15:50:27)
> > On 21/01/2019 22:21, Chris Wilson wrote:
> > > The global seqno is defunct and so we have no meaningful indicator of
> > > forward progress for an engine. You need to listen to the request
> > > signaling tracepoints instead.
> > >
> > > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > > ---
> > >    drivers/gpu/drm/i915/i915_irq.c   |  2 --
> > >    drivers/gpu/drm/i915/i915_trace.h | 25 -------------------------
> > >    2 files changed, 27 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > index 5fd5080c4ccb..71d11dc2c235 100644
> > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > @@ -1209,8 +1209,6 @@ static void notify_ring(struct intel_engine_cs *engine)
> > >                wake_up_process(tsk);
> > >    
> > >        rcu_read_unlock();
> > > -
> > > -     trace_intel_engine_notify(engine, wait);
> > >    }
> > >    
> > >    static void vlv_c0_read(struct drm_i915_private *dev_priv,
> > > diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
> > > index 33d90eca9cdd..cb5bc65d575d 100644
> > > --- a/drivers/gpu/drm/i915/i915_trace.h
> > > +++ b/drivers/gpu/drm/i915/i915_trace.h
> > > @@ -750,31 +750,6 @@ trace_i915_request_out(struct i915_request *rq)
> > >    #endif
> > >    #endif
> > >    
> > > -TRACE_EVENT(intel_engine_notify,
> > > -         TP_PROTO(struct intel_engine_cs *engine, bool waiters),
> > > -         TP_ARGS(engine, waiters),
> > > -
> > > -         TP_STRUCT__entry(
> > > -                          __field(u32, dev)
> > > -                          __field(u16, class)
> > > -                          __field(u16, instance)
> > > -                          __field(u32, seqno)
> > > -                          __field(bool, waiters)
> > > -                          ),
> > > -
> > > -         TP_fast_assign(
> > > -                        __entry->dev = engine->i915->drm.primary->index;
> > > -                        __entry->class = engine->uabi_class;
> > > -                        __entry->instance = engine->instance;
> > > -                        __entry->seqno = intel_engine_get_seqno(engine);
> > > -                        __entry->waiters = waiters;
> > > -                        ),
> > > -
> > > -         TP_printk("dev=%u, engine=%u:%u, seqno=%u, waiters=%u",
> > > -                   __entry->dev, __entry->class, __entry->instance,
> > > -                   __entry->seqno, __entry->waiters)
> > > -);
> > > -
> > >    DEFINE_EVENT(i915_request, i915_request_retire,
> > >            TP_PROTO(struct i915_request *rq),
> > >            TP_ARGS(rq)
> >
> > I cannot decide if keeping what we can would make it useful. Certainly
> > not for debugging intel_engine_breadcrumbs_irq.. a sequence of
> > intel_engine_notify(dev, class, instance) -> dma_fence_signaled would be
> > a very unreliable trace of what engine actually executed something. What
> > do you think?
>
> All we get is a tracepoint to say an user-interrupt occurred, but nothing to
> tie it to any request. We are debugging interrupt generation at that
> point, and I feel a tracepoint ill-suited. We want something geared
> towards CI instead, so a bunch of selftests... That would be sensible!

We get the engine as well, so could look at sequence of dma fence 
signaling happening following that and imply something, sometimes. Like 
which physical engine executed what. Since the signaling is done 
directly from the interrupt handler and engines are handled in serial 
fashion.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx