Re: [PATCH 1/4] drm/i915: Use i915_hw_context to set reset stats

[Ben Widawsky <ben@bwidawsk.net>]

On Wed, Jan 29, 2014 at 05:05:36PM +0200, Mika Kuoppala wrote:
> With full ppgtt support drm_i915_file_private gained knowledge
> about the default context. Also reset stats are now inside
> i915_hw_context so we can use proper abstraction.
> 
> Suggested-by: Ben Widawsky <ben@bwidawsk.net>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c |   44 ++++++++++++++++++++-------------------
>  1 file changed, 23 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 39770f7..e41d520 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2305,11 +2305,17 @@ 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 i915_hw_context *ctx)
>  {
> -	const unsigned long elapsed = get_seconds() - hs->guilty_ts;
> +	struct drm_i915_private *dev_priv;
> +	unsigned long elapsed;
>  
> -	if (hs->banned)
> +	BUG_ON(!ctx->last_ring);

If you're that worried, just pass in dev, or dev_priv. I think it's a
valid BUG_ON btw, just a weird place to put it. I'd vote to shove the
BUG_ON in i915_set_reset_status() - and drop this completely.

> +
> +	dev_priv = ctx->last_ring->dev->dev_private;
> +	elapsed = get_seconds() - ctx->hang_stats.guilty_ts;
> +
> +	if (ctx->hang_stats.banned)
>  		return true;
>  
>  	if (elapsed <= DRM_I915_CTX_BAN_PERIOD) {
> @@ -2324,9 +2330,10 @@ static void i915_set_reset_status(struct intel_ring_buffer *ring,
>  				  struct drm_i915_gem_request *request,
>  				  u32 acthd)
>  {
> -	struct i915_ctx_hang_stats *hs = NULL;
>  	bool inside, guilty;
>  	unsigned long offset = 0;
> +	struct i915_hw_context *ctx = request->ctx;
> +	struct i915_ctx_hang_stats *hs;
>  
>  	/* Innocent until proven guilty */
>  	guilty = false;
> @@ -2341,28 +2348,23 @@ static void i915_set_reset_status(struct intel_ring_buffer *ring,
>  			  ring->name,
>  			  inside ? "inside" : "flushing",
>  			  offset,
> -			  request->ctx ? request->ctx->id : 0,
> +			  ctx ? ctx->id : 0,

You may as well drop this ternary and move your WARN_ON(!ctx) up above.


With or without my requests:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

>  			  acthd);
>  
>  		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)
> -		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->batch_active++;
> -			hs->guilty_ts = get_seconds();
> -		} else {
> -			hs->batch_pending++;
> -		}
> +	if (WARN_ON(!ctx))
> +		return;
> +
> +	hs = &ctx->hang_stats;
> +
> +	if (guilty) {
> +		hs->banned = i915_context_is_banned(ctx);
> +		hs->batch_active++;
> +		hs->guilty_ts = get_seconds();
> +	} else {
> +		hs->batch_pending++;
>  	}
>  }
>  

-- 
Ben Widawsky, Intel Open Source Technology Center