[Intel-gfx] [PATCH v2 8/9] drm/i915: Apply a mb between emitting the request and hangcheck

[Mika Kuoppala]

Chris Wilson <chris at chris-wilson.co.uk> writes:

> [ text/plain ]
> Seal the request and mark it as pending execution before we submit it to
> hardware. We assume that the actual submission cannot fail (that
> guarantee is provided by preallocating space in the request for the
> submission). As we may inspect this state without holding any locks
> during hangcheck we should apply a barrier to ensure that we do
> not see a more recent value in the HWS than we are tracking.
>
> Based on a patch by Mika Kuoppala.
>
> Suggested-by: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>

Reviewed-by: Mika Kuoppala <mika.kuoppala at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 39 ++++++++++++++++++++++-----------------
>  drivers/gpu/drm/i915/i915_irq.c |  3 ++-
>  2 files changed, 24 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 7506e850913e..5a65a7663b88 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2575,6 +2575,28 @@ void __i915_add_request(struct drm_i915_gem_request *request,
>  		WARN(ret, "*_ring_flush_all_caches failed: %d!\n", ret);
>  	}
>  
> +	trace_i915_gem_request_add(request);
> +
> +	request->head = request_start;
> +
> +	/* Whilst this request exists, batch_obj will be on the
> +	 * active_list, and so will hold the active reference. Only when this
> +	 * request is retired will the the batch_obj be moved onto the
> +	 * inactive_list and lose its active reference. Hence we do not need
> +	 * to explicitly hold another reference here.
> +	 */
> +	request->batch_obj = obj;
> +
> +	/* Seal the request and mark it as pending execution. Note that
> +	 * we may inspect this state, without holding any locks, during
> +	 * hangcheck. Hence we apply the barrier to ensure that we do not
> +	 * see a more recent value in the hws than we are tracking.
> +	 */
> +	request->emitted_jiffies = jiffies;
> +	request->previous_seqno = engine->last_submitted_seqno;
> +	smp_store_mb(engine->last_submitted_seqno, request->seqno);
> +	list_add_tail(&request->list, &engine->request_list);
> +
>  	/* Record the position of the start of the request so that
>  	 * should we detect the updated seqno part-way through the
>  	 * GPU processing the request, we never over-estimate the
> @@ -2592,23 +2614,6 @@ void __i915_add_request(struct drm_i915_gem_request *request,
>  	/* Not allowed to fail! */
>  	WARN(ret, "emit|add_request failed: %d!\n", ret);
>  
> -	request->head = request_start;
> -
> -	/* Whilst this request exists, batch_obj will be on the
> -	 * active_list, and so will hold the active reference. Only when this
> -	 * request is retired will the the batch_obj be moved onto the
> -	 * inactive_list and lose its active reference. Hence we do not need
> -	 * to explicitly hold another reference here.
> -	 */
> -	request->batch_obj = obj;
> -
> -	request->emitted_jiffies = jiffies;
> -	request->previous_seqno = engine->last_submitted_seqno;
> -	engine->last_submitted_seqno = request->seqno;
> -	list_add_tail(&request->list, &engine->request_list);
> -
> -	trace_i915_gem_request_add(request);
> -
>  	i915_queue_hangcheck(engine->dev);
>  
>  	queue_delayed_work(dev_priv->wq,
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 5bec13844800..a5eb77d1f8cb 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2806,7 +2806,8 @@ static bool
>  ring_idle(struct intel_engine_cs *engine, u32 seqno)
>  {
>  	return (list_empty(&engine->request_list) ||
> -		i915_seqno_passed(seqno, engine->last_submitted_seqno));
> +		i915_seqno_passed(seqno,
> +				  READ_ONCE(engine->last_submitted_seqno)));
>  }
>  
>  static bool
> -- 
> 2.8.0.rc3