Quoting Mika Kuoppala (2019-01-25 08:34:37)
> Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
> > ---
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
