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

[Chris Wilson]

Quoting Daniel Vetter (2017-07-07 11:04:00)
> On Mon, Jun 19, 2017 at 11:06:48AM +0100, 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;
> > +         }
> > +      }
> 
> If we ever do a write to the batch this goes boom I think. Can we move the
> if (write_domain) out of the batch check? Means we need to cache the batch
> exec entry somewhere. Just having a batch->batch_exec_flags would do it I
> think.

You are strictly not allowed to write to the batch; it was overkill in
hindsight, but the kernel will reject such execbuf.

> > +      offset64 = exec->offset;
> > +   } else {
> > +      offset64 = target->offset64;
> 
> You lost the READ_ONCE for the above two, and since that's now at least
> locally defined, we don't even need the comment.
> 
> With the above two issues addressed:

Where's the second issue? There's no need for a READ_ONCE for offset
here as both are local to the context (and a context is not supposed to
be accessed concurrently, hence the lack of locking around here) and not
shared.
-Chris