[Intel-gfx] [PATCH v2 1/5] drm/i915: Implement a framework for batch buffer pools

[Volkin, Bradley D]

On Wed, Jul 09, 2014 at 08:30:08AM -0700, Chris Wilson wrote:
> On Wed, Jul 09, 2014 at 08:10:47AM -0700, Volkin, Bradley D wrote:
> > On Wed, Jul 09, 2014 at 12:37:08AM -0700, Chris Wilson wrote:
> > > On Tue, Jul 08, 2014 at 03:26:36PM -0700, bradley.d.volkin at intel.com wrote:
> > > > From: Brad Volkin <bradley.d.volkin at intel.com>
> > > > 
> > > > This adds a small module for managing a pool of batch buffers.
> > > > The only current use case is for the command parser, as described
> > > > in the kerneldoc in the patch. The code is simple, but separating
> > > > it out makes it easier to change the underlying algorithms and to
> > > > extend to future use cases should they arise.
> > > > 
> > > > The interface is simple: init to create an empty pool, fini to
> > > > clean it up; get to obtain a new buffer, put to return it to the
> > > > pool. Note that all buffers must be returned to the pool before
> > > > freeing it.
> > > > 
> > > > Buffers are purgeable while in the pool, but not explicitly
> > > > truncated in order to avoid overhead during execbuf.
> > > > 
> > > > Locking is currently based on the caller holding the struct_mutex.
> > > > We already do that in the places where we will use the batch pool
> > > > for the command parser.
> > > > 
> > > > v2:
> > > > - s/BUG_ON/WARN_ON/ for locking assertions
> > > > - Remove the cap on pool size
> > > > - Switch from alloc/free to init/fini
> > > 
> > > You are missing the madvise mechanics.
> > 
> > _get() and _put() set obj->madv, which I thought was enough for what I've
> > described (i.e. purgeable while in the pool but not explicitly truncated).
> > Is that not enough for the shrinker to truncate as needed? Or do you want
> > them explicitly truncated while in the pool? I was worried about the extra
> > cost of getting the backing storage back during execbuf.
> 
> You need to check that the shrinker hasn't truncated your backing
> storage whilst it was marked DONTNEED. It just amounts to checking for
> obj->madv == __I915_MADV_PURGED in pool_get() and discarding the cache
> entry if it has been truncated.

Ok. When we hit a purged object, do you think it's worth doing a retry loop (as
in libdrm's cache) or just fall straight through to allocating a new object?

Brad

> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre