[Intel-gfx] [PATCH 3/6] drm/i915: Only track live rings for retiring

Chris Wilson chris at chris-wilson.co.uk
Mon Apr 23 10:36:40 UTC 2018


Quoting Tvrtko Ursulin (2018-04-23 11:25:54)
> 
> On 23/04/2018 11:13, Chris Wilson wrote:
> > We don't need to track every ring for its lifetime as they are managed
> > by the contexts/engines. What we do want to track are the live rings so
> > that we can sporadically clean up requests if userspace falls behind. We
> > can simply restrict the gt->rings list to being only gt->live_rings.
> > 
> > Suggested-by: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_drv.h                  | 2 +-
> >   drivers/gpu/drm/i915/i915_gem.c                  | 3 ++-
> >   drivers/gpu/drm/i915/i915_request.c              | 6 +++++-
> >   drivers/gpu/drm/i915/i915_utils.h                | 6 ++++++
> >   drivers/gpu/drm/i915/intel_ringbuffer.c          | 4 ----
> >   drivers/gpu/drm/i915/intel_ringbuffer.h          | 2 +-
> >   drivers/gpu/drm/i915/selftests/mock_engine.c     | 4 ----
> >   drivers/gpu/drm/i915/selftests/mock_gem_device.c | 2 +-
> >   8 files changed, 16 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 73936be90aed..a7787c2cb53c 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2060,7 +2060,7 @@ struct drm_i915_private {
> >   
> >               struct i915_gem_timeline global_timeline;
> >               struct list_head timelines;
> > -             struct list_head rings;
> > +             struct list_head live_rings;
> >               u32 active_requests;
> >   
> >               /**
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 906e2395c245..0097a77fae8d 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -141,6 +141,7 @@ static u32 __i915_gem_park(struct drm_i915_private *i915)
> >   {
> >       lockdep_assert_held(&i915->drm.struct_mutex);
> >       GEM_BUG_ON(i915->gt.active_requests);
> > +     GEM_BUG_ON(!list_empty(&i915->gt.live_rings));
> >   
> >       if (!i915->gt.awake)
> >               return I915_EPOCH_INVALID;
> > @@ -5600,7 +5601,7 @@ int i915_gem_init_early(struct drm_i915_private *dev_priv)
> >               goto err_dependencies;
> >   
> >       mutex_lock(&dev_priv->drm.struct_mutex);
> > -     INIT_LIST_HEAD(&dev_priv->gt.rings);
> > +     INIT_LIST_HEAD(&dev_priv->gt.live_rings);
> >       INIT_LIST_HEAD(&dev_priv->gt.timelines);
> >       err = i915_gem_timeline_init__global(dev_priv);
> >       mutex_unlock(&dev_priv->drm.struct_mutex);
> > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> > index 0bf949ec9f1a..534b8d684cef 100644
> > --- a/drivers/gpu/drm/i915/i915_request.c
> > +++ b/drivers/gpu/drm/i915/i915_request.c
> > @@ -316,6 +316,7 @@ static void advance_ring(struct i915_request *request)
> >                * noops - they are safe to be replayed on a reset.
> >                */
> >               tail = READ_ONCE(request->tail);
> > +             list_del(&ring->live);
> >       } else {
> >               tail = request->postfix;
> >       }
> > @@ -1046,6 +1047,8 @@ void __i915_request_add(struct i915_request *request, bool flush_caches)
> >       i915_gem_active_set(&timeline->last_request, request);
> >   
> >       list_add_tail(&request->ring_link, &ring->request_list);
> > +     if (list_is_first(&request->ring_link, &ring->request_list))
> > +             list_add(&ring->live, &request->i915->gt.live_rings);
> 
> If you re-order the two list adds you could use list_empty and wouldn't 
> have to add list_is_first.

list_is_first tallies nicely with the list_is_last used before the
corresponding list_del.

> 
> >       request->emitted_jiffies = jiffies;
> >   
> >       /*
> > @@ -1375,7 +1378,8 @@ void i915_retire_requests(struct drm_i915_private *i915)
> >       if (!i915->gt.active_requests)
> >               return;
> >   
> > -     list_for_each_entry_safe(ring, next, &i915->gt.rings, link)
> > +     GEM_BUG_ON(list_empty(&i915->gt.live_rings));
> 
> Maybe blank line here since the assert is not logically associated with 
> the list but with the !i915.active_requests?

I was thinking list when I wrote it. It's small enough we can argue both
and both be right.

> 
> > +     list_for_each_entry_safe(ring, next, &i915->gt.live_rings, live)
> >               ring_retire_requests(ring);
> >   }
> >   
> > diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
> > index 0695717522ea..00165ad55fb3 100644
> > --- a/drivers/gpu/drm/i915/i915_utils.h
> > +++ b/drivers/gpu/drm/i915/i915_utils.h
> > @@ -120,6 +120,12 @@ static inline u64 ptr_to_u64(const void *ptr)
> >   
> >   #include <linux/list.h>
> >   
> > +static inline int list_is_first(const struct list_head *list,
> > +                             const struct list_head *head)
> 
> Return bool if you decide you prefer to keep list_is_first?

Copy'n'paste from list_is_last().

> 
> > +{
> > +     return head->next == list;
> > +}
> > +
> >   static inline void __list_del_many(struct list_head *head,
> >                                  struct list_head *first)
> >   {
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index 792a2ca95872..3453e7426f6b 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -1150,8 +1150,6 @@ intel_engine_create_ring(struct intel_engine_cs *engine, int size)
> >       }
> >       ring->vma = vma;
> >   
> > -     list_add(&ring->link, &engine->i915->gt.rings);
> > -
> >       return ring;
> >   }
> >   
> > @@ -1163,8 +1161,6 @@ intel_ring_free(struct intel_ring *ring)
> >       i915_vma_close(ring->vma);
> >       __i915_gem_object_release_unless_active(obj);
> >   
> > -     list_del(&ring->link);
> > -
> >       kfree(ring);
> >   }
> >   
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> > index d816f8dea245..fd5a6363ab1d 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> > @@ -129,7 +129,7 @@ struct intel_ring {
> >       void *vaddr;
> >   
> >       struct list_head request_list;
> > -     struct list_head link;
> > +     struct list_head live;
> 
> live_link?

live or active.

active_rings ties in with active_requests, so active_link here.
-Chris


More information about the Intel-gfx mailing list