[Intel-gfx] [PATCH 8/8] drm/i915: Wait on external rendering for GEM objects

[Chris Wilson]

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