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@xxxxxxxxxxxx>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxx>
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
