On Thu, Jun 27, 2013 at 04:43:40PM -0700, Ben Widawsky wrote:
> On Thu, Jun 27, 2013 at 04:31:02PM -0700, Ben Widawsky wrote:
> > This requires doing an actual switch of the page tables during the
> > context switch/execbuf.
> >
> > Along the way, cut away as much "aliasing" ppgtt as possible
> >
> > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> > ---
> > drivers/gpu/drm/i915/i915_gem.c | 22 +++++++++++++---------
> > drivers/gpu/drm/i915/i915_gem_context.c | 29 +++++++++++++++++------------
> > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 27 ++++++++++++++++++++-------
> > 3 files changed, 50 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index af0150e..f05d585 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -2170,7 +2170,10 @@ 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;
> > + if (request->ctx)
> > + vm = &request->ctx->ppgtt.base;
> > + else
> > + vm = &dev_priv->gtt.base;
> >
> > return vm;
> > }
> > @@ -2676,10 +2679,10 @@ i915_gem_object_unbind(struct drm_i915_gem_object *obj,
> >
> > if (obj->has_global_gtt_mapping && is_i915_ggtt(vm))
> > i915_gem_gtt_unbind_object(obj);
> > - if (obj->has_aliasing_ppgtt_mapping) {
> > - i915_ppgtt_unbind_object(dev_priv->gtt.aliasing_ppgtt, obj);
> > - obj->has_aliasing_ppgtt_mapping = 0;
> > - }
> > +
> > + vm->clear_range(vm, i915_gem_obj_offset(obj, vm) >> PAGE_SHIFT,
> > + obj->base.size >> PAGE_SHIFT);
> > +
> > i915_gem_gtt_finish_object(obj);
> > i915_gem_object_unpin_pages(obj);
> >
> > @@ -3444,11 +3447,12 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> > return ret;
> > }
> >
> > - if (obj->has_global_gtt_mapping)
> > + if (!is_i915_ggtt(vm) && obj->has_global_gtt_mapping)
> > i915_gem_gtt_bind_object(obj, cache_level);
Are you planning to kill i915_gem_gtt_(un)bind_object? In many cases you
seem to end up writing the global GTT PTEs twice because of it. I guess
the only catch is that obj->has_gtt_mapping must be kept in sync w/
reality if you kill it.
> > - if (obj->has_aliasing_ppgtt_mapping)
> > - i915_ppgtt_bind_object(dev_priv->gtt.aliasing_ppgtt,
> > - obj, cache_level);
> > +
> > + vm->insert_entries(vm, obj->pages,
> > + i915_gem_obj_offset(obj, vm) >> PAGE_SHIFT,
> > + cache_level);
> >
> > i915_gem_obj_set_color(obj, vm, cache_level);
This cache level stuff ends up looking a bit wonky, but I guess you
didn't spend much time on it yet.
I'll just note that I don't think we can allow per address space cache
levels through this interface since that would break the case where the
client renders to the buffer, then the server/compositor sets it to
uncached and does a page flip. After this the client has to also use
uncached mappings or the server/compositor won't realize that it would
need to clflush again before page flipping.
So I think you can just eliminate the vm argument from this function and
then the function could look something like this (pardon my pseudo
code):
...
if (bound_any(obj)) {
finish_gpu
finish_gtt
put_fence
...
}
for_each_safe(obj->vma_list) {
vm = vma->vm;
node = vma->node;
if (verify_gtt(node, cache_level)) {
unbind(ovj, vm);
continue;
}
vm->insert_range();
vma->color = cacle_level;
}
...
This also got me thinking about MOCS. That think seems a bit dangerous
in this case since the client can easily override the cache level w/o
the server/compositor noticing. I guess we just have to make a rule that
clients aren't allowed to override cache level w/ MOCS for window
system buffers.
Also IIRC someone told me that w/ uncached mappings the caches aren't
snooped even on LLC platforms. If that's true, MOCS seems even more
dangerous since the client could easily mix cached and uncached
accesses. I don't really understand why uncached mappings wouldn't
snoop on LLC platforms since the snooping should be more or less free.
--
Ville Syrj?l?
Intel OTC
