[Intel-gfx] [PATCH v2 1/3] drm/i915: Tune down debug output when context is banned

[Ben Widawsky]

On Wed, Jan 22, 2014 at 05:41:29PM +0200, Mika Kuoppala wrote:
> If we have stopped rings then we know that test is running
> so no need for spam. In addition, only spam when default
> context gets banned.
> 
> v2: - make sure default context ban gets shown (Chris)
>     - use helper for checking for default context, everywhere (Chris)
> 

Overall, passing requests around this much to these functions seems
awkward to me. A lot of my comments are targeting that. If you're
totally of a different mind, I can reconsider.

> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=73652
> Signed-off-by: Mika Kuoppala <mika.kuoppala at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h         |    5 +++++
>  drivers/gpu/drm/i915/i915_gem.c         |   18 +++++++++++-------
>  drivers/gpu/drm/i915/i915_gem_context.c |    9 ++-------
>  3 files changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f888fea..3a43388 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2267,6 +2267,11 @@ static inline void i915_gem_context_unreference(struct i915_hw_context *ctx)
>  		kref_put(&ctx->ref, i915_gem_context_free);
>  }
>  
> +static inline bool i915_gem_context_is_default(struct i915_hw_context *ctx)
> +{
> +	return (ctx->id == DEFAULT_CONTEXT_ID);
> +}
> +

Drop the parenthesis - I know that it's my bad.

>  int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
>  				  struct drm_file *file);
>  int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 5fcdb14..36d18d8 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2305,15 +2305,20 @@ static bool i915_request_guilty(struct drm_i915_gem_request *request,
>  	return false;
>  }
>  
> -static bool i915_context_is_banned(const struct i915_ctx_hang_stats *hs)
> +static bool i915_context_is_banned(const struct drm_i915_gem_request *request,
> +				   const struct i915_ctx_hang_stats *hs)
>  {
>  	const unsigned long elapsed = get_seconds() - hs->guilty_ts;
> +	struct drm_i915_private *dev_priv = request->ring->dev->dev_private;
>  
>  	if (hs->banned)
>  		return true;
>  
>  	if (elapsed <= DRM_I915_CTX_BAN_PERIOD) {
> -		DRM_ERROR("context hanging too fast, declaring banned!\n");
> +		if (dev_priv->gpu_error.stop_rings == 0 &&
> +		    request->ctx && i915_gem_context_is_default(request->ctx))
> +			DRM_ERROR("context hanging too fast, declaring banned!\n");
> +

Correct me if I am wrong, but, request->ctx should always be non-NULL
when using full PPGTT, and hardware contexts (not sure if this patch is
only targeted for that). In case of the latter, HAS_HW_CONTEXTS should
be == request->ctx. Passing a request to a function which shouldn't care
about requests feels bad to me. As such, I'd vote for:

i915_context_is_banned(struct i915_hw_context *ctx) - you can similarly
thread the ctx up through the call stack, instead of the hs.

I'd also suggest making i915_gem_context_is_default() check regardless
of stop_rings. The last request is optional.

>  		return true;
>  	}
>  
> @@ -2347,17 +2352,16 @@ static void i915_set_reset_status(struct intel_ring_buffer *ring,
>  		guilty = true;
>  	}
>  
> -	/* If contexts are disabled or this is the default context, use
> -	 * file_priv->reset_state
> -	 */
> -	if (request->ctx && request->ctx->id != DEFAULT_CONTEXT_ID)
> +	/* For default context, we track reset statistics per file_priv
> +	 * to allow more fine grained control */
> +	if (request->ctx && !i915_gem_context_is_default(request->ctx))

As I said above, you could replace request->ctx here if you wanted.

>  		hs = &request->ctx->hang_stats;
>  	else if (request->file_priv)
>  		hs = &request->file_priv->private_default_ctx->hang_stats;
>  
>  	if (hs) {
>  		if (guilty) {
> -			hs->banned = i915_context_is_banned(hs);
> +			hs->banned = i915_context_is_banned(request, hs);
>  			hs->batch_active++;
>  			hs->guilty_ts = get_seconds();
>  		} else {
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 112f865..4c55730 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -228,11 +228,6 @@ err_out:
>  	return ERR_PTR(ret);
>  }
>  
> -static inline bool is_default_context(struct i915_hw_context *ctx)
> -{
> -	return (ctx->id == DEFAULT_CONTEXT_ID);
> -}
> -
>  /**
>   * The default context needs to exist per ring that uses contexts. It stores the
>   * context state of the GPU for applications that don't utilize HW contexts, as
> @@ -474,7 +469,7 @@ static int context_idr_cleanup(int id, void *p, void *data)
>  	struct i915_hw_context *ctx = p;
>  
>  	/* Ignore the default context because close will handle it */
> -	if (is_default_context(ctx))
> +	if (i915_gem_context_is_default(ctx))
>  		return 0;
>  
>  	i915_gem_context_unreference(ctx);
> @@ -649,7 +644,7 @@ static int do_switch(struct intel_ring_buffer *ring,
>  		vma->bind_vma(vma, to->obj->cache_level, GLOBAL_BIND);
>  	}
>  
> -	if (!to->is_initialized || is_default_context(to))
> +	if (!to->is_initialized || i915_gem_context_is_default(to))
>  		hw_flags |= MI_RESTORE_INHIBIT;
>  
>  	ret = mi_set_context(ring, to, hw_flags);
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center