From: Daniel Vetter <daniel@ffwll.ch>
To: Ben Widawsky <ben@bwidawsk.net>
Cc: intel-gfx@lists.freedesktop.org,
Ben Widawsky <benjamin.widawsky@intel.com>
Subject: Re: [PATCH 01/10] drm/i915: remove do_retire from i915_wait_request
Date: Sat, 21 Apr 2012 19:17:11 +0200 [thread overview]
Message-ID: <20120421171711.GK5019@phenom.ffwll.local> (raw)
In-Reply-To: <1334971412-4826-2-git-send-email-ben@bwidawsk.net>
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
next prev parent reply other threads:[~2012-04-21 17:16 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-04-21 1:23 [PATCH 00/10] wait for BO with timeout Ben Widawsky
2012-04-21 1:23 ` [PATCH 01/10] drm/i915: remove do_retire from i915_wait_request Ben Widawsky
2012-04-21 17:17 ` Daniel Vetter [this message]
2012-04-21 17:27 ` Ben Widawsky
2012-04-21 17:36 ` Daniel Vetter
2012-04-21 1:23 ` [PATCH 02/10] drm/i915: move vbetool invoked ier stuff Ben Widawsky
2012-04-21 9:26 ` Chris Wilson
2012-04-21 1:23 ` [PATCH 03/10] drm/i915: kill waiting_seqno Ben Widawsky
2012-04-22 13:46 ` Chris Wilson
2012-04-22 17:47 ` Ben Widawsky
2012-04-21 1:23 ` [PATCH 04/10] drm/i915: drop polled waits from i915_wait_request Ben Widawsky
2012-04-21 9:29 ` Chris Wilson
2012-04-21 16:14 ` Ben Widawsky
2012-04-21 1:23 ` [PATCH 05/10] drm/i915: extract __wait_seqno " Ben Widawsky
2012-04-21 1:23 ` [PATCH 06/10] drm/i915: use __wait_seqno for ring throttle Ben Widawsky
2012-04-22 14:17 ` Chris Wilson
2012-04-21 1:23 ` [PATCH 07/10] drm/i915: timeout parameter for seqno wait Ben Widawsky
2012-04-22 12:52 ` Daniel Vetter
2012-04-21 1:23 ` [PATCH 08/10] drm/i915: real wait seqno with timeout Ben Widawsky
2012-04-21 1:23 ` [PATCH 09/10] drm/i915: wait render timeout ioctl Ben Widawsky
2012-04-21 9:41 ` Chris Wilson
2012-04-21 16:12 ` Ben Widawsky
2012-04-21 20:37 ` Ben Widawsky
2012-04-22 9:37 ` Chris Wilson
2012-04-22 9:48 ` Chris Wilson
2012-04-22 10:11 ` Daniel Vetter
2012-04-22 12:45 ` Daniel Vetter
2012-04-23 15:28 ` Ben Widawsky
2012-04-22 14:14 ` Chris Wilson
2012-04-21 1:23 ` [PATCH 10/10] drm/i915: s/i915_wait_reqest/i915_wait_seqno/g Ben Widawsky
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20120421171711.GK5019@phenom.ffwll.local \
--to=daniel@ffwll.ch \
--cc=ben@bwidawsk.net \
--cc=benjamin.widawsky@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox