[Intel-gfx] [PATCH 1/4] drm/i915/bdw: Clean up execlist queue items in retire_work

[Daniel Vetter]

On Mon, Nov 03, 2014 at 04:05:03PM +0000, Daniel, Thomas wrote:
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel
> > Vetter
> > Sent: Monday, November 03, 2014 3:33 PM
> > To: Daniel, Thomas
> > Cc: intel-gfx at lists.freedesktop.org; shuang.he at linux.intel.com
> > Subject: Re: [Intel-gfx] [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 at intel.com>
> > > ---
> > >  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