<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Jun 19, 2017 at 12:53 PM, Chris Wilson <span dir="ltr"><<a href="mailto:chris@chris-wilson.co.uk" target="_blank">chris@chris-wilson.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Quoting Kenneth Graunke (2017-06-19 20:28:31)<br>
<span class="">> On Monday, June 19, 2017 3:06:48 AM PDT Chris Wilson wrote:<br>
</span><span class="">> > -   if (target != batch->bo)<br>
> > -      add_exec_bo(batch, target);<br>
> > +   if (target != batch->bo) {<br>
> > +      unsigned int index = add_exec_bo(batch, target);<br>
> > +      struct drm_i915_gem_exec_object2 *exec = &batch->exec_objects[index];<br>
> > +<br>
> > +      if (write_domain) {<br>
> > +         exec->flags |= EXEC_OBJECT_WRITE;<br>
> > +<br>
> > +         /* PIPECONTROL needs a w/a on gen6 */<br>
> > +         if (write_domain == I915_GEM_DOMAIN_INSTRUCTION) {<br>
> > +            struct brw_context *brw = container_of(batch, brw, batch);<br>
> > +            if (brw->gen == 6)<br>
> > +               exec->flags |= EXEC_OBJECT_NEEDS_GTT;<br>
> > +         }<br>
> > +      }<br>
> > +<br>
> > +      offset64 = exec->offset;<br>
><br>
> I don't think this works.  brw_bo still has a single offset64 value that's<br>
> shared across all contexts.  So, in a multi-context/multi-threaded case, we<br>
> may put a bogus offset in the validation list.<br>
<br>
</span>But the value that we put there is the *only* value that we then use for<br>
all of the relocation entries. We are consistently wrong which is what<br>
is mandated by the interface, i.e. we don't prevent a relocation stall<br>
(unless the kernel can move the buffers for us) but more importantly the<br>
relocation.presumed_offset == execobject.offset == batch[reloc.offset]<br>
for all relocations.<span class=""><br></span></blockquote><div><br></div><div>Is the BO address in the kernel per-context or per-drm-fd?  If it's the later then we may be safe though it sounds scary.  If it's the former, then we definitely have a problem.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> Pulling the value out of the validation list is nice, but it's still<br>
> potentially bogus.<br>
><br>
> I think we still need per-context-bo fields.  I have a patch series in<br>
> progress to do that, but I moved it down the priority list a ways...<br>
> need to get back to that...<br>
<br>
</span>Per-context tracking will allow us to avoid stalls between contexts,<br>
which will be an improvement above and beyond this patch. This patch<br>
will work even with conflicting contexts, just no better than today. For<br>
the single context (or no conflict cases), we do realise the improvement<br>
from avoiding always having to inspect reloc[].<br>
<br>
Hope that helps,<br>
-Chris<br>
<div class="HOEnZb"><div class="h5">______________________________<wbr>_________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a><br>
<a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/<wbr>mailman/listinfo/mesa-dev</a><br>
</div></div></blockquote></div><br></div></div>