| Message ID | 1450899235-14463-3-git-send-email-david.s.gordon@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Dec 23, 2015 at 07:33:53PM +0000, Dave Gordon wrote: > There are quite a number of places where the driver tests whether > a given context is or is not the global default context, usually by > checking whether an engine's default_pointer points to the context. > Now that we have a 'is_global_default' flag in the context itself, > all these tests these can be rewritten to use it. This makes the > logic more obvious, and usually saves at least one memory reference. > In addition, with these uses eliminated, a future patch will be able > to get rid of engine::default_context entirely. All the execlists use of ctx != ring->default_context stems from a misstep in execlists - if you stop treating that default_context as special during request processing and just take the pin/unpin at init/fini of the ring, they all disappear. And please stop conflating is_global_context when we have already a very good expression for when the context is owned by no file. -Chris
On 23/12/15 21:02, Chris Wilson wrote: > On Wed, Dec 23, 2015 at 07:33:53PM +0000, Dave Gordon wrote: >> There are quite a number of places where the driver tests whether >> a given context is or is not the global default context, usually by >> checking whether an engine's default_pointer points to the context. >> Now that we have a 'is_global_default' flag in the context itself, >> all these tests these can be rewritten to use it. This makes the >> logic more obvious, and usually saves at least one memory reference. >> In addition, with these uses eliminated, a future patch will be able >> to get rid of engine::default_context entirely. > > All the execlists use of ctx != ring->default_context stems from a > misstep in execlists - if you stop treating that default_context as > special during request processing and just take the pin/unpin at > init/fini of the ring, they all disappear. We do already pin/unpin the default context at creation/deletion; AFAICS the extra tests are probably an attempt not to do an extra pin/unpin on an object which is by definition already pinned. And I'd be quite happy to get rid of those tests, and just issue a pin for *every* request issues on a context -- indeed, I think Nick may have just such a patch. But his changes are blocked on getting the elimination of ring->default_context (patch 4 of THIS set) merged first, since having those backpointers dictates the order of creation and destruction. > And please stop conflating is_global_context when we have already a very > good expression for when the context is owned by no file. > -Chris No existing code uses that as a test. And patch 1 of this set dropped file_priv anyway, since there was no actual use for it anywhere at all! This way we don't risk having dangling pointers to deallocated file objects. .Dave.
On Mon, Jan 04, 2016 at 05:43:10PM +0000, Dave Gordon wrote: > On 23/12/15 21:02, Chris Wilson wrote: > >On Wed, Dec 23, 2015 at 07:33:53PM +0000, Dave Gordon wrote: > >>There are quite a number of places where the driver tests whether > >>a given context is or is not the global default context, usually by > >>checking whether an engine's default_pointer points to the context. > >>Now that we have a 'is_global_default' flag in the context itself, > >>all these tests these can be rewritten to use it. This makes the > >>logic more obvious, and usually saves at least one memory reference. > >>In addition, with these uses eliminated, a future patch will be able > >>to get rid of engine::default_context entirely. > > > >All the execlists use of ctx != ring->default_context stems from a > >misstep in execlists - if you stop treating that default_context as > >special during request processing and just take the pin/unpin at > >init/fini of the ring, they all disappear. > > We do already pin/unpin the default context at creation/deletion; > AFAICS the extra tests are probably an attempt not to do an extra > pin/unpin on an object which is by definition already pinned. And > I'd be quite happy to get rid of those tests, and just issue a pin > for *every* request issues on a context -- indeed, I think Nick may > have just such a patch. But his changes are blocked on getting the > elimination of ring->default_context (patch 4 of THIS set) merged > first, since having those backpointers dictates the order of > creation and destruction. This series is NAKed. -Chris
On 01/04/2016 11:39 AM, Chris Wilson wrote: > On Mon, Jan 04, 2016 at 05:43:10PM +0000, Dave Gordon wrote: >> On 23/12/15 21:02, Chris Wilson wrote: >>> On Wed, Dec 23, 2015 at 07:33:53PM +0000, Dave Gordon wrote: >>>> There are quite a number of places where the driver tests whether >>>> a given context is or is not the global default context, usually by >>>> checking whether an engine's default_pointer points to the context. >>>> Now that we have a 'is_global_default' flag in the context itself, >>>> all these tests these can be rewritten to use it. This makes the >>>> logic more obvious, and usually saves at least one memory reference. >>>> In addition, with these uses eliminated, a future patch will be able >>>> to get rid of engine::default_context entirely. >>> >>> All the execlists use of ctx != ring->default_context stems from a >>> misstep in execlists - if you stop treating that default_context as >>> special during request processing and just take the pin/unpin at >>> init/fini of the ring, they all disappear. >> >> We do already pin/unpin the default context at creation/deletion; >> AFAICS the extra tests are probably an attempt not to do an extra >> pin/unpin on an object which is by definition already pinned. And >> I'd be quite happy to get rid of those tests, and just issue a pin >> for *every* request issues on a context -- indeed, I think Nick may >> have just such a patch. But his changes are blocked on getting the >> elimination of ring->default_context (patch 4 of THIS set) merged >> first, since having those backpointers dictates the order of >> creation and destruction. > > This series is NAKed. Why? Because you want things in a different order? Or do you object to something in Dave's reply? Jesse
On Mon, Jan 04, 2016 at 01:38:26PM -0800, Jesse Barnes wrote: > On 01/04/2016 11:39 AM, Chris Wilson wrote: > > This series is NAKed. > > Why? Because you want things in a different order? Or do you object to something in Dave's reply? The series was intended as a code cleanup and in the process tried to introduce a false concept that I objected to. Since the cleanup was not predicated upon that idea, the patches would have been much tidier without it. The fundamental issue at stake here is execlists behaves badly and we have to futz around in higher level code to undo that mistake. -Chris
On 05/01/16 10:06, Chris Wilson wrote: > On Mon, Jan 04, 2016 at 01:38:26PM -0800, Jesse Barnes wrote: >> On 01/04/2016 11:39 AM, Chris Wilson wrote: >>> This series is NAKed. >> >> Why? Because you want things in a different order? Or do you object to something in Dave's reply? > > The series was intended as a code cleanup and in the process tried to > introduce a false concept that I objected to. Since the cleanup was not > predicated upon that idea, the patches would have been much tidier > without it. I don't think it's a false concept; there very evidently *IS* a global default context, so why not flag it as such by name, rather than by implication. and the subsequent cleanup *does* require it. > The fundamental issue at stake here is execlists behaves badly and we > have to futz around in higher level code to undo that mistake. > -Chris Now I agree that the execlist->default_context is a bad idea, and that's exactly what we will get rid of in patch 4, but we can't do it until these precursor patches have separated the various purposes for which it is used into distinct categories, each of which is then replaced by a cleaner alternative. So it *is* a step towards rewriting execlists, even if it doesn't (at this stage) fix everything you might eventually want. .Dave.
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 0fc38bb..5e6e02c 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -1942,11 +1942,8 @@ static int i915_context_status(struct seq_file *m, void *unused) seq_puts(m, "HW context "); describe_ctx(m, ctx); - for_each_ring(ring, dev_priv, i) { - if (ring->default_context == ctx) - seq_printf(m, "(default context %s) ", - ring->name); - } + if (ctx->is_global_default) + seq_printf(m, "(default context) "); if (i915.enable_execlists) { seq_putc(m, '\n'); @@ -2037,13 +2034,11 @@ static int i915_dump_lrc(struct seq_file *m, void *unused) if (ret) return ret; - list_for_each_entry(ctx, &dev_priv->context_list, link) { - for_each_ring(ring, dev_priv, i) { - if (ring->default_context != ctx) + list_for_each_entry(ctx, &dev_priv->context_list, link) + if (!ctx->is_global_default) + for_each_ring(ring, dev_priv, i) i915_dump_lrc_obj(m, ring, ctx->engine[i].state); - } - } mutex_unlock(&dev->struct_mutex); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 6c60e04..d9a3304 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2677,10 +2677,8 @@ void i915_gem_request_free(struct kref *req_ref) i915_gem_request_remove_from_client(req); if (ctx) { - if (i915.enable_execlists) { - if (ctx != req->ring->default_context) - intel_lr_context_unpin(req); - } + if (i915.enable_execlists && !ctx->is_global_default) + intel_lr_context_unpin(req); i915_gem_context_unreference(ctx); } diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 3aa6147..cf76d28 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -571,7 +571,7 @@ static int execlists_context_queue(struct drm_i915_gem_request *request) struct drm_i915_gem_request *cursor; int num_elements = 0; - if (request->ctx != ring->default_context) + if (!request->ctx->is_global_default) intel_lr_context_pin(request); i915_gem_request_reference(request); @@ -660,17 +660,14 @@ static int execlists_move_to_gpu(struct drm_i915_gem_request *req, int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request) { - int ret; + int ret = 0; request->ringbuf = request->ctx->engine[request->ring->id].ringbuf; - if (request->ctx != request->ring->default_context) { + if (!request->ctx->is_global_default) ret = intel_lr_context_pin(request); - if (ret) - return ret; - } - return 0; + return ret; } static int logical_ring_wait_for_space(struct drm_i915_gem_request *req, @@ -967,7 +964,7 @@ void intel_execlists_retire_requests(struct intel_engine_cs *ring) struct drm_i915_gem_object *ctx_obj = ctx->engine[ring->id].state; - if (ctx_obj && (ctx != ring->default_context)) + if (ctx_obj && !ctx->is_global_default) intel_lr_context_unpin(req); list_del(&req->execlist_link); i915_gem_request_unreference(req); @@ -2367,22 +2364,21 @@ void intel_lr_context_free(struct intel_context *ctx) { int i; - for (i = 0; i < I915_NUM_RINGS; i++) { + for (i = I915_NUM_RINGS; --i >= 0; ) { + struct intel_ringbuffer *ringbuf = ctx->engine[i].ringbuf; struct drm_i915_gem_object *ctx_obj = ctx->engine[i].state; - if (ctx_obj) { - struct intel_ringbuffer *ringbuf = - ctx->engine[i].ringbuf; - struct intel_engine_cs *ring = ringbuf->ring; + if (!ctx_obj) + continue; - if (ctx == ring->default_context) { - intel_unpin_ringbuffer_obj(ringbuf); - i915_gem_object_ggtt_unpin(ctx_obj); - } - WARN_ON(ctx->engine[ring->id].pin_count); - intel_ringbuffer_free(ringbuf); - drm_gem_object_unreference(&ctx_obj->base); + if (ctx->is_global_default) { + intel_unpin_ringbuffer_obj(ringbuf); + i915_gem_object_ggtt_unpin(ctx_obj); } + + WARN_ON(ctx->engine[i].pin_count); + intel_ringbuffer_free(ringbuf); + drm_gem_object_unreference(&ctx_obj->base); } } @@ -2443,7 +2439,7 @@ static void lrc_setup_hardware_status_page(struct intel_engine_cs *ring, */ int intel_lr_context_deferred_alloc(struct intel_context *ctx, - struct intel_engine_cs *ring) + struct intel_engine_cs *ring) { struct drm_device *dev = ring->dev; struct drm_i915_gem_object *ctx_obj; @@ -2480,7 +2476,7 @@ int intel_lr_context_deferred_alloc(struct intel_context *ctx, ctx->engine[ring->id].ringbuf = ringbuf; ctx->engine[ring->id].state = ctx_obj; - if (ctx != ring->default_context && ring->init_context) { + if (!ctx->is_global_default && ring->init_context) { struct drm_i915_gem_request *req; ret = i915_gem_request_alloc(ring,
There are quite a number of places where the driver tests whether a given context is or is not the global default context, usually by checking whether an engine's default_pointer points to the context. Now that we have a 'is_global_default' flag in the context itself, all these tests these can be rewritten to use it. This makes the logic more obvious, and usually saves at least one memory reference. In addition, with these uses eliminated, a future patch will be able to get rid of engine::default_context entirely. Signed-off-by: Dave Gordon <david.s.gordon@intel.com> --- drivers/gpu/drm/i915/i915_debugfs.c | 15 +++++--------- drivers/gpu/drm/i915/i915_gem.c | 6 ++---- drivers/gpu/drm/i915/intel_lrc.c | 40 +++++++++++++++++-------------------- 3 files changed, 25 insertions(+), 36 deletions(-)