| Message ID | 20090911143028.542c6dc4@jbarnes-g45 (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Sep 11, 2009 at 02:30:28PM -0700, Jesse Barnes wrote: > Owain pointed out a potential bug in i915_gem_evict_something. In the > case where we get there without any buffers on the inactive list, we > may end up waiting on an outstanding request to finish. If a request > does finish but doesn't end up on the inactive list, we can't assume > that a buffer has been freed and that space is available, since that > path doesn't actually free space (and since we're holding the struct > mutex at that point, no one else will have freed it either). > > So remove the break, fixup the comment, and try flushing something > instead when we hit that case (looping back up to the top as necessary). I also note that the DRM_ERROR at the end of that function is really worrying. whenever (must be rare or never) someone fails to pin all buffers in execbuffer they'll get that worrying error print out, even though it's technically normal operation. I'd suggest removing it. -0-
Excerpts from Owain Ainsworth's message of Fri Sep 11 23:20:00 +0100 2009: > I also note that the DRM_ERROR at the end of that function is really > worrying. whenever (must be rare or never) someone fails to pin all > buffers in execbuffer they'll get that worrying error print out, even > though it's technically normal operation. > > I'd suggest removing it. Indeed as part of my behave-better-under-memory-pressure patch, I'd made both the alterations Owain pointed out. By accident of course... I'll clean up the shrinker patches for review shortly. -ickle
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index f3758f9..ef4065f 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2003,13 +2003,10 @@ i915_gem_evict_something(struct drm_device *dev) break; /* if waiting caused an object to become inactive, - * then loop around and wait for it. Otherwise, we - * assume that waiting freed and unbound something, - * so there should now be some space in the GTT + * then loop around and wait for it. */ if (!list_empty(&dev_priv->mm.inactive_list)) continue; - break; } /* If we didn't have anything on the request list but there
Owain pointed out a potential bug in i915_gem_evict_something. In the case where we get there without any buffers on the inactive list, we may end up waiting on an outstanding request to finish. If a request does finish but doesn't end up on the inactive list, we can't assume that a buffer has been freed and that space is available, since that path doesn't actually free space (and since we're holding the struct mutex at that point, no one else will have freed it either). So remove the break, fixup the comment, and try flushing something instead when we hit that case (looping back up to the top as necessary). Reported-by: Owain Ainsworth <zerooa@googlemail.com> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>