On Wed, Oct 07, 2015 at 04:57:25PM +0100, Tvrtko Ursulin wrote:
>
> Hi,
>
> On 06/10/15 11:39, Chris Wilson wrote:
> >Since the remove of the pin-ioctl, we only care about not changing the
> >cache level on buffers pinned to the hardware as indicated by
> >obj->pin_display. So we can safely replace i915_gem_object_is_pinned()
>
> i915_gem_obj_is_pinned
What? That's not the normal prefix, who named that monstrosity!
>
> >here with a plain obj->pin_display check. During rebinding, we will check
> >sanity checks in case vma->pin_count is erroneously set.
>
> "do sanity checks" or something.
>
> >At the same time, we can micro-optimise GTT mmap() behaviour since we
> >only need to relinquish the mmaps before Sandybridge.
> >
> >Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> >---
> > drivers/gpu/drm/i915/i915_gem.c | 40 ++++++++++++++++++++++++----------------
> > 1 file changed, 24 insertions(+), 16 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >index d4a3bdf0c5b6..2b8ed7a2faab 100644
> >--- a/drivers/gpu/drm/i915/i915_gem.c
> >+++ b/drivers/gpu/drm/i915/i915_gem.c
> >@@ -3629,31 +3629,34 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> > {
> > struct drm_device *dev = obj->base.dev;
> > struct i915_vma *vma, *next;
> >+ bool bound = false;
> > int ret = 0;
> >
> > if (obj->cache_level == cache_level)
> > goto out;
> >
> >- if (i915_gem_obj_is_pinned(obj)) {
> >- DRM_DEBUG("can not change the cache level of pinned objects\n");
> >- return -EBUSY;
> >- }
> >-
> > list_for_each_entry_safe(vma, next, &obj->vma_list, vma_link) {
> >+ if (!drm_mm_node_allocated(&vma->node))
> >+ continue;
> >+
> >+ if (vma->pin_count) {
> >+ DRM_DEBUG("can not change the cache level of pinned objects\n");
> >+ return -EBUSY;
> >+ }
> >+
>
> But this is the same as i915_gem_obj_is_pinned, where is the
> obj->pin_display change commit message talks about?
Right here. The difference is that we are only iterating the vma list
once rather than 3x.
> > if (!i915_gem_valid_gtt_space(vma, cache_level)) {
> > ret = i915_vma_unbind(vma);
> > if (ret)
> > return ret;
> >- }
> >+ } else
> >+ bound = true;
> > }
> >
> >- if (i915_gem_obj_bound_any(obj)) {
> >+ if (bound) {
> > ret = i915_gem_object_wait_rendering(obj, false);
> > if (ret)
> > return ret;
> >
> >- i915_gem_object_finish_gtt(obj);
> >-
> > /* Before SandyBridge, you could not use tiling or fence
> > * registers with snooped memory, so relinquish any fences
> > * currently pointing to our region in the aperture.
> >@@ -3664,13 +3667,18 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> > return ret;
> > }
> >
> >- list_for_each_entry(vma, &obj->vma_list, vma_link)
> >- if (drm_mm_node_allocated(&vma->node)) {
> >- ret = i915_vma_bind(vma, cache_level,
> >- PIN_UPDATE);
> >- if (ret)
> >- return ret;
> >- }
> >+ /* Access to snoopable pages through the GTT is incoherent. */
> >+ if (cache_level != I915_CACHE_NONE && !HAS_LLC(dev))
> >+ i915_gem_release_mmap(obj);
>
> Don't fully understand this one - but my question is this.
> Previously userspace would lose mappings on cache level changes any
> time, after this only on !LLC when turning on caching mode. So this
> means userspace needs to know about this change and modify it's
> behavior? Or what exactly would happen in practice?
No. Userspace has no knowledge of the kernel handling the PTEs, its
mapping is persistent (i.e. the obj->mmap_offset inside the dev->mappping).
Otoh, we are improving the situation so even if userspace tries to avoid
set-cache-level nothing is lost.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
