| Message ID | 20180423101400.27418-3-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@linux.intel.com> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@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. > 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? > + 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? > +{ > + 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? > > u32 head; > u32 tail; > diff --git a/drivers/gpu/drm/i915/selftests/mock_engine.c b/drivers/gpu/drm/i915/selftests/mock_engine.c > index a0bc324edadd..74a88913623f 100644 > --- a/drivers/gpu/drm/i915/selftests/mock_engine.c > +++ b/drivers/gpu/drm/i915/selftests/mock_engine.c > @@ -140,15 +140,11 @@ static struct intel_ring *mock_ring(struct intel_engine_cs *engine) > INIT_LIST_HEAD(&ring->request_list); > intel_ring_update_space(ring); > > - list_add(&ring->link, &engine->i915->gt.rings); > - > return ring; > } > > static void mock_ring_free(struct intel_ring *ring) > { > - list_del(&ring->link); > - > kfree(ring); > } > > diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c > index ac4bacf8b5b9..9335b09d8b04 100644 > --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c > +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c > @@ -225,7 +225,7 @@ struct drm_i915_private *mock_gem_device(void) > goto err_dependencies; > > mutex_lock(&i915->drm.struct_mutex); > - INIT_LIST_HEAD(&i915->gt.rings); > + INIT_LIST_HEAD(&i915->gt.live_rings); > INIT_LIST_HEAD(&i915->gt.timelines); > err = i915_gem_timeline_init__global(i915); > if (err) { > Regards, Tvrtko
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@linux.intel.com> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@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
On 23/04/2018 11:36, Chris Wilson wrote: > 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@linux.intel.com> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@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. Yes but to me that's minor, basically immaterial as argument whether or not to add our own list helper. >> >>> 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. Hm obviosuly it is not an error to call i915_retire_requests with nothing active (early return). So I even briefly wanted to suggest to make it 100% explicit and have the assert at the top of the function as: GEM_BUG_ON(!!i915->gt.active_requests ^ !!list_empty(..)); Unless I messed it up, the idea is to check those two are always in the same state. > >> >>> + 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. Fine by me. Regards, Tvrtko
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); 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)); + 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 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; u32 head; u32 tail; diff --git a/drivers/gpu/drm/i915/selftests/mock_engine.c b/drivers/gpu/drm/i915/selftests/mock_engine.c index a0bc324edadd..74a88913623f 100644 --- a/drivers/gpu/drm/i915/selftests/mock_engine.c +++ b/drivers/gpu/drm/i915/selftests/mock_engine.c @@ -140,15 +140,11 @@ static struct intel_ring *mock_ring(struct intel_engine_cs *engine) INIT_LIST_HEAD(&ring->request_list); intel_ring_update_space(ring); - list_add(&ring->link, &engine->i915->gt.rings); - return ring; } static void mock_ring_free(struct intel_ring *ring) { - list_del(&ring->link); - kfree(ring); } diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c index ac4bacf8b5b9..9335b09d8b04 100644 --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c @@ -225,7 +225,7 @@ struct drm_i915_private *mock_gem_device(void) goto err_dependencies; mutex_lock(&i915->drm.struct_mutex); - INIT_LIST_HEAD(&i915->gt.rings); + INIT_LIST_HEAD(&i915->gt.live_rings); INIT_LIST_HEAD(&i915->gt.timelines); err = i915_gem_timeline_init__global(i915); if (err) {
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@linux.intel.com> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@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(-)