From: Daniel Vetter <daniel@ffwll.ch>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 2/6] drm/i915: Remove the defunct flushing list
Date: Fri, 13 Jul 2012 11:12:40 +0200 [thread overview]
Message-ID: <20120713091240.GC5721@phenom.ffwll.local> (raw)
In-Reply-To: <1342106019-17806-3-git-send-email-chris@chris-wilson.co.uk>
On Thu, Jul 12, 2012 at 04:13:35PM +0100, Chris Wilson wrote:
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
I'd like to keep the analysis of why things blow up on the BUG_ON(seqno ==
0) here in the commit message, i.e. the two patches that together caused
the regression (I think we need both the flushing list prep patch an
-ERESTART from intel_ring_begin). Plus the scenario of what exactly needs
to be submitted and interrupted that you've laid out on irc.
Although I have to admit that I still fail to understand how the
unconditional flush can't plug all holes, i.e. if you come up with a
recipe to blow things up with only that broken patch applied, I'd much
appreciate it ;-)
/me feels way to dense about all the complexity we have here
One thing popped out a bit while reading this patch again, comment below.
Thanks, Daniel
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 7 ----
> drivers/gpu/drm/i915/i915_drv.h | 19 +++--------
> drivers/gpu/drm/i915/i915_gem.c | 57 ++++++---------------------------
> drivers/gpu/drm/i915/i915_gem_evict.c | 20 ------------
> 4 files changed, 13 insertions(+), 90 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 359f6e8..d149890 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -44,7 +44,6 @@
>
> enum {
> ACTIVE_LIST,
> - FLUSHING_LIST,
> INACTIVE_LIST,
> PINNED_LIST,
> };
> @@ -177,10 +176,6 @@ static int i915_gem_object_list_info(struct seq_file *m, void *data)
> seq_printf(m, "Inactive:\n");
> head = &dev_priv->mm.inactive_list;
> break;
> - case FLUSHING_LIST:
> - seq_printf(m, "Flushing:\n");
> - head = &dev_priv->mm.flushing_list;
> - break;
> default:
> mutex_unlock(&dev->struct_mutex);
> return -EINVAL;
> @@ -238,7 +233,6 @@ static int i915_gem_object_info(struct seq_file *m, void* data)
>
> size = count = mappable_size = mappable_count = 0;
> count_objects(&dev_priv->mm.active_list, mm_list);
> - count_objects(&dev_priv->mm.flushing_list, mm_list);
> seq_printf(m, " %u [%u] active objects, %zu [%zu] bytes\n",
> count, mappable_count, size, mappable_size);
>
> @@ -2006,7 +2000,6 @@ static struct drm_info_list i915_debugfs_list[] = {
> {"i915_gem_gtt", i915_gem_gtt_info, 0},
> {"i915_gem_pinned", i915_gem_gtt_info, 0, (void *) PINNED_LIST},
> {"i915_gem_active", i915_gem_object_list_info, 0, (void *) ACTIVE_LIST},
> - {"i915_gem_flushing", i915_gem_object_list_info, 0, (void *) FLUSHING_LIST},
> {"i915_gem_inactive", i915_gem_object_list_info, 0, (void *) INACTIVE_LIST},
> {"i915_gem_pageflip", i915_gem_pageflip_info, 0},
> {"i915_gem_request", i915_gem_request_info, 0},
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 51e234e..38b95be 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -696,17 +696,6 @@ typedef struct drm_i915_private {
> struct list_head active_list;
>
> /**
> - * List of objects which are not in the ringbuffer but which
> - * still have a write_domain which needs to be flushed before
> - * unbinding.
> - *
> - * last_rendering_seqno is 0 while an object is in this list.
> - *
> - * A reference is held on the buffer while on this list.
> - */
> - struct list_head flushing_list;
> -
> - /**
> * LRU list of objects which are not in the ringbuffer and
> * are ready to unbind, but are still in the GTT.
> *
> @@ -873,7 +862,7 @@ struct drm_i915_gem_object {
> struct drm_mm_node *gtt_space;
> struct list_head gtt_list;
>
> - /** This object's place on the active/flushing/inactive lists */
> + /** This object's place on the active/inactive lists */
> struct list_head ring_list;
> struct list_head mm_list;
> /** This object's place on GPU write list */
> @@ -882,9 +871,9 @@ struct drm_i915_gem_object {
> struct list_head exec_list;
>
> /**
> - * This is set if the object is on the active or flushing lists
> - * (has pending rendering), and is not set if it's on inactive (ready
> - * to be unbound).
> + * This is set if the object is on the active lists (has pending
> + * rendering and so a non-zero seqno), and is not set if it i s on
> + * inactive (ready to be unbound) list.
> */
> unsigned int active:1;
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 875b745..f69d897 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1458,26 +1458,6 @@ i915_gem_object_move_to_active(struct drm_i915_gem_object *obj,
> }
>
> static void
> -i915_gem_object_move_off_active(struct drm_i915_gem_object *obj)
> -{
> - list_del_init(&obj->ring_list);
> - obj->last_rendering_seqno = 0;
> - obj->last_fenced_seqno = 0;
> -}
> -
> -static void
> -i915_gem_object_move_to_flushing(struct drm_i915_gem_object *obj)
> -{
> - struct drm_device *dev = obj->base.dev;
> - drm_i915_private_t *dev_priv = dev->dev_private;
> -
> - BUG_ON(!obj->active);
> - list_move_tail(&obj->mm_list, &dev_priv->mm.flushing_list);
> -
> - i915_gem_object_move_off_active(obj);
> -}
> -
> -static void
> i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
> {
> struct drm_device *dev = obj->base.dev;
> @@ -1487,13 +1467,18 @@ i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
>
> BUG_ON(!list_empty(&obj->gpu_write_list));
> BUG_ON(!obj->active);
> +
> + list_del_init(&obj->ring_list);
> obj->ring = NULL;
>
> - i915_gem_object_move_off_active(obj);
> + obj->last_rendering_seqno = 0;
> + obj->last_fenced_seqno = 0;
> obj->fenced_gpu_access = false;
>
> - obj->active = 0;
> + obj->base.write_domain &= ~I915_GEM_GPU_DOMAINS;
Clearing the write_domain here makes me rather uneasy ... I see that we
should plug gpu write domains leaking out of obj->active (at least until
we've ripped out all that gpu domain tracking complexity for good). But I
also fear that this papers over some issues - at least in the old code we
should never be able to see this. Maybe just add a comment to explain
what's going on?
> obj->pending_gpu_write = false;
> +
> + obj->active = 0;
> drm_gem_object_unreference(&obj->base);
>
> WARN_ON(i915_verify_lists(dev));
> @@ -1728,20 +1713,6 @@ void i915_gem_reset(struct drm_device *dev)
> for_each_ring(ring, dev_priv, i)
> i915_gem_reset_ring_lists(dev_priv, ring);
>
> - /* Remove anything from the flushing lists. The GPU cache is likely
> - * to be lost on reset along with the data, so simply move the
> - * lost bo to the inactive list.
> - */
> - while (!list_empty(&dev_priv->mm.flushing_list)) {
> - obj = list_first_entry(&dev_priv->mm.flushing_list,
> - struct drm_i915_gem_object,
> - mm_list);
> -
> - obj->base.write_domain = 0;
> - list_del_init(&obj->gpu_write_list);
> - i915_gem_object_move_to_inactive(obj);
> - }
> -
> /* Move everything out of the GPU domains to ensure we do any
> * necessary invalidation upon reuse.
> */
> @@ -1812,10 +1783,7 @@ i915_gem_retire_requests_ring(struct intel_ring_buffer *ring)
> if (!i915_seqno_passed(seqno, obj->last_rendering_seqno))
> break;
>
> - if (obj->base.write_domain != 0)
> - i915_gem_object_move_to_flushing(obj);
> - else
> - i915_gem_object_move_to_inactive(obj);
> + i915_gem_object_move_to_inactive(obj);
> }
>
> if (unlikely(ring->trace_irq_seqno &&
> @@ -3877,7 +3845,6 @@ i915_gem_entervt_ioctl(struct drm_device *dev, void *data,
> }
>
> BUG_ON(!list_empty(&dev_priv->mm.active_list));
> - BUG_ON(!list_empty(&dev_priv->mm.flushing_list));
> BUG_ON(!list_empty(&dev_priv->mm.inactive_list));
> mutex_unlock(&dev->struct_mutex);
>
> @@ -3935,7 +3902,6 @@ i915_gem_load(struct drm_device *dev)
> drm_i915_private_t *dev_priv = dev->dev_private;
>
> INIT_LIST_HEAD(&dev_priv->mm.active_list);
> - INIT_LIST_HEAD(&dev_priv->mm.flushing_list);
> INIT_LIST_HEAD(&dev_priv->mm.inactive_list);
> INIT_LIST_HEAD(&dev_priv->mm.fence_list);
> INIT_LIST_HEAD(&dev_priv->mm.gtt_list);
> @@ -4186,12 +4152,7 @@ static int
> i915_gpu_is_active(struct drm_device *dev)
> {
> drm_i915_private_t *dev_priv = dev->dev_private;
> - int lists_empty;
> -
> - lists_empty = list_empty(&dev_priv->mm.flushing_list) &&
> - list_empty(&dev_priv->mm.active_list);
> -
> - return !lists_empty;
> + return !list_empty(&dev_priv->mm.active_list);
> }
>
> static int
> diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
> index ae7c24e..f61af59 100644
> --- a/drivers/gpu/drm/i915/i915_gem_evict.c
> +++ b/drivers/gpu/drm/i915/i915_gem_evict.c
> @@ -92,23 +92,6 @@ i915_gem_evict_something(struct drm_device *dev, int min_size,
>
> /* Now merge in the soon-to-be-expired objects... */
> list_for_each_entry(obj, &dev_priv->mm.active_list, mm_list) {
> - /* Does the object require an outstanding flush? */
> - if (obj->base.write_domain)
> - continue;
> -
> - if (mark_free(obj, &unwind_list))
> - goto found;
> - }
> -
> - /* Finally add anything with a pending flush (in order of retirement) */
> - list_for_each_entry(obj, &dev_priv->mm.flushing_list, mm_list) {
> - if (mark_free(obj, &unwind_list))
> - goto found;
> - }
> - list_for_each_entry(obj, &dev_priv->mm.active_list, mm_list) {
> - if (!obj->base.write_domain)
> - continue;
> -
> if (mark_free(obj, &unwind_list))
> goto found;
> }
> @@ -171,7 +154,6 @@ i915_gem_evict_everything(struct drm_device *dev, bool purgeable_only)
> int ret;
>
> lists_empty = (list_empty(&dev_priv->mm.inactive_list) &&
> - list_empty(&dev_priv->mm.flushing_list) &&
> list_empty(&dev_priv->mm.active_list));
> if (lists_empty)
> return -ENOSPC;
> @@ -188,8 +170,6 @@ i915_gem_evict_everything(struct drm_device *dev, bool purgeable_only)
>
> i915_gem_retire_requests(dev);
>
> - BUG_ON(!list_empty(&dev_priv->mm.flushing_list));
> -
> /* Having flushed everything, unbind() should never raise an error */
> list_for_each_entry_safe(obj, next,
> &dev_priv->mm.inactive_list, mm_list) {
> --
> 1.7.10.4
>
> _______________________________________________
> 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-07-13 9:12 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-07-12 15:13 Kill the flushing list Chris Wilson
2012-07-12 15:13 ` [PATCH 1/6] drm/i915: Allow late allocation of request for i915_add_request() Chris Wilson
2012-07-12 17:43 ` Daniel Vetter
2012-07-12 17:56 ` Chris Wilson
2012-07-12 18:02 ` [PATCH] " Chris Wilson
2012-07-12 15:13 ` [PATCH 2/6] drm/i915: Remove the defunct flushing list Chris Wilson
2012-07-13 9:12 ` Daniel Vetter [this message]
2012-07-12 15:13 ` [PATCH 3/6] drm/i915: Remove the per-ring write list Chris Wilson
2012-07-12 19:37 ` Daniel Vetter
2012-07-12 20:07 ` Chris Wilson
2012-07-13 8:34 ` Daniel Vetter
2012-07-13 8:49 ` Chris Wilson
2012-07-13 8:53 ` Chris Wilson
2012-07-13 9:05 ` Daniel Vetter
2012-07-12 15:13 ` [PATCH 4/6] drm/i915: Remove explicit flush from i915_gem_object_flush_fence() Chris Wilson
2012-07-12 15:13 ` [PATCH 5/6] drm/i915: Remove the explicit flush of the GPU write domain Chris Wilson
2012-07-12 15:13 ` [PATCH 6/6] drm/i915: Split i915_gem_flush_ring() into seperate invalidate/flush funcs Chris Wilson
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=20120713091240.GC5721@phenom.ffwll.local \
--to=daniel@ffwll.ch \
--cc=chris@chris-wilson.co.uk \
--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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.