Re: [PATCH 61/66] drm/i915: Use multiple VMs

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@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