Re: [PATCH 02/10] drm/i915/execlists: Leave tell-tales as to why pending[] is bad

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

On 10/10/2019 08:14, Chris Wilson wrote:
> Before we BUG out with bad pending state, leave a telltale as to which
> test failed.
>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> ---
>   drivers/gpu/drm/i915/gt/intel_lrc.c | 30 ++++++++++++++++++++++++-----
>   drivers/gpu/drm/i915/i915_gem.h     |  8 ++++----
>   2 files changed, 29 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index a0777b3ad68a..5040fbdd81af 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -1138,25 +1138,45 @@ assert_pending_valid(const struct intel_engine_execlists *execlists,
>   
>   	trace_ports(execlists, msg, execlists->pending);
>   
> -	if (!execlists->pending[0])
> +	if (!execlists->pending[0]) {
> +		GEM_TRACE_ERR("Nothing pending for promotion!\n");
>   		return false;
> +	}
>   
> -	if (execlists->pending[execlists_num_ports(execlists)])
> +	if (execlists->pending[execlists_num_ports(execlists)]) {
> +		GEM_TRACE_ERR("Excess pending[%d] for promotion!\n",
> +			      execlists_num_ports(execlists));
>   		return false;
> +	}
>   
>   	for (port = execlists->pending; (rq = *port); port++) {
> -		if (ce == rq->hw_context)
> +		if (ce == rq->hw_context) {
> +			GEM_TRACE_ERR("Duplicate context in pending[%zd]\n",
> +				      port - execlists->pending);
>   			return false;
> +		}
>   
>   		ce = rq->hw_context;
>   		if (i915_request_completed(rq))
>   			continue;
>   
> -		if (i915_active_is_idle(&ce->active))
> +		if (i915_active_is_idle(&ce->active)) {
> +			GEM_TRACE_ERR("Inactive context in pending[%zd]\n",
> +				      port - execlists->pending);
> +			return false;
> +		}
> +
> +		if (!i915_vma_is_pinned(ce->state)) {
> +			GEM_TRACE_ERR("Unpinned context in pending[%zd]\n",
> +				      port - execlists->pending);
>   			return false;
> +		}
>   
> -		if (!i915_vma_is_pinned(ce->state))
> +		if (!i915_vma_is_pinned(ce->ring->vma)) {
> +			GEM_TRACE_ERR("Unpinned ringbuffer in pending[%zd]\n",
> +				      port - execlists->pending);
>   			return false;
> +		}
>   	}
>   
>   	return ce;
> diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h
> index 6795f1daa3d5..63dab3765106 100644
> --- a/drivers/gpu/drm/i915/i915_gem.h
> +++ b/drivers/gpu/drm/i915/i915_gem.h
> @@ -37,10 +37,8 @@ struct drm_i915_private;
>   #define GEM_SHOW_DEBUG() (drm_debug & DRM_UT_DRIVER)
>   
>   #define GEM_BUG_ON(condition) do { if (unlikely((condition))) {	\
> -		pr_err("%s:%d GEM_BUG_ON(%s)\n", \
> -		       __func__, __LINE__, __stringify(condition)); \
> -		GEM_TRACE("%s:%d GEM_BUG_ON(%s)\n", \
> -			  __func__, __LINE__, __stringify(condition)); \
> +		GEM_TRACE_ERR("%s:%d GEM_BUG_ON(%s)\n", \
> +			      __func__, __LINE__, __stringify(condition)); \
>   		BUG(); \
>   		} \
>   	} while(0)
> @@ -66,11 +64,13 @@ struct drm_i915_private;
>   
>   #if IS_ENABLED(CONFIG_DRM_I915_TRACE_GEM)
>   #define GEM_TRACE(...) trace_printk(__VA_ARGS__)
> +#define GEM_TRACE_ERR(...) do { pr_err(__VA_ARGS__); trace_printk(__VA_ARGS__); } while (0)
>   #define GEM_TRACE_DUMP() ftrace_dump(DUMP_ALL)
>   #define GEM_TRACE_DUMP_ON(expr) \
>   	do { if (expr) ftrace_dump(DUMP_ALL); } while (0)
>   #else
>   #define GEM_TRACE(...) do { } while (0)
> +#define GEM_TRACE_ERR(...) do { } while (0)
>   #define GEM_TRACE_DUMP() do { } while (0)
>   #define GEM_TRACE_DUMP_ON(expr) BUILD_BUG_ON_INVALID(expr)
>   #endif

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx