Re: [PATCH 4/6] drm/i915: Add bind/unbind object functions to VM

[Ben Widawsky <ben@bwidawsk.net>]

On Mon, Sep 16, 2013 at 11:13:02PM +0100, Chris Wilson wrote:
> On Mon, Sep 16, 2013 at 11:23:43AM -0700, Ben Widawsky wrote:
> > On Mon, Sep 16, 2013 at 10:25:28AM +0100, Chris Wilson wrote:
> > > On Sat, Sep 14, 2013 at 03:03:17PM -0700, Ben Widawsky wrote:
> > > > +static void gen6_ggtt_bind_vma(struct i915_vma *vma,
> > > > +			       enum i915_cache_level cache_level,
> > > > +			       u32 flags)
> > > > +{
> > > > +	struct drm_device *dev = vma->vm->dev;
> > > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > > +	struct drm_i915_gem_object *obj = vma->obj;
> > > > +	const unsigned long entry = vma->node.start >> PAGE_SHIFT;
> > > > +
> > > > +	/* If there is an aliasing PPGTT, and the user didn't explicitly ask for
> > > > +	 * the global, just use aliasing */
> > > > +	if (!dev_priv->mm.aliasing_ppgtt || flags & GLOBAL_BIND) {
> > > > +		/* If the object is unbound, or we're change the cache bits */
> > > > +		if (!obj->has_global_gtt_mapping ||
> > > > +		    (cache_level != obj->cache_level)) {
> > > > +			gen6_ggtt_insert_entries(vma->vm, obj->pages, entry,
> > > > +						 cache_level);
> > > > +			obj->has_global_gtt_mapping = 1;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	/* If put the mapping in the aliasing PPGTT as well as Global if we have
> > > > +	 * aliasing, but the user requested global. */
> > > 
> > > Why? As a proponent of full-ppgtt I thought you would be envisoning a
> > > future where the aliasing_ppgtt was used far less (i.e. never), and the
> > > ggtt would only continue to be used for the truly global entries such as
> > > scanouts, contexts, pdes, execlists etc.
> > > 
> > 
> > Firstly, I've still yet to expose the grand plan at this point in the
> > series, so I am not really certain if you're just complaining for the
> > fun of it, or what. I'd like to make everything functionally the same,
> > just with VMA support.
> 
> I'm complaining because the comment is awful: telling me what the code
> is doing but not why. It doesn't seem obvious that if the user
> explicitly wanted a global mapping and that the object is not already in
> aliasing ppgtt that it is likely to be used in the aliasing ppgtt in the
> near future.
> 
> > Secondly, I was under the impression that for Sandybridge we had to have
> > all global mappings in the aliasing to support PIPE_CONTROL, or some
> > command like that. It's a bit mixed up in my head atm, and I'm too lazy
> > to look at the exact reason.
> 
> It does, but if we never enable full-ppgtt for SNB we don't have to
> worry about full-ppgtt being unusable for OpenGL (at least not without a
> 1:1 ppgtt to global mapping of all oq objects).
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

I'm sorry. After reading my comments again, you're absolutely right.

How's this?

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 2a71a29..fcf36ae 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -658,10 +658,17 @@ static void gen6_ggtt_bind_vma(struct i915_vma *vma,
        struct drm_i915_gem_object *obj = vma->obj;
        const unsigned long entry = vma->node.start >> PAGE_SHIFT;
 
-       /* If there is an aliasing PPGTT, and the user didn't explicitly ask for
-        * the global, just use aliasing */
+       /* If there is no aliasing PPGTT, or the caller needs a global mapping,
+        * or we have a global mapping already but the cacheability flags have
+        * changed, set the global PTES.
+        *
+        * If there is an aliasing PPGTT it is anecdotally faster, so use that
+        * instead if none of the above hold true.
+        *
+        * NB: A global mapping should only be needed for special regions like
+        * "gtt mappable", SNB errata, or if specified via special execbuf flags
+        */
        if (!dev_priv->mm.aliasing_ppgtt || flags & GLOBAL_BIND) {
-               /* If the object is unbound, or we're change the cache bits */
                if (!obj->has_global_gtt_mapping ||
                    (cache_level != obj->cache_level)) {
                        gen6_ggtt_insert_entries(vma->vm, obj->pages, entry,
@@ -670,8 +677,6 @@ static void gen6_ggtt_bind_vma(struct i915_vma *vma,
                }
        }
 
-       /* If put the mapping in the aliasing PPGTT as well as Global if we have
-        * aliasing, but the user requested global. */
        if (dev_priv->mm.aliasing_ppgtt &&
            (!obj->has_aliasing_ppgtt_mapping ||
             (cache_level != obj->cache_level))) {


-- 
Ben Widawsky, Intel Open Source Technology Center