Re: [PATCH 01/16] drm/i915: Drop i915_gem_obj_is_pinned() from set-cache-level

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 04/27/2015 01:41 PM, 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.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_gem.c | 33 +++++++++++++++++++++------------
>   1 file changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index f0a6d03e9ba5..afdb604e4005 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3768,31 +3768,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;
>
>   	if (obj->cache_level == cache_level)
>   		return 0;
>
> -	if (i915_gem_obj_is_pinned(obj)) {
> +	if (obj->pin_display) {
>   		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;
> +

Another micro-optimisation?

Side note - this was puzzling and then I realized 
i915_gem_valid_gtt_space says true when node is not allocated. It seems 
to be only concerned by node colouring - so is that one badly name 
function or I am missing something?

>   		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_finish_gpu(obj);
>   		if (ret)
>   			return ret;
>
> -		i915_gem_object_finish_gtt(obj);
> -
>   		/* Before SandyBridge, you could not use tiling or fence
>   		 * registers with snooped memory, so relinquish any fences
>   		 * currently pointing to our region in the aperture.
> @@ -3801,15 +3804,21 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
>   			ret = i915_gem_object_put_fence(obj);
>   			if (ret)
>   				return ret;
> +
> +			i915_gem_release_mmap(obj);
>   		}

If only < gen6 we need to drop the mmap, what happens with the domain 
tracking - who removes the GTT bit from there now?

>
> -		list_for_each_entry(vma, &obj->vma_list, vma_link)
> -			if (drm_mm_node_allocated(&vma->node)) {
> -				ret = i915_vma_bind(vma, cache_level,
> -						    PIN_UPDATE);
> -				if (ret)
> -					return ret;
> -			}
> +		list_for_each_entry(vma, &obj->vma_list, vma_link) {
> +			if (!drm_mm_node_allocated(&vma->node))
> +				continue;
> +
> +			if (vma->pin_count)
> +				return -EBUSY;

Preserve DRM_DEBUG as it was before?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx