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 at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Mail: daniel at ffwll.ch
Mobile: +41 (0)79 365 57 48
