On Tue, Jul 09, 2013 at 09:15:01AM +0200, Daniel Vetter wrote:
> On Mon, Jul 08, 2013 at 11:08:37PM -0700, Ben Widawsky wrote:
> > This patch was formerly known as:
> > "drm/i915: Create VMAs (part 3) - plumbing"
> >
> > This patch adds a VM argument, bind/unbind, and the object
> > offset/size/color getters/setters. It preserves the old ggtt helper
> > functions because things still need, and will continue to need them.
> >
> > Some code will still need to be ported over after this.
> >
> > v2: Fix purge to pick an object and unbind all vmas
> > This was doable because of the global bound list change.
> >
> > v3: With the commit to actually pin/unpin pages in place, there is no
> > longer a need to check if unbind succeeded before calling put_pages().
> > Make put_pages only BUG() after checking pin count.
> >
> > v4: Rebased on top of the new hangcheck work by Mika
> > plumbed eb_destroy also
> > Many checkpatch related fixes
> >
> > v5: Very large rebase
> >
> > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
>
> This one is a rather large beast. Any chance we could split it into
> topics, e.g. convert execbuf code, convert shrinker code? Or does that get
> messy, fast?
>
> > ---
> > drivers/gpu/drm/i915/i915_debugfs.c | 31 ++-
> > drivers/gpu/drm/i915/i915_dma.c | 4 -
> > drivers/gpu/drm/i915/i915_drv.h | 107 +++++-----
> > drivers/gpu/drm/i915/i915_gem.c | 316 +++++++++++++++++++++--------
> > drivers/gpu/drm/i915/i915_gem_context.c | 9 +-
> > drivers/gpu/drm/i915/i915_gem_evict.c | 51 +++--
> > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 85 +++++---
> > drivers/gpu/drm/i915/i915_gem_gtt.c | 41 ++--
> > drivers/gpu/drm/i915/i915_gem_stolen.c | 6 +-
> > drivers/gpu/drm/i915/i915_gem_tiling.c | 10 +-
> > drivers/gpu/drm/i915/i915_irq.c | 6 +-
> > drivers/gpu/drm/i915/i915_trace.h | 20 +-
> > drivers/gpu/drm/i915/intel_fb.c | 1 -
> > drivers/gpu/drm/i915/intel_overlay.c | 2 +-
> > drivers/gpu/drm/i915/intel_pm.c | 2 +-
> > drivers/gpu/drm/i915/intel_ringbuffer.c | 16 +-
> > 16 files changed, 468 insertions(+), 239 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 16b2aaf..867ed07 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -122,9 +122,18 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
> > seq_printf(m, " (pinned x %d)", obj->pin_count);
> > if (obj->fence_reg != I915_FENCE_REG_NONE)
> > seq_printf(m, " (fence: %d)", obj->fence_reg);
> > - if (i915_gem_obj_ggtt_bound(obj))
> > - seq_printf(m, " (gtt offset: %08lx, size: %08x)",
> > - i915_gem_obj_ggtt_offset(obj), (unsigned int)i915_gem_obj_ggtt_size(obj));
> > + if (i915_gem_obj_bound_any(obj)) {
>
> list_for_each will short-circuit already, so this is redundant.
>
Got it.
> > + struct i915_vma *vma;
> > + list_for_each_entry(vma, &obj->vma_list, vma_link) {
> > + if (!i915_is_ggtt(vma->vm))
> > + seq_puts(m, " (pp");
> > + else
> > + seq_puts(m, " (g");
> > + seq_printf(m, " gtt offset: %08lx, size: %08lx)",
>
> ^ that space looks superflous now
Got it.
>
> > + i915_gem_obj_offset(obj, vma->vm),
> > + i915_gem_obj_size(obj, vma->vm));
> > + }
> > + }
> > if (obj->stolen)
> > seq_printf(m, " (stolen: %08lx)", obj->stolen->start);
> > if (obj->pin_mappable || obj->fault_mappable) {
> > @@ -186,6 +195,7 @@ static int i915_gem_object_list_info(struct seq_file *m, void *data)
> > return 0;
> > }
> >
> > +/* FIXME: Support multiple VM? */
> > #define count_objects(list, member) do { \
> > list_for_each_entry(obj, list, member) { \
> > size += i915_gem_obj_ggtt_size(obj); \
> > @@ -2049,18 +2059,21 @@ i915_drop_caches_set(void *data, u64 val)
> >
> > if (val & DROP_BOUND) {
> > list_for_each_entry_safe(obj, next, &vm->inactive_list,
> > - mm_list)
> > - if (obj->pin_count == 0) {
> > - ret = i915_gem_object_unbind(obj);
> > - if (ret)
> > - goto unlock;
> > - }
> > + mm_list) {
> > + if (obj->pin_count)
> > + continue;
> > +
> > + ret = i915_gem_object_unbind(obj, &dev_priv->gtt.base);
> > + if (ret)
> > + goto unlock;
> > + }
> > }
> >
> > if (val & DROP_UNBOUND) {
> > list_for_each_entry_safe(obj, next, &dev_priv->mm.unbound_list,
> > global_list)
> > if (obj->pages_pin_count == 0) {
> > + /* FIXME: Do this for all vms? */
> > ret = i915_gem_object_put_pages(obj);
> > if (ret)
> > goto unlock;
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index d13e21f..b190439 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1497,10 +1497,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> >
> > i915_dump_device_info(dev_priv);
> >
> > - INIT_LIST_HEAD(&dev_priv->vm_list);
> > - INIT_LIST_HEAD(&dev_priv->gtt.base.global_link);
> > - list_add(&dev_priv->gtt.base.global_link, &dev_priv->vm_list);
> > -
> > if (i915_get_bridge_dev(dev)) {
> > ret = -EIO;
> > goto free_priv;
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 38cccc8..48baccc 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1363,52 +1363,6 @@ struct drm_i915_gem_object {
> >
> > #define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base)
> >
> > -/* This is a temporary define to help transition us to real VMAs. If you see
> > - * this, you're either reviewing code, or bisecting it. */
> > -static inline struct i915_vma *
> > -__i915_gem_obj_to_vma(struct drm_i915_gem_object *obj)
> > -{
> > - if (list_empty(&obj->vma_list))
> > - return NULL;
> > - return list_first_entry(&obj->vma_list, struct i915_vma, vma_link);
> > -}
> > -
> > -/* Whether or not this object is currently mapped by the translation tables */
> > -static inline bool
> > -i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *o)
> > -{
> > - struct i915_vma *vma = __i915_gem_obj_to_vma(o);
> > - if (vma == NULL)
> > - return false;
> > - return drm_mm_node_allocated(&vma->node);
> > -}
> > -
> > -/* Offset of the first PTE pointing to this object */
> > -static inline unsigned long
> > -i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *o)
> > -{
> > - BUG_ON(list_empty(&o->vma_list));
> > - return __i915_gem_obj_to_vma(o)->node.start;
> > -}
> > -
> > -/* The size used in the translation tables may be larger than the actual size of
> > - * the object on GEN2/GEN3 because of the way tiling is handled. See
> > - * i915_gem_get_gtt_size() for more details.
> > - */
> > -static inline unsigned long
> > -i915_gem_obj_ggtt_size(struct drm_i915_gem_object *o)
> > -{
> > - BUG_ON(list_empty(&o->vma_list));
> > - return __i915_gem_obj_to_vma(o)->node.size;
> > -}
> > -
> > -static inline void
> > -i915_gem_obj_ggtt_set_color(struct drm_i915_gem_object *o,
> > - enum i915_cache_level color)
> > -{
> > - __i915_gem_obj_to_vma(o)->node.color = color;
> > -}
> > -
> > /**
> > * Request queue structure.
> > *
> > @@ -1726,11 +1680,13 @@ struct i915_vma *i915_gem_vma_create(struct drm_i915_gem_object *obj,
> > 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_unpin(struct drm_i915_gem_object *obj);
> > -int __must_check i915_gem_object_unbind(struct drm_i915_gem_object *obj);
> > +int __must_check i915_gem_object_unbind(struct drm_i915_gem_object *obj,
> > + struct i915_address_space *vm);
> > int i915_gem_object_put_pages(struct drm_i915_gem_object *obj);
> > void i915_gem_release_mmap(struct drm_i915_gem_object *obj);
> > void i915_gem_lastclose(struct drm_device *dev);
> > @@ -1760,6 +1716,7 @@ int __must_check i915_mutex_lock_interruptible(struct drm_device *dev);
> > int i915_gem_object_sync(struct drm_i915_gem_object *obj,
> > struct intel_ring_buffer *to);
> > void i915_gem_object_move_to_active(struct drm_i915_gem_object *obj,
> > + struct i915_address_space *vm,
> > struct intel_ring_buffer *ring);
> >
> > int i915_gem_dumb_create(struct drm_file *file_priv,
> > @@ -1866,6 +1823,7 @@ i915_gem_get_gtt_alignment(struct drm_device *dev, uint32_t size,
> > int tiling_mode, bool fenced);
> >
> > int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> > + struct i915_address_space *vm,
> > enum i915_cache_level cache_level);
> >
> > struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
> > @@ -1876,6 +1834,56 @@ struct dma_buf *i915_gem_prime_export(struct drm_device *dev,
> >
> > void i915_gem_restore_fences(struct drm_device *dev);
> >
> > +unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o,
> > + struct i915_address_space *vm);
> > +bool i915_gem_obj_bound_any(struct drm_i915_gem_object *o);
> > +bool i915_gem_obj_bound(struct drm_i915_gem_object *o,
> > + struct i915_address_space *vm);
> > +unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
> > + struct i915_address_space *vm);
> > +void i915_gem_obj_set_color(struct drm_i915_gem_object *o,
> > + struct i915_address_space *vm,
> > + enum i915_cache_level color);
> > +struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
> > + struct i915_address_space *vm);
> > +/* Some GGTT VM helpers */
> > +#define obj_to_ggtt(obj) \
> > + (&((struct drm_i915_private *)(obj)->base.dev->dev_private)->gtt.base)
> > +static inline bool i915_is_ggtt(struct i915_address_space *vm)
> > +{
> > + struct i915_address_space *ggtt =
> > + &((struct drm_i915_private *)(vm)->dev->dev_private)->gtt.base;
> > + return vm == ggtt;
> > +}
> > +
> > +static inline bool i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *obj)
> > +{
> > + return i915_gem_obj_bound(obj, obj_to_ggtt(obj));
> > +}
> > +
> > +static inline unsigned long
> > +i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *obj)
> > +{
> > + return i915_gem_obj_offset(obj, obj_to_ggtt(obj));
> > +}
> > +
> > +static inline unsigned long
> > +i915_gem_obj_ggtt_size(struct drm_i915_gem_object *obj)
> > +{
> > + return i915_gem_obj_size(obj, obj_to_ggtt(obj));
> > +}
> > +
> > +static inline int __must_check
> > +i915_gem_ggtt_pin(struct drm_i915_gem_object *obj,
> > + uint32_t alignment,
> > + bool map_and_fenceable,
> > + bool nonblocking)
> > +{
> > + return i915_gem_object_pin(obj, obj_to_ggtt(obj), alignment,
> > + map_and_fenceable, nonblocking);
> > +}
> > +#undef obj_to_ggtt
> > +
> > /* i915_gem_context.c */
> > void i915_gem_context_init(struct drm_device *dev);
> > void i915_gem_context_fini(struct drm_device *dev);
> > @@ -1912,6 +1920,7 @@ void i915_ppgtt_unbind_object(struct i915_hw_ppgtt *ppgtt,
> >
> > void i915_gem_restore_gtt_mappings(struct drm_device *dev);
> > int __must_check i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj);
> > +/* FIXME: this is never okay with full PPGTT */
> > void i915_gem_gtt_bind_object(struct drm_i915_gem_object *obj,
> > enum i915_cache_level cache_level);
> > void i915_gem_gtt_unbind_object(struct drm_i915_gem_object *obj);
> > @@ -1928,7 +1937,9 @@ static inline void i915_gem_chipset_flush(struct drm_device *dev)
> >
> >
> > /* i915_gem_evict.c */
> > -int __must_check i915_gem_evict_something(struct drm_device *dev, int min_size,
> > +int __must_check i915_gem_evict_something(struct drm_device *dev,
> > + struct i915_address_space *vm,
> > + int min_size,
> > unsigned alignment,
> > unsigned cache_level,
> > bool mappable,
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 058ad44..21015cd 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -38,10 +38,12 @@
> >
> > static void i915_gem_object_flush_gtt_write_domain(struct drm_i915_gem_object *obj);
> > static void i915_gem_object_flush_cpu_write_domain(struct drm_i915_gem_object *obj);
> > -static __must_check int i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
> > - unsigned alignment,
> > - bool map_and_fenceable,
> > - bool nonblocking);
> > +static __must_check int
> > +i915_gem_object_bind_to_gtt(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,
> > @@ -135,7 +137,7 @@ int i915_mutex_lock_interruptible(struct drm_device *dev)
> > static inline bool
> > i915_gem_object_is_inactive(struct drm_i915_gem_object *obj)
> > {
> > - return i915_gem_obj_ggtt_bound(obj) && !obj->active;
> > + return i915_gem_obj_bound_any(obj) && !obj->active;
> > }
> >
> > int
> > @@ -422,7 +424,7 @@ i915_gem_shmem_pread(struct drm_device *dev,
> > * anyway again before the next pread happens. */
> > if (obj->cache_level == I915_CACHE_NONE)
> > needs_clflush = 1;
> > - if (i915_gem_obj_ggtt_bound(obj)) {
> > + if (i915_gem_obj_bound_any(obj)) {
> > ret = i915_gem_object_set_to_gtt_domain(obj, false);
>
> This is essentially a very convoluted version of "if there's gpu rendering
> outstanding, please wait for it". Maybe we should switch this to
>
> if (obj->active)
> wait_rendering(obj, true);
>
> Same for the shmem_pwrite case below. Would be a separate patch to prep
> things though. Can I volunteer you for that? The ugly part is to review
> whether any of the lru list updating that set_domain does in addition to
> wait_rendering is required, but on a quick read that's not the case.
Just reading the comment above it says we need the clflush. I don't
actually understand why we do that even after reading the comment, but
meh. You tell me, I don't mind doing this as a prep first.
>
> > if (ret)
> > return ret;
> > @@ -594,7 +596,7 @@ i915_gem_gtt_pwrite_fast(struct drm_device *dev,
> > char __user *user_data;
> > int page_offset, page_length, ret;
> >
> > - ret = i915_gem_object_pin(obj, 0, true, true);
> > + ret = i915_gem_ggtt_pin(obj, 0, true, true);
> > if (ret)
> > goto out;
> >
> > @@ -739,7 +741,7 @@ i915_gem_shmem_pwrite(struct drm_device *dev,
> > * right away and we therefore have to clflush anyway. */
> > if (obj->cache_level == I915_CACHE_NONE)
> > needs_clflush_after = 1;
> > - if (i915_gem_obj_ggtt_bound(obj)) {
> > + if (i915_gem_obj_bound_any(obj)) {
>
> ... see above.
> > ret = i915_gem_object_set_to_gtt_domain(obj, true);
> > if (ret)
> > return ret;
> > @@ -1346,7 +1348,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_object_pin(obj, 0, true, false);
> > + ret = i915_gem_ggtt_pin(obj, 0, true, false);
> > if (ret)
> > goto unlock;
> >
> > @@ -1668,11 +1670,11 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
> > if (obj->pages == NULL)
> > return 0;
> >
> > - BUG_ON(i915_gem_obj_ggtt_bound(obj));
> > -
> > if (obj->pages_pin_count)
> > return -EBUSY;
> >
> > + BUG_ON(i915_gem_obj_bound_any(obj));
> > +
> > /* ->put_pages might need to allocate memory for the bit17 swizzle
> > * array, hence protect them from being reaped by removing them from gtt
> > * lists early. */
> > @@ -1692,7 +1694,6 @@ __i915_gem_shrink(struct drm_i915_private *dev_priv, long target,
> > bool purgeable_only)
> > {
> > struct drm_i915_gem_object *obj, *next;
> > - struct i915_address_space *vm = &dev_priv->gtt.base;
> > long count = 0;
> >
> > list_for_each_entry_safe(obj, next,
> > @@ -1706,14 +1707,22 @@ __i915_gem_shrink(struct drm_i915_private *dev_priv, long target,
> > }
> > }
> >
> > - list_for_each_entry_safe(obj, next, &vm->inactive_list, mm_list) {
> > - if ((i915_gem_object_is_purgeable(obj) || !purgeable_only) &&
> > - i915_gem_object_unbind(obj) == 0 &&
> > - i915_gem_object_put_pages(obj) == 0) {
> > + list_for_each_entry_safe(obj, next, &dev_priv->mm.bound_list,
> > + global_list) {
> > + struct i915_vma *vma, *v;
> > +
> > + if (!i915_gem_object_is_purgeable(obj) && purgeable_only)
> > + continue;
> > +
> > + list_for_each_entry_safe(vma, v, &obj->vma_list, vma_link)
> > + if (i915_gem_object_unbind(obj, vma->vm))
> > + break;
> > +
> > + if (!i915_gem_object_put_pages(obj))
> > count += obj->base.size >> PAGE_SHIFT;
> > - if (count >= target)
> > - return count;
> > - }
> > +
> > + if (count >= target)
> > + return count;
> > }
> >
> > return count;
> > @@ -1873,11 +1882,11 @@ i915_gem_object_get_pages(struct drm_i915_gem_object *obj)
> >
> > void
> > i915_gem_object_move_to_active(struct drm_i915_gem_object *obj,
> > + struct i915_address_space *vm,
> > struct intel_ring_buffer *ring)
> > {
> > struct drm_device *dev = obj->base.dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > - struct i915_address_space *vm = &dev_priv->gtt.base;
> > u32 seqno = intel_ring_get_seqno(ring);
> >
> > BUG_ON(ring == NULL);
> > @@ -1910,12 +1919,9 @@ i915_gem_object_move_to_active(struct drm_i915_gem_object *obj,
> > }
> >
> > static void
> > -i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
> > +i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj,
> > + struct i915_address_space *vm)
> > {
> > - struct drm_device *dev = obj->base.dev;
> > - struct drm_i915_private *dev_priv = dev->dev_private;
> > - struct i915_address_space *vm = &dev_priv->gtt.base;
> > -
> > BUG_ON(obj->base.write_domain & ~I915_GEM_GPU_DOMAINS);
> > BUG_ON(!obj->active);
> >
> > @@ -2117,10 +2123,11 @@ i915_gem_request_remove_from_client(struct drm_i915_gem_request *request)
> > spin_unlock(&file_priv->mm.lock);
> > }
> >
> > -static bool i915_head_inside_object(u32 acthd, struct drm_i915_gem_object *obj)
> > +static bool i915_head_inside_object(u32 acthd, struct drm_i915_gem_object *obj,
> > + struct i915_address_space *vm)
> > {
> > - if (acthd >= i915_gem_obj_ggtt_offset(obj) &&
> > - acthd < i915_gem_obj_ggtt_offset(obj) + obj->base.size)
> > + if (acthd >= i915_gem_obj_offset(obj, vm) &&
> > + acthd < i915_gem_obj_offset(obj, vm) + obj->base.size)
> > return true;
> >
> > return false;
> > @@ -2143,6 +2150,17 @@ static bool i915_head_inside_request(const u32 acthd_unmasked,
> > return false;
> > }
> >
> > +static struct i915_address_space *
> > +request_to_vm(struct drm_i915_gem_request *request)
> > +{
> > + struct drm_i915_private *dev_priv = request->ring->dev->dev_private;
> > + struct i915_address_space *vm;
> > +
> > + vm = &dev_priv->gtt.base;
> > +
> > + return vm;
> > +}
> > +
> > static bool i915_request_guilty(struct drm_i915_gem_request *request,
> > const u32 acthd, bool *inside)
> > {
> > @@ -2150,9 +2168,9 @@ static bool i915_request_guilty(struct drm_i915_gem_request *request,
> > * pointing inside the ring, matches the batch_obj address range.
> > * However this is extremely unlikely.
> > */
> > -
> > if (request->batch_obj) {
> > - if (i915_head_inside_object(acthd, request->batch_obj)) {
> > + if (i915_head_inside_object(acthd, request->batch_obj,
> > + request_to_vm(request))) {
> > *inside = true;
> > return true;
> > }
> > @@ -2172,17 +2190,21 @@ static void i915_set_reset_status(struct intel_ring_buffer *ring,
> > {
> > struct i915_ctx_hang_stats *hs = NULL;
> > bool inside, guilty;
> > + unsigned long offset = 0;
> >
> > /* Innocent until proven guilty */
> > guilty = false;
> >
> > + if (request->batch_obj)
> > + offset = i915_gem_obj_offset(request->batch_obj,
> > + request_to_vm(request));
> > +
> > if (ring->hangcheck.action != wait &&
> > i915_request_guilty(request, acthd, &inside)) {
> > DRM_ERROR("%s hung %s bo (0x%lx ctx %d) at 0x%x\n",
> > ring->name,
> > inside ? "inside" : "flushing",
> > - request->batch_obj ?
> > - i915_gem_obj_ggtt_offset(request->batch_obj) : 0,
> > + offset,
> > request->ctx ? request->ctx->id : 0,
> > acthd);
> >
> > @@ -2239,13 +2261,15 @@ static void i915_gem_reset_ring_lists(struct drm_i915_private *dev_priv,
> > }
> >
> > while (!list_empty(&ring->active_list)) {
> > + struct i915_address_space *vm;
> > struct drm_i915_gem_object *obj;
> >
> > obj = list_first_entry(&ring->active_list,
> > struct drm_i915_gem_object,
> > ring_list);
> >
> > - i915_gem_object_move_to_inactive(obj);
> > + list_for_each_entry(vm, &dev_priv->vm_list, global_link)
> > + i915_gem_object_move_to_inactive(obj, vm);
> > }
> > }
> >
> > @@ -2263,7 +2287,7 @@ void i915_gem_restore_fences(struct drm_device *dev)
> > void i915_gem_reset(struct drm_device *dev)
> > {
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > - struct i915_address_space *vm = &dev_priv->gtt.base;
> > + struct i915_address_space *vm;
> > struct drm_i915_gem_object *obj;
> > struct intel_ring_buffer *ring;
> > int i;
> > @@ -2274,8 +2298,9 @@ void i915_gem_reset(struct drm_device *dev)
> > /* Move everything out of the GPU domains to ensure we do any
> > * necessary invalidation upon reuse.
> > */
> > - list_for_each_entry(obj, &vm->inactive_list, mm_list)
> > - obj->base.read_domains &= ~I915_GEM_GPU_DOMAINS;
> > + list_for_each_entry(vm, &dev_priv->vm_list, global_link)
> > + list_for_each_entry(obj, &vm->inactive_list, mm_list)
> > + obj->base.read_domains &= ~I915_GEM_GPU_DOMAINS;
> >
> > i915_gem_restore_fences(dev);
> > }
> > @@ -2320,6 +2345,8 @@ i915_gem_retire_requests_ring(struct intel_ring_buffer *ring)
> > * by the ringbuffer to the flushing/inactive lists as appropriate.
> > */
> > while (!list_empty(&ring->active_list)) {
> > + struct drm_i915_private *dev_priv = ring->dev->dev_private;
> > + struct i915_address_space *vm;
> > struct drm_i915_gem_object *obj;
> >
> > obj = list_first_entry(&ring->active_list,
> > @@ -2329,7 +2356,8 @@ i915_gem_retire_requests_ring(struct intel_ring_buffer *ring)
> > if (!i915_seqno_passed(seqno, obj->last_read_seqno))
> > break;
> >
> > - i915_gem_object_move_to_inactive(obj);
> > + list_for_each_entry(vm, &dev_priv->vm_list, global_link)
> > + i915_gem_object_move_to_inactive(obj, vm);
> > }
> >
> > if (unlikely(ring->trace_irq_seqno &&
> > @@ -2575,13 +2603,14 @@ static void i915_gem_object_finish_gtt(struct drm_i915_gem_object *obj)
> > * Unbinds an object from the GTT aperture.
> > */
> > int
> > -i915_gem_object_unbind(struct drm_i915_gem_object *obj)
> > +i915_gem_object_unbind(struct drm_i915_gem_object *obj,
> > + struct i915_address_space *vm)
> > {
> > drm_i915_private_t *dev_priv = obj->base.dev->dev_private;
> > struct i915_vma *vma;
> > int ret;
> >
> > - if (!i915_gem_obj_ggtt_bound(obj))
> > + if (!i915_gem_obj_bound(obj, vm))
> > return 0;
> >
> > if (obj->pin_count)
> > @@ -2604,7 +2633,7 @@ i915_gem_object_unbind(struct drm_i915_gem_object *obj)
> > if (ret)
> > return ret;
> >
> > - trace_i915_gem_object_unbind(obj);
> > + trace_i915_gem_object_unbind(obj, vm);
> >
> > if (obj->has_global_gtt_mapping)
> > i915_gem_gtt_unbind_object(obj);
> > @@ -2619,7 +2648,7 @@ i915_gem_object_unbind(struct drm_i915_gem_object *obj)
> > /* Avoid an unnecessary call to unbind on rebind. */
> > obj->map_and_fenceable = true;
> >
> > - vma = __i915_gem_obj_to_vma(obj);
> > + vma = i915_gem_obj_to_vma(obj, vm);
> > list_del(&vma->vma_link);
> > drm_mm_remove_node(&vma->node);
> > i915_gem_vma_destroy(vma);
> > @@ -2748,6 +2777,7 @@ static void i830_write_fence_reg(struct drm_device *dev, int reg,
> > "object 0x%08lx not 512K or pot-size 0x%08x aligned\n",
> > i915_gem_obj_ggtt_offset(obj), size);
> >
> > +
> > pitch_val = obj->stride / 128;
> > pitch_val = ffs(pitch_val) - 1;
> >
> > @@ -3069,23 +3099,25 @@ static void i915_gem_verify_gtt(struct drm_device *dev)
> > */
> > static int
> > i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
> > + struct i915_address_space *vm,
> > unsigned alignment,
> > bool map_and_fenceable,
> > bool nonblocking)
> > {
> > struct drm_device *dev = obj->base.dev;
> > drm_i915_private_t *dev_priv = dev->dev_private;
> > - struct i915_address_space *vm = &dev_priv->gtt.base;
> > u32 size, fence_size, fence_alignment, unfenced_alignment;
> > bool mappable, fenceable;
> > - size_t gtt_max = map_and_fenceable ?
> > - dev_priv->gtt.mappable_end : dev_priv->gtt.base.total;
> > + size_t gtt_max =
> > + map_and_fenceable ? dev_priv->gtt.mappable_end : vm->total;
> > struct i915_vma *vma;
> > int ret;
> >
> > if (WARN_ON(!list_empty(&obj->vma_list)))
> > return -EBUSY;
> >
> > + BUG_ON(!i915_is_ggtt(vm));
> > +
> > fence_size = i915_gem_get_gtt_size(dev,
> > obj->base.size,
> > obj->tiling_mode);
> > @@ -3125,18 +3157,21 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
> > i915_gem_object_pin_pages(obj);
> >
> > vma = i915_gem_vma_create(obj, &dev_priv->gtt.base);
> > + /* For now we only ever use 1 vma per object */
> > + WARN_ON(!list_empty(&obj->vma_list));
> > +
> > + vma = i915_gem_vma_create(obj, vm);
> > if (vma == NULL) {
> > i915_gem_object_unpin_pages(obj);
> > return -ENOMEM;
> > }
> >
> > search_free:
> > - ret = drm_mm_insert_node_in_range_generic(&dev_priv->gtt.base.mm,
> > - &vma->node,
> > + ret = drm_mm_insert_node_in_range_generic(&vm->mm, &vma->node,
> > size, alignment,
> > obj->cache_level, 0, gtt_max);
> > if (ret) {
> > - ret = i915_gem_evict_something(dev, size, alignment,
> > + ret = i915_gem_evict_something(dev, vm, size, alignment,
> > obj->cache_level,
> > map_and_fenceable,
> > nonblocking);
> > @@ -3162,18 +3197,25 @@ search_free:
> >
> > list_move_tail(&obj->global_list, &dev_priv->mm.bound_list);
> > list_add_tail(&obj->mm_list, &vm->inactive_list);
> > - list_add(&vma->vma_link, &obj->vma_list);
> > +
> > + /* Keep GGTT vmas first to make debug easier */
> > + if (i915_is_ggtt(vm))
> > + list_add(&vma->vma_link, &obj->vma_list);
> > + else
> > + list_add_tail(&vma->vma_link, &obj->vma_list);
> >
> > fenceable =
> > + i915_is_ggtt(vm) &&
> > i915_gem_obj_ggtt_size(obj) == fence_size &&
> > (i915_gem_obj_ggtt_offset(obj) & (fence_alignment - 1)) == 0;
> >
> > - mappable = i915_gem_obj_ggtt_offset(obj) + obj->base.size <=
> > - dev_priv->gtt.mappable_end;
> > + mappable =
> > + i915_is_ggtt(vm) &&
> > + vma->node.start + obj->base.size <= dev_priv->gtt.mappable_end;
> >
> > obj->map_and_fenceable = mappable && fenceable;
> >
> > - trace_i915_gem_object_bind(obj, map_and_fenceable);
> > + trace_i915_gem_object_bind(obj, vm, map_and_fenceable);
> > i915_gem_verify_gtt(dev);
> > return 0;
> > }
> > @@ -3271,7 +3313,7 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write)
> > int ret;
> >
> > /* Not valid to be called on unbound objects. */
> > - if (!i915_gem_obj_ggtt_bound(obj))
> > + if (!i915_gem_obj_bound_any(obj))
> > return -EINVAL;
>
> If we're converting the shmem paths over to wait_rendering then there's
> only the fault handler and the set_domain ioctl left. For the later it
> would make sense to clflush even when an object is on the unbound list, to
> allow userspace to optimize when the clflushing happens. But that would
> only make sense in conjunction with Chris' create2 ioctl and a flag to
> preallocate the storage (and so putting the object onto the unbound list).
> So nothing to do here.
>
> >
> > if (obj->base.write_domain == I915_GEM_DOMAIN_GTT)
> > @@ -3317,11 +3359,12 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write)
> > }
> >
> > int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> > + struct i915_address_space *vm,
> > enum i915_cache_level cache_level)
> > {
> > struct drm_device *dev = obj->base.dev;
> > drm_i915_private_t *dev_priv = dev->dev_private;
> > - struct i915_vma *vma = __i915_gem_obj_to_vma(obj);
> > + struct i915_vma *vma = i915_gem_obj_to_vma(obj, vm);
> > int ret;
> >
> > if (obj->cache_level == cache_level)
> > @@ -3333,12 +3376,15 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> > }
> >
> > if (!i915_gem_valid_gtt_space(dev, &vma->node, cache_level)) {
> > - ret = i915_gem_object_unbind(obj);
> > + ret = i915_gem_object_unbind(obj, vm);
> > if (ret)
> > return ret;
> > }
> >
> > - if (i915_gem_obj_ggtt_bound(obj)) {
> > + list_for_each_entry(vm, &dev_priv->vm_list, global_link) {
> > + if (!i915_gem_obj_bound(obj, vm))
> > + continue;
>
> Hm, shouldn't we have a per-object list of vmas? Or will that follow later
> on?
>
> Self-correction: It exists already ... why can't we use this here?
Yes. That should work, I'll fix it and test it. It looks slightly worse
IMO in terms of code clarity, but I don't mind the change.
>
> > +
> > ret = i915_gem_object_finish_gpu(obj);
> > if (ret)
> > return ret;
> > @@ -3361,7 +3407,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> > i915_ppgtt_bind_object(dev_priv->mm.aliasing_ppgtt,
> > obj, cache_level);
> >
> > - i915_gem_obj_ggtt_set_color(obj, cache_level);
> > + i915_gem_obj_set_color(obj, vm, cache_level);
> > }
> >
> > if (cache_level == I915_CACHE_NONE) {
> > @@ -3421,6 +3467,7 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data,
> > struct drm_file *file)
> > {
> > struct drm_i915_gem_caching *args = data;
> > + struct drm_i915_private *dev_priv;
> > struct drm_i915_gem_object *obj;
> > enum i915_cache_level level;
> > int ret;
> > @@ -3445,8 +3492,10 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data,
> > ret = -ENOENT;
> > goto unlock;
> > }
> > + dev_priv = obj->base.dev->dev_private;
> >
> > - ret = i915_gem_object_set_cache_level(obj, level);
> > + /* FIXME: Add interface for specific VM? */
> > + ret = i915_gem_object_set_cache_level(obj, &dev_priv->gtt.base, level);
> >
> > drm_gem_object_unreference(&obj->base);
> > unlock:
> > @@ -3464,6 +3513,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> > u32 alignment,
> > struct intel_ring_buffer *pipelined)
> > {
> > + struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
> > u32 old_read_domains, old_write_domain;
> > int ret;
> >
> > @@ -3482,7 +3532,8 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> > * of uncaching, which would allow us to flush all the LLC-cached data
> > * with that bit in the PTE to main memory with just one PIPE_CONTROL.
> > */
> > - ret = i915_gem_object_set_cache_level(obj, I915_CACHE_NONE);
> > + ret = i915_gem_object_set_cache_level(obj, &dev_priv->gtt.base,
> > + I915_CACHE_NONE);
> > if (ret)
> > return ret;
> >
> > @@ -3490,7 +3541,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_object_pin(obj, alignment, true, false);
> > + ret = i915_gem_ggtt_pin(obj, alignment, true, false);
> > if (ret)
> > return ret;
> >
> > @@ -3633,6 +3684,7 @@ i915_gem_ring_throttle(struct drm_device *dev, struct drm_file *file)
> >
> > 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)
> > @@ -3642,26 +3694,29 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
> > if (WARN_ON(obj->pin_count == DRM_I915_GEM_OBJECT_MAX_PIN_COUNT))
> > return -EBUSY;
> >
> > - if (i915_gem_obj_ggtt_bound(obj)) {
> > - if ((alignment && i915_gem_obj_ggtt_offset(obj) & (alignment - 1)) ||
> > + BUG_ON(map_and_fenceable && !i915_is_ggtt(vm));
>
> WARN_ON, since presumably we can keep on going if we get this wrong
> (albeit with slightly corrupted state, so render corruptions might
> follow).
Can we make a deal, can we leave this as BUG_ON with a FIXME to convert
it at the end of merging?
>
> > +
> > + if (i915_gem_obj_bound(obj, vm)) {
> > + if ((alignment &&
> > + i915_gem_obj_offset(obj, vm) & (alignment - 1)) ||
> > (map_and_fenceable && !obj->map_and_fenceable)) {
> > WARN(obj->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_ggtt_offset(obj), alignment,
> > + i915_gem_obj_offset(obj, vm), alignment,
> > map_and_fenceable,
> > obj->map_and_fenceable);
> > - ret = i915_gem_object_unbind(obj);
> > + ret = i915_gem_object_unbind(obj, vm);
> > if (ret)
> > return ret;
> > }
> > }
> >
> > - if (!i915_gem_obj_ggtt_bound(obj)) {
> > + if (!i915_gem_obj_bound(obj, vm)) {
> > struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
> >
> > - ret = i915_gem_object_bind_to_gtt(obj, alignment,
> > + ret = i915_gem_object_bind_to_gtt(obj, vm, alignment,
> > map_and_fenceable,
> > nonblocking);
> > if (ret)
> > @@ -3684,7 +3739,7 @@ void
> > i915_gem_object_unpin(struct drm_i915_gem_object *obj)
> > {
> > BUG_ON(obj->pin_count == 0);
> > - BUG_ON(!i915_gem_obj_ggtt_bound(obj));
> > + BUG_ON(!i915_gem_obj_bound_any(obj));
> >
> > if (--obj->pin_count == 0)
> > obj->pin_mappable = false;
> > @@ -3722,7 +3777,7 @@ i915_gem_pin_ioctl(struct drm_device *dev, void *data,
> > }
> >
> > if (obj->user_pin_count == 0) {
> > - ret = i915_gem_object_pin(obj, args->alignment, true, false);
> > + ret = i915_gem_ggtt_pin(obj, args->alignment, true, false);
> > if (ret)
> > goto out;
> > }
> > @@ -3957,6 +4012,7 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
> > struct drm_i915_gem_object *obj = to_intel_bo(gem_obj);
> > struct drm_device *dev = obj->base.dev;
> > drm_i915_private_t *dev_priv = dev->dev_private;
> > + struct i915_vma *vma, *next;
> >
> > trace_i915_gem_object_destroy(obj);
> >
> > @@ -3964,15 +4020,21 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
> > i915_gem_detach_phys_object(dev, obj);
> >
> > obj->pin_count = 0;
> > - if (WARN_ON(i915_gem_object_unbind(obj) == -ERESTARTSYS)) {
> > - bool was_interruptible;
> > + /* NB: 0 or 1 elements */
> > + WARN_ON(!list_empty(&obj->vma_list) &&
> > + !list_is_singular(&obj->vma_list));
> > + list_for_each_entry_safe(vma, next, &obj->vma_list, vma_link) {
> > + int ret = i915_gem_object_unbind(obj, vma->vm);
> > + if (WARN_ON(ret == -ERESTARTSYS)) {
> > + bool was_interruptible;
> >
> > - was_interruptible = dev_priv->mm.interruptible;
> > - dev_priv->mm.interruptible = false;
> > + was_interruptible = dev_priv->mm.interruptible;
> > + dev_priv->mm.interruptible = false;
> >
> > - WARN_ON(i915_gem_object_unbind(obj));
> > + WARN_ON(i915_gem_object_unbind(obj, vma->vm));
> >
> > - dev_priv->mm.interruptible = was_interruptible;
> > + dev_priv->mm.interruptible = was_interruptible;
> > + }
> > }
> >
> > /* Stolen objects don't hold a ref, but do hold pin count. Fix that up
> > @@ -4332,6 +4394,16 @@ init_ring_lists(struct intel_ring_buffer *ring)
> > INIT_LIST_HEAD(&ring->request_list);
> > }
> >
> > +static void i915_init_vm(struct drm_i915_private *dev_priv,
> > + struct i915_address_space *vm)
> > +{
> > + vm->dev = dev_priv->dev;
> > + INIT_LIST_HEAD(&vm->active_list);
> > + INIT_LIST_HEAD(&vm->inactive_list);
> > + INIT_LIST_HEAD(&vm->global_link);
> > + list_add(&vm->global_link, &dev_priv->vm_list);
> > +}
> > +
> > void
> > i915_gem_load(struct drm_device *dev)
> > {
> > @@ -4344,8 +4416,9 @@ i915_gem_load(struct drm_device *dev)
> > SLAB_HWCACHE_ALIGN,
> > NULL);
> >
> > - INIT_LIST_HEAD(&dev_priv->gtt.base.active_list);
> > - INIT_LIST_HEAD(&dev_priv->gtt.base.inactive_list);
> > + INIT_LIST_HEAD(&dev_priv->vm_list);
> > + i915_init_vm(dev_priv, &dev_priv->gtt.base);
> > +
> > INIT_LIST_HEAD(&dev_priv->mm.unbound_list);
> > INIT_LIST_HEAD(&dev_priv->mm.bound_list);
> > INIT_LIST_HEAD(&dev_priv->mm.fence_list);
> > @@ -4616,9 +4689,9 @@ i915_gem_inactive_shrink(struct shrinker *shrinker, struct shrink_control *sc)
> > struct drm_i915_private,
> > mm.inactive_shrinker);
> > struct drm_device *dev = dev_priv->dev;
> > - struct i915_address_space *vm = &dev_priv->gtt.base;
> > + struct i915_address_space *vm;
> > struct drm_i915_gem_object *obj;
> > - int nr_to_scan = sc->nr_to_scan;
> > + int nr_to_scan;
> > bool unlock = true;
> > int cnt;
> >
> > @@ -4632,6 +4705,7 @@ i915_gem_inactive_shrink(struct shrinker *shrinker, struct shrink_control *sc)
> > unlock = false;
> > }
> >
> > + nr_to_scan = sc->nr_to_scan;
> > if (nr_to_scan) {
> > nr_to_scan -= i915_gem_purge(dev_priv, nr_to_scan);
> > if (nr_to_scan > 0)
> > @@ -4645,11 +4719,93 @@ i915_gem_inactive_shrink(struct shrinker *shrinker, struct shrink_control *sc)
> > list_for_each_entry(obj, &dev_priv->mm.unbound_list, global_list)
> > if (obj->pages_pin_count == 0)
> > cnt += obj->base.size >> PAGE_SHIFT;
> > - list_for_each_entry(obj, &vm->inactive_list, mm_list)
> > - if (obj->pin_count == 0 && obj->pages_pin_count == 0)
> > - cnt += obj->base.size >> PAGE_SHIFT;
> > +
> > + list_for_each_entry(vm, &dev_priv->vm_list, global_link)
> > + list_for_each_entry(obj, &vm->inactive_list, global_list)
> > + if (obj->pin_count == 0 && obj->pages_pin_count == 0)
> > + cnt += obj->base.size >> PAGE_SHIFT;
>
> Isn't this now double-counting objects? In the shrinker we only care about
> how much physical RAM an object occupies, not how much virtual space it
> occupies. So just walking the bound list of objects here should be good
> enough ...
>
Maybe I've misunderstood you. My code is wrong, but I think you're idea
requires a prep patch because it changes functionality, right?
So let me know if I've understood you.
> >
> > if (unlock)
> > mutex_unlock(&dev->struct_mutex);
> > return cnt;
> > }
> > +
> > +/* All the new VM stuff */
> > +unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o,
> > + struct i915_address_space *vm)
> > +{
> > + struct drm_i915_private *dev_priv = o->base.dev->dev_private;
> > + struct i915_vma *vma;
> > +
> > + if (vm == &dev_priv->mm.aliasing_ppgtt->base)
> > + vm = &dev_priv->gtt.base;
> > +
> > + BUG_ON(list_empty(&o->vma_list));
> > + list_for_each_entry(vma, &o->vma_list, vma_link) {
>
> Imo the vma list walking here and in the other helpers below indicates
> that we should deal more often in vmas instead of (object, vm) pairs. Or
> is this again something that'll get fixed later on?
>
> I just want to avoid diff churn, and it also makes reviewing easier if the
> foreshadowing is correct ;-) So generally I'd vote for more liberal
> sprinkling of obj_to_vma in callers.
It's not something I fixed in the whole series. I think it makes sense
conceptually, to keep some things as <obj,vm> and others as direct vma.
If you want me to change something, you need to be more specific since
no action specifically comes to mind at this point in the series.
>
> > + if (vma->vm == vm)
> > + return vma->node.start;
> > +
> > + }
> > + return -1;
> > +}
> > +
> > +bool i915_gem_obj_bound_any(struct drm_i915_gem_object *o)
> > +{
> > + return !list_empty(&o->vma_list);
> > +}
> > +
> > +bool i915_gem_obj_bound(struct drm_i915_gem_object *o,
> > + struct i915_address_space *vm)
> > +{
> > + struct i915_vma *vma;
> > +
> > + list_for_each_entry(vma, &o->vma_list, vma_link) {
> > + if (vma->vm == vm)
> > + return true;
> > + }
> > + return false;
> > +}
> > +
> > +unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
> > + struct i915_address_space *vm)
> > +{
> > + struct drm_i915_private *dev_priv = o->base.dev->dev_private;
> > + struct i915_vma *vma;
> > +
> > + if (vm == &dev_priv->mm.aliasing_ppgtt->base)
> > + vm = &dev_priv->gtt.base;
> > + BUG_ON(list_empty(&o->vma_list));
> > + list_for_each_entry(vma, &o->vma_list, vma_link) {
> > + if (vma->vm == vm)
> > + return vma->node.size;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +void i915_gem_obj_set_color(struct drm_i915_gem_object *o,
> > + struct i915_address_space *vm,
> > + enum i915_cache_level color)
> > +{
> > + struct i915_vma *vma;
> > + BUG_ON(list_empty(&o->vma_list));
> > + list_for_each_entry(vma, &o->vma_list, vma_link) {
> > + if (vma->vm == vm) {
> > + vma->node.color = color;
> > + return;
> > + }
> > + }
> > +
> > + WARN(1, "Couldn't set color for VM %p\n", vm);
> > +}
> > +
> > +struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
> > + struct i915_address_space *vm)
> > +{
> > + struct i915_vma *vma;
> > + list_for_each_entry(vma, &obj->vma_list, vma_link)
> > + if (vma->vm == vm)
> > + return vma;
> > +
> > + return NULL;
> > +}
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > index 2074544..c92fd81 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -155,6 +155,7 @@ create_hw_context(struct drm_device *dev,
> >
> > if (INTEL_INFO(dev)->gen >= 7) {
> > ret = i915_gem_object_set_cache_level(ctx->obj,
> > + &dev_priv->gtt.base,
> > I915_CACHE_LLC_MLC);
> > /* Failure shouldn't ever happen this early */
> > if (WARN_ON(ret))
> > @@ -214,7 +215,7 @@ static int create_default_context(struct drm_i915_private *dev_priv)
> > * default context.
> > */
> > dev_priv->ring[RCS].default_context = ctx;
> > - ret = i915_gem_object_pin(ctx->obj, CONTEXT_ALIGN, false, false);
> > + ret = i915_gem_ggtt_pin(ctx->obj, CONTEXT_ALIGN, false, false);
> > if (ret) {
> > DRM_DEBUG_DRIVER("Couldn't pin %d\n", ret);
> > goto err_destroy;
> > @@ -398,6 +399,7 @@ mi_set_context(struct intel_ring_buffer *ring,
> > static int do_switch(struct i915_hw_context *to)
> > {
> > struct intel_ring_buffer *ring = to->ring;
> > + struct drm_i915_private *dev_priv = ring->dev->dev_private;
> > struct i915_hw_context *from = ring->last_context;
> > u32 hw_flags = 0;
> > int ret;
> > @@ -407,7 +409,7 @@ static int do_switch(struct i915_hw_context *to)
> > if (from == to)
> > return 0;
> >
> > - ret = i915_gem_object_pin(to->obj, CONTEXT_ALIGN, false, false);
> > + ret = i915_gem_ggtt_pin(to->obj, CONTEXT_ALIGN, false, false);
> > if (ret)
> > return ret;
> >
> > @@ -444,7 +446,8 @@ static int do_switch(struct i915_hw_context *to)
> > */
> > if (from != NULL) {
> > from->obj->base.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
> > - i915_gem_object_move_to_active(from->obj, ring);
> > + i915_gem_object_move_to_active(from->obj, &dev_priv->gtt.base,
> > + ring);
> > /* As long as MI_SET_CONTEXT is serializing, ie. it flushes the
> > * whole damn pipeline, we don't need to explicitly mark the
> > * object dirty. The only exception is that the context must be
> > diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
> > index df61f33..32efdc0 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_evict.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_evict.c
> > @@ -32,24 +32,21 @@
> > #include "i915_trace.h"
> >
> > static bool
> > -mark_free(struct drm_i915_gem_object *obj, struct list_head *unwind)
> > +mark_free(struct i915_vma *vma, struct list_head *unwind)
> > {
> > - struct i915_vma *vma = __i915_gem_obj_to_vma(obj);
> > -
> > - if (obj->pin_count)
> > + if (vma->obj->pin_count)
> > return false;
> >
> > - list_add(&obj->exec_list, unwind);
> > + list_add(&vma->obj->exec_list, unwind);
> > return drm_mm_scan_add_block(&vma->node);
> > }
> >
> > int
> > -i915_gem_evict_something(struct drm_device *dev, int min_size,
> > - unsigned alignment, unsigned cache_level,
> > +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)
> > {
> > drm_i915_private_t *dev_priv = dev->dev_private;
> > - struct i915_address_space *vm = &dev_priv->gtt.base;
> > struct list_head eviction_list, unwind_list;
> > struct i915_vma *vma;
> > struct drm_i915_gem_object *obj;
> > @@ -81,16 +78,18 @@ i915_gem_evict_something(struct drm_device *dev, int min_size,
> > */
> >
> > INIT_LIST_HEAD(&unwind_list);
> > - if (mappable)
> > + if (mappable) {
> > + BUG_ON(!i915_is_ggtt(vm));
> > drm_mm_init_scan_with_range(&vm->mm, min_size,
> > alignment, cache_level, 0,
> > dev_priv->gtt.mappable_end);
> > - else
> > + } else
> > drm_mm_init_scan(&vm->mm, min_size, alignment, cache_level);
> >
> > /* First see if there is a large enough contiguous idle region... */
> > list_for_each_entry(obj, &vm->inactive_list, mm_list) {
> > - if (mark_free(obj, &unwind_list))
> > + struct i915_vma *vma = i915_gem_obj_to_vma(obj, vm);
> > + if (mark_free(vma, &unwind_list))
> > goto found;
> > }
> >
> > @@ -99,7 +98,8 @@ i915_gem_evict_something(struct drm_device *dev, int min_size,
> >
> > /* Now merge in the soon-to-be-expired objects... */
> > list_for_each_entry(obj, &vm->active_list, mm_list) {
> > - if (mark_free(obj, &unwind_list))
> > + struct i915_vma *vma = i915_gem_obj_to_vma(obj, vm);
> > + if (mark_free(vma, &unwind_list))
> > goto found;
> > }
> >
> > @@ -109,7 +109,7 @@ none:
> > obj = list_first_entry(&unwind_list,
> > struct drm_i915_gem_object,
> > exec_list);
> > - vma = __i915_gem_obj_to_vma(obj);
> > + vma = i915_gem_obj_to_vma(obj, vm);
> > ret = drm_mm_scan_remove_block(&vma->node);
> > BUG_ON(ret);
> >
> > @@ -130,7 +130,7 @@ found:
> > obj = list_first_entry(&unwind_list,
> > struct drm_i915_gem_object,
> > exec_list);
> > - vma = __i915_gem_obj_to_vma(obj);
> > + vma = i915_gem_obj_to_vma(obj, vm);
> > if (drm_mm_scan_remove_block(&vma->node)) {
> > list_move(&obj->exec_list, &eviction_list);
> > drm_gem_object_reference(&obj->base);
> > @@ -145,7 +145,7 @@ found:
> > struct drm_i915_gem_object,
> > exec_list);
> > if (ret == 0)
> > - ret = i915_gem_object_unbind(obj);
> > + ret = i915_gem_object_unbind(obj, vm);
> >
> > list_del_init(&obj->exec_list);
> > drm_gem_object_unreference(&obj->base);
> > @@ -158,13 +158,18 @@ int
> > i915_gem_evict_everything(struct drm_device *dev)
>
> I suspect evict_everything eventually wants a address_space *vm argument
> for those cases where we only want to evict everything in a given vm. Atm
> we have two use-cases of this:
> - Called from the shrinker as a last-ditch effort. For that it should move
> _every_ object onto the unbound list.
> - Called from execbuf for badly-fragmented address spaces to clean up the
> mess. For that case we only care about one address space.
The current thing is more or less a result of Chris' suggestions. A
non-posted iteration did plumb the vm, and after reworking to the
suggestion made by Chris, the vm didn't make much sense anymore.
For point #1, it requires VM prioritization I think. I don't really see
any other way to fairly manage it.
For point #2, that I agree it might be useful, but we can easily create
a new function, and not call it "shrinker" to do it.
>
> > {
> > drm_i915_private_t *dev_priv = dev->dev_private;
> > - struct i915_address_space *vm = &dev_priv->gtt.base;
> > + struct i915_address_space *vm;
> > struct drm_i915_gem_object *obj, *next;
> > - bool lists_empty;
> > + bool lists_empty = true;
> > int ret;
> >
> > - lists_empty = (list_empty(&vm->inactive_list) &&
> > - list_empty(&vm->active_list));
> > + list_for_each_entry(vm, &dev_priv->vm_list, global_link) {
> > + lists_empty = (list_empty(&vm->inactive_list) &&
> > + list_empty(&vm->active_list));
> > + if (!lists_empty)
> > + lists_empty = false;
> > + }
> > +
> > if (lists_empty)
> > return -ENOSPC;
> >
> > @@ -181,9 +186,11 @@ i915_gem_evict_everything(struct drm_device *dev)
> > i915_gem_retire_requests(dev);
> >
> > /* Having flushed everything, unbind() should never raise an error */
> > - list_for_each_entry_safe(obj, next, &vm->inactive_list, mm_list)
> > - if (obj->pin_count == 0)
> > - WARN_ON(i915_gem_object_unbind(obj));
> > + list_for_each_entry(vm, &dev_priv->vm_list, global_link) {
> > + list_for_each_entry_safe(obj, next, &vm->inactive_list, mm_list)
> > + if (obj->pin_count == 0)
> > + WARN_ON(i915_gem_object_unbind(obj, vm));
> > + }
> >
> > return 0;
> > }
> > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > index 5aeb447..e90182d 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -150,7 +150,7 @@ eb_get_object(struct eb_objects *eb, unsigned long handle)
> > }
> >
> > static void
> > -eb_destroy(struct eb_objects *eb)
> > +eb_destroy(struct eb_objects *eb, struct i915_address_space *vm)
> > {
> > while (!list_empty(&eb->objects)) {
> > struct drm_i915_gem_object *obj;
> > @@ -174,7 +174,8 @@ static inline int use_cpu_reloc(struct drm_i915_gem_object *obj)
> > static int
> > i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj,
> > struct eb_objects *eb,
> > - struct drm_i915_gem_relocation_entry *reloc)
> > + struct drm_i915_gem_relocation_entry *reloc,
> > + struct i915_address_space *vm)
> > {
> > struct drm_device *dev = obj->base.dev;
> > struct drm_gem_object *target_obj;
> > @@ -297,7 +298,8 @@ i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj,
> >
> > static int
> > i915_gem_execbuffer_relocate_object(struct drm_i915_gem_object *obj,
> > - struct eb_objects *eb)
> > + struct eb_objects *eb,
> > + struct i915_address_space *vm)
> > {
> > #define N_RELOC(x) ((x) / sizeof(struct drm_i915_gem_relocation_entry))
> > struct drm_i915_gem_relocation_entry stack_reloc[N_RELOC(512)];
> > @@ -321,7 +323,8 @@ i915_gem_execbuffer_relocate_object(struct drm_i915_gem_object *obj,
> > do {
> > u64 offset = r->presumed_offset;
> >
> > - ret = i915_gem_execbuffer_relocate_entry(obj, eb, r);
> > + ret = i915_gem_execbuffer_relocate_entry(obj, eb, r,
> > + vm);
> > if (ret)
> > return ret;
> >
> > @@ -344,13 +347,15 @@ i915_gem_execbuffer_relocate_object(struct drm_i915_gem_object *obj,
> > static int
> > i915_gem_execbuffer_relocate_object_slow(struct drm_i915_gem_object *obj,
> > struct eb_objects *eb,
> > - struct drm_i915_gem_relocation_entry *relocs)
> > + struct drm_i915_gem_relocation_entry *relocs,
> > + struct i915_address_space *vm)
> > {
> > const struct drm_i915_gem_exec_object2 *entry = obj->exec_entry;
> > int i, ret;
> >
> > for (i = 0; i < entry->relocation_count; i++) {
> > - ret = i915_gem_execbuffer_relocate_entry(obj, eb, &relocs[i]);
> > + ret = i915_gem_execbuffer_relocate_entry(obj, eb, &relocs[i],
> > + vm);
> > if (ret)
> > return ret;
> > }
> > @@ -359,7 +364,8 @@ i915_gem_execbuffer_relocate_object_slow(struct drm_i915_gem_object *obj,
> > }
> >
> > static int
> > -i915_gem_execbuffer_relocate(struct eb_objects *eb)
> > +i915_gem_execbuffer_relocate(struct eb_objects *eb,
> > + struct i915_address_space *vm)
> > {
> > struct drm_i915_gem_object *obj;
> > int ret = 0;
> > @@ -373,7 +379,7 @@ i915_gem_execbuffer_relocate(struct eb_objects *eb)
> > */
> > pagefault_disable();
> > list_for_each_entry(obj, &eb->objects, exec_list) {
> > - ret = i915_gem_execbuffer_relocate_object(obj, eb);
> > + ret = i915_gem_execbuffer_relocate_object(obj, eb, vm);
> > if (ret)
> > break;
> > }
> > @@ -395,6 +401,7 @@ need_reloc_mappable(struct drm_i915_gem_object *obj)
> > static int
> > i915_gem_execbuffer_reserve_object(struct drm_i915_gem_object *obj,
> > struct intel_ring_buffer *ring,
> > + struct i915_address_space *vm,
> > bool *need_reloc)
> > {
> > struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
> > @@ -409,7 +416,8 @@ i915_gem_execbuffer_reserve_object(struct drm_i915_gem_object *obj,
> > obj->tiling_mode != I915_TILING_NONE;
> > need_mappable = need_fence || need_reloc_mappable(obj);
> >
> > - ret = i915_gem_object_pin(obj, entry->alignment, need_mappable, false);
> > + ret = i915_gem_object_pin(obj, vm, entry->alignment, need_mappable,
> > + false);
> > if (ret)
> > return ret;
> >
> > @@ -436,8 +444,8 @@ i915_gem_execbuffer_reserve_object(struct drm_i915_gem_object *obj,
> > obj->has_aliasing_ppgtt_mapping = 1;
> > }
> >
> > - if (entry->offset != i915_gem_obj_ggtt_offset(obj)) {
> > - entry->offset = i915_gem_obj_ggtt_offset(obj);
> > + if (entry->offset != i915_gem_obj_offset(obj, vm)) {
> > + entry->offset = i915_gem_obj_offset(obj, vm);
> > *need_reloc = true;
> > }
> >
> > @@ -458,7 +466,7 @@ i915_gem_execbuffer_unreserve_object(struct drm_i915_gem_object *obj)
> > {
> > struct drm_i915_gem_exec_object2 *entry;
> >
> > - if (!i915_gem_obj_ggtt_bound(obj))
> > + if (!i915_gem_obj_bound_any(obj))
> > return;
> >
> > entry = obj->exec_entry;
> > @@ -475,6 +483,7 @@ i915_gem_execbuffer_unreserve_object(struct drm_i915_gem_object *obj)
> > static int
> > i915_gem_execbuffer_reserve(struct intel_ring_buffer *ring,
> > struct list_head *objects,
> > + struct i915_address_space *vm,
> > bool *need_relocs)
> > {
> > struct drm_i915_gem_object *obj;
> > @@ -529,32 +538,37 @@ i915_gem_execbuffer_reserve(struct intel_ring_buffer *ring,
> > list_for_each_entry(obj, objects, exec_list) {
> > struct drm_i915_gem_exec_object2 *entry = obj->exec_entry;
> > bool need_fence, need_mappable;
> > + u32 obj_offset;
> >
> > - if (!i915_gem_obj_ggtt_bound(obj))
> > + if (!i915_gem_obj_bound(obj, vm))
> > continue;
>
> I wonder a bit how we could avoid the multipler (obj, vm) -> vma lookups
> here ... Maybe we should cache them in some pointer somewhere (either in
> the eb object or by adding a new pointer to the object struct, e.g.
> obj->eb_vma, similar to obj->eb_list).
>
I agree, and even did this at one unposted patch too. However, I think
it's a premature optimization which risks code correctness. So I think
somewhere a FIXME needs to happen to address that issue. (Or if Chris
complains bitterly about some perf hit).
> >
> > + obj_offset = i915_gem_obj_offset(obj, vm);
> > 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(obj);
> >
> > + BUG_ON((need_mappable || need_fence) &&
> > + !i915_is_ggtt(vm));
> > +
> > if ((entry->alignment &&
> > - i915_gem_obj_ggtt_offset(obj) & (entry->alignment - 1)) ||
> > + obj_offset & (entry->alignment - 1)) ||
> > (need_mappable && !obj->map_and_fenceable))
> > - ret = i915_gem_object_unbind(obj);
> > + ret = i915_gem_object_unbind(obj, vm);
> > else
> > - ret = i915_gem_execbuffer_reserve_object(obj, ring, need_relocs);
> > + ret = i915_gem_execbuffer_reserve_object(obj, ring, vm, need_relocs);
> > if (ret)
> > goto err;
> > }
> >
> > /* Bind fresh objects */
> > list_for_each_entry(obj, objects, exec_list) {
> > - if (i915_gem_obj_ggtt_bound(obj))
> > + if (i915_gem_obj_bound(obj, vm))
> > continue;
> >
> > - ret = i915_gem_execbuffer_reserve_object(obj, ring, need_relocs);
> > + ret = i915_gem_execbuffer_reserve_object(obj, ring, vm, need_relocs);
> > if (ret)
> > goto err;
> > }
> > @@ -578,7 +592,8 @@ i915_gem_execbuffer_relocate_slow(struct drm_device *dev,
> > struct drm_file *file,
> > struct intel_ring_buffer *ring,
> > struct eb_objects *eb,
> > - struct drm_i915_gem_exec_object2 *exec)
> > + struct drm_i915_gem_exec_object2 *exec,
> > + struct i915_address_space *vm)
> > {
> > struct drm_i915_gem_relocation_entry *reloc;
> > struct drm_i915_gem_object *obj;
> > @@ -662,14 +677,15 @@ i915_gem_execbuffer_relocate_slow(struct drm_device *dev,
> > goto err;
> >
> > need_relocs = (args->flags & I915_EXEC_NO_RELOC) == 0;
> > - ret = i915_gem_execbuffer_reserve(ring, &eb->objects, &need_relocs);
> > + ret = i915_gem_execbuffer_reserve(ring, &eb->objects, vm, &need_relocs);
> > if (ret)
> > goto err;
> >
> > list_for_each_entry(obj, &eb->objects, exec_list) {
> > int offset = obj->exec_entry - exec;
> > ret = i915_gem_execbuffer_relocate_object_slow(obj, eb,
> > - reloc + reloc_offset[offset]);
> > + reloc + reloc_offset[offset],
> > + vm);
> > if (ret)
> > goto err;
> > }
> > @@ -768,6 +784,7 @@ validate_exec_list(struct drm_i915_gem_exec_object2 *exec,
> >
> > static void
> > i915_gem_execbuffer_move_to_active(struct list_head *objects,
> > + struct i915_address_space *vm,
> > struct intel_ring_buffer *ring)
> > {
> > struct drm_i915_gem_object *obj;
> > @@ -782,7 +799,7 @@ i915_gem_execbuffer_move_to_active(struct list_head *objects,
> > obj->base.read_domains = obj->base.pending_read_domains;
> > obj->fenced_gpu_access = obj->pending_fenced_gpu_access;
> >
> > - i915_gem_object_move_to_active(obj, ring);
> > + i915_gem_object_move_to_active(obj, vm, ring);
> > if (obj->base.write_domain) {
> > obj->dirty = 1;
> > obj->last_write_seqno = intel_ring_get_seqno(ring);
> > @@ -836,7 +853,8 @@ static int
> > i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> > struct drm_file *file,
> > struct drm_i915_gem_execbuffer2 *args,
> > - struct drm_i915_gem_exec_object2 *exec)
> > + struct drm_i915_gem_exec_object2 *exec,
> > + struct i915_address_space *vm)
> > {
> > drm_i915_private_t *dev_priv = dev->dev_private;
> > struct eb_objects *eb;
> > @@ -998,17 +1016,17 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> >
> > /* Move the objects en-masse into the GTT, evicting if necessary. */
> > need_relocs = (args->flags & I915_EXEC_NO_RELOC) == 0;
> > - ret = i915_gem_execbuffer_reserve(ring, &eb->objects, &need_relocs);
> > + ret = i915_gem_execbuffer_reserve(ring, &eb->objects, vm, &need_relocs);
> > if (ret)
> > goto err;
> >
> > /* The objects are in their final locations, apply the relocations. */
> > if (need_relocs)
> > - ret = i915_gem_execbuffer_relocate(eb);
> > + ret = i915_gem_execbuffer_relocate(eb, vm);
> > if (ret) {
> > if (ret == -EFAULT) {
> > ret = i915_gem_execbuffer_relocate_slow(dev, args, file, ring,
> > - eb, exec);
> > + eb, exec, vm);
> > BUG_ON(!mutex_is_locked(&dev->struct_mutex));
> > }
> > if (ret)
> > @@ -1059,7 +1077,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> > goto err;
> > }
> >
> > - exec_start = i915_gem_obj_ggtt_offset(batch_obj) + args->batch_start_offset;
> > + exec_start = i915_gem_obj_offset(batch_obj, vm) +
> > + args->batch_start_offset;
> > exec_len = args->batch_len;
> > if (cliprects) {
> > for (i = 0; i < args->num_cliprects; i++) {
> > @@ -1084,11 +1103,11 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> >
> > trace_i915_gem_ring_dispatch(ring, intel_ring_get_seqno(ring), flags);
> >
> > - i915_gem_execbuffer_move_to_active(&eb->objects, ring);
> > + i915_gem_execbuffer_move_to_active(&eb->objects, vm, ring);
> > i915_gem_execbuffer_retire_commands(dev, file, ring, batch_obj);
> >
> > err:
> > - eb_destroy(eb);
> > + eb_destroy(eb, vm);
> >
> > mutex_unlock(&dev->struct_mutex);
> >
> > @@ -1105,6 +1124,7 @@ int
> > i915_gem_execbuffer(struct drm_device *dev, void *data,
> > struct drm_file *file)
> > {
> > + struct drm_i915_private *dev_priv = dev->dev_private;
> > struct drm_i915_gem_execbuffer *args = data;
> > struct drm_i915_gem_execbuffer2 exec2;
> > struct drm_i915_gem_exec_object *exec_list = NULL;
> > @@ -1160,7 +1180,8 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
> > exec2.flags = I915_EXEC_RENDER;
> > i915_execbuffer2_set_context_id(exec2, 0);
> >
> > - ret = i915_gem_do_execbuffer(dev, data, file, &exec2, exec2_list);
> > + ret = i915_gem_do_execbuffer(dev, data, file, &exec2, exec2_list,
> > + &dev_priv->gtt.base);
> > if (!ret) {
> > /* Copy the new buffer offsets back to the user's exec list. */
> > for (i = 0; i < args->buffer_count; i++)
> > @@ -1186,6 +1207,7 @@ int
> > i915_gem_execbuffer2(struct drm_device *dev, void *data,
> > struct drm_file *file)
> > {
> > + struct drm_i915_private *dev_priv = dev->dev_private;
> > struct drm_i915_gem_execbuffer2 *args = data;
> > struct drm_i915_gem_exec_object2 *exec2_list = NULL;
> > int ret;
> > @@ -1216,7 +1238,8 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data,
> > return -EFAULT;
> > }
> >
> > - ret = i915_gem_do_execbuffer(dev, data, file, args, exec2_list);
> > + ret = i915_gem_do_execbuffer(dev, data, file, args, exec2_list,
> > + &dev_priv->gtt.base);
> > if (!ret) {
> > /* Copy the new buffer offsets back to the user's exec list. */
> > ret = copy_to_user(to_user_ptr(args->buffers_ptr),
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index 298fc42..70ce2f6 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -367,6 +367,8 @@ static int i915_gem_init_aliasing_ppgtt(struct drm_device *dev)
> > ppgtt->base.total);
> > }
> >
> > + /* i915_init_vm(dev_priv, &ppgtt->base) */
> > +
> > return ret;
> > }
> >
> > @@ -386,17 +388,22 @@ void i915_ppgtt_bind_object(struct i915_hw_ppgtt *ppgtt,
> > struct drm_i915_gem_object *obj,
> > enum i915_cache_level cache_level)
> > {
> > - ppgtt->base.insert_entries(&ppgtt->base, obj->pages,
> > - i915_gem_obj_ggtt_offset(obj) >> PAGE_SHIFT,
> > - cache_level);
> > + struct i915_address_space *vm = &ppgtt->base;
> > + unsigned long obj_offset = i915_gem_obj_offset(obj, vm);
> > +
> > + vm->insert_entries(vm, obj->pages,
> > + obj_offset >> PAGE_SHIFT,
> > + cache_level);
> > }
> >
> > void i915_ppgtt_unbind_object(struct i915_hw_ppgtt *ppgtt,
> > struct drm_i915_gem_object *obj)
> > {
> > - ppgtt->base.clear_range(&ppgtt->base,
> > - i915_gem_obj_ggtt_offset(obj) >> PAGE_SHIFT,
> > - obj->base.size >> PAGE_SHIFT);
> > + struct i915_address_space *vm = &ppgtt->base;
> > + unsigned long obj_offset = i915_gem_obj_offset(obj, vm);
> > +
> > + vm->clear_range(vm, obj_offset >> PAGE_SHIFT,
> > + obj->base.size >> PAGE_SHIFT);
> > }
> >
> > extern int intel_iommu_gfx_mapped;
> > @@ -447,6 +454,9 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev)
> > dev_priv->gtt.base.start / PAGE_SIZE,
> > dev_priv->gtt.base.total / PAGE_SIZE);
> >
> > + if (dev_priv->mm.aliasing_ppgtt)
> > + gen6_write_pdes(dev_priv->mm.aliasing_ppgtt);
> > +
> > list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
> > i915_gem_clflush_object(obj);
> > i915_gem_gtt_bind_object(obj, obj->cache_level);
> > @@ -625,7 +635,8 @@ void i915_gem_setup_global_gtt(struct drm_device *dev,
> > * aperture. One page should be enough to keep any prefetching inside
> > * of the aperture.
> > */
> > - drm_i915_private_t *dev_priv = dev->dev_private;
> > + struct drm_i915_private *dev_priv = dev->dev_private;
> > + struct i915_address_space *ggtt_vm = &dev_priv->gtt.base;
> > struct drm_mm_node *entry;
> > struct drm_i915_gem_object *obj;
> > unsigned long hole_start, hole_end;
> > @@ -633,19 +644,19 @@ void i915_gem_setup_global_gtt(struct drm_device *dev,
> > BUG_ON(mappable_end > end);
> >
> > /* Subtract the guard page ... */
> > - drm_mm_init(&dev_priv->gtt.base.mm, start, end - start - PAGE_SIZE);
> > + drm_mm_init(&ggtt_vm->mm, start, end - start - PAGE_SIZE);
> > if (!HAS_LLC(dev))
> > dev_priv->gtt.base.mm.color_adjust = i915_gtt_color_adjust;
> >
> > /* Mark any preallocated objects as occupied */
> > list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
> > - struct i915_vma *vma = __i915_gem_obj_to_vma(obj);
> > + struct i915_vma *vma = i915_gem_obj_to_vma(obj, ggtt_vm);
> > int ret;
> > DRM_DEBUG_KMS("reserving preallocated space: %lx + %zx\n",
> > i915_gem_obj_ggtt_offset(obj), obj->base.size);
> >
> > WARN_ON(i915_gem_obj_ggtt_bound(obj));
> > - ret = drm_mm_reserve_node(&dev_priv->gtt.base.mm, &vma->node);
> > + ret = drm_mm_reserve_node(&ggtt_vm->mm, &vma->node);
> > if (ret)
> > DRM_DEBUG_KMS("Reservation failed\n");
> > obj->has_global_gtt_mapping = 1;
> > @@ -656,19 +667,15 @@ void i915_gem_setup_global_gtt(struct drm_device *dev,
> > dev_priv->gtt.base.total = end - start;
> >
> > /* Clear any non-preallocated blocks */
> > - drm_mm_for_each_hole(entry, &dev_priv->gtt.base.mm,
> > - hole_start, hole_end) {
> > + drm_mm_for_each_hole(entry, &ggtt_vm->mm, hole_start, hole_end) {
> > const unsigned long count = (hole_end - hole_start) / PAGE_SIZE;
> > DRM_DEBUG_KMS("clearing unused GTT space: [%lx, %lx]\n",
> > hole_start, hole_end);
> > - dev_priv->gtt.base.clear_range(&dev_priv->gtt.base,
> > - hole_start / PAGE_SIZE,
> > - count);
> > + ggtt_vm->clear_range(ggtt_vm, hole_start / PAGE_SIZE, count);
> > }
> >
> > /* And finally clear the reserved guard page */
> > - dev_priv->gtt.base.clear_range(&dev_priv->gtt.base,
> > - end / PAGE_SIZE - 1, 1);
> > + ggtt_vm->clear_range(ggtt_vm, end / PAGE_SIZE - 1, 1);
> > }
> >
> > static bool
> > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> > index 245eb1d..bfe61fa 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> > @@ -391,7 +391,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
> > if (gtt_offset == I915_GTT_OFFSET_NONE)
> > return obj;
> >
> > - vma = i915_gem_vma_create(obj, &dev_priv->gtt.base);
> > + vma = i915_gem_vma_create(obj, vm);
> > if (!vma) {
> > drm_gem_object_unreference(&obj->base);
> > return NULL;
> > @@ -404,8 +404,8 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
> > */
> > vma->node.start = gtt_offset;
> > vma->node.size = size;
> > - if (drm_mm_initialized(&dev_priv->gtt.base.mm)) {
> > - ret = drm_mm_reserve_node(&dev_priv->gtt.base.mm, &vma->node);
> > + if (drm_mm_initialized(&vm->mm)) {
> > + ret = drm_mm_reserve_node(&vm->mm, &vma->node);
>
> These two hunks here for stolen look fishy - we only ever use the stolen
> preallocated stuff for objects with mappings in the global gtt. So keeping
> that explicit is imo the better approach. And tbh I'm confused where the
> local variable vm is from ...
If we don't create a vma for it, we potentially have to special case a
bunch of places, I think. I'm not actually sure of this, but the
overhead to do it is quite small.
Anyway, I'll look this over again nd see what I think.
>
> > if (ret) {
> > DRM_DEBUG_KMS("failed to allocate stolen GTT space\n");
> > goto unref_out;
> > diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
> > index 92a8d27..808ca2a 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_tiling.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
> > @@ -360,17 +360,19 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
> >
> > obj->map_and_fenceable =
> > !i915_gem_obj_ggtt_bound(obj) ||
> > - (i915_gem_obj_ggtt_offset(obj) + obj->base.size <= dev_priv->gtt.mappable_end &&
> > + (i915_gem_obj_ggtt_offset(obj) +
> > + obj->base.size <= dev_priv->gtt.mappable_end &&
> > i915_gem_object_fence_ok(obj, args->tiling_mode));
> >
> > /* Rebind if we need a change of alignment */
> > if (!obj->map_and_fenceable) {
> > - u32 unfenced_alignment =
> > + struct i915_address_space *ggtt = &dev_priv->gtt.base;
> > + u32 unfenced_align =
> > i915_gem_get_gtt_alignment(dev, obj->base.size,
> > args->tiling_mode,
> > false);
> > - if (i915_gem_obj_ggtt_offset(obj) & (unfenced_alignment - 1))
> > - ret = i915_gem_object_unbind(obj);
> > + if (i915_gem_obj_ggtt_offset(obj) & (unfenced_align - 1))
> > + ret = i915_gem_object_unbind(obj, ggtt);
> > }
> >
> > if (ret == 0) {
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 79fbb17..28fa0ff 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1716,6 +1716,9 @@ i915_error_first_batchbuffer(struct drm_i915_private *dev_priv,
> > if (HAS_BROKEN_CS_TLB(dev_priv->dev)) {
> > u32 acthd = I915_READ(ACTHD);
> >
> > + if (WARN_ON(HAS_HW_CONTEXTS(dev_priv->dev)))
> > + return NULL;
> > +
> > if (WARN_ON(ring->id != RCS))
> > return NULL;
> >
> > @@ -1802,7 +1805,8 @@ static void i915_gem_record_active_context(struct intel_ring_buffer *ring,
> > return;
> >
> > list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
> > - if ((error->ccid & PAGE_MASK) == i915_gem_obj_ggtt_offset(obj)) {
> > + if ((error->ccid & PAGE_MASK) ==
> > + i915_gem_obj_ggtt_offset(obj)) {
> > ering->ctx = i915_error_object_create_sized(dev_priv,
> > obj, 1);
> > break;
> > diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
> > index 7d283b5..3f019d3 100644
> > --- a/drivers/gpu/drm/i915/i915_trace.h
> > +++ b/drivers/gpu/drm/i915/i915_trace.h
> > @@ -34,11 +34,13 @@ TRACE_EVENT(i915_gem_object_create,
> > );
> >
> > TRACE_EVENT(i915_gem_object_bind,
> > - TP_PROTO(struct drm_i915_gem_object *obj, bool mappable),
> > - TP_ARGS(obj, mappable),
> > + TP_PROTO(struct drm_i915_gem_object *obj,
> > + struct i915_address_space *vm, bool mappable),
> > + TP_ARGS(obj, vm, mappable),
> >
> > 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)
> > @@ -46,8 +48,8 @@ TRACE_EVENT(i915_gem_object_bind,
> >
> > TP_fast_assign(
> > __entry->obj = obj;
> > - __entry->offset = i915_gem_obj_ggtt_offset(obj);
> > - __entry->size = i915_gem_obj_ggtt_size(obj);
> > + __entry->offset = i915_gem_obj_offset(obj, vm);
> > + __entry->size = i915_gem_obj_size(obj, vm);
> > __entry->mappable = mappable;
> > ),
> >
> > @@ -57,19 +59,21 @@ TRACE_EVENT(i915_gem_object_bind,
> > );
> >
> > TRACE_EVENT(i915_gem_object_unbind,
> > - TP_PROTO(struct drm_i915_gem_object *obj),
> > - TP_ARGS(obj),
> > + TP_PROTO(struct drm_i915_gem_object *obj,
> > + struct i915_address_space *vm),
> > + TP_ARGS(obj, vm),
> >
> > TP_STRUCT__entry(
> > __field(struct drm_i915_gem_object *, obj)
> > + __field(struct i915_address_space *, vm)
> > __field(u32, offset)
> > __field(u32, size)
> > ),
> >
> > TP_fast_assign(
> > __entry->obj = obj;
> > - __entry->offset = i915_gem_obj_ggtt_offset(obj);
> > - __entry->size = i915_gem_obj_ggtt_size(obj);
> > + __entry->offset = i915_gem_obj_offset(obj, vm);
> > + __entry->size = i915_gem_obj_size(obj, vm);
> > ),
> >
> > TP_printk("obj=%p, offset=%08x size=%x",
> > diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
> > index f3c97e0..b69cc63 100644
> > --- a/drivers/gpu/drm/i915/intel_fb.c
> > +++ b/drivers/gpu/drm/i915/intel_fb.c
> > @@ -170,7 +170,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
> > fb->width, fb->height,
> > i915_gem_obj_ggtt_offset(obj), obj);
> >
> > -
> > mutex_unlock(&dev->struct_mutex);
> > vga_switcheroo_client_fb_set(dev->pdev, info);
> > return 0;
> > diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> > index 81c3ca1..517e278 100644
> > --- a/drivers/gpu/drm/i915/intel_overlay.c
> > +++ b/drivers/gpu/drm/i915/intel_overlay.c
> > @@ -1350,7 +1350,7 @@ void intel_setup_overlay(struct drm_device *dev)
> > }
> > overlay->flip_addr = reg_bo->phys_obj->handle->busaddr;
> > } else {
> > - ret = i915_gem_object_pin(reg_bo, PAGE_SIZE, true, false);
> > + ret = i915_gem_ggtt_pin(reg_bo, PAGE_SIZE, true, false);
> > 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 125a741..449e57c 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -2858,7 +2858,7 @@ intel_alloc_context_page(struct drm_device *dev)
> > return NULL;
> > }
> >
> > - ret = i915_gem_object_pin(ctx, 4096, true, false);
> > + ret = i915_gem_ggtt_pin(ctx, 4096, true, false);
> > 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 bc4c11b..ebed61d 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -481,6 +481,7 @@ out:
> > static int
> > init_pipe_control(struct intel_ring_buffer *ring)
> > {
> > + struct drm_i915_private *dev_priv = ring->dev->dev_private;
> > struct pipe_control *pc;
> > struct drm_i915_gem_object *obj;
> > int ret;
> > @@ -499,9 +500,10 @@ init_pipe_control(struct intel_ring_buffer *ring)
> > goto err;
> > }
> >
> > - i915_gem_object_set_cache_level(obj, I915_CACHE_LLC);
> > + i915_gem_object_set_cache_level(obj, &dev_priv->gtt.base,
> > + I915_CACHE_LLC);
> >
> > - ret = i915_gem_object_pin(obj, 4096, true, false);
> > + ret = i915_gem_ggtt_pin(obj, 4096, true, false);
> > if (ret)
> > goto err_unref;
> >
> > @@ -1212,6 +1214,7 @@ static void cleanup_status_page(struct intel_ring_buffer *ring)
> > static int init_status_page(struct intel_ring_buffer *ring)
> > {
> > struct drm_device *dev = ring->dev;
> > + struct drm_i915_private *dev_priv = dev->dev_private;
> > struct drm_i915_gem_object *obj;
> > int ret;
> >
> > @@ -1222,9 +1225,10 @@ static int init_status_page(struct intel_ring_buffer *ring)
> > goto err;
> > }
> >
> > - i915_gem_object_set_cache_level(obj, I915_CACHE_LLC);
> > + i915_gem_object_set_cache_level(obj, &dev_priv->gtt.base,
> > + I915_CACHE_LLC);
> >
> > - ret = i915_gem_object_pin(obj, 4096, true, false);
> > + ret = i915_gem_ggtt_pin(obj, 4096, true, false);
> > if (ret != 0) {
> > goto err_unref;
> > }
> > @@ -1307,7 +1311,7 @@ static int intel_init_ring_buffer(struct drm_device *dev,
> >
> > ring->obj = obj;
> >
> > - ret = i915_gem_object_pin(obj, PAGE_SIZE, true, false);
> > + ret = i915_gem_ggtt_pin(obj, PAGE_SIZE, true, false);
> > if (ret)
> > goto err_unref;
> >
> > @@ -1828,7 +1832,7 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
> > return -ENOMEM;
> > }
> >
> > - ret = i915_gem_object_pin(obj, 0, true, false);
> > + ret = i915_gem_ggtt_pin(obj, 0, true, false);
> > if (ret != 0) {
> > drm_gem_object_unreference(&obj->base);
> > DRM_ERROR("Failed to ping batch bo\n");
> > --
> > 1.8.3.2
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
Ben Widawsky, Intel Open Source Technology Center
