[Intel-gfx] [PATCH 17/41] drm/i915: Pass around sg_table to get_pages/put_pages backend

[Chris Wilson]

On Mon, Oct 17, 2016 at 02:51:01PM +0100, Tvrtko Ursulin wrote:
> 
> On 17/10/2016 12:31, Chris Wilson wrote:
> >On Mon, Oct 17, 2016 at 11:55:54AM +0100, Tvrtko Ursulin wrote:
> >>On 14/10/2016 13:18, Chris Wilson wrote:
> >>>  static void
> >>>-i915_gem_object_put_pages_gtt(struct drm_i915_gem_object *obj)
> >>>+i915_gem_object_put_pages_gtt(struct drm_i915_gem_object *obj,
> >>>+			      struct sg_table *pages)
> >>>  {
> >>>  	struct sgt_iter sgt_iter;
> >>>  	struct page *page;
> >>>-	int ret;
> >>>-	GEM_BUG_ON(obj->mm.madv == __I915_MADV_PURGED);
> >>>-
> >>>-	ret = i915_gem_object_set_to_cpu_domain(obj, true);
> >>>-	if (WARN_ON(ret)) {
> >>>-		/* In the event of a disaster, abandon all caches and
> >>>-		 * hope for the best.
> >>>-		 */
> >>>-		i915_gem_clflush_object(obj, true);
> >>>-		obj->base.read_domains = obj->base.write_domain = I915_GEM_DOMAIN_CPU;
> >>>-	}
> >>>+	__i915_gem_object_release_shmem(obj);
> >>Waiting for the object to become inactive is now gone. It did not
> >>spot that that changed with this patch. Did it?
> >There's no wait here. We have BUG_ONs today (that remain in place)
> >to catch trying to free pages that are in use by the GPU. This is just a
> >change of domains (and I wanted to keep the set-to-cpu-domain asserts,
> >and avoid the other side effects).
> 
> Oh right, makes sense.
> 
> >>>  int __i915_gem_object_get_pages(struct drm_i915_gem_object *obj)
> >>>  {
> >>>-	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
> >>>-	const struct drm_i915_gem_object_ops *ops = obj->ops;
> >>>-	int ret;
> >>>+	int err;
> >>>  	lockdep_assert_held(&obj->base.dev->struct_mutex);
> >>>  	if (obj->mm.pages)
> >>>  		return 0;
> >>>-	if (obj->mm.madv != I915_MADV_WILLNEED) {
> >>>-		DRM_DEBUG("Attempting to obtain a purgeable object\n");
> >>>-		__i915_gem_object_unpin_pages(obj);
> >>>-		return -EFAULT;
> >>>-	}
> >>>-
> >>>-	ret = ops->get_pages(obj);
> >>>-	if (ret) {
> >>>+	err = ____i915_gem_object_get_pages(obj);
> >>>+	if (err)
> >>>  		__i915_gem_object_unpin_pages(obj);
> >>>-		return ret;
> >>>-	}
> >>>-	list_add_tail(&obj->global_list, &dev_priv->mm.unbound_list);
> >>Objects will no longer appear on the unbound list when they have
> >>backing store?
> >They still do. We don't hold the struct_mutex here, so we defer the
> >global list tracking to the domain management which is slightly later.
> 
> Later as when VMA gets pinned?...

On unbind, and whilst unbound upon use by the user. 
 
> >However, there is still a window of opportunity where we have pages
> >pinned for our use but not yet visible to the shrinker. A bit of
> >rejigging should be mean we only need to move the unbound upon
> >pages_pin_count==0 and it will require a spinlock, but that make
> >actually work out to be less code. But it won't yet reduce the
> >struct_mutex hold time in the shrinker, so I'm not seeing the upside
> >yet.
> 
> ... in which case userspace just creating objects and not touching
> them avoids putting them on bound/unbound lists?

We don't allocate pages until the user uses them and that goes through
the domain tracking (should). Yes, there is a gap there and to solve that
we need to manage the mm.unbound_list using a spinlock rather than a mutex.
Just trying to decide if it is worth closing the hole now, or later when
we have a better idea of how to remove struct_mutex from the shrinker.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre