| Message ID | 20180605071949.14159-3-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Chris Wilson (2018-06-05 10:19:40) > In preparation, for having non-vma objects stored inside the ggtt, to > handle restoration of the GGTT following resume, we need to walk over > the ggtt address space rebinding vma, as opposed to walking over bound > objects looking for ggtt entries. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > Cc: Matthew Auld <matthew.william.auld@gmail.com> Comment below. Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > @@ -3578,21 +3578,15 @@ void i915_gem_restore_gtt_mappings(struct drm_i915_private *dev_priv) > ggtt->vm.closed = true; /* skip rewriting PTE on VMA unbind */ > > /* clflush objects bound into the GGTT and rebind them. */ > - list_for_each_entry_safe(obj, on, &dev_priv->mm.bound_list, mm.link) { > - bool ggtt_bound = false; > - struct i915_vma *vma; > - > - for_each_ggtt_vma(vma, obj) { > - if (!i915_vma_unbind(vma)) > - continue; > + GEM_BUG_ON(!list_empty(&ggtt->vm.active_list)); > + list_for_each_entry_safe(vma, vn, &ggtt->vm.inactive_list, vm_link) { > + struct drm_i915_gem_object *obj = vma->obj; > > - WARN_ON(i915_vma_bind(vma, obj->cache_level, > - PIN_UPDATE)); > - ggtt_bound = true; > - } > + if (!i915_vma_unbind(vma)) > + continue; > > - if (ggtt_bound) > - WARN_ON(i915_gem_object_set_to_gtt_domain(obj, false)); > + WARN_ON(i915_vma_bind(vma, obj->cache_level, PIN_UPDATE)); > + WARN_ON(i915_gem_object_set_to_gtt_domain(obj, false)); This gets called multiple times per an object for partial and rotated views, but that should not be our performance bottleneck. Regards, Joonas
Quoting Joonas Lahtinen (2018-06-05 08:51:39) > Quoting Chris Wilson (2018-06-05 10:19:40) > > In preparation, for having non-vma objects stored inside the ggtt, to > > handle restoration of the GGTT following resume, we need to walk over > > the ggtt address space rebinding vma, as opposed to walking over bound > > objects looking for ggtt entries. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > Cc: Matthew Auld <matthew.william.auld@gmail.com> > > Comment below. > > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > > @@ -3578,21 +3578,15 @@ void i915_gem_restore_gtt_mappings(struct drm_i915_private *dev_priv) > > ggtt->vm.closed = true; /* skip rewriting PTE on VMA unbind */ > > > > /* clflush objects bound into the GGTT and rebind them. */ > > - list_for_each_entry_safe(obj, on, &dev_priv->mm.bound_list, mm.link) { > > - bool ggtt_bound = false; > > - struct i915_vma *vma; > > - > > - for_each_ggtt_vma(vma, obj) { > > - if (!i915_vma_unbind(vma)) > > - continue; > > + GEM_BUG_ON(!list_empty(&ggtt->vm.active_list)); > > + list_for_each_entry_safe(vma, vn, &ggtt->vm.inactive_list, vm_link) { > > + struct drm_i915_gem_object *obj = vma->obj; > > > > - WARN_ON(i915_vma_bind(vma, obj->cache_level, > > - PIN_UPDATE)); > > - ggtt_bound = true; > > - } > > + if (!i915_vma_unbind(vma)) > > + continue; > > > > - if (ggtt_bound) > > - WARN_ON(i915_gem_object_set_to_gtt_domain(obj, false)); > > + WARN_ON(i915_vma_bind(vma, obj->cache_level, PIN_UPDATE)); > > + WARN_ON(i915_gem_object_set_to_gtt_domain(obj, false)); > > This gets called multiple times per an object for partial and rotated > views, but that should not be our performance bottleneck. Correct. I thought I had mentioned the drawback, but it appears I just thought about it. It should be the case that for the majority of those, we will be able to unbind them anyway. It's only the few objects pinned for use that we have to flush, and they are the scanout and context objects. Furthermore, repeated calls to gtt_domain should come in the wash as being no-ops, just many checks to determine we have nothing to do. Improving the whole domain management is on the permanent wishlist (killing struct_mutex is of immediate priority). -Chris
Quoting Joonas Lahtinen (2018-06-05 08:51:39) > Quoting Chris Wilson (2018-06-05 10:19:40) > > In preparation, for having non-vma objects stored inside the ggtt, to > > handle restoration of the GGTT following resume, we need to walk over > > the ggtt address space rebinding vma, as opposed to walking over bound > > objects looking for ggtt entries. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > Cc: Matthew Auld <matthew.william.auld@gmail.com> > > Comment below. > > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > > @@ -3578,21 +3578,15 @@ void i915_gem_restore_gtt_mappings(struct drm_i915_private *dev_priv) > > ggtt->vm.closed = true; /* skip rewriting PTE on VMA unbind */ > > > > /* clflush objects bound into the GGTT and rebind them. */ > > - list_for_each_entry_safe(obj, on, &dev_priv->mm.bound_list, mm.link) { > > - bool ggtt_bound = false; > > - struct i915_vma *vma; > > - > > - for_each_ggtt_vma(vma, obj) { > > - if (!i915_vma_unbind(vma)) > > - continue; > > + GEM_BUG_ON(!list_empty(&ggtt->vm.active_list)); > > + list_for_each_entry_safe(vma, vn, &ggtt->vm.inactive_list, vm_link) { > > + struct drm_i915_gem_object *obj = vma->obj; > > > > - WARN_ON(i915_vma_bind(vma, obj->cache_level, > > - PIN_UPDATE)); > > - ggtt_bound = true; > > - } > > + if (!i915_vma_unbind(vma)) > > + continue; > > > > - if (ggtt_bound) > > - WARN_ON(i915_gem_object_set_to_gtt_domain(obj, false)); > > + WARN_ON(i915_vma_bind(vma, obj->cache_level, PIN_UPDATE)); > > + WARN_ON(i915_gem_object_set_to_gtt_domain(obj, false)); > > This gets called multiple times per an object for partial and rotated > views, but that should not be our performance bottleneck. It is however worth mentioning that rewriting the GTT is one of the pain points for resume. A few GiB of WC writes is not free, so we have tried hard not to rewrite it multiple times and to defer setup until use. (With ppGTT, the GGTT is emptier so full-ppgtt should be a net win.) -Chris
Quoting Chris Wilson (2018-06-05 09:19:07) > Quoting Joonas Lahtinen (2018-06-05 08:51:39) > > Quoting Chris Wilson (2018-06-05 10:19:40) > > > In preparation, for having non-vma objects stored inside the ggtt, to > > > handle restoration of the GGTT following resume, we need to walk over > > > the ggtt address space rebinding vma, as opposed to walking over bound > > > objects looking for ggtt entries. > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > > Cc: Matthew Auld <matthew.william.auld@gmail.com> > > > > Comment below. > > > > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > > > > @@ -3578,21 +3578,15 @@ void i915_gem_restore_gtt_mappings(struct drm_i915_private *dev_priv) > > > ggtt->vm.closed = true; /* skip rewriting PTE on VMA unbind */ > > > > > > /* clflush objects bound into the GGTT and rebind them. */ > > > - list_for_each_entry_safe(obj, on, &dev_priv->mm.bound_list, mm.link) { > > > - bool ggtt_bound = false; > > > - struct i915_vma *vma; > > > - > > > - for_each_ggtt_vma(vma, obj) { > > > - if (!i915_vma_unbind(vma)) > > > - continue; > > > + GEM_BUG_ON(!list_empty(&ggtt->vm.active_list)); > > > + list_for_each_entry_safe(vma, vn, &ggtt->vm.inactive_list, vm_link) { > > > + struct drm_i915_gem_object *obj = vma->obj; > > > > > > - WARN_ON(i915_vma_bind(vma, obj->cache_level, > > > - PIN_UPDATE)); > > > - ggtt_bound = true; > > > - } > > > + if (!i915_vma_unbind(vma)) > > > + continue; > > > > > > - if (ggtt_bound) > > > - WARN_ON(i915_gem_object_set_to_gtt_domain(obj, false)); > > > + WARN_ON(i915_vma_bind(vma, obj->cache_level, PIN_UPDATE)); > > > + WARN_ON(i915_gem_object_set_to_gtt_domain(obj, false)); > > > > This gets called multiple times per an object for partial and rotated > > views, but that should not be our performance bottleneck. > > It is however worth mentioning that rewriting the GTT is one of the pain > points for resume. A few GiB of WC writes is not free, so we have tried > hard not to rewrite it multiple times and to defer setup until use. > (With ppGTT, the GGTT is emptier so full-ppgtt should be a net win.) Hmm, we can filter on vma->flags & PIN_GLOBAL so that we don't touch aliasing_ppgtt only vma... -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 12b1386e47e9..4fb5c79ac24b 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -3568,7 +3568,7 @@ void i915_ggtt_disable_guc(struct drm_i915_private *i915) void i915_gem_restore_gtt_mappings(struct drm_i915_private *dev_priv) { struct i915_ggtt *ggtt = &dev_priv->ggtt; - struct drm_i915_gem_object *obj, *on; + struct i915_vma *vma, *vn; i915_check_and_clear_faults(dev_priv); @@ -3578,21 +3578,15 @@ void i915_gem_restore_gtt_mappings(struct drm_i915_private *dev_priv) ggtt->vm.closed = true; /* skip rewriting PTE on VMA unbind */ /* clflush objects bound into the GGTT and rebind them. */ - list_for_each_entry_safe(obj, on, &dev_priv->mm.bound_list, mm.link) { - bool ggtt_bound = false; - struct i915_vma *vma; - - for_each_ggtt_vma(vma, obj) { - if (!i915_vma_unbind(vma)) - continue; + GEM_BUG_ON(!list_empty(&ggtt->vm.active_list)); + list_for_each_entry_safe(vma, vn, &ggtt->vm.inactive_list, vm_link) { + struct drm_i915_gem_object *obj = vma->obj; - WARN_ON(i915_vma_bind(vma, obj->cache_level, - PIN_UPDATE)); - ggtt_bound = true; - } + if (!i915_vma_unbind(vma)) + continue; - if (ggtt_bound) - WARN_ON(i915_gem_object_set_to_gtt_domain(obj, false)); + WARN_ON(i915_vma_bind(vma, obj->cache_level, PIN_UPDATE)); + WARN_ON(i915_gem_object_set_to_gtt_domain(obj, false)); } ggtt->vm.closed = false;
In preparation, for having non-vma objects stored inside the ggtt, to handle restoration of the GGTT following resume, we need to walk over the ggtt address space rebinding vma, as opposed to walking over bound objects looking for ggtt entries. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> Cc: Matthew Auld <matthew.william.auld@gmail.com> --- drivers/gpu/drm/i915/i915_gem_gtt.c | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-)