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

[Chris Wilson]

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.
 
> 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[].

Hope that helps,
-Chris