On Wed, Jul 10, 2013 at 09:39:30AM -0700, Ben Widawsky wrote:
> On Tue, Jul 09, 2013 at 09:45:09AM +0200, Daniel Vetter wrote:
> > On Mon, Jul 08, 2013 at 11:08:42PM -0700, Ben Widawsky wrote:
> > > Probably need to squash whole thing, or just the inactive part, tbd...
> > >
> > > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> >
> > I agree that we need vma->active, but I'm not sold on removing
> > obj->active. Atm we have to use-cases for checking obj->active:
> > - In the evict/unbind code to check whether the gpu is still using this
> > specific mapping. This use-case nicely fits into checking vma->active.
> > - In the shrinker code and everywhere we want to do cpu access we only
> > care about whether the gpu is accessing the object, not at all through
> > which mapping precisely. There a vma-independant obj->active sounds much
> > saner.
> >
> > Note though that just keeping track of vma->active isn't too useful, since
> > if some other vma is keeping the object busy we'll still stall on that one
> > for eviction. So we'd need a vma->ring and vma->last_rendering_seqno, too.
> >
> > At that point I wonder a bit whether all this complexity is worth it ...
> >
> > I need to ponder this some more.
> > -Daniel
>
> I think eventually the complexity might prove worthwhile, it might not.
>
> In the meanwhile, I see vma->active as just a bookkeeping thing, and not
> really useful in determining what we actually care about. As you mention
> obj->active is really what we care about, and I used the getter
> i915_gem_object_is_active() as a way to avoid the confusion of having
> two active members.
>
> I think we're in the same state of mind on this, and I've picked what I
> consider to be a less offensive solution which is easy to clean up
> later.
>
> Let me know when you make a decision.
Since you don't seem to be too keen on defending it for now I'd say let's
keep it in obj->active. This way obj->active still reflects accurately
whether the object is active on a ring or not.
That leaves us with updating the vm->active list. I'm leaning somewhat
towards simply merging the inactive and active vm list since in the only
place we care about those list (eviction) we treat them essentially as one
big lru. That would cut down on complexity in retire_request to keep all
the vmas on the rigth per-vm active/inactive list, too.
Optional cleanup after-the-fact imo (or upfront if it makes request
retiring much simpler). Whatever suits you.
-Daniel
>
> >
> > > ---
> > > drivers/gpu/drm/i915/i915_drv.h | 14 ++++++------
> > > drivers/gpu/drm/i915/i915_gem.c | 47 ++++++++++++++++++++++++-----------------
> > > 2 files changed, 35 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 38d07f2..e6694ae 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;
> > >
> > > @@ -1250,13 +1257,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
> > > */
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > index c2ecb78..b87073b 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > @@ -137,7 +137,13 @@ int i915_mutex_lock_interruptible(struct drm_device *dev)
> > > /* NB: Not the same as !i915_gem_object_is_inactive */
> > > bool i915_gem_object_is_active(struct drm_i915_gem_object *obj)
> > > {
> > > - return obj->active;
> > > + struct i915_vma *vma;
> > > +
> > > + list_for_each_entry(vma, &obj->vma_list, vma_link)
> > > + if (vma->active)
> > > + return true;
> > > +
> > > + return false;
> > > }
> > >
> > > static inline bool
> > > @@ -1899,14 +1905,14 @@ i915_gem_object_move_to_active(struct drm_i915_gem_object *obj,
> > > BUG_ON(ring == NULL);
> > > 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 (!i915_gem_object_is_active(obj)) {
> > > + 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);
> > >
> > > @@ -1927,16 +1933,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(!i915_gem_object_is_active(obj));
> > >
> > > - 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);
> > For paranoia we might want to track the vm used to run a batch in it
> > request struct, then we
> >
> > > + 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;
> > > @@ -1948,8 +1961,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));
> > > }
> > > @@ -2272,15 +2285,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);
> > > }
> > > }
> > >
> > > @@ -2356,8 +2367,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,
> > > @@ -2367,8 +2376,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 &&
> > > --
> > > 1.8.3.2
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx at 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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
