Re: [Intel-gfx] [PATCH 2/7] drm/i915: Mark the addition of the initial-breadcrumb in the request

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

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

> The initial-breadcrumb is used to mark the end of the awaiting and the
> beginning of the user payload. We verify that we do not start the user
> payload before all signaler are completed, checking our semaphore setup
> by looking for the initial breadcrumb being written too early. We also
> want to ensure that we do not add semaphore waits after we have already
> closed the semaphore section, an issue for later deferred waits.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/gt/intel_lrc.c |  5 ++++-
>  drivers/gpu/drm/i915/i915_request.c |  7 ++++++-
>  drivers/gpu/drm/i915/i915_request.h | 27 ++++++++++++++++++++-------
>  3 files changed, 30 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 32feb2a27dfc..51470e96cdd4 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -1886,7 +1886,7 @@ static void defer_request(struct i915_request *rq, struct list_head * const pl)
>  				continue;
>  
>  			/* No waiter should start before its signaler */
> -			GEM_BUG_ON(w->context->timeline->has_initial_breadcrumb &&
> +			GEM_BUG_ON(i915_request_has_initial_breadcrumb(w) &&
>  				   i915_request_started(w) &&
>  				   !i915_request_completed(rq));
>  
> @@ -3493,6 +3493,7 @@ static int gen8_emit_init_breadcrumb(struct i915_request *rq)
>  {
>  	u32 *cs;
>  
> +	GEM_BUG_ON(i915_request_has_initial_breadcrumb(rq));
>  	if (!i915_request_timeline(rq)->has_initial_breadcrumb)
>  		return 0;
>  
> @@ -3519,6 +3520,8 @@ static int gen8_emit_init_breadcrumb(struct i915_request *rq)
>  	/* Record the updated position of the request's payload */
>  	rq->infix = intel_ring_offset(rq, cs);
>  
> +	__set_bit(I915_FENCE_FLAG_INITIAL_BREADCRUMB, &rq->fence.flags);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 2d5b98549ddc..00b7c4eb3f32 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -951,6 +951,7 @@ __emit_semaphore_wait(struct i915_request *to,
>  	u32 *cs;
>  
>  	GEM_BUG_ON(INTEL_GEN(to->i915) < 8);
> +	GEM_BUG_ON(i915_request_has_initial_breadcrumb(to));
>  
>  	/* We need to pin the signaler's HWSP until we are finished reading. */
>  	err = intel_timeline_read_hwsp(from, to, &hwsp_offset);
> @@ -1000,6 +1001,9 @@ emit_semaphore_wait(struct i915_request *to,
>  	if (!intel_context_use_semaphores(to->context))
>  		goto await_fence;
>  
> +	if (i915_request_has_initial_breadcrumb(to))
> +		goto await_fence;
> +
>  	if (!rcu_access_pointer(from->hwsp_cacheline))
>  		goto await_fence;
>  
> @@ -1256,7 +1260,8 @@ __i915_request_await_execution(struct i915_request *to,
>  	 * immediate execution, and so we must wait until it reaches the
>  	 * active slot.
>  	 */
> -	if (intel_engine_has_semaphores(to->engine)) {
> +	if (intel_engine_has_semaphores(to->engine) &&
> +	    !i915_request_has_initial_breadcrumb(to)) {
>  		err = __emit_semaphore_wait(to, from, from->fence.seqno - 1);
>  		if (err < 0)
>  			return err;
> diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h
> index d8ce908e1346..98ae2dc82371 100644
> --- a/drivers/gpu/drm/i915/i915_request.h
> +++ b/drivers/gpu/drm/i915/i915_request.h
> @@ -84,19 +84,26 @@ enum {
>  	I915_FENCE_FLAG_PQUEUE,
>  
>  	/*
> -	 * I915_FENCE_FLAG_SIGNAL - this request is currently on signal_list
> +	 * I915_FENCE_FLAG_HOLD - this request is currently on hold
>  	 *
> -	 * Internal bookkeeping used by the breadcrumb code to track when
> -	 * a request is on the various signal_list.
> +	 * This request has been suspended, pending an ongoing investigation.
>  	 */
> -	I915_FENCE_FLAG_SIGNAL,
> +	I915_FENCE_FLAG_HOLD,
>  
>  	/*
> -	 * I915_FENCE_FLAG_HOLD - this request is currently on hold
> +	 * I915_FENCE_FLAG_INITIAL_BREADCRUMB - this request has the initial
> +	 * breadcrumb that marks the end of semaphore waits and start of the
> +	 * user payload.
> +	 */
> +	I915_FENCE_FLAG_INITIAL_BREADCRUMB,
> +
> +	/*
> +	 * I915_FENCE_FLAG_SIGNAL - this request is currently on signal_list
>  	 *
> -	 * This request has been suspended, pending an ongoing investigation.
> +	 * Internal bookkeeping used by the breadcrumb code to track when
> +	 * a request is on the various signal_list.
>  	 */
> -	I915_FENCE_FLAG_HOLD,
> +	I915_FENCE_FLAG_SIGNAL,
>  
>  	/*
>  	 * I915_FENCE_FLAG_NOPREEMPT - this request should not be preempted
> @@ -390,6 +397,12 @@ static inline bool i915_request_in_priority_queue(const struct i915_request *rq)
>  	return test_bit(I915_FENCE_FLAG_PQUEUE, &rq->fence.flags);
>  }
>  
> +static inline bool
> +i915_request_has_initial_breadcrumb(const struct i915_request *rq)
> +{
> +	return test_bit(I915_FENCE_FLAG_INITIAL_BREADCRUMB, &rq->fence.flags);
> +}
> +
>  /**
>   * Returns true if seq1 is later than seq2.
>   */
> -- 
> 2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx