[Intel-gfx] [PATCH v3 2/5] drm/i915: Use batch pools with the command parser

[Chris Wilson]

On Thu, Nov 06, 2014 at 09:38:00AM -0800, Volkin, Bradley D wrote:
> On Thu, Nov 06, 2014 at 05:56:36AM -0800, Daniel Vetter wrote:
> > On Thu, Nov 06, 2014 at 07:36:55AM +0000, Chris Wilson wrote:
> > > On Wed, Nov 05, 2014 at 02:42:00PM -0800, Volkin, Bradley D wrote:
> > > > For this part, I've got an implementation that works ok but one difference is
> > > > that if we stop submitting batches, and therefore stop calling batch_pool_get,
> > > > we stop moving buffers to the batch pool's inactive list. This means some buffers
> > > > don't get marked purgeable even when they are. The solution that I see is to
> > > > add a function to do the batch pool active -> inactive work and then call that
> > > > from the appropriate place(s), but that seems to defeat the purpose of the
> > > > proposed change. Suggestions?
> > > 
> > > Just mark them always as purgeable.
> > 
> > Yeah the trick with purgeable is that the shrinker will wait for the
> > buffers to retire if they're still active. So you can mark the purgeable
> > right after the move_to_active call. Then the only part that doesn't
> > happen automatically is the batch-pool internal accounting. But we also
> > don't really care about that until we want a new shadow batch.
> 
> Ok. I was concerned about leaving objects purgeable because there are various
> places where the driver checks that an object is not purgeable. Looking at it
> again, the only one I'm nervous about is i915_gem_object_get_pages(), but I'll
> put something together and see if it's a problem. I imagine we can avoid the
> issue by carefully setting madv during/after the parser flow.

Once we have the pages we will only lose them to the shrinker.

  batch_get_obj() {
        // presume we have sufficient i915_gem_retire_requests_ring()
	// see i915_gem_execbuffer_reserver()
	list_for_each(cache, list) {
		obj = cache->obj;
			
		if (obj->active)
			break;
		if (obj->madv == __I915_MADV_PURGED) {
			free(cache+obj);
			continue;
		}
		list_move_tail(&cache->link, &list);
		return obj;
	}

	obj = i915_gem_obj_alloc();
	i915_gem_object_get_pages(obj);
	obj->madv = I915_MADV_DONTNEED;

	cache = kmalloc();
	cache->obj = obj;
	list_add_tail(&cache->link, &cache);

	return obj;
 }

Whilst the object is active, it won't be preferrentially shrunk, but
will still be discarded in dire straits. So we don't need to touch madv
or worry about pages after allocation. However, as the free of the
kernel structs will not happen until the next execbuffer, we may consider
explicitly adding the cleanup of the cache to the oom-notifier.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre