[Intel-gfx] [PATCH v2 4/4] drm/i915: Fix i915_gem_evict_for_vma (soft-pinning)

[Joonas Lahtinen]

On to, 2016-11-17 at 12:08 +0000, Chris Wilson wrote: 
> -int
> -i915_gem_evict_for_vma(struct i915_vma *target)
> +int i915_gem_evict_for_vma(struct i915_vma *target, unsigned int flags)
>  {
> -	struct drm_mm_node *node, *next;
> +	LIST_HEAD(eviction_list);
> +	struct drm_mm_node *node;
> +	u64 start = target->node.start;
> +	u64 end = start + target->node.size;
> +	struct i915_vma *vma, *next;
> +	bool check_color;
> +	int ret = 0;
>  
>  	lockdep_assert_held(&target->vm->dev->struct_mutex);
> +	trace_i915_gem_evict_vma(target, flags);
> +

Daniel already misread this, so I think it might be worth commenting;

> +	check_color = target->vm->mm.color_adjust;
> +	if (check_color) {

		/* Extend the search area to cover guard pages. */

> +		if (start > target->vm->start)
> +			start -= 4096;
> +		if (end < target->vm->start + target->vm->total)
> +			end += 4096;
> +	}

<SNIP>

> +		if (flags & PIN_NONBLOCK &&
> +		    (i915_vma_is_pinned(vma) || i915_vma_is_active(vma))) {
> +			ret = -ENOSPC;
> +			break;
> +		}

i915_vma_is_pinned() being true will exit this loop with -ENOSPC with
or without NOBLOCK, just skipping the exec_entry test without it. I
would clarify that. Now it's bit odd.

>  
> -			return -ENOSPC;
> +		/* Overlap of objects in the same batch? */
> +		if (i915_vma_is_pinned(vma)) {
> +			ret = -ENOSPC;
> +			if (vma->exec_entry &&
> +			    vma->exec_entry->flags & EXEC_OBJECT_PINNED)
> +				ret = -EINVAL;
> +			break;
>  		}
>  
> -		ret = i915_vma_unbind(vma);
> -		if (ret)
> -			return ret;
> +		__i915_vma_pin(vma);

I don't quite see why? Are you expecting the iteration to hit same vma
twice? Or somebody moving it while we iterate.

> +		list_add(&vma->exec_list, &eviction_list);

I'd prefer an union instead of brutally reusing member for other
purposes.

>  	}
>  
> -	return 0;
> +	list_for_each_entry_safe(vma, next, &eviction_list, exec_list) {
> +		list_del_init(&vma->exec_list);
> +		__i915_vma_unpin(vma);
> +		if (ret == 0)
> +			ret = i915_vma_unbind(vma);
> +	}
> +
> +	return ret;
>  }
>  

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation