[Intel-gfx] [PATCH v2 1/3] drm/i915: Tune down debug output when context is banned
Mika Kuoppala
mika.kuoppala at linux.intel.com
Wed Jan 29 16:28:55 CET 2014
Ben Widawsky <ben at bwidawsk.net> writes:
> 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 tried to take advantage of above and came up with:
1391007939-5741-2-git-send-email-mika.kuoppala at intel.com
With 4/4 applied on that series, we only pass i915_hw_context *'s around.
> I'd also suggest making i915_gem_context_is_default() check regardless
> of stop_rings. The last request is optional.
>
Hmm. I think logic is correct here.
Only output if tests are not running and if so, only if we are
dealing with default context.
-Mika
>> 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
More information about the Intel-gfx
mailing list