On Mon, Nov 03, 2014 at 04:05:03PM +0000, Daniel, Thomas wrote: > > -----Original Message----- > > From: Daniel Vetter [mailto:daniel.vetter@xxxxxxxx] On Behalf Of Daniel > > Vetter > > Sent: Monday, November 03, 2014 3:33 PM > > To: Daniel, Thomas > > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; shuang.he@xxxxxxxxxxxxxxx > > Subject: Re: [PATCH 1/4] drm/i915/bdw: Clean up execlist queue > > items in retire_work > > > > On Wed, Oct 29, 2014 at 09:52:50AM +0000, Thomas Daniel wrote: > > > No longer create a work item to clean each execlist queue item. > > > Instead, move retired execlist requests to a queue and clean up the > > > items during retire_requests. > > > > > > v2: Fix legacy ring path broken during overzealous cleanup > > > > > > v3: Update idle detection to take execlists queue into account > > > > > > Issue: VIZ-4274 > > > Signed-off-by: Thomas Daniel <thomas.daniel@xxxxxxxxx> > > > --- > > > drivers/gpu/drm/i915/i915_gem.c | 4 +++ > > > drivers/gpu/drm/i915/intel_lrc.c | 52 ++++++++++++++++++----------- > > -- > > > drivers/gpu/drm/i915/intel_lrc.h | 2 +- > > > drivers/gpu/drm/i915/intel_ringbuffer.h | 1 + > > > 4 files changed, 36 insertions(+), 23 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c > > > b/drivers/gpu/drm/i915/i915_gem.c index 827edb5..df28202 100644 > > > --- a/drivers/gpu/drm/i915/i915_gem.c > > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > > @@ -2718,6 +2718,10 @@ i915_gem_retire_requests(struct drm_device > > *dev) > > > for_each_ring(ring, dev_priv, i) { > > > i915_gem_retire_requests_ring(ring); > > > idle &= list_empty(&ring->request_list); > > > + if (i915.enable_execlists) { > > > + idle &= list_empty(&ring->execlist_queue); > > > + intel_execlists_retire_requests(ring); > > > > This needs to be the other way round I think - we care about idleness after all > > the currently processed stuff is retired, not before. Otherwise we might > > notice the busy->idle transition one invocation too late. > I thought for a while about this. The GPU will be idle when the > execlist_queues are empty. > Intel_execlists_retire_requests() cleans up requests which have already > finished so it is more conservative (in terms of CPU idleness) to check the > queue beforehand. I thought this would be more desirable than > potentially reporting idleness early... > Intel_execlists_retire_requests() can not affect the state of the queue. > And there is no point checking the execlist_retired_req_list because > execlists_retire_requests() always empties it. Ok, I mixed things up without looking ;-) But that means you acces the execlist_queue, which is also accessed from irq code, without holding the required locks? This is all a bit confusing to poor me ... -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
