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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev