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@xxxxxxxxx> > --- > 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@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
