[Mesa-dev] [PATCH 2/4] i965: Use I915_EXEC_NO_RELOC

[Daniel Vetter]

On Mon, Jun 19, 2017 at 08:53:33PM +0100, Chris Wilson wrote:
> Quoting Kenneth Graunke (2017-06-19 20:28:31)
> > On Monday, June 19, 2017 3:06:48 AM PDT Chris Wilson wrote:
> > > -   if (target != batch->bo)
> > > -      add_exec_bo(batch, target);
> > > +   if (target != batch->bo) {
> > > +      unsigned int index = add_exec_bo(batch, target);
> > > +      struct drm_i915_gem_exec_object2 *exec = &batch->exec_objects[index];
> > > +
> > > +      if (write_domain) {
> > > +         exec->flags |= EXEC_OBJECT_WRITE;
> > > +
> > > +         /* PIPECONTROL needs a w/a on gen6 */
> > > +         if (write_domain == I915_GEM_DOMAIN_INSTRUCTION) {
> > > +            struct brw_context *brw = container_of(batch, brw, batch);
> > > +            if (brw->gen == 6)
> > > +               exec->flags |= EXEC_OBJECT_NEEDS_GTT;
> > > +         }
> > > +      }
> > > +
> > > +      offset64 = exec->offset;
> > 
> > I don't think this works.  brw_bo still has a single offset64 value that's
> > shared across all contexts.  So, in a multi-context/multi-threaded case, we
> > may put a bogus offset in the validation list.
> 
> But the value that we put there is the *only* value that we then use for
> all of the relocation entries. We are consistently wrong which is what
> is mandated by the interface, i.e. we don't prevent a relocation stall
> (unless the kernel can move the buffers for us) but more importantly the
> relocation.presumed_offset == execobject.offset == batch[reloc.offset]
> for all relocations.

Yeah the only invariant NO_RELOC needs is that for all relocations
targeting the same buffer in a batch the relocation.presumed_offset must
match execobject.offset. It doesn't matter at all what value you put in
there, you could stuff random() in it and the kernel will fix things up.

It won't be fast ofc, because you're going to thrash relocs and hence
stall like crazy.

> > Pulling the value out of the validation list is nice, but it's still
> > potentially bogus.
> > 
> > I think we still need per-context-bo fields.  I have a patch series in
> > progress to do that, but I moved it down the priority list a ways...
> > need to get back to that...
> 
> Per-context tracking will allow us to avoid stalls between contexts,
> which will be an improvement above and beyond this patch. This patch
> will work even with conflicting contexts, just no better than today. For
> the single context (or no conflict cases), we do realise the improvement
> from avoiding always having to inspect reloc[].

Yup, for single contexts and disjoint bo sets in multi-context this will
be a great improvement. For shared buffers that somehow ended up with
different offsets in different contexts you're still going to thrash the
relocs every time and simply fall back to the old paths where you have to
process every reloc individually in the kernel. I think this here should
be a strict improvement over what we have, and should give the full
NO_RELOC benefits for all common cases (well maybe not browsers with too
many tabs and just a single gl renderer).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch