On Tue, Jul 19, 2016 at 05:12:22PM +0300, Joonas Lahtinen wrote:
> On ti, 2016-07-19 at 11:51 +0100, Chris Wilson wrote:
> > + resv = i915_gem_object_get_dmabuf_resv(obj);
> > + if (resv) {
> > + long err;
>
> We already have ret in the function scope.
ret is int, we need a long. At least I can attest to our test coverage!
> > @@ -3402,13 +3414,13 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write)
> > struct i915_vma *vma;
> > int ret;
> >
> > - if (obj->base.write_domain == I915_GEM_DOMAIN_GTT)
> > - return 0;
> > -
> > ret = i915_gem_object_wait_rendering(obj, !write);
> > if (ret)
> > return ret;
> >
> > + if (obj->base.write_domain == I915_GEM_DOMAIN_GTT)
> > + return 0;
> > +
>
> Not sure I follow this change, wait rendering would only be issued if
> DOMAIN_CPU was in place, this function was about moving to gtt_domain
> so should really be NOP if in GTT domain already, no? Maybe calling
> site should do an explicit wait_rendering if needed.
No, this is where we do the wait rendering. The API says
set-to-gtt-domain implies that it is out of the GPU domain (no
rendering) and out of the CPU domain. (Similarly for set-to-cpu-domain.)
The relaxation I've applied here is to try and catch third parties that
have not updated our domain tracking for their access.
Moving the wait_rendering to the parent is a fair amount of burden.
> > vma->pin_count = 0;
> > - ret = i915_vma_unbind(vma);
>
> Maybe add a comment/TODO/FIXME: about the potential WARN.
__i915_vma_unbind_no_wait() is itself an automatic FIXME - it only
exists to cover up a WARN. (I wish we had this series in place first so
that such a hack wasn't applied.)
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
