[Intel-gfx] [PATCH v3 3/3] drm/i915: tidy up a few leftovers
Dave Gordon
david.s.gordon at intel.com
Wed Jan 13 04:41:57 PST 2016
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 at 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 at 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.
More information about the Intel-gfx
mailing list