Re: [PATCH 01/10] drm/i915: remove do_retire from i915_wait_request

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Apr 20, 2012 at 06:23:23PM -0700, Ben Widawsky wrote:
> This originates from a hack by me to quickly fix a bug in an earlier
> patch where we needed control over whether or not waiting on a seqno
> actually did any retire list processing. Since the two operations aren't
> clearly related, we should pull the parameter out of the wait function,
> and make the caller responsible for retiring if the action is desired.
> 
> NOTE: this patch has a functional change. I've only made the callers
> which are requiring the retirement do the retirement. This move was
> blasted by Keith when I tried it before in a more subtle manner; so I am
> making it very clear this time around.

See below for why it's still not a good idea to combine refactoring with
code changes ;-)

> Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c            |    2 +-
>  drivers/gpu/drm/i915/i915_drv.h            |    5 ++---
>  drivers/gpu/drm/i915/i915_gem.c            |   33 +++++++++-------------------
>  drivers/gpu/drm/i915/i915_gem_evict.c      |   14 ++++++++++--
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |    2 +-
>  drivers/gpu/drm/i915/i915_gem_gtt.c        |    2 +-
>  drivers/gpu/drm/i915/intel_overlay.c       |    6 ++---
>  drivers/gpu/drm/i915/intel_ringbuffer.c    |    4 +++-
>  8 files changed, 32 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index a813f65..f8fdc5b 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c

[cut]

> @@ -3440,7 +3427,7 @@ i915_gem_idle(struct drm_device *dev)
>  		return 0;
>  	}
>  
> -	ret = i915_gpu_idle(dev, true);
> +	ret = i915_gpu_idle(dev);
>  	if (ret) {
>  		mutex_unlock(&dev->struct_mutex);
>  		return ret;


gem_idle is called by our suspend freeze function and leaking unretired
seqnos over a s/r cycle was the root cause our -rc2 regression on gen3. In
other words: I'm pretty sure this will blow up. I do like the idea of the
patch, but:

Please separate refactoring from actual code changes.

Cheers, Daniel

> @@ -4018,7 +4005,7 @@ rescan:
>  		 * This has a dramatic impact to reduce the number of
>  		 * OOM-killer events whilst running the GPU aggressively.
>  		 */
> -		if (i915_gpu_idle(dev, true) == 0)
> +		if (i915_gpu_idle(dev) == 0)
>  			goto rescan;
>  	}
>  	mutex_unlock(&dev->struct_mutex);
> diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
> index 21a8271..df9354c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_evict.c
> +++ b/drivers/gpu/drm/i915/i915_gem_evict.c
> @@ -168,6 +168,7 @@ i915_gem_evict_everything(struct drm_device *dev, bool purgeable_only)
>  	drm_i915_private_t *dev_priv = dev->dev_private;
>  	int ret;
>  	bool lists_empty;
> +	int i;
>  
>  	lists_empty = (list_empty(&dev_priv->mm.inactive_list) &&
>  		       list_empty(&dev_priv->mm.flushing_list) &&
> @@ -177,11 +178,20 @@ i915_gem_evict_everything(struct drm_device *dev, bool purgeable_only)
>  
>  	trace_i915_gem_evict_everything(dev, purgeable_only);
>  
> -	/* Flush everything (on to the inactive lists) and evict */
> -	ret = i915_gpu_idle(dev, true);
> +	ret = i915_gpu_idle(dev);
>  	if (ret)
>  		return ret;
>  
> +	/* The gpu_idle will flush everything in the write domain to the
> +	 * active list. Then we must move everything off the active list
> +	 * with retire requests.
> +	 */
> +	for (i = 0; i < I915_NUM_RINGS; i++)
> +		if (WARN_ON(!list_empty(&dev_priv->ring[i].gpu_write_list)))
> +			return -EBUSY;
> +
> +	i915_gem_retire_requests(dev);
> +
>  	BUG_ON(!list_empty(&dev_priv->mm.flushing_list));
>  
>  	return i915_gem_evict_inactive(dev, purgeable_only);
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 68ec013..582f6c4 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1220,7 +1220,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  			 * so every billion or so execbuffers, we need to stall
>  			 * the GPU in order to reset the counters.
>  			 */
> -			ret = i915_gpu_idle(dev, true);
> +			ret = i915_gpu_idle(dev);
>  			if (ret)
>  				goto err;
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 25c8bf9..29d573c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -317,7 +317,7 @@ static bool do_idling(struct drm_i915_private *dev_priv)
>  
>  	if (unlikely(dev_priv->mm.gtt->do_idle_maps)) {
>  		dev_priv->mm.interruptible = false;
> -		if (i915_gpu_idle(dev_priv->dev, false)) {
> +		if (i915_gpu_idle(dev_priv->dev)) {
>  			DRM_ERROR("Couldn't idle GPU\n");
>  			/* Wait a bit, in hopes it avoids the hang */
>  			udelay(10);
> diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> index 80b331c..5ade12e 100644
> --- a/drivers/gpu/drm/i915/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/intel_overlay.c
> @@ -225,8 +225,7 @@ static int intel_overlay_do_wait_request(struct intel_overlay *overlay,
>  	}
>  	overlay->last_flip_req = request->seqno;
>  	overlay->flip_tail = tail;
> -	ret = i915_wait_request(LP_RING(dev_priv), overlay->last_flip_req,
> -				true);
> +	ret = i915_wait_request(LP_RING(dev_priv), overlay->last_flip_req);
>  	if (ret)
>  		return ret;
>  
> @@ -447,8 +446,7 @@ static int intel_overlay_recover_from_interrupt(struct intel_overlay *overlay)
>  	if (overlay->last_flip_req == 0)
>  		return 0;
>  
> -	ret = i915_wait_request(LP_RING(dev_priv), overlay->last_flip_req,
> -				true);
> +	ret = i915_wait_request(LP_RING(dev_priv), overlay->last_flip_req);
>  	if (ret)
>  		return ret;
>  
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 12d9bc7..13eaf6a 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1049,9 +1049,11 @@ static int intel_ring_wait_seqno(struct intel_ring_buffer *ring, u32 seqno)
>  	was_interruptible = dev_priv->mm.interruptible;
>  	dev_priv->mm.interruptible = false;
>  
> -	ret = i915_wait_request(ring, seqno, true);
> +	ret = i915_wait_request(ring, seqno);
>  
>  	dev_priv->mm.interruptible = was_interruptible;
> +	if (!ret)
> +		i915_gem_retire_requests_ring(ring);
>  
>  	return ret;
>  }
> -- 
> 1.7.10
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48