| Message ID | 1452162052-22573-4-git-send-email-david.s.gordon@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jan 07, 2016 at 10:20:52AM +0000, Dave Gordon wrote: > There are a few bits of code which the transformations implemented by > the previous patch reveal to be suboptimal, once the notion of a per- > ring default context has gone away. So this tidies up the leftovers. > > It could have been squashed into the previous patch, but that would have > made that patch less clearly a simple transformation. In particular, any > change which alters the code block structure or indentation has been > deferred into this separate patch, because such things tend to make diffs > more difficult to read. > > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> Yeah I think this was good to be split up, since I'm not convinced it's a net improvement (from a quick look at least). Still plenty of default context checks that seem superfluous (e.g. not pinning the default context when going through execlist submission is imo a needless special case - besides that we really should use active tracking). So I'll refrain from ack or nack on this one. -Daniel > --- > drivers/gpu/drm/i915/i915_debugfs.c | 15 +++++---------- > drivers/gpu/drm/i915/i915_gem.c | 6 ++---- > drivers/gpu/drm/i915/intel_lrc.c | 38 +++++++++++++++++-------------------- > 3 files changed, 24 insertions(+), 35 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 2613708..bbb23da 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 (dev_priv->kernel_context == ctx) > - seq_printf(m, "(default context %s) ", > - ring->name); > - } > + if (ctx == dev_priv->kernel_context) > + seq_printf(m, "(kernel 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 (dev_priv->kernel_context != ctx) > + list_for_each_entry(ctx, &dev_priv->context_list, link) > + if (ctx != dev_priv->kernel_context) > + 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 8f101121..4f45eb2 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->i915->kernel_context) > - intel_lr_context_unpin(req); > - } > + if (i915.enable_execlists && ctx != req->i915->kernel_context) > + 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 aaaa5a3..8c4c9b9 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -660,16 +660,10 @@ 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->i915->kernel_context) { > - ret = intel_lr_context_pin(request); > - if (ret) > - return ret; > - } > - > if (i915.enable_guc_submission) { > /* > * Check that the GuC has space for the request before > @@ -683,7 +677,10 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request > return ret; > } > > - return 0; > + if (request->ctx != request->i915->kernel_context) > + ret = intel_lr_context_pin(request); > + > + return ret; > } > > static int logical_ring_wait_for_space(struct drm_i915_gem_request *req, > @@ -2382,22 +2379,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 == ctx->i915->kernel_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 == ctx->i915->kernel_context) { > + 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); > } > } > > @@ -2472,7 +2468,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; > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 12/01/16 13:53, Daniel Vetter wrote: > On Thu, Jan 07, 2016 at 10:20:52AM +0000, Dave Gordon wrote: >> There are a few bits of code which the transformations implemented by >> the previous patch reveal to be suboptimal, once the notion of a per- >> ring default context has gone away. So this tidies up the leftovers. >> >> It could have been squashed into the previous patch, but that would have >> made that patch less clearly a simple transformation. In particular, any >> change which alters the code block structure or indentation has been >> deferred into this separate patch, because such things tend to make diffs >> more difficult to read. >> >> Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > > Yeah I think this was good to be split up, since I'm not convinced it's > a net improvement (from a quick look at least). Still plenty of default > context checks that seem superfluous (e.g. not pinning the default context > when going through execlist submission is imo a needless special case - > besides that we really should use active tracking). > > So I'll refrain from ack or nack on this one. > -Daniel > >> --- >> drivers/gpu/drm/i915/i915_debugfs.c | 15 +++++---------- >> drivers/gpu/drm/i915/i915_gem.c | 6 ++---- >> drivers/gpu/drm/i915/intel_lrc.c | 38 +++++++++++++++++-------------------- >> 3 files changed, 24 insertions(+), 35 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c >> index 2613708..bbb23da 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 (dev_priv->kernel_context == ctx) >> - seq_printf(m, "(default context %s) ", >> - ring->name); >> - } >> + if (ctx == dev_priv->kernel_context) >> + seq_printf(m, "(kernel context) "); This is replacing the multiple messages in the debugfs output: "(default context render ring) (default context blitter ring) (default context ..." (which are redundant because the same context is the default for all rings) with the single more concise message "(kernel context)". One part of one of Chris' patches does the same or an equivalent thing, so I don't think it's contentious. >> >> 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 (dev_priv->kernel_context != ctx) >> + list_for_each_entry(ctx, &dev_priv->context_list, link) >> + if (ctx != dev_priv->kernel_context) >> + for_each_ring(ring, dev_priv, i) >> i915_dump_lrc_obj(m, ring, >> ctx->engine[i].state); >> - } >> - } Since we know that there is only one kernel context, the if() should be outside rather than outside the for_each_ring(). The {} are redundant anyway. >> >> mutex_unlock(&dev->struct_mutex); >> >> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c >> index 8f101121..4f45eb2 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->i915->kernel_context) >> - intel_lr_context_unpin(req); >> - } >> + if (i915.enable_execlists && ctx != req->i915->kernel_context) >> + intel_lr_context_unpin(req); Trivial rewrite of the nested if() to reduce indentation. Actually removing this test is left for a subsequent rationalisation of the execlist code (one of Chris' patches will do it). >> >> i915_gem_context_unreference(ctx); >> } >> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c >> index aaaa5a3..8c4c9b9 100644 >> --- a/drivers/gpu/drm/i915/intel_lrc.c >> +++ b/drivers/gpu/drm/i915/intel_lrc.c >> @@ -660,16 +660,10 @@ 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->i915->kernel_context) { >> - ret = intel_lr_context_pin(request); >> - if (ret) >> - return ret; >> - } >> - >> if (i915.enable_guc_submission) { >> /* >> * Check that the GuC has space for the request before >> @@ -683,7 +677,10 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request >> return ret; >> } >> >> - return 0; >> + if (request->ctx != request->i915->kernel_context) >> + ret = intel_lr_context_pin(request); >> + >> + return ret; >> } Two things here, one is restructuring the if/ret tests, which is trivial. More importantly, the pinning is moved to AFTER the GuC space pre-check, otherwise the pinning is not undone if this check fails. I can move this to a separate patch if required, as it's actually a fix for a potential bug. >> >> static int logical_ring_wait_for_space(struct drm_i915_gem_request *req, >> @@ -2382,22 +2379,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 == ctx->i915->kernel_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 == ctx->i915->kernel_context) { >> + 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); >> } >> } Again two things; one is the trivial restructuring of the loop using continue to reduce indentation depth. The other (more interesting) one is to run the teardown loop in reverse. This avoided a bug in some *other* destructor-type code that got called from here, which assumed that RCS state was still valid while releasing resources on the other rings. That specific bug has been eliminated by the change from per-engine to a global default context, but nonetheless it seems a good policy to release resources in reverse order of acquisition; hence teardown loops should in general run backwards just in case there are cross-engine dependencies. >> >> @@ -2472,7 +2468,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) Trivial whitespace fix. >> { >> struct drm_device *dev = ring->dev; >> struct drm_i915_gem_object *ctx_obj; >> -- >> 1.9.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > Let me know whether this all looks OK as-is, with the explanations above ('cos I don't think there's anything problematic there), or whether some bits should be posted separately. The only bit that really matters is the pinning-vs-GuC-precheck error path fix, and that error won't be possible once the scheduler is in, so it's just to avoid leaks if it happens in the short term. .Dave.
On 07/01/2016 10:20, Dave Gordon wrote: > There are a few bits of code which the transformations implemented by > the previous patch reveal to be suboptimal, once the notion of a per- > ring default context has gone away. So this tidies up the leftovers. > > It could have been squashed into the previous patch, but that would have > made that patch less clearly a simple transformation. In particular, any > change which alters the code block structure or indentation has been > deferred into this separate patch, because such things tend to make diffs > more difficult to read. > > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> Reviewed-by: Nick Hoath <nicholas.hoath@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 | 38 +++++++++++++++++-------------------- > 3 files changed, 24 insertions(+), 35 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 2613708..bbb23da 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 (dev_priv->kernel_context == ctx) > - seq_printf(m, "(default context %s) ", > - ring->name); > - } > + if (ctx == dev_priv->kernel_context) > + seq_printf(m, "(kernel 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 (dev_priv->kernel_context != ctx) > + list_for_each_entry(ctx, &dev_priv->context_list, link) > + if (ctx != dev_priv->kernel_context) > + 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 8f101121..4f45eb2 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->i915->kernel_context) > - intel_lr_context_unpin(req); > - } > + if (i915.enable_execlists && ctx != req->i915->kernel_context) > + 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 aaaa5a3..8c4c9b9 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -660,16 +660,10 @@ 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->i915->kernel_context) { > - ret = intel_lr_context_pin(request); > - if (ret) > - return ret; > - } > - > if (i915.enable_guc_submission) { > /* > * Check that the GuC has space for the request before > @@ -683,7 +677,10 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request > return ret; > } > > - return 0; > + if (request->ctx != request->i915->kernel_context) > + ret = intel_lr_context_pin(request); > + > + return ret; > } > > static int logical_ring_wait_for_space(struct drm_i915_gem_request *req, > @@ -2382,22 +2379,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 == ctx->i915->kernel_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 == ctx->i915->kernel_context) { > + 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); > } > } > > @@ -2472,7 +2468,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; >
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 2613708..bbb23da 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 (dev_priv->kernel_context == ctx) - seq_printf(m, "(default context %s) ", - ring->name); - } + if (ctx == dev_priv->kernel_context) + seq_printf(m, "(kernel 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 (dev_priv->kernel_context != ctx) + list_for_each_entry(ctx, &dev_priv->context_list, link) + if (ctx != dev_priv->kernel_context) + 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 8f101121..4f45eb2 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->i915->kernel_context) - intel_lr_context_unpin(req); - } + if (i915.enable_execlists && ctx != req->i915->kernel_context) + 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 aaaa5a3..8c4c9b9 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -660,16 +660,10 @@ 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->i915->kernel_context) { - ret = intel_lr_context_pin(request); - if (ret) - return ret; - } - if (i915.enable_guc_submission) { /* * Check that the GuC has space for the request before @@ -683,7 +677,10 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request return ret; } - return 0; + if (request->ctx != request->i915->kernel_context) + ret = intel_lr_context_pin(request); + + return ret; } static int logical_ring_wait_for_space(struct drm_i915_gem_request *req, @@ -2382,22 +2379,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 == ctx->i915->kernel_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 == ctx->i915->kernel_context) { + 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); } } @@ -2472,7 +2468,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;
There are a few bits of code which the transformations implemented by the previous patch reveal to be suboptimal, once the notion of a per- ring default context has gone away. So this tidies up the leftovers. It could have been squashed into the previous patch, but that would have made that patch less clearly a simple transformation. In particular, any change which alters the code block structure or indentation has been deferred into this separate patch, because such things tend to make diffs more difficult to read. 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 | 38 +++++++++++++++++-------------------- 3 files changed, 24 insertions(+), 35 deletions(-)