[Intel-gfx] [PATCH] drm/i915/gem: Unbind all current vma on changing cache-level
Matthew Auld
matthew.william.auld at gmail.com
Mon Dec 2 17:17:48 UTC 2019
On Thu, 28 Nov 2019 at 22:42, Chris Wilson <chris at chris-wilson.co.uk> wrote:
>
> Avoid dangerous race handling of destroyed vma by unbinding all vma
> instead. Unfortunately, this stops us from trying to be clever and only
> doing the minimal change required, so on first use of scanout we may
> encounter an annoying stall as it transitions to a new cache level.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=112413
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/gem/i915_gem_domain.c | 124 ++-------------------
> 1 file changed, 7 insertions(+), 117 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> index 9aebcf263191..bd846b4fec20 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> @@ -192,126 +192,16 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> if (obj->cache_level == cache_level)
> return 0;
>
> - /* Inspect the list of currently bound VMA and unbind any that would
> - * be invalid given the new cache-level. This is principally to
> - * catch the issue of the CS prefetch crossing page boundaries and
> - * reading an invalid PTE on older architectures.
> - */
> -restart:
> - list_for_each_entry(vma, &obj->vma.list, obj_link) {
> - if (!drm_mm_node_allocated(&vma->node))
> - continue;
> -
> - if (i915_vma_is_pinned(vma)) {
> - DRM_DEBUG("can not change the cache level of pinned objects\n");
> - return -EBUSY;
> - }
> -
> - if (!i915_vma_is_closed(vma) &&
> - i915_gem_valid_gtt_space(vma, cache_level))
> - continue;
> -
> - ret = i915_vma_unbind(vma);
> - if (ret)
> - return ret;
> -
> - /* As unbinding may affect other elements in the
> - * obj->vma_list (due to side-effects from retiring
> - * an active vma), play safe and restart the iterator.
> - */
> - goto restart;
> - }
> -
> - /* We can reuse the existing drm_mm nodes but need to change the
> - * cache-level on the PTE. We could simply unbind them all and
> - * rebind with the correct cache-level on next use. However since
> - * we already have a valid slot, dma mapping, pages etc, we may as
> - * rewrite the PTE in the belief that doing so tramples upon less
> - * state and so involves less work.
> - */
> - if (atomic_read(&obj->bind_count)) {
> - struct drm_i915_private *i915 = to_i915(obj->base.dev);
> -
> - /* Before we change the PTE, the GPU must not be accessing it.
> - * If we wait upon the object, we know that all the bound
> - * VMA are no longer active.
> - */
> - ret = i915_gem_object_wait(obj,
> - I915_WAIT_INTERRUPTIBLE |
> - I915_WAIT_ALL,
> - MAX_SCHEDULE_TIMEOUT);
> - if (ret)
> - return ret;
> -
> - if (!HAS_LLC(i915) && cache_level != I915_CACHE_NONE) {
> - intel_wakeref_t wakeref =
> - intel_runtime_pm_get(&i915->runtime_pm);
> -
> - /*
> - * Access to snoopable pages through the GTT is
> - * incoherent and on some machines causes a hard
> - * lockup. Relinquish the CPU mmaping to force
> - * userspace to refault in the pages and we can
> - * then double check if the GTT mapping is still
> - * valid for that pointer access.
> - */
> - ret = mutex_lock_interruptible(&i915->ggtt.vm.mutex);
> - if (ret) {
> - intel_runtime_pm_put(&i915->runtime_pm,
> - wakeref);
> - return ret;
> - }
> -
> - if (obj->userfault_count)
> - __i915_gem_object_release_mmap(obj);
> -
> - /*
> - * As we no longer need a fence for GTT access,
> - * we can relinquish it now (and so prevent having
> - * to steal a fence from someone else on the next
> - * fence request). Note GPU activity would have
> - * dropped the fence as all snoopable access is
> - * supposed to be linear.
> - */
> - for_each_ggtt_vma(vma, obj) {
> - ret = i915_vma_revoke_fence(vma);
> - if (ret)
> - break;
> - }
> - mutex_unlock(&i915->ggtt.vm.mutex);
> - intel_runtime_pm_put(&i915->runtime_pm, wakeref);
> - if (ret)
> - return ret;
> - } else {
> - /*
> - * We either have incoherent backing store and
> - * so no GTT access or the architecture is fully
> - * coherent. In such cases, existing GTT mmaps
> - * ignore the cache bit in the PTE and we can
> - * rewrite it without confusing the GPU or having
> - * to force userspace to fault back in its mmaps.
> - */
> - }
> -
> - list_for_each_entry(vma, &obj->vma.list, obj_link) {
> - if (!drm_mm_node_allocated(&vma->node))
> - continue;
> -
> - /* Wait for an earlier async bind, need to rewrite it */
> - ret = i915_vma_sync(vma);
> - if (ret)
> - return ret;
> -
> - ret = i915_vma_bind(vma, cache_level, PIN_UPDATE, NULL);
> - if (ret)
> - return ret;
> - }
> - }
> + ret = i915_gem_object_unbind(obj, I915_GEM_OBJECT_UNBIND_ACTIVE);
> + if (ret)
> + return ret;
>
> - list_for_each_entry(vma, &obj->vma.list, obj_link) {
> + spin_lock(&obj->vma.lock);
> + list_for_each_entry(vma, &obj->vma.list, obj_link)
> if (i915_vm_has_cache_coloring(vma->vm))
> vma->node.color = cache_level;
> - }
> + spin_unlock(&obj->vma.lock);
Do we still need to bother setting node.color; if we unbind the vma,
the color will be set again if we re-bind it? Although we only set the
cache-level below so seems kinda racy either way?
Reviewed-by: Matthew Auld <matthew.auld at intel.com>
> +
> i915_gem_object_set_cache_coherency(obj, cache_level);
> obj->cache_dirty = true; /* Always invalidate stale cachelines */
>
> --
> 2.24.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list