Re: [PATCH] drm/i915: Prevent recursion by retiring requests when the ring is full

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Jan 28, 2014 at 08:22:32AM +0000, Chris Wilson wrote:
> On Tue, Jan 28, 2014 at 09:11:17AM +0100, Daniel Vetter wrote:
> > On Mon, Jan 27, 2014 at 10:43:07PM +0000, Chris Wilson wrote:
> > > As the VM do not track activity of objects and instead use a large
> > > hammer to forcibly idle and evict all of their associated objects when
> > > one is released, it is possible for that to cause a recursion when we
> > > need to wait for free space on a ring and call retire requests.
> > > (intel_ring_begin -> intel_ring_wait_request ->
> > > i915_gem_retire_requests_ring -> i915_gem_context_free ->
> > > i915_gem_evict_vm -> i915_gpu_idle -> intel_ring_begin etc)
> > 
> > Is there no way to get rid of the gpu_idle? Imo doing a gpu idle as part
> > of the cleanup of a retired request is just not great and has a good
> > chance to trip us up all over the place since it's _really_ unexpected.
> 
> Yes, that gpu-idle is only an artifact of lacking activity counting on
> the vm.
>  
> > So if we can do it (and iirc we've agreed that all the ppgtt cleanup is
> > mostly fluff) then I prefer we remove the root-cause instead of just
> > chopping off another head of the hydra.
> 
> Look again, I think this is a welcome simplicity to that piece of code.
> We lose the optimisation of having the most accurate ring->space
> following the wait in exchange for having a straightforward wait&update.
> Plus retiring_requests at random points of time has turned out to be a
> bad idea - wait long enough and I can probably find several other code
> paths that could explode here.

Hm, good point, I'll agree this patch has value in itself. I'll quote our
discussion here in the commit message when merging it and will look for a
review victim now.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch