Re: [PATCH 08/62] drm/i915: Remove stop-rings debugfs interface

[Arun Siluvery <arun.siluvery@xxxxxxxxxxxxxxx>]

On 03/06/2016 22:06, Chris Wilson wrote:
> Now that we have (near) universal GPU recovery code, we can inject a
> real hang from userspace and not need any fakery. Not only does this
> mean that the testing is far more realistic, but we can simplify the
> kernel in the process.
>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c     | 35 --------------------------
>   drivers/gpu/drm/i915/i915_drv.c         | 17 ++-----------
>   drivers/gpu/drm/i915/i915_drv.h         | 19 --------------
>   drivers/gpu/drm/i915/i915_gem.c         | 44 ++++++++++-----------------------
>   drivers/gpu/drm/i915/intel_lrc.c        |  3 ---
>   drivers/gpu/drm/i915/intel_ringbuffer.c |  8 ------
>   drivers/gpu/drm/i915/intel_ringbuffer.h |  1 -
>   7 files changed, 15 insertions(+), 112 deletions(-)

looks good to me,
Reviewed-by: Arun Siluvery <arun.siluvery@xxxxxxxxxxxxxxx>

regards
Arun

> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index dd6cf222e8f5..8f576b443ff6 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4821,40 +4821,6 @@ DEFINE_SIMPLE_ATTRIBUTE(i915_wedged_fops,
>   			"%llu\n");
>
>   static int
> -i915_ring_stop_get(void *data, u64 *val)
> -{
> -	struct drm_device *dev = data;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -
> -	*val = dev_priv->gpu_error.stop_rings;
> -
> -	return 0;
> -}
> -
> -static int
> -i915_ring_stop_set(void *data, u64 val)
> -{
> -	struct drm_device *dev = data;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	int ret;
> -
> -	DRM_DEBUG_DRIVER("Stopping rings 0x%08llx\n", val);
> -
> -	ret = mutex_lock_interruptible(&dev->struct_mutex);
> -	if (ret)
> -		return ret;
> -
> -	dev_priv->gpu_error.stop_rings = val;
> -	mutex_unlock(&dev->struct_mutex);
> -
> -	return 0;
> -}
> -
> -DEFINE_SIMPLE_ATTRIBUTE(i915_ring_stop_fops,
> -			i915_ring_stop_get, i915_ring_stop_set,
> -			"0x%08llx\n");
> -
> -static int
>   i915_ring_missed_irq_get(void *data, u64 *val)
>   {
>   	struct drm_device *dev = data;
> @@ -5457,7 +5423,6 @@ static const struct i915_debugfs_files {
>   	{"i915_max_freq", &i915_max_freq_fops},
>   	{"i915_min_freq", &i915_min_freq_fops},
>   	{"i915_cache_sharing", &i915_cache_sharing_fops},
> -	{"i915_ring_stop", &i915_ring_stop_fops},
>   	{"i915_ring_missed_irq", &i915_ring_missed_irq_fops},
>   	{"i915_ring_test_irq", &i915_ring_test_irq_fops},
>   	{"i915_gem_drop_caches", &i915_drop_caches_fops},
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 7ba040141722..f2ac0cae929b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -2125,24 +2125,11 @@ int i915_reset(struct drm_i915_private *dev_priv)
>   		goto error;
>   	}
>
> +	pr_notice("drm/i915: Resetting chip after gpu hang\n");
> +
>   	i915_gem_reset(dev);
>
>   	ret = intel_gpu_reset(dev_priv, ALL_ENGINES);
> -
> -	/* Also reset the gpu hangman. */
> -	if (error->stop_rings != 0) {
> -		DRM_INFO("Simulated gpu hang, resetting stop_rings\n");
> -		error->stop_rings = 0;
> -		if (ret == -ENODEV) {
> -			DRM_INFO("Reset not implemented, but ignoring "
> -				 "error for simulated gpu hangs\n");
> -			ret = 0;
> -		}
> -	}
> -
> -	if (i915_stop_ring_allow_warn(dev_priv))
> -		pr_notice("drm/i915: Resetting chip after gpu hang\n");
> -
>   	if (ret) {
>   		if (ret != -ENODEV)
>   			DRM_ERROR("Failed to reset chip: %i\n", ret);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 3f075adf9e84..a48c0f4e1d42 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1393,13 +1393,6 @@ struct i915_gpu_error {
>   	 */
>   	wait_queue_head_t reset_queue;
>
> -	/* Userspace knobs for gpu hang simulation;
> -	 * combines both a ring mask, and extra flags
> -	 */
> -	u32 stop_rings;
> -#define I915_STOP_RING_ALLOW_BAN       (1 << 31)
> -#define I915_STOP_RING_ALLOW_WARN      (1 << 30)
> -
>   	/* For missed irq/seqno simulation. */
>   	unsigned long test_irq_rings;
>   };
> @@ -3292,18 +3285,6 @@ static inline u32 i915_reset_count(struct i915_gpu_error *error)
>   	return ((i915_reset_counter(error) & ~I915_WEDGED) + 1) / 2;
>   }
>
> -static inline bool i915_stop_ring_allow_ban(struct drm_i915_private *dev_priv)
> -{
> -	return dev_priv->gpu_error.stop_rings == 0 ||
> -		dev_priv->gpu_error.stop_rings & I915_STOP_RING_ALLOW_BAN;
> -}
> -
> -static inline bool i915_stop_ring_allow_warn(struct drm_i915_private *dev_priv)
> -{
> -	return dev_priv->gpu_error.stop_rings == 0 ||
> -		dev_priv->gpu_error.stop_rings & I915_STOP_RING_ALLOW_WARN;
> -}
> -
>   void i915_gem_reset(struct drm_device *dev);
>   bool i915_gem_clflush_object(struct drm_i915_gem_object *obj, bool force);
>   int __must_check i915_gem_init(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 0f487e3b920c..f48f54193972 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2703,44 +2703,30 @@ void __i915_add_request(struct drm_i915_gem_request *request,
>   	i915_gem_mark_busy(dev_priv, engine);
>   }
>
> -static bool i915_context_is_banned(struct drm_i915_private *dev_priv,
> -				   const struct i915_gem_context *ctx)
> +static bool i915_context_is_banned(const struct i915_gem_context *ctx)
>   {
>   	unsigned long elapsed;
>
> -	elapsed = get_seconds() - ctx->hang_stats.guilty_ts;
> -
>   	if (ctx->hang_stats.banned)
>   		return true;
>
> +	elapsed = get_seconds() - ctx->hang_stats.guilty_ts;
>   	if (ctx->hang_stats.ban_period_seconds &&
>   	    elapsed <= ctx->hang_stats.ban_period_seconds) {
> -		if (!i915_gem_context_is_default(ctx)) {
> -			DRM_DEBUG("context hanging too fast, banning!\n");
> -			return true;
> -		} else if (i915_stop_ring_allow_ban(dev_priv)) {
> -			if (i915_stop_ring_allow_warn(dev_priv))
> -				DRM_ERROR("gpu hanging too fast, banning!\n");
> -			return true;
> -		}
> +		DRM_DEBUG("context hanging too fast, banning!\n");
> +		return true;
>   	}
>
>   	return false;
>   }
>
> -static void i915_set_reset_status(struct drm_i915_private *dev_priv,
> -				  struct i915_gem_context *ctx,
> +static void i915_set_reset_status(struct i915_gem_context *ctx,
>   				  const bool guilty)
>   {
> -	struct i915_ctx_hang_stats *hs;
> -
> -	if (WARN_ON(!ctx))
> -		return;
> -
> -	hs = &ctx->hang_stats;
> +	struct i915_ctx_hang_stats *hs = &ctx->hang_stats;
>
>   	if (guilty) {
> -		hs->banned = i915_context_is_banned(dev_priv, ctx);
> +		hs->banned = i915_context_is_banned(ctx);
>   		hs->batch_active++;
>   		hs->guilty_ts = get_seconds();
>   	} else {
> @@ -2867,27 +2853,23 @@ i915_gem_find_active_request(struct intel_engine_cs *engine)
>   	return NULL;
>   }
>
> -static void i915_gem_reset_engine_status(struct drm_i915_private *dev_priv,
> -				       struct intel_engine_cs *engine)
> +static void i915_gem_reset_engine_status(struct intel_engine_cs *engine)
>   {
>   	struct drm_i915_gem_request *request;
>   	bool ring_hung;
>
>   	request = i915_gem_find_active_request(engine);
> -
>   	if (request == NULL)
>   		return;
>
>   	ring_hung = engine->hangcheck.score >= HANGCHECK_SCORE_RING_HUNG;
>
> -	i915_set_reset_status(dev_priv, request->ctx, ring_hung);
> -
> +	i915_set_reset_status(request->ctx, ring_hung);
>   	list_for_each_entry_continue(request, &engine->request_list, list)
> -		i915_set_reset_status(dev_priv, request->ctx, false);
> +		i915_set_reset_status(request->ctx, false);
>   }
>
> -static void i915_gem_reset_engine_cleanup(struct drm_i915_private *dev_priv,
> -					struct intel_engine_cs *engine)
> +static void i915_gem_reset_engine_cleanup(struct intel_engine_cs *engine)
>   {
>   	struct intel_ringbuffer *buffer;
>
> @@ -2957,10 +2939,10 @@ void i915_gem_reset(struct drm_device *dev)
>   	 * their reference to the objects, the inspection must be done first.
>   	 */
>   	for_each_engine(engine, dev_priv)
> -		i915_gem_reset_engine_status(dev_priv, engine);
> +		i915_gem_reset_engine_status(engine);
>
>   	for_each_engine(engine, dev_priv)
> -		i915_gem_reset_engine_cleanup(dev_priv, engine);
> +		i915_gem_reset_engine_cleanup(engine);
>
>   	i915_gem_context_reset(dev);
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 9e19b2c5b3ae..0742a849acce 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -764,9 +764,6 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request)
>   	intel_logical_ring_emit(ringbuf, MI_NOOP);
>   	intel_logical_ring_advance(ringbuf);
>
> -	if (intel_engine_stopped(engine))
> -		return 0;
> -
>   	/* We keep the previous context alive until we retire the following
>   	 * request. This ensures that any the context object is still pinned
>   	 * for any residual writes the HW makes into it on the context switch
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 161c0792b1bf..327ad7fdf118 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -58,18 +58,10 @@ void intel_ring_update_space(struct intel_ringbuffer *ringbuf)
>   					    ringbuf->tail, ringbuf->size);
>   }
>
> -bool intel_engine_stopped(struct intel_engine_cs *engine)
> -{
> -	struct drm_i915_private *dev_priv = engine->i915;
> -	return dev_priv->gpu_error.stop_rings & intel_engine_flag(engine);
> -}
> -
>   static void __intel_ring_advance(struct intel_engine_cs *engine)
>   {
>   	struct intel_ringbuffer *ringbuf = engine->buffer;
>   	ringbuf->tail &= ringbuf->size - 1;
> -	if (intel_engine_stopped(engine))
> -		return;
>   	engine->write_tail(engine, ringbuf->tail);
>   }
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index d0cd9a1aa80e..6017367e94fb 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -480,7 +480,6 @@ static inline void intel_ring_advance(struct intel_engine_cs *engine)
>   }
>   int __intel_ring_space(int head, int tail, int size);
>   void intel_ring_update_space(struct intel_ringbuffer *ringbuf);
> -bool intel_engine_stopped(struct intel_engine_cs *engine);
>
>   int __must_check intel_engine_idle(struct intel_engine_cs *engine);
>   void intel_ring_init_seqno(struct intel_engine_cs *engine, u32 seqno);

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