Commit c8725f3d authored by Chris Wilson's avatar Chris Wilson Committed by Daniel Vetter

drm/i915: Do not call retire_requests from wait_for_rendering

A common issue we have is that retiring requests causes recursion
through GTT manipulation or page table manipulation which we can only
handle at very specific points. However, to maintain internal
consistency (enforced through our sanity checks on write_domain at
various points in the GEM object lifecycle) we do need to retire the
object prior to marking it with a new write_domain, and also clear the
write_domain for the implicit flush following a batch.

Note that this then allows the unbound objects to still be on the active
lists, and so care must be taken when removing objects from unbound lists
(similar to the caveats we face processing the bound lists).

v2: Fix i915_gem_shrink_all() to handle updated object lifetime rules,
by refactoring it to call into __i915_gem_shrink().

v3: Missed an object-retire prior to changing cache domains in
i915_gem_object_set_cache_leve()

v4: Rebase
Signed-off-by: default avatarChris Wilson <chris@chris-wilson.co.uk>
Tested-by: default avatarVille Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: default avatarBrad Volkin <bradley.d.volkin@intel.com>
Signed-off-by: default avatarDaniel Vetter <daniel.vetter@ffwll.ch>
parent a88cc108
...@@ -43,6 +43,9 @@ static void i915_gem_object_flush_cpu_write_domain(struct drm_i915_gem_object *o ...@@ -43,6 +43,9 @@ static void i915_gem_object_flush_cpu_write_domain(struct drm_i915_gem_object *o
static __must_check int static __must_check int
i915_gem_object_wait_rendering(struct drm_i915_gem_object *obj, i915_gem_object_wait_rendering(struct drm_i915_gem_object *obj,
bool readonly); bool readonly);
static void
i915_gem_object_retire(struct drm_i915_gem_object *obj);
static int i915_gem_phys_pwrite(struct drm_device *dev, static int i915_gem_phys_pwrite(struct drm_device *dev,
struct drm_i915_gem_object *obj, struct drm_i915_gem_object *obj,
struct drm_i915_gem_pwrite *args, struct drm_i915_gem_pwrite *args,
...@@ -352,6 +355,8 @@ int i915_gem_obj_prepare_shmem_read(struct drm_i915_gem_object *obj, ...@@ -352,6 +355,8 @@ int i915_gem_obj_prepare_shmem_read(struct drm_i915_gem_object *obj,
ret = i915_gem_object_wait_rendering(obj, true); ret = i915_gem_object_wait_rendering(obj, true);
if (ret) if (ret)
return ret; return ret;
i915_gem_object_retire(obj);
} }
ret = i915_gem_object_get_pages(obj); ret = i915_gem_object_get_pages(obj);
...@@ -767,6 +772,8 @@ i915_gem_shmem_pwrite(struct drm_device *dev, ...@@ -767,6 +772,8 @@ i915_gem_shmem_pwrite(struct drm_device *dev,
ret = i915_gem_object_wait_rendering(obj, false); ret = i915_gem_object_wait_rendering(obj, false);
if (ret) if (ret)
return ret; return ret;
i915_gem_object_retire(obj);
} }
/* Same trick applies to invalidate partially written cachelines read /* Same trick applies to invalidate partially written cachelines read
* before writing. */ * before writing. */
...@@ -1154,7 +1161,8 @@ static int ...@@ -1154,7 +1161,8 @@ static int
i915_gem_object_wait_rendering__tail(struct drm_i915_gem_object *obj, i915_gem_object_wait_rendering__tail(struct drm_i915_gem_object *obj,
struct intel_ring_buffer *ring) struct intel_ring_buffer *ring)
{ {
i915_gem_retire_requests_ring(ring); if (!obj->active)
return 0;
/* Manually manage the write flush as we may have not yet /* Manually manage the write flush as we may have not yet
* retired the buffer. * retired the buffer.
...@@ -1164,7 +1172,6 @@ i915_gem_object_wait_rendering__tail(struct drm_i915_gem_object *obj, ...@@ -1164,7 +1172,6 @@ i915_gem_object_wait_rendering__tail(struct drm_i915_gem_object *obj,
* we know we have passed the last write. * we know we have passed the last write.
*/ */
obj->last_write_seqno = 0; obj->last_write_seqno = 0;
obj->base.write_domain &= ~I915_GEM_GPU_DOMAINS;
return 0; return 0;
} }
...@@ -1785,58 +1792,58 @@ static unsigned long ...@@ -1785,58 +1792,58 @@ static unsigned long
__i915_gem_shrink(struct drm_i915_private *dev_priv, long target, __i915_gem_shrink(struct drm_i915_private *dev_priv, long target,
bool purgeable_only) bool purgeable_only)
{ {
struct list_head still_bound_list; struct list_head still_in_list;
struct drm_i915_gem_object *obj, *next; struct drm_i915_gem_object *obj;
unsigned long count = 0; unsigned long count = 0;
list_for_each_entry_safe(obj, next,
&dev_priv->mm.unbound_list,
global_list) {
if ((i915_gem_object_is_purgeable(obj) || !purgeable_only) &&
i915_gem_object_put_pages(obj) == 0) {
count += obj->base.size >> PAGE_SHIFT;
if (count >= target)
return count;
}
}
/* /*
* As we may completely rewrite the bound list whilst unbinding * As we may completely rewrite the (un)bound list whilst unbinding
* (due to retiring requests) we have to strictly process only * (due to retiring requests) we have to strictly process only
* one element of the list at the time, and recheck the list * one element of the list at the time, and recheck the list
* on every iteration. * on every iteration.
*
* In particular, we must hold a reference whilst removing the
* object as we may end up waiting for and/or retiring the objects.
* This might release the final reference (held by the active list)
* and result in the object being freed from under us. This is
* similar to the precautions the eviction code must take whilst
* removing objects.
*
* Also note that although these lists do not hold a reference to
* the object we can safely grab one here: The final object
* unreferencing and the bound_list are both protected by the
* dev->struct_mutex and so we won't ever be able to observe an
* object on the bound_list with a reference count equals 0.
*/ */
INIT_LIST_HEAD(&still_bound_list); INIT_LIST_HEAD(&still_in_list);
while (count < target && !list_empty(&dev_priv->mm.unbound_list)) {
obj = list_first_entry(&dev_priv->mm.unbound_list,
typeof(*obj), global_list);
list_move_tail(&obj->global_list, &still_in_list);
if (!i915_gem_object_is_purgeable(obj) && purgeable_only)
continue;
drm_gem_object_reference(&obj->base);
if (i915_gem_object_put_pages(obj) == 0)
count += obj->base.size >> PAGE_SHIFT;
drm_gem_object_unreference(&obj->base);
}
list_splice(&still_in_list, &dev_priv->mm.unbound_list);
INIT_LIST_HEAD(&still_in_list);
while (count < target && !list_empty(&dev_priv->mm.bound_list)) { while (count < target && !list_empty(&dev_priv->mm.bound_list)) {
struct i915_vma *vma, *v; struct i915_vma *vma, *v;
obj = list_first_entry(&dev_priv->mm.bound_list, obj = list_first_entry(&dev_priv->mm.bound_list,
typeof(*obj), global_list); typeof(*obj), global_list);
list_move_tail(&obj->global_list, &still_bound_list); list_move_tail(&obj->global_list, &still_in_list);
if (!i915_gem_object_is_purgeable(obj) && purgeable_only) if (!i915_gem_object_is_purgeable(obj) && purgeable_only)
continue; continue;
/*
* Hold a reference whilst we unbind this object, as we may
* end up waiting for and retiring requests. This might
* release the final reference (held by the active list)
* and result in the object being freed from under us.
* in this object being freed.
*
* Note 1: Shrinking the bound list is special since only active
* (and hence bound objects) can contain such limbo objects, so
* we don't need special tricks for shrinking the unbound list.
* The only other place where we have to be careful with active
* objects suddenly disappearing due to retiring requests is the
* eviction code.
*
* Note 2: Even though the bound list doesn't hold a reference
* to the object we can safely grab one here: The final object
* unreferencing and the bound_list are both protected by the
* dev->struct_mutex and so we won't ever be able to observe an
* object on the bound_list with a reference count equals 0.
*/
drm_gem_object_reference(&obj->base); drm_gem_object_reference(&obj->base);
list_for_each_entry_safe(vma, v, &obj->vma_list, vma_link) list_for_each_entry_safe(vma, v, &obj->vma_list, vma_link)
...@@ -1848,7 +1855,7 @@ __i915_gem_shrink(struct drm_i915_private *dev_priv, long target, ...@@ -1848,7 +1855,7 @@ __i915_gem_shrink(struct drm_i915_private *dev_priv, long target,
drm_gem_object_unreference(&obj->base); drm_gem_object_unreference(&obj->base);
} }
list_splice(&still_bound_list, &dev_priv->mm.bound_list); list_splice(&still_in_list, &dev_priv->mm.bound_list);
return count; return count;
} }
...@@ -1862,17 +1869,8 @@ i915_gem_purge(struct drm_i915_private *dev_priv, long target) ...@@ -1862,17 +1869,8 @@ i915_gem_purge(struct drm_i915_private *dev_priv, long target)
static unsigned long static unsigned long
i915_gem_shrink_all(struct drm_i915_private *dev_priv) i915_gem_shrink_all(struct drm_i915_private *dev_priv)
{ {
struct drm_i915_gem_object *obj, *next;
long freed = 0;
i915_gem_evict_everything(dev_priv->dev); i915_gem_evict_everything(dev_priv->dev);
return __i915_gem_shrink(dev_priv, LONG_MAX, false);
list_for_each_entry_safe(obj, next, &dev_priv->mm.unbound_list,
global_list) {
if (i915_gem_object_put_pages(obj) == 0)
freed += obj->base.size >> PAGE_SHIFT;
}
return freed;
} }
static int static int
...@@ -2089,6 +2087,19 @@ i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj) ...@@ -2089,6 +2087,19 @@ i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
WARN_ON(i915_verify_lists(dev)); WARN_ON(i915_verify_lists(dev));
} }
static void
i915_gem_object_retire(struct drm_i915_gem_object *obj)
{
struct intel_ring_buffer *ring = obj->ring;
if (ring == NULL)
return;
if (i915_seqno_passed(ring->get_seqno(ring, true),
obj->last_read_seqno))
i915_gem_object_move_to_inactive(obj);
}
static int static int
i915_gem_init_seqno(struct drm_device *dev, u32 seqno) i915_gem_init_seqno(struct drm_device *dev, u32 seqno)
{ {
...@@ -3429,6 +3440,7 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write) ...@@ -3429,6 +3440,7 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write)
if (ret) if (ret)
return ret; return ret;
i915_gem_object_retire(obj);
i915_gem_object_flush_cpu_write_domain(obj, false); i915_gem_object_flush_cpu_write_domain(obj, false);
/* Serialise direct access to this object with the barriers for /* Serialise direct access to this object with the barriers for
...@@ -3527,6 +3539,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj, ...@@ -3527,6 +3539,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
* in obj->write_domain and have been skipping the clflushes. * in obj->write_domain and have been skipping the clflushes.
* Just set it to the CPU cache for now. * Just set it to the CPU cache for now.
*/ */
i915_gem_object_retire(obj);
WARN_ON(obj->base.write_domain & ~I915_GEM_DOMAIN_CPU); WARN_ON(obj->base.write_domain & ~I915_GEM_DOMAIN_CPU);
old_read_domains = obj->base.read_domains; old_read_domains = obj->base.read_domains;
...@@ -3749,6 +3762,7 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write) ...@@ -3749,6 +3762,7 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write)
if (ret) if (ret)
return ret; return ret;
i915_gem_object_retire(obj);
i915_gem_object_flush_gtt_write_domain(obj); i915_gem_object_flush_gtt_write_domain(obj);
old_write_domain = obj->base.write_domain; old_write_domain = obj->base.write_domain;
......
...@@ -955,6 +955,9 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas, ...@@ -955,6 +955,9 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas,
if (i915_gem_obj_ggtt_bound(obj) && if (i915_gem_obj_ggtt_bound(obj) &&
i915_gem_obj_to_ggtt(obj)->pin_count) i915_gem_obj_to_ggtt(obj)->pin_count)
intel_mark_fb_busy(obj, ring); intel_mark_fb_busy(obj, ring);
/* update for the implicit flush after a batch */
obj->base.write_domain &= ~I915_GEM_GPU_DOMAINS;
} }
trace_i915_gem_object_change_domain(obj, old_read, old_write); trace_i915_gem_object_change_domain(obj, old_read, old_write);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment