Re: [Intel-gfx] [PATCH] drm/i915: Implement fair lru eviction across both rings. (v2)

2010-08-06 Thread Daniel Vetter 
On Fri, Aug 06, 2010 at 12:25:45PM +0100, Chris Wilson wrote:
 Based in a large part upon Daniel Vetter's implementation and adapted
 for handling multiple rings in a single pass.
 
 This should lead to better gtt usage and fixes the page-fault-of-doom
 triggered. The fairness is provided by scanning through the GTT space
 amalgamating space in rendering order. As soon as we have a contiguous
 space in the GTT large enough for the new object (and its alignment),
 evict any object which lies within that space. This should keep more
 objects resident in the GTT.
 
 Doing throughput testing on a PineView machine with cairo-perf-trace
 indicates that there is very little difference with the new LRU scan,
 perhaps a small improvement... Except oddly for the poppler trace.
 
 Reference:
 
   Bug 15911 - Intermittent X crash (freeze)
   https://bugzilla.kernel.org/show_bug.cgi?id=15911
 
   Bug 20152 - cannot view JPG in firefox when running UXA
   https://bugs.freedesktop.org/show_bug.cgi?id=20152
 
   Bug 24369 - Hang when scrolling firefox page with window in front
   https://bugs.freedesktop.org/show_bug.cgi?id=24369
 
   Bug 28478 - Intermittent graphics lockups due to overflow/loop
   https://bugs.freedesktop.org/show_bug.cgi?id=28478
 
 v2: Attempt to clarify the logic and order of eviction through the use
 of comments and macros.
 
 Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
 Cc: Daniel Vetter dan...@ffwll.ch

One small nitpick below.

  int
 -i915_gem_evict_something(struct drm_device *dev,
 -  int min_size, unsigned alignment)
 +i915_gem_evict_something(struct drm_device *dev, int min_size, unsigned 
 alignment)
  {

 [cut]

 + return i915_gem_evict_everything(dev);

I think this should be equivalent to

return -ENOMEM;

(minus wasting less cpu, of course ;) But that's easily fixable in another
patch (and probably better for bisection). Otherwise the patch looks good,
besides that my gut still thinks that One Scan to Rule Them All is
overkill. But that's simply bike-shedding, hence:

Reviewed-by: Daniel Vetter daniel.vet...@ffwll.ch
-- 
Daniel Vetter
Mail: dan...@ffwll.ch
Mobile: +41 (0)79 365 57 48
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Implement fair lru eviction across both rings. (v2)

2010-08-06 Thread Chris Wilson 
On Fri, 6 Aug 2010 20:58:30 +0200, Daniel Vetter dan...@ffwll.ch wrote:
 On Fri, Aug 06, 2010 at 12:25:45PM +0100, Chris Wilson wrote:
  +   return i915_gem_evict_everything(dev);
 
 I think this should be equivalent to
 
   return -ENOMEM;

Yes, we could return -ENOSPC and then do_execbuffer() would unpin and call
i915_gem_evict_everything() before retrying.

I'm happy for anybody to paint it a completely different colour just as
soon as it is heading upstream. :)

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx