Re: [PATCH v3 3/3] drm/i915: tidy up a few leftovers

From: Daniel Vetter <daniel@ffwll.ch>
To: Dave Gordon <david.s.gordon@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v3 3/3] drm/i915: tidy up a few leftovers
Date: Tue, 12 Jan 2016 14:53:01 +0100	[thread overview]
Message-ID: <20160112135301.GJ4588@phenom.ffwll.local> (raw)
In-Reply-To: <1452162052-22573-4-git-send-email-david.s.gordon@intel.com>

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx