[Intel-gfx] [PATCH 2/2] drm/i915: Drop i915_gem_obj_is_pinned() from set-cache-level

[Chris Wilson]

On Tue, Oct 06, 2015 at 01:28:07PM +0200, Daniel Vetter wrote:
> On Tue, Oct 06, 2015 at 11:39:56AM +0100, Chris Wilson wrote:
> > Since the remove of the pin-ioctl, we only care about not changing the
> > cache level on buffers pinned to the hardware as indicated by
> > obj->pin_display. So we can safely replace i915_gem_object_is_pinned()
> > here with a plain obj->pin_display check. During rebinding, we will check
> > sanity checks in case vma->pin_count is erroneously set.
> > 
> > At the same time, we can micro-optimise GTT mmap() behaviour since we
> > only need to relinquish the mmaps before Sandybridge.
> 
> Actual condition is !LLC so would need to be updated (and split out imo).
>  
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 40 ++++++++++++++++++++++++----------------
> >  1 file changed, 24 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index d4a3bdf0c5b6..2b8ed7a2faab 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3629,31 +3629,34 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> >  {
> >  	struct drm_device *dev = obj->base.dev;
> >  	struct i915_vma *vma, *next;
> > +	bool bound = false;
> >  	int ret = 0;
> >  
> >  	if (obj->cache_level == cache_level)
> >  		goto out;
> >  
> > -	if (i915_gem_obj_is_pinned(obj)) {
> > -		DRM_DEBUG("can not change the cache level of pinned objects\n");
> > -		return -EBUSY;
> > -	}
> > -
> >  	list_for_each_entry_safe(vma, next, &obj->vma_list, vma_link) {
> > +		if (!drm_mm_node_allocated(&vma->node))
> > +			continue;
> > +
> > +		if (vma->pin_count) {
> > +			DRM_DEBUG("can not change the cache level of pinned objects\n");
> > +			return -EBUSY;
> > +		}
> > +
> >  		if (!i915_gem_valid_gtt_space(vma, cache_level)) {
> >  			ret = i915_vma_unbind(vma);
> >  			if (ret)
> >  				return ret;
> > -		}
> > +		} else
> > +			bound = true;
> >  	}
> >  
> > -	if (i915_gem_obj_bound_any(obj)) {
> > +	if (bound) {
> >  		ret = i915_gem_object_wait_rendering(obj, false);
> >  		if (ret)
> >  			return ret;
> 
> Shouldn't the below be split out into a separate patch? And maybe for
> paranoia keep calling finish_gtt but restrict it to !LLC && snooped like
> you do below.

Hmm, I don't have a finish-gtt. The serialisation is based on
release-mmaps (we have to be sure that any concurrent access is
prohibited). So the question is: is i915_gem_release_mmap() a sufficient
barrier and if not, why not. In release-mmap we are revoking the CPU's PTE,
but that can be ordered with the memory accesses, but before we continue
we should be sure that they have been revoked. Paranoia says we should
be moving the mb() we have from outside of release-mmaps into
release-mmaps.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre