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
