Re: [PATCH 1/3] drm/i915: Only bind each object rather than for every execbuffer

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jan 01, 2014 at 02:00:54PM +0000, Chris Wilson wrote:
> One side-effect of the introduction of ppgtt was that we needed to
> rebind the object into the appropriate vm (and global gtt in some
> peculiar cases). For simplicity this was done twice for every object on
> every call to execbuffer. However, that adds a tremendous amount of CPU
> overhead (rewriting all the PTE for all objects into WC memory) per
> draw. The fix is to push all the decision about which vm to bind into
> and when down into the low-level bind routines through hints rather than
> inside the execbuffer routine.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=72906
> Tested-by: jianx.zhou@xxxxxxxxx
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Ben Widawsky <benjamin.widawsky@xxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>

My original plan with full PPGTT was to avoid all this deferred bind,
and other complications in the bind logic. My intention had been, if a
VMA exists, it is bound (with global GTT as the special case). It
would have therefore been relatively easy to determine when to bind an
object into a VM. One thing which I didn't plan for was having to change
the caching type. To a large extent, I feel this patch heads towards
that goal, except uses drm_mm_node_allocated where I had expected to
simply check if a VMA exists.

Anyway, no point to this really other than to explain why things are
somewhat messy.

> ---
>  drivers/gpu/drm/i915/i915_drv.h            |  25 +++---
>  drivers/gpu/drm/i915/i915_gem.c            | 119 +++++++++++------------------
>  drivers/gpu/drm/i915/i915_gem_context.c    |   9 +--
>  drivers/gpu/drm/i915/i915_gem_evict.c      |  12 +--
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  18 +++--
>  drivers/gpu/drm/i915/i915_gem_gtt.c        |   2 +-
>  drivers/gpu/drm/i915/i915_trace.h          |  20 ++---
>  drivers/gpu/drm/i915/intel_overlay.c       |   2 +-
>  drivers/gpu/drm/i915/intel_pm.c            |   2 +-
>  drivers/gpu/drm/i915/intel_ringbuffer.c    |  19 +++--
>  10 files changed, 103 insertions(+), 125 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index cc8afff878b1..65379474f312 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2033,11 +2033,11 @@ void i915_gem_vma_destroy(struct i915_vma *vma);
>  int __must_check i915_gem_object_pin(struct drm_i915_gem_object *obj,
>  				     struct i915_address_space *vm,
>  				     uint32_t alignment,
> -				     bool map_and_fenceable,
> -				     bool nonblocking);
> -void i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj);
> +				     unsigned flags);
> +#define PIN_MAPPABLE 0x1
> +#define PIN_GLOBAL 0x2
> +#define PIN_NONBLOCK 0x4
>  int __must_check i915_vma_unbind(struct i915_vma *vma);
> -int __must_check i915_gem_object_ggtt_unbind(struct drm_i915_gem_object *obj);
>  int i915_gem_object_put_pages(struct drm_i915_gem_object *obj);
>  void i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv);
>  void i915_gem_release_mmap(struct drm_i915_gem_object *obj);
> @@ -2237,13 +2237,19 @@ i915_gem_obj_ggtt_size(struct drm_i915_gem_object *obj)
>  static inline int __must_check
>  i915_gem_obj_ggtt_pin(struct drm_i915_gem_object *obj,
>  		      uint32_t alignment,
> -		      bool map_and_fenceable,
> -		      bool nonblocking)
> +		      unsigned flags)
>  {
> -	return i915_gem_object_pin(obj, obj_to_ggtt(obj), alignment,
> -				   map_and_fenceable, nonblocking);
> +	return i915_gem_object_pin(obj, obj_to_ggtt(obj), alignment, flags | PIN_GLOBAL);
>  }

I'm torn as to whether or not it's okay to not pass PIN_GLOBAL in flags.

>  
> +static inline int
> +i915_gem_object_ggtt_unbind(struct drm_i915_gem_object *obj)
> +{
> +	return i915_vma_unbind(i915_gem_obj_to_ggtt(obj));
> +}
> +
> +void i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj);
> +
>  /* i915_gem_context.c */
>  #define ctx_to_ppgtt(ctx) container_of((ctx)->vm, struct i915_hw_ppgtt, base)
>  int __must_check i915_gem_context_init(struct drm_device *dev);
> @@ -2280,8 +2286,7 @@ int __must_check i915_gem_evict_something(struct drm_device *dev,
>  					  int min_size,
>  					  unsigned alignment,
>  					  unsigned cache_level,
> -					  bool mappable,
> -					  bool nonblock);
> +					  unsigned flags);
>  int i915_gem_evict_vm(struct i915_address_space *vm, bool do_idle);
>  int i915_gem_evict_everything(struct drm_device *dev);
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 656406deada0..d9acfa78d1a5 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -43,12 +43,6 @@ static void i915_gem_object_flush_cpu_write_domain(struct drm_i915_gem_object *o
>  static __must_check int
>  i915_gem_object_wait_rendering(struct drm_i915_gem_object *obj,
>  			       bool readonly);
> -static __must_check int
> -i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
> -			   struct i915_address_space *vm,
> -			   unsigned alignment,
> -			   bool map_and_fenceable,
> -			   bool nonblocking);
>  static int i915_gem_phys_pwrite(struct drm_device *dev,
>  				struct drm_i915_gem_object *obj,
>  				struct drm_i915_gem_pwrite *args,
> @@ -605,7 +599,7 @@ i915_gem_gtt_pwrite_fast(struct drm_device *dev,
>  	char __user *user_data;
>  	int page_offset, page_length, ret;
>  
> -	ret = i915_gem_obj_ggtt_pin(obj, 0, true, true);
> +	ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE | PIN_NONBLOCK);
>  	if (ret)
>  		goto out;
>  
> @@ -1399,7 +1393,7 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
>  	}
>  
>  	/* Now bind it into the GTT if needed */
> -	ret = i915_gem_obj_ggtt_pin(obj,  0, true, false);
> +	ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
>  	if (ret)
>  		goto unlock;
>  
> @@ -2767,7 +2761,6 @@ int i915_vma_unbind(struct i915_vma *vma)
>  
>  	if (!drm_mm_node_allocated(&vma->node)) {
>  		i915_gem_vma_destroy(vma);
> -
>  		return 0;
>  	}
>  
> @@ -2819,26 +2812,6 @@ int i915_vma_unbind(struct i915_vma *vma)
>  	return 0;
>  }
>  
> -/**
> - * Unbinds an object from the global GTT aperture.
> - */
> -int
> -i915_gem_object_ggtt_unbind(struct drm_i915_gem_object *obj)
> -{
> -	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
> -	struct i915_address_space *ggtt = &dev_priv->gtt.base;
> -
> -	if (!i915_gem_obj_ggtt_bound(obj))
> -		return 0;
> -
> -	if (i915_gem_obj_to_ggtt(obj)->pin_count)
> -		return -EBUSY;
> -
> -	BUG_ON(obj->pages == NULL);
> -
> -	return i915_vma_unbind(i915_gem_obj_to_vma(obj, ggtt));
> -}
> -

My gut tells me this hunk should be in a different patch.

>  int i915_gpu_idle(struct drm_device *dev)
>  {
>  	drm_i915_private_t *dev_priv = dev->dev_private;
> @@ -3256,18 +3229,17 @@ static void i915_gem_verify_gtt(struct drm_device *dev)
>  /**
>   * Finds free space in the GTT aperture and binds the object there.
>   */
> -static int
> +static struct i915_vma *
>  i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
>  			   struct i915_address_space *vm,
>  			   unsigned alignment,
> -			   bool map_and_fenceable,
> -			   bool nonblocking)
> +			   unsigned flags)
>  {
>  	struct drm_device *dev = obj->base.dev;
>  	drm_i915_private_t *dev_priv = dev->dev_private;
>  	u32 size, fence_size, fence_alignment, unfenced_alignment;
>  	size_t gtt_max =
> -		map_and_fenceable ? dev_priv->gtt.mappable_end : vm->total;
> +		flags & PIN_MAPPABLE ? dev_priv->gtt.mappable_end : vm->total;
>  	struct i915_vma *vma;
>  	int ret;
>  
> @@ -3279,18 +3251,18 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
>  						     obj->tiling_mode, true);
>  	unfenced_alignment =
>  		i915_gem_get_gtt_alignment(dev,
> -						    obj->base.size,
> -						    obj->tiling_mode, false);
> +					   obj->base.size,
> +					   obj->tiling_mode, false);
>  
>  	if (alignment == 0)
> -		alignment = map_and_fenceable ? fence_alignment :
> +		alignment = flags & PIN_MAPPABLE ? fence_alignment :
>  						unfenced_alignment;
> -	if (map_and_fenceable && alignment & (fence_alignment - 1)) {
> +	if (flags & PIN_MAPPABLE && alignment & (fence_alignment - 1)) {
>  		DRM_ERROR("Invalid object alignment requested %u\n", alignment);
> -		return -EINVAL;
> +		return ERR_PTR(-EINVAL);
>  	}
>  
> -	size = map_and_fenceable ? fence_size : obj->base.size;
> +	size = flags & PIN_MAPPABLE ? fence_size : obj->base.size;
>  
>  	/* If the object is bigger than the entire aperture, reject it early
>  	 * before evicting everything in a vain attempt to find space.
> @@ -3298,22 +3270,20 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
>  	if (obj->base.size > gtt_max) {
>  		DRM_ERROR("Attempting to bind an object larger than the aperture: object=%zd > %s aperture=%zu\n",
>  			  obj->base.size,
> -			  map_and_fenceable ? "mappable" : "total",
> +			  flags & PIN_MAPPABLE ? "mappable" : "total",
>  			  gtt_max);
> -		return -E2BIG;
> +		return ERR_PTR(-E2BIG);
>  	}
>  
>  	ret = i915_gem_object_get_pages(obj);
>  	if (ret)
> -		return ret;
> +		return ERR_PTR(ret);
>  
>  	i915_gem_object_pin_pages(obj);
>  
>  	vma = i915_gem_obj_lookup_or_create_vma(obj, vm);
> -	if (IS_ERR(vma)) {
> -		ret = PTR_ERR(vma);
> +	if (IS_ERR(vma))
>  		goto err_unpin;
> -	}
>  
>  search_free:
>  	ret = drm_mm_insert_node_in_range_generic(&vm->mm, &vma->node,
> @@ -3322,9 +3292,7 @@ search_free:
>  						  DRM_MM_SEARCH_DEFAULT);
>  	if (ret) {
>  		ret = i915_gem_evict_something(dev, vm, size, alignment,
> -					       obj->cache_level,
> -					       map_and_fenceable,
> -					       nonblocking);
> +					       obj->cache_level, flags);
>  		if (ret == 0)
>  			goto search_free;
>  
> @@ -3355,19 +3323,23 @@ search_free:
>  		obj->map_and_fenceable = mappable && fenceable;
>  	}
>  
> -	WARN_ON(map_and_fenceable && !obj->map_and_fenceable);
> +	WARN_ON(flags & PIN_MAPPABLE && !obj->map_and_fenceable);
> +
> +	trace_i915_vma_bind(vma, flags);
> +	vma->bind_vma(vma, obj->cache_level,
> +		      flags & (PIN_MAPPABLE | PIN_GLOBAL) ? GLOBAL_BIND : 0);
>  
> -	trace_i915_vma_bind(vma, map_and_fenceable);
>  	i915_gem_verify_gtt(dev);
> -	return 0;
> +	return vma;
>  
>  err_remove_node:
>  	drm_mm_remove_node(&vma->node);
>  err_free_vma:
>  	i915_gem_vma_destroy(vma);
> +	vma = ERR_PTR(ret);
>  err_unpin:
>  	i915_gem_object_unpin_pages(obj);
> -	return ret;
> +	return vma;
>  }
>  
>  bool
> @@ -3559,7 +3531,9 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
>  		}
>  
>  		list_for_each_entry(vma, &obj->vma_list, vma_link)
> -			vma->bind_vma(vma, cache_level, 0);
> +			if (drm_mm_node_allocated(&vma->node))
> +				vma->bind_vma(vma, cache_level,
> +					      obj->has_global_gtt_mapping ? GLOBAL_BIND : 0);

will if (vma->node.color != cache_level) work here?

>  	}
>  
>  	list_for_each_entry(vma, &obj->vma_list, vma_link)
> @@ -3728,7 +3702,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>  	 * (e.g. libkms for the bootup splash), we have to ensure that we
>  	 * always use map_and_fenceable for all scanout buffers.
>  	 */
> -	ret = i915_gem_obj_ggtt_pin(obj, alignment, true, false);
> +	ret = i915_gem_obj_ggtt_pin(obj, alignment, PIN_MAPPABLE);
>  	if (ret)
>  		goto err_unpin_display;
>  
> @@ -3884,52 +3858,49 @@ int
>  i915_gem_object_pin(struct drm_i915_gem_object *obj,
>  		    struct i915_address_space *vm,
>  		    uint32_t alignment,
> -		    bool map_and_fenceable,
> -		    bool nonblocking)
> +		    unsigned flags)
>  {
> -	const u32 flags = map_and_fenceable ? GLOBAL_BIND : 0;
>  	struct i915_vma *vma;
>  	int ret;
>  
> -	WARN_ON(map_and_fenceable && !i915_is_ggtt(vm));
> +	if (WARN_ON(flags & (PIN_GLOBAL | PIN_MAPPABLE) && !i915_is_ggtt(vm)))
> +		return -EINVAL;
>  
>  	vma = i915_gem_obj_to_vma(obj, vm);
> -
>  	if (vma) {
>  		if (WARN_ON(vma->pin_count == DRM_I915_GEM_OBJECT_MAX_PIN_COUNT))
>  			return -EBUSY;
>  
>  		if ((alignment &&
>  		     vma->node.start & (alignment - 1)) ||
> -		    (map_and_fenceable && !obj->map_and_fenceable)) {
> +		    (flags & PIN_MAPPABLE && !obj->map_and_fenceable)) {
>  			WARN(vma->pin_count,
>  			     "bo is already pinned with incorrect alignment:"
>  			     " offset=%lx, req.alignment=%x, req.map_and_fenceable=%d,"
>  			     " obj->map_and_fenceable=%d\n",
>  			     i915_gem_obj_offset(obj, vm), alignment,
> -			     map_and_fenceable,
> +			     flags & PIN_MAPPABLE,
>  			     obj->map_and_fenceable);
>  			ret = i915_vma_unbind(vma);
>  			if (ret)
>  				return ret;
> +
> +			vma = NULL;
>  		}
>  	}
>  
> -	if (!i915_gem_obj_bound(obj, vm)) {
> -		ret = i915_gem_object_bind_to_vm(obj, vm, alignment,
> -						 map_and_fenceable,
> -						 nonblocking);
> -		if (ret)
> -			return ret;
> -
> +	if (vma == NULL || !drm_mm_node_allocated(&vma->node)) {
> +		vma = i915_gem_object_bind_to_vm(obj, vm, alignment, flags);
> +		if (IS_ERR(vma))
> +			return PTR_ERR(vma);
>  	}
>  
> -	vma = i915_gem_obj_to_vma(obj, vm);
> -
> -	vma->bind_vma(vma, obj->cache_level, flags);
> +	if (flags & PIN_GLOBAL && !obj->has_global_gtt_mapping)
> +		vma->bind_vma(vma, obj->cache_level, GLOBAL_BIND);

For some reason I remember seeing a regression when I tried this. It
should be safe to do this without most of the other changes - now I
wonder what I had seen.

>  
> -	i915_gem_obj_to_vma(obj, vm)->pin_count++;
> -	obj->pin_mappable |= map_and_fenceable;
> +	vma->pin_count++;
> +	if (flags & PIN_MAPPABLE)
> +		obj->pin_mappable |= true;

just '= true'

>  
>  	return 0;
>  }
> @@ -3987,7 +3958,7 @@ i915_gem_pin_ioctl(struct drm_device *dev, void *data,
>  	}
>  
>  	if (obj->user_pin_count == 0) {
> -		ret = i915_gem_obj_ggtt_pin(obj, args->alignment, true, false);
> +		ret = i915_gem_obj_ggtt_pin(obj, args->alignment, PIN_MAPPABLE);
>  		if (ret)
>  			goto out;
>  	}
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index ebe0f67eac08..b428b101f9d0 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -282,8 +282,7 @@ i915_gem_create_context(struct drm_device *dev,
>  			 * context.
>  			 */
>  			ret = i915_gem_obj_ggtt_pin(ctx->obj,
> -						    get_context_alignment(dev),
> -						    false, false);
> +						    get_context_alignment(dev), 0);
>  			if (ret) {
>  				DRM_DEBUG_DRIVER("Couldn't pin %d\n", ret);
>  				goto err_destroy;
> @@ -336,8 +335,7 @@ void i915_gem_context_reset(struct drm_device *dev)
>  
>  		if (i == RCS) {
>  			WARN_ON(i915_gem_obj_ggtt_pin(dctx->obj,
> -						      get_context_alignment(dev),
> -						      false, false));
> +						      get_context_alignment(dev), 0));
>  			/* Fake a finish/inactive */
>  			dctx->obj->base.write_domain = 0;
>  			dctx->obj->active = 0;
> @@ -600,8 +598,7 @@ static int do_switch(struct intel_ring_buffer *ring,
>  	/* Trying to pin first makes error handling easier. */
>  	if (ring == &dev_priv->ring[RCS]) {
>  		ret = i915_gem_obj_ggtt_pin(to->obj,
> -					    get_context_alignment(ring->dev),
> -					    false, false);
> +					    get_context_alignment(ring->dev), 0);
>  		if (ret)
>  			return ret;
>  	}
> diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
> index 54413ed46a46..fed72a67afd2 100644
> --- a/drivers/gpu/drm/i915/i915_gem_evict.c
> +++ b/drivers/gpu/drm/i915/i915_gem_evict.c
> @@ -48,14 +48,14 @@ mark_free(struct i915_vma *vma, struct list_head *unwind)
>  int
>  i915_gem_evict_something(struct drm_device *dev, struct i915_address_space *vm,
>  			 int min_size, unsigned alignment, unsigned cache_level,
> -			 bool mappable, bool nonblocking)
> +			 unsigned flags)
>  {
>  	drm_i915_private_t *dev_priv = dev->dev_private;
>  	struct list_head eviction_list, unwind_list;
>  	struct i915_vma *vma;
>  	int ret = 0;
>  
> -	trace_i915_gem_evict(dev, min_size, alignment, mappable);
> +	trace_i915_gem_evict(dev, min_size, alignment, flags);
>  
>  	/*
>  	 * The goal is to evict objects and amalgamate space in LRU order.
> @@ -81,7 +81,7 @@ i915_gem_evict_something(struct drm_device *dev, struct i915_address_space *vm,
>  	 */
>  
>  	INIT_LIST_HEAD(&unwind_list);
> -	if (mappable) {
> +	if (flags & PIN_MAPPABLE) {
>  		BUG_ON(!i915_is_ggtt(vm));
>  		drm_mm_init_scan_with_range(&vm->mm, min_size,
>  					    alignment, cache_level, 0,
> @@ -96,7 +96,7 @@ search_again:
>  			goto found;
>  	}
>  
> -	if (nonblocking)
> +	if (flags & PIN_NONBLOCK)
>  		goto none;
>  
>  	/* Now merge in the soon-to-be-expired objects... */
> @@ -120,13 +120,13 @@ none:
>  	/* Can we unpin some objects such as idle hw contents,
>  	 * or pending flips?
>  	 */
> -	ret = nonblocking ? -ENOSPC : i915_gpu_idle(dev);
> +	ret = flags & PIN_NONBLOCK ? -ENOSPC : i915_gpu_idle(dev);
>  	if (ret)
>  		return ret;
>  
>  	/* Only idle the GPU and repeat the search once */
>  	i915_gem_retire_requests(dev);
> -	nonblocking = true;
> +	flags |= PIN_NONBLOCK;
>  	goto search_again;
>  
>  found:
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index bbff8f9b6549..cd09d42f507b 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -544,19 +544,23 @@ i915_gem_execbuffer_reserve_vma(struct i915_vma *vma,
>  	struct drm_i915_gem_object *obj = vma->obj;
>  	struct drm_i915_gem_exec_object2 *entry = vma->exec_entry;
>  	bool has_fenced_gpu_access = INTEL_INFO(ring->dev)->gen < 4;
> -	bool need_fence, need_mappable;
> -	u32 flags = (entry->flags & EXEC_OBJECT_NEEDS_GTT) &&
> -		!vma->obj->has_global_gtt_mapping ? GLOBAL_BIND : 0;
> +	bool need_fence;
> +	unsigned flags;
>  	int ret;
>  
> +	flags = 0;
> +
>  	need_fence =
>  		has_fenced_gpu_access &&
>  		entry->flags & EXEC_OBJECT_NEEDS_FENCE &&
>  		obj->tiling_mode != I915_TILING_NONE;
> -	need_mappable = need_fence || need_reloc_mappable(vma);
> +	if (need_fence || need_reloc_mappable(vma))
> +		flags |= PIN_MAPPABLE;
>  
> -	ret = i915_gem_object_pin(obj, vma->vm, entry->alignment, need_mappable,
> -				  false);
> +	if (entry->flags & EXEC_OBJECT_NEEDS_GTT)
> +		flags |= PIN_GLOBAL;
> +
> +	ret = i915_gem_object_pin(obj, vma->vm, entry->alignment, flags);
>  	if (ret)
>  		return ret;
>  
> @@ -585,8 +589,6 @@ i915_gem_execbuffer_reserve_vma(struct i915_vma *vma,
>  		obj->base.pending_write_domain = I915_GEM_DOMAIN_RENDER;
>  	}
>  
> -	vma->bind_vma(vma, obj->cache_level, flags);
> -
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 998f9a0b322a..d113eb5e2f5b 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -865,7 +865,7 @@ alloc:
>  	if (ret == -ENOSPC && !retried) {
>  		ret = i915_gem_evict_something(dev, &dev_priv->gtt.base,
>  					       GEN6_PD_SIZE, GEN6_PD_ALIGN,
> -					       I915_CACHE_NONE, false, true);
> +					       I915_CACHE_NONE, 0);

You sure you want to stick this behavioral change in here?

>  		if (ret)
>  			return ret;
>  
> diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
> index 6e580c98dede..b95a380958db 100644
> --- a/drivers/gpu/drm/i915/i915_trace.h
> +++ b/drivers/gpu/drm/i915/i915_trace.h
> @@ -34,15 +34,15 @@ TRACE_EVENT(i915_gem_object_create,
>  );
>  
>  TRACE_EVENT(i915_vma_bind,
> -	    TP_PROTO(struct i915_vma *vma, bool mappable),
> -	    TP_ARGS(vma, mappable),
> +	    TP_PROTO(struct i915_vma *vma, unsigned flags),
> +	    TP_ARGS(vma, flags),
>  
>  	    TP_STRUCT__entry(
>  			     __field(struct drm_i915_gem_object *, obj)
>  			     __field(struct i915_address_space *, vm)
>  			     __field(u32, offset)
>  			     __field(u32, size)
> -			     __field(bool, mappable)
> +			     __field(unsigned, flags)
>  			     ),
>  
>  	    TP_fast_assign(
> @@ -50,12 +50,12 @@ TRACE_EVENT(i915_vma_bind,
>  			   __entry->vm = vma->vm;
>  			   __entry->offset = vma->node.start;
>  			   __entry->size = vma->node.size;
> -			   __entry->mappable = mappable;
> +			   __entry->flags = flags;
>  			   ),
>  
>  	    TP_printk("obj=%p, offset=%08x size=%x%s vm=%p",
>  		      __entry->obj, __entry->offset, __entry->size,
> -		      __entry->mappable ? ", mappable" : "",
> +		      __entry->flags & PIN_MAPPABLE ? ", mappable" : "",
>  		      __entry->vm)
>  );
>  
> @@ -196,26 +196,26 @@ DEFINE_EVENT(i915_gem_object, i915_gem_object_destroy,
>  );
>  
>  TRACE_EVENT(i915_gem_evict,
> -	    TP_PROTO(struct drm_device *dev, u32 size, u32 align, bool mappable),
> -	    TP_ARGS(dev, size, align, mappable),
> +	    TP_PROTO(struct drm_device *dev, u32 size, u32 align, unsigned flags),
> +	    TP_ARGS(dev, size, align, flags),
>  
>  	    TP_STRUCT__entry(
>  			     __field(u32, dev)
>  			     __field(u32, size)
>  			     __field(u32, align)
> -			     __field(bool, mappable)
> +			     __field(unsigned, flags)
>  			    ),
>  
>  	    TP_fast_assign(
>  			   __entry->dev = dev->primary->index;
>  			   __entry->size = size;
>  			   __entry->align = align;
> -			   __entry->mappable = mappable;
> +			   __entry->flags = flags;
>  			  ),
>  
>  	    TP_printk("dev=%d, size=%d, align=%d %s",
>  		      __entry->dev, __entry->size, __entry->align,
> -		      __entry->mappable ? ", mappable" : "")
> +		      __entry->flags & PIN_MAPPABLE ? ", mappable" : "")
>  );
>  
>  TRACE_EVENT(i915_gem_evict_everything,
> diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> index a1397b1646af..1d46608bbd8b 100644
> --- a/drivers/gpu/drm/i915/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/intel_overlay.c
> @@ -1349,7 +1349,7 @@ void intel_setup_overlay(struct drm_device *dev)
>  		}
>  		overlay->flip_addr = reg_bo->phys_obj->handle->busaddr;
>  	} else {
> -		ret = i915_gem_obj_ggtt_pin(reg_bo, PAGE_SIZE, true, false);
> +		ret = i915_gem_obj_ggtt_pin(reg_bo, PAGE_SIZE, PIN_MAPPABLE);
>  		if (ret) {
>  			DRM_ERROR("failed to pin overlay register bo\n");
>  			goto out_free_bo;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 469170c11bb4..e29804eda113 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2753,7 +2753,7 @@ intel_alloc_context_page(struct drm_device *dev)
>  		return NULL;
>  	}
>  
> -	ret = i915_gem_obj_ggtt_pin(ctx, 4096, true, false);
> +	ret = i915_gem_obj_ggtt_pin(ctx, 4096, 0);

and here?

>  	if (ret) {
>  		DRM_ERROR("failed to pin power context: %d\n", ret);
>  		goto err_unref;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 442c9a66ebab..5744841669e4 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -531,9 +531,11 @@ init_pipe_control(struct intel_ring_buffer *ring)
>  		goto err;
>  	}
>  
> -	i915_gem_object_set_cache_level(ring->scratch.obj, I915_CACHE_LLC);
> +	ret = i915_gem_object_set_cache_level(ring->scratch.obj, I915_CACHE_LLC);
> +	if (ret)
> +		goto err_unref;
>  
> -	ret = i915_gem_obj_ggtt_pin(ring->scratch.obj, 4096, true, false);
> +	ret = i915_gem_obj_ggtt_pin(ring->scratch.obj, 4096, 0);
>  	if (ret)
>  		goto err_unref;

and here?

>  
> @@ -1269,12 +1271,13 @@ static int init_status_page(struct intel_ring_buffer *ring)
>  		goto err;
>  	}
>  
> -	i915_gem_object_set_cache_level(obj, I915_CACHE_LLC);
> +	ret = i915_gem_object_set_cache_level(obj, I915_CACHE_LLC);
> +	if (ret)
> +		goto err_unref;
>  
> -	ret = i915_gem_obj_ggtt_pin(obj, 4096, true, false);
> -	if (ret != 0) {
> +	ret = i915_gem_obj_ggtt_pin(obj, 4096, 0);
> +	if (ret)
>  		goto err_unref;
> -	}

and here?

>  
>  	ring->status_page.gfx_addr = i915_gem_obj_ggtt_offset(obj);
>  	ring->status_page.page_addr = kmap(sg_page(obj->pages->sgl));
> @@ -1354,7 +1357,7 @@ static int intel_init_ring_buffer(struct drm_device *dev,
>  
>  	ring->obj = obj;
>  
> -	ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, true, false);
> +	ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, PIN_MAPPABLE);
>  	if (ret)
>  		goto err_unref;
>  
> @@ -1927,7 +1930,7 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
>  			return -ENOMEM;
>  		}
>  
> -		ret = i915_gem_obj_ggtt_pin(obj, 0, true, false);
> +		ret = i915_gem_obj_ggtt_pin(obj, 0, 0);

and here?

>  		if (ret != 0) {
>  			drm_gem_object_unreference(&obj->base);
>  			DRM_ERROR("Failed to ping batch bo\n");
> -- 
> 1.8.5.2
> 

I think it would have been better to do a prep patch with all m&f ->
flags changes - and then the actual fixes. And all the changes for
the mappable/fenceable attributes in a separate patch as well.

Past those relatively minor suggestions, lgtm. I'd like to see the patch
merged and run through as much QA/use as possible.

In its current state:
Acked-by: Ben Widawsky <ben@xxxxxxxxxxxx>

If you want to break it up a bit, I'll try to turn it into an r-b


-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux