| Message ID | 1452503961-14837-75-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On 11/01/16 09:17, Chris Wilson wrote: > With the introduction of requests, we amplified the number of atomic > refcounted objects we use and update every execbuffer; from none to > several references, and a set of references that need to be changed. We > also introduced interesting side-effects in the order of retiring > requests and objects. > > Instead of independently tracking the last request for an object, track > the active objects for each request. The object will reside in the > buffer list of its most recent active request and so we reduce the kref > interchange to a list_move. Now retirements are entirely driven by the > request, dramatically simplifying activity tracking on the object > themselves, and removing the ambiguity between retiring objects and > retiring requests. > > All told, less code, simpler and faster, and more extensible. I've looked in this in detail before holidays and unfortunately a lot if if evaporated from my head since. I remember I thought the idea was good and really simplifies things. But it is also difficult to apply the subset of patches to look at the resulting code in more detail. So would it be possible to extract and rebase relevant patches? I think that would be from 73 to 76. (Together with the renaming we agreed already. And those trivial renames of list/link already have r-b's.) That would be step one, rework of active tracking. Then the next step could be adding VMA tracking, patches 81 to 87 I think. Regards, Tvrtko > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/Makefile | 1 - > drivers/gpu/drm/i915/i915_drv.h | 10 -- > drivers/gpu/drm/i915/i915_gem.c | 160 ++++++++------------------------ > drivers/gpu/drm/i915/i915_gem_debug.c | 70 -------------- > drivers/gpu/drm/i915/i915_gem_fence.c | 10 +- > drivers/gpu/drm/i915/i915_gem_request.c | 44 +++++++-- > drivers/gpu/drm/i915/i915_gem_request.h | 16 +++- > drivers/gpu/drm/i915/intel_lrc.c | 1 - > drivers/gpu/drm/i915/intel_ringbuffer.c | 1 - > drivers/gpu/drm/i915/intel_ringbuffer.h | 12 --- > 10 files changed, 89 insertions(+), 236 deletions(-) > delete mode 100644 drivers/gpu/drm/i915/i915_gem_debug.c > > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile > index b0a83215db80..79d657f29241 100644 > --- a/drivers/gpu/drm/i915/Makefile > +++ b/drivers/gpu/drm/i915/Makefile > @@ -23,7 +23,6 @@ i915-$(CONFIG_DEBUG_FS) += i915_debugfs.o > i915-y += i915_cmd_parser.o \ > i915_gem_batch_pool.o \ > i915_gem_context.o \ > - i915_gem_debug.o \ > i915_gem_dmabuf.o \ > i915_gem_evict.o \ > i915_gem_execbuffer.o \ > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index c577f86d94f8..c9c1a5cdc1e5 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -435,8 +435,6 @@ void intel_link_compute_m_n(int bpp, int nlanes, > #define DRIVER_MINOR 6 > #define DRIVER_PATCHLEVEL 0 > > -#define WATCH_LISTS 0 > - > struct opregion_header; > struct opregion_acpi; > struct opregion_swsci; > @@ -2024,7 +2022,6 @@ struct drm_i915_gem_object { > struct drm_mm_node *stolen; > struct list_head global_list; > > - struct list_head ring_list[I915_NUM_RINGS]; > /** Used in execbuf to temporarily hold a ref */ > struct list_head obj_exec_link; > > @@ -3068,13 +3065,6 @@ static inline bool i915_gem_object_needs_bit17_swizzle(struct drm_i915_gem_objec > obj->tiling_mode != I915_TILING_NONE; > } > > -/* i915_gem_debug.c */ > -#if WATCH_LISTS > -int i915_verify_lists(struct drm_device *dev); > -#else > -#define i915_verify_lists(dev) 0 > -#endif > - > /* i915_debugfs.c */ > int i915_debugfs_init(struct drm_minor *minor); > void i915_debugfs_cleanup(struct drm_minor *minor); > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index f314b3ea2726..4eef13ebdaf3 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -40,10 +40,6 @@ > > static void i915_gem_object_flush_gtt_write_domain(struct drm_i915_gem_object *obj); > static void i915_gem_object_flush_cpu_write_domain(struct drm_i915_gem_object *obj); > -static void > -i915_gem_object_retire__write(struct drm_i915_gem_object *obj); > -static void > -i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int ring); > > static bool cpu_cache_is_coherent(struct drm_device *dev, > enum i915_cache_level level) > @@ -117,7 +113,6 @@ int i915_mutex_lock_interruptible(struct drm_device *dev) > if (ret) > return ret; > > - WARN_ON(i915_verify_lists(dev)); > return 0; > } > > @@ -1117,27 +1112,14 @@ i915_gem_object_wait_rendering(struct drm_i915_gem_object *obj, > return 0; > > if (readonly) { > - if (obj->last_write.request != NULL) { > - ret = i915_wait_request(obj->last_write.request); > - if (ret) > - return ret; > - > - i = obj->last_write.request->engine->id; > - if (obj->last_read[i].request == obj->last_write.request) > - i915_gem_object_retire__read(obj, i); > - else > - i915_gem_object_retire__write(obj); > - } > + ret = i915_wait_request(obj->last_write.request); > + if (ret) > + return ret; > } else { > for (i = 0; i < I915_NUM_RINGS; i++) { > - if (obj->last_read[i].request == NULL) > - continue; > - > ret = i915_wait_request(obj->last_read[i].request); > if (ret) > return ret; > - > - i915_gem_object_retire__read(obj, i); > } > GEM_BUG_ON(obj->active); > } > @@ -1145,20 +1127,6 @@ i915_gem_object_wait_rendering(struct drm_i915_gem_object *obj, > return 0; > } > > -static void > -i915_gem_object_retire_request(struct drm_i915_gem_object *obj, > - struct drm_i915_gem_request *req) > -{ > - int ring = req->engine->id; > - > - if (obj->last_read[ring].request == req) > - i915_gem_object_retire__read(obj, ring); > - else if (obj->last_write.request == req) > - i915_gem_object_retire__write(obj); > - > - i915_gem_request_retire_upto(req); > -} > - > /* A nonblocking variant of the above wait. This is a highly dangerous routine > * as the object state may change during this call. > */ > @@ -1206,7 +1174,7 @@ i915_gem_object_wait_rendering__nonblocking(struct drm_i915_gem_object *obj, > > for (i = 0; i < n; i++) { > if (ret == 0) > - i915_gem_object_retire_request(obj, requests[i]); > + i915_gem_request_retire_upto(requests[i]); > i915_gem_request_put(requests[i]); > } > > @@ -2069,35 +2037,37 @@ void i915_vma_move_to_active(struct i915_vma *vma, > drm_gem_object_reference(&obj->base); > obj->active |= intel_engine_flag(engine); > > - list_move_tail(&obj->ring_list[engine->id], &engine->active_list); > i915_gem_request_mark_active(req, &obj->last_read[engine->id]); > - > list_move_tail(&vma->mm_list, &vma->vm->active_list); > } > > static void > -i915_gem_object_retire__write(struct drm_i915_gem_object *obj) > +i915_gem_object_retire__fence(struct i915_gem_active *active, > + struct drm_i915_gem_request *req) > { > - GEM_BUG_ON(obj->last_write.request == NULL); > - GEM_BUG_ON(!(obj->active & intel_engine_flag(obj->last_write.request->engine))); > +} > > - i915_gem_request_assign(&obj->last_write.request, NULL); > - intel_fb_obj_flush(obj, true, ORIGIN_CS); > +static void > +i915_gem_object_retire__write(struct i915_gem_active *active, > + struct drm_i915_gem_request *request) > +{ > + intel_fb_obj_flush(container_of(active, > + struct drm_i915_gem_object, > + last_write), > + true, > + ORIGIN_CS); > } > > static void > -i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int ring) > +i915_gem_object_retire__read(struct i915_gem_active *active, > + struct drm_i915_gem_request *request) > { > + int ring = request->engine->id; > + struct drm_i915_gem_object *obj = > + container_of(active, struct drm_i915_gem_object, last_read[ring]); > struct i915_vma *vma; > > - GEM_BUG_ON(obj->last_read[ring].request == NULL); > - GEM_BUG_ON(!(obj->active & (1 << ring))); > - > - list_del_init(&obj->ring_list[ring]); > - i915_gem_request_assign(&obj->last_read[ring].request, NULL); > - > - if (obj->last_write.request && obj->last_write.request->engine->id == ring) > - i915_gem_object_retire__write(obj); > + GEM_BUG_ON((obj->active & (1 << ring)) == 0); > > obj->active &= ~(1 << ring); > if (obj->active) > @@ -2107,15 +2077,13 @@ i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int ring) > * so that we don't steal from recently used but inactive objects > * (unless we are forced to ofc!) > */ > - list_move_tail(&obj->global_list, > - &to_i915(obj->base.dev)->mm.bound_list); > + list_move_tail(&obj->global_list, &request->i915->mm.bound_list); > > list_for_each_entry(vma, &obj->vma_list, vma_link) { > if (!list_empty(&vma->mm_list)) > list_move_tail(&vma->mm_list, &vma->vm->inactive_list); > } > > - i915_gem_request_assign(&obj->last_fence.request, NULL); > drm_gem_object_unreference(&obj->base); > } > > @@ -2216,16 +2184,6 @@ static void i915_gem_reset_ring_cleanup(struct intel_engine_cs *engine) > { > struct intel_ring *ring; > > - while (!list_empty(&engine->active_list)) { > - struct drm_i915_gem_object *obj; > - > - obj = list_first_entry(&engine->active_list, > - struct drm_i915_gem_object, > - ring_list[engine->id]); > - > - i915_gem_object_retire__read(obj, engine->id); > - } > - > /* > * Clear the execlists queue up before freeing the requests, as those > * are the ones that keep the context and ringbuffer backing objects > @@ -2295,8 +2253,6 @@ void i915_gem_reset(struct drm_device *dev) > i915_gem_context_reset(dev); > > i915_gem_restore_fences(dev); > - > - WARN_ON(i915_verify_lists(dev)); > } > > /** > @@ -2305,13 +2261,6 @@ void i915_gem_reset(struct drm_device *dev) > void > i915_gem_retire_requests_ring(struct intel_engine_cs *ring) > { > - WARN_ON(i915_verify_lists(ring->dev)); > - > - /* Retire requests first as we use it above for the early return. > - * If we retire requests last, we may use a later seqno and so clear > - * the requests lists without clearing the active list, leading to > - * confusion. > - */ > while (!list_empty(&ring->request_list)) { > struct drm_i915_gem_request *request; > > @@ -2324,25 +2273,6 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring) > > i915_gem_request_retire_upto(request); > } > - > - /* Move any buffers on the active list that are no longer referenced > - * by the ringbuffer to the flushing/inactive lists as appropriate, > - * before we free the context associated with the requests. > - */ > - while (!list_empty(&ring->active_list)) { > - struct drm_i915_gem_object *obj; > - > - obj = list_first_entry(&ring->active_list, > - struct drm_i915_gem_object, > - ring_list[ring->id]); > - > - if (!list_empty(&obj->last_read[ring->id].request->link)) > - break; > - > - i915_gem_object_retire__read(obj, ring->id); > - } > - > - WARN_ON(i915_verify_lists(ring->dev)); > } > > void > @@ -2434,13 +2364,13 @@ out: > * write domains, emitting any outstanding lazy request and retiring and > * completed requests. > */ > -static int > +static void > i915_gem_object_flush_active(struct drm_i915_gem_object *obj) > { > int i; > > if (!obj->active) > - return 0; > + return; > > for (i = 0; i < I915_NUM_RINGS; i++) { > struct drm_i915_gem_request *req; > @@ -2449,17 +2379,9 @@ i915_gem_object_flush_active(struct drm_i915_gem_object *obj) > if (req == NULL) > continue; > > - if (list_empty(&req->link)) > - goto retire; > - > - if (i915_gem_request_completed(req)) { > + if (i915_gem_request_completed(req)) > i915_gem_request_retire_upto(req); > -retire: > - i915_gem_object_retire__read(obj, i); > - } > } > - > - return 0; > } > > /** > @@ -2507,10 +2429,7 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file) > } > > /* Need to make sure the object gets inactive eventually. */ > - ret = i915_gem_object_flush_active(obj); > - if (ret) > - goto out; > - > + i915_gem_object_flush_active(obj); > if (!obj->active) > goto out; > > @@ -2522,8 +2441,6 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file) > goto out; > } > > - drm_gem_object_unreference(&obj->base); > - > for (i = 0; i < I915_NUM_RINGS; i++) { > if (obj->last_read[i].request == NULL) > continue; > @@ -2531,6 +2448,8 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file) > req[n++] = i915_gem_request_get(obj->last_read[i].request); > } > > +out: > + drm_gem_object_unreference(&obj->base); > mutex_unlock(&dev->struct_mutex); > > for (i = 0; i < n; i++) { > @@ -2541,11 +2460,6 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file) > i915_gem_request_put(req[i]); > } > return ret; > - > -out: > - drm_gem_object_unreference(&obj->base); > - mutex_unlock(&dev->struct_mutex); > - return ret; > } > > static int > @@ -2569,7 +2483,7 @@ __i915_gem_object_sync(struct drm_i915_gem_object *obj, > if (ret) > return ret; > > - i915_gem_object_retire_request(obj, from); > + i915_gem_request_retire_upto(from); > } else { > int idx = intel_engine_sync_index(from->engine, to->engine); > if (from->fence.seqno <= from->engine->semaphore.sync_seqno[idx]) > @@ -2760,7 +2674,6 @@ int i915_gpu_idle(struct drm_device *dev) > return ret; > } > > - WARN_ON(i915_verify_lists(dev)); > return 0; > } > > @@ -3689,16 +3602,13 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, > * become non-busy without any further actions, therefore emit any > * necessary flushes here. > */ > - ret = i915_gem_object_flush_active(obj); > - if (ret) > - goto unref; > + i915_gem_object_flush_active(obj); > > BUILD_BUG_ON(I915_NUM_RINGS > 16); > args->busy = obj->active << 16; > if (obj->last_write.request) > args->busy |= obj->last_write.request->engine->id; > > -unref: > drm_gem_object_unreference(&obj->base); > unlock: > mutex_unlock(&dev->struct_mutex); > @@ -3776,7 +3686,12 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj, > > INIT_LIST_HEAD(&obj->global_list); > for (i = 0; i < I915_NUM_RINGS; i++) > - INIT_LIST_HEAD(&obj->ring_list[i]); > + init_request_active(&obj->last_read[i], > + i915_gem_object_retire__read); > + init_request_active(&obj->last_write, > + i915_gem_object_retire__write); > + init_request_active(&obj->last_fence, > + i915_gem_object_retire__fence); > INIT_LIST_HEAD(&obj->obj_exec_link); > INIT_LIST_HEAD(&obj->vma_list); > INIT_LIST_HEAD(&obj->batch_pool_link); > @@ -4372,7 +4287,6 @@ i915_gem_cleanup_ringbuffer(struct drm_device *dev) > static void > init_ring_lists(struct intel_engine_cs *ring) > { > - INIT_LIST_HEAD(&ring->active_list); > INIT_LIST_HEAD(&ring->request_list); > } > > diff --git a/drivers/gpu/drm/i915/i915_gem_debug.c b/drivers/gpu/drm/i915/i915_gem_debug.c > deleted file mode 100644 > index 17299d04189f..000000000000 > --- a/drivers/gpu/drm/i915/i915_gem_debug.c > +++ /dev/null > @@ -1,70 +0,0 @@ > -/* > - * Copyright © 2008 Intel Corporation > - * > - * Permission is hereby granted, free of charge, to any person obtaining a > - * copy of this software and associated documentation files (the "Software"), > - * to deal in the Software without restriction, including without limitation > - * the rights to use, copy, modify, merge, publish, distribute, sublicense, > - * and/or sell copies of the Software, and to permit persons to whom the > - * Software is furnished to do so, subject to the following conditions: > - * > - * The above copyright notice and this permission notice (including the next > - * paragraph) shall be included in all copies or substantial portions of the > - * Software. > - * > - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > - * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > - * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > - * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > - * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS > - * IN THE SOFTWARE. > - * > - * Authors: > - * Keith Packard <keithp@keithp.com> > - * > - */ > - > -#include <drm/drmP.h> > -#include <drm/i915_drm.h> > -#include "i915_drv.h" > - > -#if WATCH_LISTS > -int > -i915_verify_lists(struct drm_device *dev) > -{ > - static int warned; > - struct drm_i915_private *dev_priv = to_i915(dev); > - struct drm_i915_gem_object *obj; > - struct intel_engine_cs *ring; > - int err = 0; > - int i; > - > - if (warned) > - return 0; > - > - for_each_ring(ring, dev_priv, i) { > - list_for_each_entry(obj, &ring->active_list, ring_list[ring->id]) { > - if (obj->base.dev != dev || > - !atomic_read(&obj->base.refcount.refcount)) { > - DRM_ERROR("%s: freed active obj %p\n", > - ring->name, obj); > - err++; > - break; > - } else if (!obj->active || > - obj->last_read_req[ring->id] == NULL) { > - DRM_ERROR("%s: invalid active obj %p\n", > - ring->name, obj); > - err++; > - } else if (obj->base.write_domain) { > - DRM_ERROR("%s: invalid write obj %p (w %x)\n", > - ring->name, > - obj, obj->base.write_domain); > - err++; > - } > - } > - } > - > - return warned = err; > -} > -#endif /* WATCH_LIST */ > diff --git a/drivers/gpu/drm/i915/i915_gem_fence.c b/drivers/gpu/drm/i915/i915_gem_fence.c > index ab29c237ffa9..ff085efcf0e5 100644 > --- a/drivers/gpu/drm/i915/i915_gem_fence.c > +++ b/drivers/gpu/drm/i915/i915_gem_fence.c > @@ -261,15 +261,7 @@ static inline void i915_gem_object_fence_lost(struct drm_i915_gem_object *obj) > static int > i915_gem_object_wait_fence(struct drm_i915_gem_object *obj) > { > - if (obj->last_fence.request) { > - int ret = i915_wait_request(obj->last_fence.request); > - if (ret) > - return ret; > - > - i915_gem_request_assign(&obj->last_fence.request, NULL); > - } > - > - return 0; > + return i915_wait_request(obj->last_fence.request); > } > > /** > diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c > index 7f38d8972721..069c0b9dfd95 100644 > --- a/drivers/gpu/drm/i915/i915_gem_request.c > +++ b/drivers/gpu/drm/i915/i915_gem_request.c > @@ -228,6 +228,7 @@ i915_gem_request_alloc(struct intel_engine_cs *engine, > engine->fence_context, > seqno); > > + INIT_LIST_HEAD(&req->active_list); > req->i915 = dev_priv; > req->engine = engine; > req->reset_counter = reset_counter; > @@ -320,6 +321,27 @@ static void __i915_gem_request_release(struct drm_i915_gem_request *request) > i915_gem_request_put(request); > } > > +static void __i915_gem_request_retire_active(struct drm_i915_gem_request *req) > +{ > + struct i915_gem_active *active, *next; > + > + /* Walk through the active list, calling retire on each. This allows > + * objects to track their GPU activity and mark themselves as idle > + * when their *last* active request is completed (updating state > + * tracking lists for eviction, active references for GEM, etc). > + * > + * As the ->retire() may free the node, we decouple it first and > + * pass along the auxiliary information (to avoid dereferencing > + * the node after the callback). > + */ > + list_for_each_entry_safe(active, next, &req->active_list, link) { > + INIT_LIST_HEAD(&active->link); > + active->request = NULL; > + > + active->retire(active, req); > + } > +} > + > void i915_gem_request_cancel(struct drm_i915_gem_request *req) > { > intel_ring_reserved_space_cancel(req->ring); > @@ -327,6 +349,14 @@ void i915_gem_request_cancel(struct drm_i915_gem_request *req) > if (req->ctx != req->engine->default_context) > intel_lr_context_unpin(req); > } > + > + /* If a request is to be discarded after actions have been queued upon > + * it, we cannot unwind that request and it must be submitted rather > + * than cancelled. This is not limited to activity tracking, but all > + * other state tracking (such as current register settings etc). > + */ > + GEM_BUG_ON(!list_empty(&req->active_list)); > + > __i915_gem_request_release(req); > } > > @@ -344,6 +374,8 @@ static void i915_gem_request_retire(struct drm_i915_gem_request *request) > * completion order. > */ > request->ring->last_retired_head = request->postfix; > + > + __i915_gem_request_retire_active(request); > __i915_gem_request_release(request); > } > > @@ -354,7 +386,6 @@ i915_gem_request_retire_upto(struct drm_i915_gem_request *req) > struct drm_i915_gem_request *tmp; > > lockdep_assert_held(&engine->dev->struct_mutex); > - > if (list_empty(&req->link)) > return; > > @@ -364,8 +395,6 @@ i915_gem_request_retire_upto(struct drm_i915_gem_request *req) > > i915_gem_request_retire(tmp); > } while (tmp != req); > - > - WARN_ON(i915_verify_lists(engine->dev)); > } > > static void i915_gem_mark_busy(struct drm_i915_private *dev_priv) > @@ -565,9 +594,6 @@ int __i915_wait_request(struct drm_i915_gem_request *req, > > might_sleep(); > > - if (list_empty(&req->link)) > - return 0; > - > if (i915_gem_request_completed(req)) > return 0; > > @@ -700,10 +726,12 @@ i915_wait_request(struct drm_i915_gem_request *req) > { > int ret; > > - BUG_ON(req == NULL); > + if (req == NULL) > + return 0; > > - BUG_ON(!mutex_is_locked(&req->i915->dev->struct_mutex)); > + GEM_BUG_ON(list_empty(&req->link)); > > + lockdep_assert_held(&req->i915->dev->struct_mutex); > ret = __i915_wait_request(req, req->i915->mm.interruptible, NULL, NULL); > if (ret) > return ret; > diff --git a/drivers/gpu/drm/i915/i915_gem_request.h b/drivers/gpu/drm/i915/i915_gem_request.h > index 01d589be95fd..59957d5edfdb 100644 > --- a/drivers/gpu/drm/i915/i915_gem_request.h > +++ b/drivers/gpu/drm/i915/i915_gem_request.h > @@ -84,6 +84,7 @@ struct drm_i915_gem_request { > /** Batch buffer related to this request if any (used for > error state dump only) */ > struct drm_i915_gem_object *batch_obj; > + struct list_head active_list; > > /** Time at which this request was emitted, in jiffies. */ > unsigned long emitted_jiffies; > @@ -237,13 +238,26 @@ static inline bool i915_gem_request_completed(struct drm_i915_gem_request *req) > */ > struct i915_gem_active { > struct drm_i915_gem_request *request; > + struct list_head link; > + void (*retire)(struct i915_gem_active *, > + struct drm_i915_gem_request *); > }; > > static inline void > +init_request_active(struct i915_gem_active *active, > + void (*func)(struct i915_gem_active *, > + struct drm_i915_gem_request *)) > +{ > + INIT_LIST_HEAD(&active->link); > + active->retire = func; > +} > + > +static inline void > i915_gem_request_mark_active(struct drm_i915_gem_request *request, > struct i915_gem_active *active) > { > - i915_gem_request_assign(&active->request, request); > + list_move(&active->link, &request->active_list); > + active->request = request; > } > > #endif /* I915_GEM_REQUEST_H */ > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index 0f0bf97e4032..b5f62b5f4913 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -1558,7 +1558,6 @@ static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *rin > ring->dev = dev; > ring->i915 = to_i915(dev); > ring->fence_context = fence_context_alloc(1); > - INIT_LIST_HEAD(&ring->active_list); > INIT_LIST_HEAD(&ring->request_list); > i915_gem_batch_pool_init(dev, &ring->batch_pool); > intel_engine_init_breadcrumbs(ring); > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > index 213540f92c9d..7ca4e1fc854d 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -2025,7 +2025,6 @@ static int intel_init_engine(struct drm_device *dev, > engine->dev = dev; > engine->i915 = to_i915(dev); > engine->fence_context = fence_context_alloc(1); > - INIT_LIST_HEAD(&engine->active_list); > INIT_LIST_HEAD(&engine->request_list); > INIT_LIST_HEAD(&engine->execlist_queue); > INIT_LIST_HEAD(&engine->buffers); > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h > index fc9c1e453be1..bb92d831a100 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > @@ -298,18 +298,6 @@ struct intel_engine_cs { > u32 irq_keep_mask; /* bitmask for interrupts that should not be masked */ > > /** > - * List of objects currently involved in rendering from the > - * ringbuffer. > - * > - * Includes buffers having the contents of their GPU caches > - * flushed, not necessarily primitives. last_read_req > - * represents when the rendering involved will be completed. > - * > - * A reference is held on the buffer while on this list. > - */ > - struct list_head active_list; > - > - /** > * List of breadcrumbs associated with GPU requests currently > * outstanding. > */ >
On Thu, Jan 28, 2016 at 11:41:37AM +0000, Tvrtko Ursulin wrote: > > Hi, > > On 11/01/16 09:17, Chris Wilson wrote: > >With the introduction of requests, we amplified the number of atomic > >refcounted objects we use and update every execbuffer; from none to > >several references, and a set of references that need to be changed. We > >also introduced interesting side-effects in the order of retiring > >requests and objects. > > > >Instead of independently tracking the last request for an object, track > >the active objects for each request. The object will reside in the > >buffer list of its most recent active request and so we reduce the kref > >interchange to a list_move. Now retirements are entirely driven by the > >request, dramatically simplifying activity tracking on the object > >themselves, and removing the ambiguity between retiring objects and > >retiring requests. > > > >All told, less code, simpler and faster, and more extensible. > > I've looked in this in detail before holidays and unfortunately a > lot if if evaporated from my head since. I remember I thought the > idea was good and really simplifies things. > > But it is also difficult to apply the subset of patches to look at > the resulting code in more detail. > > So would it be possible to extract and rebase relevant patches? I > think that would be from 73 to 76. (Together with the renaming we > agreed already. And those trivial renames of list/link already have > r-b's.) Actually no, if you read some of the earlier patches you will see the required bug fixes. -Chris
On 28/01/16 11:46, Chris Wilson wrote: > On Thu, Jan 28, 2016 at 11:41:37AM +0000, Tvrtko Ursulin wrote: >> >> Hi, >> >> On 11/01/16 09:17, Chris Wilson wrote: >>> With the introduction of requests, we amplified the number of atomic >>> refcounted objects we use and update every execbuffer; from none to >>> several references, and a set of references that need to be changed. We >>> also introduced interesting side-effects in the order of retiring >>> requests and objects. >>> >>> Instead of independently tracking the last request for an object, track >>> the active objects for each request. The object will reside in the >>> buffer list of its most recent active request and so we reduce the kref >>> interchange to a list_move. Now retirements are entirely driven by the >>> request, dramatically simplifying activity tracking on the object >>> themselves, and removing the ambiguity between retiring objects and >>> retiring requests. >>> >>> All told, less code, simpler and faster, and more extensible. >> >> I've looked in this in detail before holidays and unfortunately a >> lot if if evaporated from my head since. I remember I thought the >> idea was good and really simplifies things. >> >> But it is also difficult to apply the subset of patches to look at >> the resulting code in more detail. >> >> So would it be possible to extract and rebase relevant patches? I >> think that would be from 73 to 76. (Together with the renaming we >> agreed already. And those trivial renames of list/link already have >> r-b's.) > > Actually no, if you read some of the earlier patches you will see the > required bug fixes. How many / which ones? Can you extract them into a smaller series (rebased so it can be applied and tested) ending with 76? Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index b0a83215db80..79d657f29241 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -23,7 +23,6 @@ i915-$(CONFIG_DEBUG_FS) += i915_debugfs.o i915-y += i915_cmd_parser.o \ i915_gem_batch_pool.o \ i915_gem_context.o \ - i915_gem_debug.o \ i915_gem_dmabuf.o \ i915_gem_evict.o \ i915_gem_execbuffer.o \ diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index c577f86d94f8..c9c1a5cdc1e5 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -435,8 +435,6 @@ void intel_link_compute_m_n(int bpp, int nlanes, #define DRIVER_MINOR 6 #define DRIVER_PATCHLEVEL 0 -#define WATCH_LISTS 0 - struct opregion_header; struct opregion_acpi; struct opregion_swsci; @@ -2024,7 +2022,6 @@ struct drm_i915_gem_object { struct drm_mm_node *stolen; struct list_head global_list; - struct list_head ring_list[I915_NUM_RINGS]; /** Used in execbuf to temporarily hold a ref */ struct list_head obj_exec_link; @@ -3068,13 +3065,6 @@ static inline bool i915_gem_object_needs_bit17_swizzle(struct drm_i915_gem_objec obj->tiling_mode != I915_TILING_NONE; } -/* i915_gem_debug.c */ -#if WATCH_LISTS -int i915_verify_lists(struct drm_device *dev); -#else -#define i915_verify_lists(dev) 0 -#endif - /* i915_debugfs.c */ int i915_debugfs_init(struct drm_minor *minor); void i915_debugfs_cleanup(struct drm_minor *minor); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index f314b3ea2726..4eef13ebdaf3 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -40,10 +40,6 @@ static void i915_gem_object_flush_gtt_write_domain(struct drm_i915_gem_object *obj); static void i915_gem_object_flush_cpu_write_domain(struct drm_i915_gem_object *obj); -static void -i915_gem_object_retire__write(struct drm_i915_gem_object *obj); -static void -i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int ring); static bool cpu_cache_is_coherent(struct drm_device *dev, enum i915_cache_level level) @@ -117,7 +113,6 @@ int i915_mutex_lock_interruptible(struct drm_device *dev) if (ret) return ret; - WARN_ON(i915_verify_lists(dev)); return 0; } @@ -1117,27 +1112,14 @@ i915_gem_object_wait_rendering(struct drm_i915_gem_object *obj, return 0; if (readonly) { - if (obj->last_write.request != NULL) { - ret = i915_wait_request(obj->last_write.request); - if (ret) - return ret; - - i = obj->last_write.request->engine->id; - if (obj->last_read[i].request == obj->last_write.request) - i915_gem_object_retire__read(obj, i); - else - i915_gem_object_retire__write(obj); - } + ret = i915_wait_request(obj->last_write.request); + if (ret) + return ret; } else { for (i = 0; i < I915_NUM_RINGS; i++) { - if (obj->last_read[i].request == NULL) - continue; - ret = i915_wait_request(obj->last_read[i].request); if (ret) return ret; - - i915_gem_object_retire__read(obj, i); } GEM_BUG_ON(obj->active); } @@ -1145,20 +1127,6 @@ i915_gem_object_wait_rendering(struct drm_i915_gem_object *obj, return 0; } -static void -i915_gem_object_retire_request(struct drm_i915_gem_object *obj, - struct drm_i915_gem_request *req) -{ - int ring = req->engine->id; - - if (obj->last_read[ring].request == req) - i915_gem_object_retire__read(obj, ring); - else if (obj->last_write.request == req) - i915_gem_object_retire__write(obj); - - i915_gem_request_retire_upto(req); -} - /* A nonblocking variant of the above wait. This is a highly dangerous routine * as the object state may change during this call. */ @@ -1206,7 +1174,7 @@ i915_gem_object_wait_rendering__nonblocking(struct drm_i915_gem_object *obj, for (i = 0; i < n; i++) { if (ret == 0) - i915_gem_object_retire_request(obj, requests[i]); + i915_gem_request_retire_upto(requests[i]); i915_gem_request_put(requests[i]); } @@ -2069,35 +2037,37 @@ void i915_vma_move_to_active(struct i915_vma *vma, drm_gem_object_reference(&obj->base); obj->active |= intel_engine_flag(engine); - list_move_tail(&obj->ring_list[engine->id], &engine->active_list); i915_gem_request_mark_active(req, &obj->last_read[engine->id]); - list_move_tail(&vma->mm_list, &vma->vm->active_list); } static void -i915_gem_object_retire__write(struct drm_i915_gem_object *obj) +i915_gem_object_retire__fence(struct i915_gem_active *active, + struct drm_i915_gem_request *req) { - GEM_BUG_ON(obj->last_write.request == NULL); - GEM_BUG_ON(!(obj->active & intel_engine_flag(obj->last_write.request->engine))); +} - i915_gem_request_assign(&obj->last_write.request, NULL); - intel_fb_obj_flush(obj, true, ORIGIN_CS); +static void +i915_gem_object_retire__write(struct i915_gem_active *active, + struct drm_i915_gem_request *request) +{ + intel_fb_obj_flush(container_of(active, + struct drm_i915_gem_object, + last_write), + true, + ORIGIN_CS); } static void -i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int ring) +i915_gem_object_retire__read(struct i915_gem_active *active, + struct drm_i915_gem_request *request) { + int ring = request->engine->id; + struct drm_i915_gem_object *obj = + container_of(active, struct drm_i915_gem_object, last_read[ring]); struct i915_vma *vma; - GEM_BUG_ON(obj->last_read[ring].request == NULL); - GEM_BUG_ON(!(obj->active & (1 << ring))); - - list_del_init(&obj->ring_list[ring]); - i915_gem_request_assign(&obj->last_read[ring].request, NULL); - - if (obj->last_write.request && obj->last_write.request->engine->id == ring) - i915_gem_object_retire__write(obj); + GEM_BUG_ON((obj->active & (1 << ring)) == 0); obj->active &= ~(1 << ring); if (obj->active) @@ -2107,15 +2077,13 @@ i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int ring) * so that we don't steal from recently used but inactive objects * (unless we are forced to ofc!) */ - list_move_tail(&obj->global_list, - &to_i915(obj->base.dev)->mm.bound_list); + list_move_tail(&obj->global_list, &request->i915->mm.bound_list); list_for_each_entry(vma, &obj->vma_list, vma_link) { if (!list_empty(&vma->mm_list)) list_move_tail(&vma->mm_list, &vma->vm->inactive_list); } - i915_gem_request_assign(&obj->last_fence.request, NULL); drm_gem_object_unreference(&obj->base); } @@ -2216,16 +2184,6 @@ static void i915_gem_reset_ring_cleanup(struct intel_engine_cs *engine) { struct intel_ring *ring; - while (!list_empty(&engine->active_list)) { - struct drm_i915_gem_object *obj; - - obj = list_first_entry(&engine->active_list, - struct drm_i915_gem_object, - ring_list[engine->id]); - - i915_gem_object_retire__read(obj, engine->id); - } - /* * Clear the execlists queue up before freeing the requests, as those * are the ones that keep the context and ringbuffer backing objects @@ -2295,8 +2253,6 @@ void i915_gem_reset(struct drm_device *dev) i915_gem_context_reset(dev); i915_gem_restore_fences(dev); - - WARN_ON(i915_verify_lists(dev)); } /** @@ -2305,13 +2261,6 @@ void i915_gem_reset(struct drm_device *dev) void i915_gem_retire_requests_ring(struct intel_engine_cs *ring) { - WARN_ON(i915_verify_lists(ring->dev)); - - /* Retire requests first as we use it above for the early return. - * If we retire requests last, we may use a later seqno and so clear - * the requests lists without clearing the active list, leading to - * confusion. - */ while (!list_empty(&ring->request_list)) { struct drm_i915_gem_request *request; @@ -2324,25 +2273,6 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring) i915_gem_request_retire_upto(request); } - - /* Move any buffers on the active list that are no longer referenced - * by the ringbuffer to the flushing/inactive lists as appropriate, - * before we free the context associated with the requests. - */ - while (!list_empty(&ring->active_list)) { - struct drm_i915_gem_object *obj; - - obj = list_first_entry(&ring->active_list, - struct drm_i915_gem_object, - ring_list[ring->id]); - - if (!list_empty(&obj->last_read[ring->id].request->link)) - break; - - i915_gem_object_retire__read(obj, ring->id); - } - - WARN_ON(i915_verify_lists(ring->dev)); } void @@ -2434,13 +2364,13 @@ out: * write domains, emitting any outstanding lazy request and retiring and * completed requests. */ -static int +static void i915_gem_object_flush_active(struct drm_i915_gem_object *obj) { int i; if (!obj->active) - return 0; + return; for (i = 0; i < I915_NUM_RINGS; i++) { struct drm_i915_gem_request *req; @@ -2449,17 +2379,9 @@ i915_gem_object_flush_active(struct drm_i915_gem_object *obj) if (req == NULL) continue; - if (list_empty(&req->link)) - goto retire; - - if (i915_gem_request_completed(req)) { + if (i915_gem_request_completed(req)) i915_gem_request_retire_upto(req); -retire: - i915_gem_object_retire__read(obj, i); - } } - - return 0; } /** @@ -2507,10 +2429,7 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file) } /* Need to make sure the object gets inactive eventually. */ - ret = i915_gem_object_flush_active(obj); - if (ret) - goto out; - + i915_gem_object_flush_active(obj); if (!obj->active) goto out; @@ -2522,8 +2441,6 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file) goto out; } - drm_gem_object_unreference(&obj->base); - for (i = 0; i < I915_NUM_RINGS; i++) { if (obj->last_read[i].request == NULL) continue; @@ -2531,6 +2448,8 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file) req[n++] = i915_gem_request_get(obj->last_read[i].request); } +out: + drm_gem_object_unreference(&obj->base); mutex_unlock(&dev->struct_mutex); for (i = 0; i < n; i++) { @@ -2541,11 +2460,6 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file) i915_gem_request_put(req[i]); } return ret; - -out: - drm_gem_object_unreference(&obj->base); - mutex_unlock(&dev->struct_mutex); - return ret; } static int @@ -2569,7 +2483,7 @@ __i915_gem_object_sync(struct drm_i915_gem_object *obj, if (ret) return ret; - i915_gem_object_retire_request(obj, from); + i915_gem_request_retire_upto(from); } else { int idx = intel_engine_sync_index(from->engine, to->engine); if (from->fence.seqno <= from->engine->semaphore.sync_seqno[idx]) @@ -2760,7 +2674,6 @@ int i915_gpu_idle(struct drm_device *dev) return ret; } - WARN_ON(i915_verify_lists(dev)); return 0; } @@ -3689,16 +3602,13 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, * become non-busy without any further actions, therefore emit any * necessary flushes here. */ - ret = i915_gem_object_flush_active(obj); - if (ret) - goto unref; + i915_gem_object_flush_active(obj); BUILD_BUG_ON(I915_NUM_RINGS > 16); args->busy = obj->active << 16; if (obj->last_write.request) args->busy |= obj->last_write.request->engine->id; -unref: drm_gem_object_unreference(&obj->base); unlock: mutex_unlock(&dev->struct_mutex); @@ -3776,7 +3686,12 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj, INIT_LIST_HEAD(&obj->global_list); for (i = 0; i < I915_NUM_RINGS; i++) - INIT_LIST_HEAD(&obj->ring_list[i]); + init_request_active(&obj->last_read[i], + i915_gem_object_retire__read); + init_request_active(&obj->last_write, + i915_gem_object_retire__write); + init_request_active(&obj->last_fence, + i915_gem_object_retire__fence); INIT_LIST_HEAD(&obj->obj_exec_link); INIT_LIST_HEAD(&obj->vma_list); INIT_LIST_HEAD(&obj->batch_pool_link); @@ -4372,7 +4287,6 @@ i915_gem_cleanup_ringbuffer(struct drm_device *dev) static void init_ring_lists(struct intel_engine_cs *ring) { - INIT_LIST_HEAD(&ring->active_list); INIT_LIST_HEAD(&ring->request_list); } diff --git a/drivers/gpu/drm/i915/i915_gem_debug.c b/drivers/gpu/drm/i915/i915_gem_debug.c deleted file mode 100644 index 17299d04189f..000000000000 --- a/drivers/gpu/drm/i915/i915_gem_debug.c +++ /dev/null @@ -1,70 +0,0 @@ -/* - * Copyright © 2008 Intel Corporation - * - * Permission is hereby granted, free of charge, to any person obtaining a - * copy of this software and associated documentation files (the "Software"), - * to deal in the Software without restriction, including without limitation - * the rights to use, copy, modify, merge, publish, distribute, sublicense, - * and/or sell copies of the Software, and to permit persons to whom the - * Software is furnished to do so, subject to the following conditions: - * - * The above copyright notice and this permission notice (including the next - * paragraph) shall be included in all copies or substantial portions of the - * Software. - * - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, - * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL - * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER - * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING - * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS - * IN THE SOFTWARE. - * - * Authors: - * Keith Packard <keithp@keithp.com> - * - */ - -#include <drm/drmP.h> -#include <drm/i915_drm.h> -#include "i915_drv.h" - -#if WATCH_LISTS -int -i915_verify_lists(struct drm_device *dev) -{ - static int warned; - struct drm_i915_private *dev_priv = to_i915(dev); - struct drm_i915_gem_object *obj; - struct intel_engine_cs *ring; - int err = 0; - int i; - - if (warned) - return 0; - - for_each_ring(ring, dev_priv, i) { - list_for_each_entry(obj, &ring->active_list, ring_list[ring->id]) { - if (obj->base.dev != dev || - !atomic_read(&obj->base.refcount.refcount)) { - DRM_ERROR("%s: freed active obj %p\n", - ring->name, obj); - err++; - break; - } else if (!obj->active || - obj->last_read_req[ring->id] == NULL) { - DRM_ERROR("%s: invalid active obj %p\n", - ring->name, obj); - err++; - } else if (obj->base.write_domain) { - DRM_ERROR("%s: invalid write obj %p (w %x)\n", - ring->name, - obj, obj->base.write_domain); - err++; - } - } - } - - return warned = err; -} -#endif /* WATCH_LIST */ diff --git a/drivers/gpu/drm/i915/i915_gem_fence.c b/drivers/gpu/drm/i915/i915_gem_fence.c index ab29c237ffa9..ff085efcf0e5 100644 --- a/drivers/gpu/drm/i915/i915_gem_fence.c +++ b/drivers/gpu/drm/i915/i915_gem_fence.c @@ -261,15 +261,7 @@ static inline void i915_gem_object_fence_lost(struct drm_i915_gem_object *obj) static int i915_gem_object_wait_fence(struct drm_i915_gem_object *obj) { - if (obj->last_fence.request) { - int ret = i915_wait_request(obj->last_fence.request); - if (ret) - return ret; - - i915_gem_request_assign(&obj->last_fence.request, NULL); - } - - return 0; + return i915_wait_request(obj->last_fence.request); } /** diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c index 7f38d8972721..069c0b9dfd95 100644 --- a/drivers/gpu/drm/i915/i915_gem_request.c +++ b/drivers/gpu/drm/i915/i915_gem_request.c @@ -228,6 +228,7 @@ i915_gem_request_alloc(struct intel_engine_cs *engine, engine->fence_context, seqno); + INIT_LIST_HEAD(&req->active_list); req->i915 = dev_priv; req->engine = engine; req->reset_counter = reset_counter; @@ -320,6 +321,27 @@ static void __i915_gem_request_release(struct drm_i915_gem_request *request) i915_gem_request_put(request); } +static void __i915_gem_request_retire_active(struct drm_i915_gem_request *req) +{ + struct i915_gem_active *active, *next; + + /* Walk through the active list, calling retire on each. This allows + * objects to track their GPU activity and mark themselves as idle + * when their *last* active request is completed (updating state + * tracking lists for eviction, active references for GEM, etc). + * + * As the ->retire() may free the node, we decouple it first and + * pass along the auxiliary information (to avoid dereferencing + * the node after the callback). + */ + list_for_each_entry_safe(active, next, &req->active_list, link) { + INIT_LIST_HEAD(&active->link); + active->request = NULL; + + active->retire(active, req); + } +} + void i915_gem_request_cancel(struct drm_i915_gem_request *req) { intel_ring_reserved_space_cancel(req->ring); @@ -327,6 +349,14 @@ void i915_gem_request_cancel(struct drm_i915_gem_request *req) if (req->ctx != req->engine->default_context) intel_lr_context_unpin(req); } + + /* If a request is to be discarded after actions have been queued upon + * it, we cannot unwind that request and it must be submitted rather + * than cancelled. This is not limited to activity tracking, but all + * other state tracking (such as current register settings etc). + */ + GEM_BUG_ON(!list_empty(&req->active_list)); + __i915_gem_request_release(req); } @@ -344,6 +374,8 @@ static void i915_gem_request_retire(struct drm_i915_gem_request *request) * completion order. */ request->ring->last_retired_head = request->postfix; + + __i915_gem_request_retire_active(request); __i915_gem_request_release(request); } @@ -354,7 +386,6 @@ i915_gem_request_retire_upto(struct drm_i915_gem_request *req) struct drm_i915_gem_request *tmp; lockdep_assert_held(&engine->dev->struct_mutex); - if (list_empty(&req->link)) return; @@ -364,8 +395,6 @@ i915_gem_request_retire_upto(struct drm_i915_gem_request *req) i915_gem_request_retire(tmp); } while (tmp != req); - - WARN_ON(i915_verify_lists(engine->dev)); } static void i915_gem_mark_busy(struct drm_i915_private *dev_priv) @@ -565,9 +594,6 @@ int __i915_wait_request(struct drm_i915_gem_request *req, might_sleep(); - if (list_empty(&req->link)) - return 0; - if (i915_gem_request_completed(req)) return 0; @@ -700,10 +726,12 @@ i915_wait_request(struct drm_i915_gem_request *req) { int ret; - BUG_ON(req == NULL); + if (req == NULL) + return 0; - BUG_ON(!mutex_is_locked(&req->i915->dev->struct_mutex)); + GEM_BUG_ON(list_empty(&req->link)); + lockdep_assert_held(&req->i915->dev->struct_mutex); ret = __i915_wait_request(req, req->i915->mm.interruptible, NULL, NULL); if (ret) return ret; diff --git a/drivers/gpu/drm/i915/i915_gem_request.h b/drivers/gpu/drm/i915/i915_gem_request.h index 01d589be95fd..59957d5edfdb 100644 --- a/drivers/gpu/drm/i915/i915_gem_request.h +++ b/drivers/gpu/drm/i915/i915_gem_request.h @@ -84,6 +84,7 @@ struct drm_i915_gem_request { /** Batch buffer related to this request if any (used for error state dump only) */ struct drm_i915_gem_object *batch_obj; + struct list_head active_list; /** Time at which this request was emitted, in jiffies. */ unsigned long emitted_jiffies; @@ -237,13 +238,26 @@ static inline bool i915_gem_request_completed(struct drm_i915_gem_request *req) */ struct i915_gem_active { struct drm_i915_gem_request *request; + struct list_head link; + void (*retire)(struct i915_gem_active *, + struct drm_i915_gem_request *); }; static inline void +init_request_active(struct i915_gem_active *active, + void (*func)(struct i915_gem_active *, + struct drm_i915_gem_request *)) +{ + INIT_LIST_HEAD(&active->link); + active->retire = func; +} + +static inline void i915_gem_request_mark_active(struct drm_i915_gem_request *request, struct i915_gem_active *active) { - i915_gem_request_assign(&active->request, request); + list_move(&active->link, &request->active_list); + active->request = request; } #endif /* I915_GEM_REQUEST_H */ diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 0f0bf97e4032..b5f62b5f4913 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -1558,7 +1558,6 @@ static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *rin ring->dev = dev; ring->i915 = to_i915(dev); ring->fence_context = fence_context_alloc(1); - INIT_LIST_HEAD(&ring->active_list); INIT_LIST_HEAD(&ring->request_list); i915_gem_batch_pool_init(dev, &ring->batch_pool); intel_engine_init_breadcrumbs(ring); diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 213540f92c9d..7ca4e1fc854d 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -2025,7 +2025,6 @@ static int intel_init_engine(struct drm_device *dev, engine->dev = dev; engine->i915 = to_i915(dev); engine->fence_context = fence_context_alloc(1); - INIT_LIST_HEAD(&engine->active_list); INIT_LIST_HEAD(&engine->request_list); INIT_LIST_HEAD(&engine->execlist_queue); INIT_LIST_HEAD(&engine->buffers); diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index fc9c1e453be1..bb92d831a100 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -298,18 +298,6 @@ struct intel_engine_cs { u32 irq_keep_mask; /* bitmask for interrupts that should not be masked */ /** - * List of objects currently involved in rendering from the - * ringbuffer. - * - * Includes buffers having the contents of their GPU caches - * flushed, not necessarily primitives. last_read_req - * represents when the rendering involved will be completed. - * - * A reference is held on the buffer while on this list. - */ - struct list_head active_list; - - /** * List of breadcrumbs associated with GPU requests currently * outstanding. */
With the introduction of requests, we amplified the number of atomic refcounted objects we use and update every execbuffer; from none to several references, and a set of references that need to be changed. We also introduced interesting side-effects in the order of retiring requests and objects. Instead of independently tracking the last request for an object, track the active objects for each request. The object will reside in the buffer list of its most recent active request and so we reduce the kref interchange to a list_move. Now retirements are entirely driven by the request, dramatically simplifying activity tracking on the object themselves, and removing the ambiguity between retiring objects and retiring requests. All told, less code, simpler and faster, and more extensible. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/Makefile | 1 - drivers/gpu/drm/i915/i915_drv.h | 10 -- drivers/gpu/drm/i915/i915_gem.c | 160 ++++++++------------------------ drivers/gpu/drm/i915/i915_gem_debug.c | 70 -------------- drivers/gpu/drm/i915/i915_gem_fence.c | 10 +- drivers/gpu/drm/i915/i915_gem_request.c | 44 +++++++-- drivers/gpu/drm/i915/i915_gem_request.h | 16 +++- drivers/gpu/drm/i915/intel_lrc.c | 1 - drivers/gpu/drm/i915/intel_ringbuffer.c | 1 - drivers/gpu/drm/i915/intel_ringbuffer.h | 12 --- 10 files changed, 89 insertions(+), 236 deletions(-) delete mode 100644 drivers/gpu/drm/i915/i915_gem_debug.c