From: Ben Widawsky <ben@bwidawsk.net>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Intel GFX <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 04/12] drm/i915: Track active by VMA instead of object
Date: Fri, 26 Jul 2013 14:48:50 -0700 [thread overview]
Message-ID: <20130726214849.GD20047@bwidawsk.net> (raw)
In-Reply-To: <20130723164809.GJ5939@phenom.ffwll.local>
On Tue, Jul 23, 2013 at 06:48:09PM +0200, Daniel Vetter wrote:
> On Sun, Jul 21, 2013 at 07:08:11PM -0700, Ben Widawsky wrote:
> > Even though we want to be able to track active by VMA, the rest of the
> > code is still using objects for most internal APIs. To solve this,
> > create an object_is_active() function to help us in converting over to
> > VMA usage.
> >
> > Because we intend to keep around some functions that care about objects,
> > and not VMAs, having this function around will be useful even as we
> > begin to use VMAs more in function arguments.
> >
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>
> Still not really convinced. For access synchronization we don't care
> through which vm a bo is still access, only how (read/write) and when was
> the last access (ring + seqno).
>
> Note that this means that the per-vm lru doesn't really need an
> active/inactive split anymore, for evict_something we only care about the
> ordering and not whether a bo is active or not. unbind() will care but I'm
> not sure that the "same bo in multiple address spaces needs to be evicted"
> use-case is something we even should care about.
>
> So imo this commit needs a good justificatio for _why_ we want to track
> active per-vma. Atm I don't see a use-case, but I see complexity.
> -Daniel
I'm fine with deferring this until needed.
>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 15 +++----
> > drivers/gpu/drm/i915/i915_gem.c | 64 ++++++++++++++++++------------
> > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
> > 3 files changed, 48 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index f809204..bdce9c1 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -541,6 +541,13 @@ struct i915_vma {
> > struct drm_i915_gem_object *obj;
> > struct i915_address_space *vm;
> >
> > + /**
> > + * 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;
> > +
> > /** This object's place on the active/inactive lists */
> > struct list_head mm_list;
> >
> > @@ -1266,13 +1273,6 @@ struct drm_i915_gem_object {
> > struct list_head exec_list;
> >
> > /**
> > - * 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;
> > -
> > - /**
> > * This is set if the object has been written to since last bound
> > * to the GTT
> > */
> > @@ -1726,6 +1726,7 @@ static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj)
> > int __must_check i915_mutex_lock_interruptible(struct drm_device *dev);
> > int i915_gem_object_sync(struct drm_i915_gem_object *obj,
> > struct intel_ring_buffer *to);
> > +bool i915_gem_object_is_active(struct drm_i915_gem_object *obj);
> > void i915_gem_object_move_to_active(struct drm_i915_gem_object *obj,
> > struct i915_address_space *vm,
> > struct intel_ring_buffer *ring);
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 6bdf89d..9ea6424 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -119,10 +119,22 @@ int i915_mutex_lock_interruptible(struct drm_device *dev)
> > return 0;
> > }
> >
> > +/* NB: Not the same as !i915_gem_object_is_inactive */
> > +bool i915_gem_object_is_active(struct drm_i915_gem_object *obj)
> > +{
> > + struct i915_vma *vma;
> > +
> > + list_for_each_entry(vma, &obj->vma_list, vma_link)
> > + if (vma->active)
> > + return true;
> > +
> > + return false;
> > +}
> > +
> > static inline bool
> > i915_gem_object_is_inactive(struct drm_i915_gem_object *obj)
> > {
> > - return i915_gem_obj_bound_any(obj) && !obj->active;
> > + return i915_gem_obj_bound_any(obj) && !i915_gem_object_is_active(obj);
> > }
> >
> > int
> > @@ -1883,14 +1895,14 @@ i915_gem_object_move_to_active(struct drm_i915_gem_object *obj,
> > }
> > obj->ring = ring;
> >
> > + /* Move from whatever list we were on to the tail of execution. */
> > + vma = i915_gem_obj_to_vma(obj, vm);
> > /* Add a reference if we're newly entering the active list. */
> > - if (!obj->active) {
> > + if (!vma->active) {
> > drm_gem_object_reference(&obj->base);
> > - obj->active = 1;
> > + vma->active = 1;
> > }
> >
> > - /* Move from whatever list we were on to the tail of execution. */
> > - vma = i915_gem_obj_to_vma(obj, vm);
> > list_move_tail(&vma->mm_list, &vm->active_list);
> > list_move_tail(&obj->ring_list, &ring->active_list);
> >
> > @@ -1911,16 +1923,23 @@ i915_gem_object_move_to_active(struct drm_i915_gem_object *obj,
> > }
> >
> > static void
> > -i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj,
> > - struct i915_address_space *vm)
> > +i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
> > {
> > + struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
> > + struct i915_address_space *vm;
> > struct i915_vma *vma;
> > + int i = 0;
> >
> > BUG_ON(obj->base.write_domain & ~I915_GEM_GPU_DOMAINS);
> > - BUG_ON(!obj->active);
> >
> > - vma = i915_gem_obj_to_vma(obj, vm);
> > - list_move_tail(&vma->mm_list, &vm->inactive_list);
> > + list_for_each_entry(vm, &dev_priv->vm_list, global_link) {
> > + vma = i915_gem_obj_to_vma(obj, vm);
> > + if (!vma || !vma->active)
> > + continue;
> > + list_move_tail(&vma->mm_list, &vm->inactive_list);
> > + vma->active = 0;
> > + i++;
> > + }
> >
> > list_del_init(&obj->ring_list);
> > obj->ring = NULL;
> > @@ -1932,8 +1951,8 @@ i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj,
> > obj->last_fenced_seqno = 0;
> > obj->fenced_gpu_access = false;
> >
> > - obj->active = 0;
> > - drm_gem_object_unreference(&obj->base);
> > + while (i--)
> > + drm_gem_object_unreference(&obj->base);
> >
> > WARN_ON(i915_verify_lists(dev));
> > }
> > @@ -2254,15 +2273,13 @@ static void i915_gem_reset_ring_lists(struct drm_i915_private *dev_priv,
> > }
> >
> > while (!list_empty(&ring->active_list)) {
> > - struct i915_address_space *vm;
> > struct drm_i915_gem_object *obj;
> >
> > obj = list_first_entry(&ring->active_list,
> > struct drm_i915_gem_object,
> > ring_list);
> >
> > - list_for_each_entry(vm, &dev_priv->vm_list, global_link)
> > - i915_gem_object_move_to_inactive(obj, vm);
> > + i915_gem_object_move_to_inactive(obj);
> > }
> > }
> >
> > @@ -2348,8 +2365,6 @@ i915_gem_retire_requests_ring(struct intel_ring_buffer *ring)
> > * by the ringbuffer to the flushing/inactive lists as appropriate.
> > */
> > while (!list_empty(&ring->active_list)) {
> > - struct drm_i915_private *dev_priv = ring->dev->dev_private;
> > - struct i915_address_space *vm;
> > struct drm_i915_gem_object *obj;
> >
> > obj = list_first_entry(&ring->active_list,
> > @@ -2359,8 +2374,8 @@ i915_gem_retire_requests_ring(struct intel_ring_buffer *ring)
> > if (!i915_seqno_passed(seqno, obj->last_read_seqno))
> > break;
> >
> > - list_for_each_entry(vm, &dev_priv->vm_list, global_link)
> > - i915_gem_object_move_to_inactive(obj, vm);
> > + BUG_ON(!i915_gem_object_is_active(obj));
> > + i915_gem_object_move_to_inactive(obj);
> > }
> >
> > if (unlikely(ring->trace_irq_seqno &&
> > @@ -2435,7 +2450,7 @@ i915_gem_object_flush_active(struct drm_i915_gem_object *obj)
> > {
> > int ret;
> >
> > - if (obj->active) {
> > + if (i915_gem_object_is_active(obj)) {
> > ret = i915_gem_check_olr(obj->ring, obj->last_read_seqno);
> > if (ret)
> > return ret;
> > @@ -2500,7 +2515,7 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
> > if (ret)
> > goto out;
> >
> > - if (obj->active) {
> > + if (i915_gem_object_is_active(obj)) {
> > seqno = obj->last_read_seqno;
> > ring = obj->ring;
> > }
> > @@ -3850,7 +3865,7 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
> > */
> > ret = i915_gem_object_flush_active(obj);
> >
> > - args->busy = obj->active;
> > + args->busy = i915_gem_object_is_active(obj);
> > if (obj->ring) {
> > BUILD_BUG_ON(I915_NUM_RINGS > 16);
> > args->busy |= intel_ring_flag(obj->ring) << 16;
> > @@ -4716,13 +4731,12 @@ i915_gem_inactive_shrink(struct shrinker *shrinker, struct shrink_control *sc)
> > cnt += obj->base.size >> PAGE_SHIFT;
> >
> > list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
> > - if (obj->active)
> > + if (i915_gem_object_is_active(obj))
> > continue;
> >
> > i915_gem_object_flush_gtt_write_domain(obj);
> > i915_gem_object_flush_cpu_write_domain(obj);
> > - /* FIXME: Can't assume global gtt */
> > - i915_gem_object_move_to_inactive(obj, &dev_priv->gtt.base);
> > + i915_gem_object_move_to_inactive(obj);
> >
> > if (obj->pin_count == 0 && obj->pages_pin_count == 0)
> > cnt += obj->base.size >> PAGE_SHIFT;
> > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > index 819d8d8..8d2643b 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -251,7 +251,7 @@ i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj,
> > }
> >
> > /* We can't wait for rendering with pagefaults disabled */
> > - if (obj->active && in_atomic())
> > + if (i915_gem_object_is_active(obj) && in_atomic())
> > return -EFAULT;
> >
> > reloc->delta += target_offset;
> > --
> > 1.8.3.3
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
Ben Widawsky, Intel Open Source Technology Center
next prev parent reply other threads:[~2013-07-26 21:48 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-07-22 2:08 [PATCH 00/12] Completion of i915 VMAs Ben Widawsky
2013-07-22 2:08 ` [PATCH 01/12] drm/i915: plumb VM into object operations Ben Widawsky
2013-07-23 16:37 ` Daniel Vetter
2013-07-26 9:51 ` Maintainer-review fluff (was: Re: [PATCH 01/12] drm/i915: plumb VM into object operations) Daniel Vetter
2013-07-26 16:59 ` Jesse Barnes
2013-07-26 17:08 ` Chris Wilson
2013-07-26 17:12 ` Jesse Barnes
2013-08-04 20:31 ` Daniel Vetter
2013-07-26 17:40 ` Daniel Vetter
2013-07-26 20:15 ` Ben Widawsky
2013-07-26 20:43 ` Daniel Vetter
2013-07-26 23:13 ` Dave Airlie
2013-07-27 0:05 ` Ben Widawsky
2013-07-27 8:52 ` Dave Airlie
2013-08-04 19:55 ` Daniel Vetter
2013-07-29 22:35 ` Jesse Barnes
2013-07-29 23:50 ` Dave Airlie
2013-08-04 20:17 ` Daniel Vetter
2013-08-05 21:33 ` Jesse Barnes
2013-08-05 22:19 ` Daniel Vetter
2013-08-05 23:34 ` Jesse Barnes
2013-08-06 6:29 ` Daniel Vetter
2013-08-06 14:50 ` Paulo Zanoni
2013-08-06 17:06 ` Daniel Vetter
2013-08-06 23:28 ` Dave Airlie
2013-07-22 2:08 ` [PATCH 02/12] drm/i915: Fix up map and fenceable for VMA Ben Widawsky
2013-07-23 16:42 ` Daniel Vetter
2013-07-23 18:14 ` Ben Widawsky
2013-07-22 2:08 ` [PATCH 03/12] drm/i915: Update error capture for VMs Ben Widawsky
2013-07-22 2:08 ` [PATCH 04/12] drm/i915: Track active by VMA instead of object Ben Widawsky
2013-07-23 16:48 ` Daniel Vetter
2013-07-26 21:48 ` Ben Widawsky [this message]
2013-07-22 2:08 ` [PATCH 05/12] drm/i915: Add map/unmap object functions to VM Ben Widawsky
2013-07-22 2:08 ` [PATCH 06/12] drm/i915: Use the new vm [un]bind functions Ben Widawsky
2013-07-23 16:54 ` Daniel Vetter
2013-07-26 21:48 ` Ben Widawsky
2013-07-26 21:56 ` Daniel Vetter
2013-07-22 2:08 ` [PATCH 07/12] drm/i915: eliminate vm->insert_entries() Ben Widawsky
2013-07-23 16:57 ` Daniel Vetter
2013-07-22 2:08 ` [PATCH 08/12] drm/i915: Add vma to list at creation Ben Widawsky
2013-07-22 2:08 ` [PATCH 09/12] drm/i915: create vmas at execbuf Ben Widawsky
2013-07-22 13:32 ` Chris Wilson
2013-07-22 2:08 ` [PATCH 10/12] drm/i915: Convert execbuf code to use vmas Ben Widawsky
2013-07-22 2:08 ` [PATCH 11/12] drm/i915: Convert object coloring to VMA Ben Widawsky
2013-07-23 17:07 ` Daniel Vetter
2013-07-22 2:08 ` [PATCH 12/12] drm/i915: Convert active API " Ben Widawsky
2013-07-22 10:42 ` [PATCH 00/12] Completion of i915 VMAs Chris Wilson
2013-07-22 16:35 ` 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=20130726214849.GD20047@bwidawsk.net \
--to=ben@bwidawsk.net \
--cc=daniel@ffwll.ch \
--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