[Intel-gfx] [PATCH 02/33] drm/i915: Measure the required reserved size for request emission

[Chris Wilson]

Quoting Mika Kuoppala (2019-01-25 08:34:37)
> Chris Wilson <chris at chris-wilson.co.uk> writes:
> 
> > Instead of tediously and fragilely counting up the number of dwords
> > required to emit the breadcrumb to seal a request, fake a request and
> > measure it automatically once during engine setup.
> >
> > The downside is that this requires a fair amount of mocking to create a
> > proper breadcrumb. Still, should be less error prone in future as the
> > breadcrumb size fluctuates!
> 
> We are quick to notice, but this method saves brains and time,
> review time.
> 
> >
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/intel_engine_cs.c       | 49 ++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_lrc.c             | 12 +++--
> >  drivers/gpu/drm/i915/intel_ringbuffer.c      | 24 +++++++---
> >  drivers/gpu/drm/i915/intel_ringbuffer.h      |  2 +-
> >  drivers/gpu/drm/i915/selftests/mock_engine.c |  4 +-
> >  5 files changed, 77 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> > index 2f3c71f6d313..883ba208d1c2 100644
> > --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> > @@ -604,6 +604,47 @@ static void __intel_context_unpin(struct i915_gem_context *ctx,
> >       intel_context_unpin(to_intel_context(ctx, engine));
> >  }
> >  
> > +struct measure_breadcrumb {
> > +     struct i915_request rq;
> > +     struct i915_timeline timeline;
> > +     struct intel_ring ring;
> > +     u32 cs[1024];
> > +};
> > +
> > +static int measure_breadcrumb_sz(struct intel_engine_cs *engine)
> > +{
> > +     struct measure_breadcrumb *frame;
> > +     unsigned int dw;
> > +
> > +     GEM_BUG_ON(!engine->i915->gt.scratch);
> > +
> > +     frame = kzalloc(sizeof(*frame), GFP_KERNEL);
> > +     if (!frame)
> > +             return -ENOMEM;
> > +
> > +     i915_timeline_init(engine->i915, &frame->timeline, engine->name);
> 
> You could init with null name. This is so short lived
> and we dont expect debugs. If it ever leaks into wild,
> blowout would be instant. Now the name is the same as
> the real deal.

Good point. I was just cutting and pasting for convenience, but having
the same name may be doubly confusing for the strange bugs where it might
matter :)

> > +     frame->ring.timeline = &frame->timeline;
> > +     frame->ring.vaddr = frame->cs;
> > +     frame->ring.size = sizeof(frame->cs);
> > +     frame->ring.effective_size = frame->ring.size;
> > +     frame->ring.space = frame->ring.size - 8;
> 
> Why 2 dwords short? Just curious as it doesn't seem
> to matter in this use case.

Just rules of the HW. Head steps in qword jumps. But should just
intel_ring_update_space ftw.

> > +     INIT_LIST_HEAD(&frame->ring.request_list);
> > +
> > +     frame->rq.i915 = engine->i915;
> > +     frame->rq.engine = engine;
> > +     frame->rq.ring = &frame->ring;
> > +     frame->rq.timeline = &frame->timeline;
> > +
> > +     dw = engine->emit_breadcrumb(&frame->rq, frame->cs) - frame->cs;
> > +     GEM_BUG_ON(dw != engine->emit_breadcrumb_sz);
> 
> Peace of mind provided =)

For full peace of mind, see the earlier runs with the BUG active.
https://patchwork.freedesktop.org/series/55683/
-Chris