Re: [PATCH 1/2] [RFC] drm/i915: Restore LRU evict order, with a twist!

[Chris Wilson <chris@chris-wilson.co.uk>]

On Thu, 01 Jul 2010 12:49:33 -0700, Eric Anholt <eric@anholt.net> wrote:
> On Thu,  1 Jul 2010 17:53:44 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > +static int
> > +i915_gem_eviction_roster_add(struct i915_gem_eviction_roster *roster,
> > +			     struct drm_i915_gem_object *obj_priv)
> > +{
> 
> This function could stand a brief comment of what it does.  But frankly,
> a lot of this file looks like a rewrite of drm_mm.c -- is there a reason
> not to reuse that for free space tracking?

I only sent this patch simply because it was the one in my tree and has
seen some testing prior to forward porting to the current drm-intel-next.
(Though obviously not any testing on HAS_BSD hardware!). Daniel Vetter
reworked drm_mm.c for the same purpose, which the change in eviction logic
here could reuse once that was ready.

> Only downside seems to be
> doing another lookup to figure out which objects take up the space when
> we decide on which block to free, since the merging of blocks wouldn't
> be aware of our list of objects we want to attach.

IIRC, Daniel used a rollback to undo the incomplete evictions. I'll port
his work to the current tree and rebase upon it.

> (but then, if we're going to talk about cpu efficiency, it seems like we
> could do a lot better than this implementation of _search that we call
> over and over to try to find the a block with the same size/align
> parameters each time)

The fun we could have, though clarity here is paramount. Making the wrong
choice over which object to evict costs far more in clflush than we could
reasonably expend searching...

Thanks for the review. This is something that I feel does need to be in
shape for the next merge window as the 2D code is still susceptible to
the page-fault-of-doom.
-- 
Chris Wilson, Intel Open Source Technology Centre