[PATCH] drm/i915: Pull seqno started checks together

[PATCH] drm/i915: Pull seqno started checks together

[Chris Wilson]

We have a few instances of checking seqno-1 to see if the HW has started
the request. Pull those together under a helper.

Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_request.c      |  9 +++---
 drivers/gpu/drm/i915/i915_request.h      | 39 +++++++++++++++---------
 drivers/gpu/drm/i915/intel_breadcrumbs.c |  6 ++--
 drivers/gpu/drm/i915/intel_engine_cs.c   |  3 +-
 drivers/gpu/drm/i915/intel_hangcheck.c   |  2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.h  | 32 +++++++++++++++----
 6 files changed, 59 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 5c2c93cbab12..09ed48833b54 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -527,7 +527,7 @@ void __i915_request_submit(struct i915_request *request)
 
 	seqno = timeline_get_seqno(&engine->timeline);
 	GEM_BUG_ON(!seqno);
-	GEM_BUG_ON(i915_seqno_passed(intel_engine_get_seqno(engine), seqno));
+	GEM_BUG_ON(intel_engine_signaled(engine, seqno));
 
 	/* We may be recursing from the signal callback of another i915 fence */
 	spin_lock_nested(&request->lock, SINGLE_DEPTH_NESTING);
@@ -579,8 +579,7 @@ void __i915_request_unsubmit(struct i915_request *request)
 	 */
 	GEM_BUG_ON(!request->global_seqno);
 	GEM_BUG_ON(request->global_seqno != engine->timeline.seqno);
-	GEM_BUG_ON(i915_seqno_passed(intel_engine_get_seqno(engine),
-				     request->global_seqno));
+	GEM_BUG_ON(intel_engine_has_completed(engine, request->global_seqno));
 	engine->timeline.seqno--;
 
 	/* We may be recursing from the signal callback of another i915 fence */
@@ -1205,7 +1204,7 @@ static bool __i915_spin_request(const struct i915_request *rq,
 	 * it is a fair assumption that it will not complete within our
 	 * relatively short timeout.
 	 */
-	if (!i915_seqno_passed(intel_engine_get_seqno(engine), seqno - 1))
+	if (!intel_engine_has_started(engine, seqno))
 		return false;
 
 	/*
@@ -1222,7 +1221,7 @@ static bool __i915_spin_request(const struct i915_request *rq,
 	irq = READ_ONCE(engine->breadcrumbs.irq_count);
 	timeout_us += local_clock_us(&cpu);
 	do {
-		if (i915_seqno_passed(intel_engine_get_seqno(engine), seqno))
+		if (intel_engine_has_completed(engine, seqno))
 			return seqno == i915_request_global_seqno(rq);
 
 		/*
diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h
index e1c9365dfefb..05888de2e045 100644
--- a/drivers/gpu/drm/i915/i915_request.h
+++ b/drivers/gpu/drm/i915/i915_request.h
@@ -272,7 +272,10 @@ long i915_request_wait(struct i915_request *rq,
 #define I915_WAIT_ALL		BIT(2) /* used by i915_gem_object_wait() */
 #define I915_WAIT_FOR_IDLE_BOOST BIT(3)
 
-static inline u32 intel_engine_get_seqno(struct intel_engine_cs *engine);
+static inline bool intel_engine_has_started(struct intel_engine_cs *engine,
+					    u32 seqno);
+static inline bool intel_engine_has_completed(struct intel_engine_cs *engine,
+					    u32 seqno);
 
 /**
  * Returns true if seq1 is later than seq2.
@@ -282,11 +285,31 @@ static inline bool i915_seqno_passed(u32 seq1, u32 seq2)
 	return (s32)(seq1 - seq2) >= 0;
 }
 
+/**
+ * i915_request_started - check if the request has begun being executed
+ * @rq: the request
+ *
+ * Returns true if the request has been submitted to hardware, and the hardware
+ * has advanced passed the end of the previous request and so should be either
+ * currently processing the request (though it may be preempted and so
+ * not necessarily the next request to complete) or have completed the request.
+ */
+static inline bool i915_request_started(const struct i915_request *rq)
+{
+	u32 seqno;
+
+	seqno = i915_request_global_seqno(rq);
+	if (!seqno) /* not yet submitted to HW */
+		return false;
+
+	return intel_engine_has_started(rq->engine, seqno);
+}
+
 static inline bool
 __i915_request_completed(const struct i915_request *rq, u32 seqno)
 {
 	GEM_BUG_ON(!seqno);
-	return i915_seqno_passed(intel_engine_get_seqno(rq->engine), seqno) &&
+	return intel_engine_has_completed(rq->engine, seqno) &&
 		seqno == i915_request_global_seqno(rq);
 }
 
@@ -301,18 +324,6 @@ static inline bool i915_request_completed(const struct i915_request *rq)
 	return __i915_request_completed(rq, seqno);
 }
 
-static inline bool i915_request_started(const struct i915_request *rq)
-{
-	u32 seqno;
-
-	seqno = i915_request_global_seqno(rq);
-	if (!seqno)
-		return false;
-
-	return i915_seqno_passed(intel_engine_get_seqno(rq->engine),
-				 seqno - 1);
-}
-
 static inline bool i915_sched_node_signaled(const struct i915_sched_node *node)
 {
 	const struct i915_request *rq =
diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index 1db6ba7d926e..84bf8d827136 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -256,8 +256,7 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
 	spin_unlock(&b->irq_lock);
 
 	rbtree_postorder_for_each_entry_safe(wait, n, &b->waiters, node) {
-		GEM_BUG_ON(!i915_seqno_passed(intel_engine_get_seqno(engine),
-					      wait->seqno));
+		GEM_BUG_ON(!intel_engine_signaled(engine, wait->seqno));
 		RB_CLEAR_NODE(&wait->node);
 		wake_up_process(wait->tsk);
 	}
@@ -508,8 +507,7 @@ bool intel_engine_add_wait(struct intel_engine_cs *engine,
 		return armed;
 
 	/* Make the caller recheck if its request has already started. */
-	return i915_seqno_passed(intel_engine_get_seqno(engine),
-				 wait->seqno - 1);
+	return intel_engine_has_started(engine, wait->seqno);
 }
 
 static inline bool chain_wakeup(struct rb_node *rb, int priority)
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 67c4fc5d737c..99d5a24219c1 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -968,8 +968,7 @@ bool intel_engine_is_idle(struct intel_engine_cs *engine)
 		return true;
 
 	/* Any inflight/incomplete requests? */
-	if (!i915_seqno_passed(intel_engine_get_seqno(engine),
-			       intel_engine_last_submit(engine)))
+	if (!intel_engine_signaled(engine, intel_engine_last_submit(engine)))
 		return false;
 
 	if (I915_SELFTEST_ONLY(engine->breadcrumbs.mock))
diff --git a/drivers/gpu/drm/i915/intel_hangcheck.c b/drivers/gpu/drm/i915/intel_hangcheck.c
index 2fc7a0dd0df9..e26d05a46451 100644
--- a/drivers/gpu/drm/i915/intel_hangcheck.c
+++ b/drivers/gpu/drm/i915/intel_hangcheck.c
@@ -142,7 +142,7 @@ static int semaphore_passed(struct intel_engine_cs *engine)
 	if (signaller->hangcheck.deadlock >= I915_NUM_ENGINES)
 		return -1;
 
-	if (i915_seqno_passed(intel_engine_get_seqno(signaller), seqno))
+	if (intel_engine_signaled(signaller, seqno))
 		return 1;
 
 	/* cursory check for an unkickable deadlock */
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 57f3787ed6ec..057b88c149f7 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -911,14 +911,10 @@ int intel_engine_stop_cs(struct intel_engine_cs *engine);
 u64 intel_engine_get_active_head(const struct intel_engine_cs *engine);
 u64 intel_engine_get_last_batch_head(const struct intel_engine_cs *engine);
 
-static inline u32 intel_engine_get_seqno(struct intel_engine_cs *engine)
-{
-	return intel_read_status_page(engine, I915_GEM_HWS_INDEX);
-}
-
 static inline u32 intel_engine_last_submit(struct intel_engine_cs *engine)
 {
-	/* We are only peeking at the tail of the submit queue (and not the
+	/*
+	 * We are only peeking at the tail of the submit queue (and not the
 	 * queue itself) in order to gain a hint as to the current active
 	 * state of the engine. Callers are not expected to be taking
 	 * engine->timeline->lock, nor are they expected to be concerned
@@ -928,6 +924,30 @@ static inline u32 intel_engine_last_submit(struct intel_engine_cs *engine)
 	return READ_ONCE(engine->timeline.seqno);
 }
 
+static inline u32 intel_engine_get_seqno(struct intel_engine_cs *engine)
+{
+	return intel_read_status_page(engine, I915_GEM_HWS_INDEX);
+}
+
+static inline bool intel_engine_signaled(struct intel_engine_cs *engine,
+					 u32 seqno)
+{
+	GEM_BUG_ON(!seqno);
+	return i915_seqno_passed(intel_engine_get_seqno(engine), seqno);
+}
+
+static inline bool intel_engine_has_completed(struct intel_engine_cs *engine,
+					      u32 seqno)
+{
+	return intel_engine_signaled(engine, seqno);
+}
+
+static inline bool intel_engine_has_started(struct intel_engine_cs *engine,
+					    u32 seqno)
+{
+	return intel_engine_signaled(engine, seqno - 1);
+}
+
 void intel_engine_get_instdone(struct intel_engine_cs *engine,
 			       struct intel_instdone *instdone);
 
-- 
2.18.0

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

Re: [PATCH] drm/i915: Pull seqno started checks together

[Mika Kuoppala]

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

> We have a few instances of checking seqno-1 to see if the HW has started
> the request. Pull those together under a helper.
>

Could you elaborate why you want both completed and signaled?
Otherwise it looks good.
-Mika

> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_request.c      |  9 +++---
>  drivers/gpu/drm/i915/i915_request.h      | 39 +++++++++++++++---------
>  drivers/gpu/drm/i915/intel_breadcrumbs.c |  6 ++--
>  drivers/gpu/drm/i915/intel_engine_cs.c   |  3 +-
>  drivers/gpu/drm/i915/intel_hangcheck.c   |  2 +-
>  drivers/gpu/drm/i915/intel_ringbuffer.h  | 32 +++++++++++++++----
>  6 files changed, 59 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 5c2c93cbab12..09ed48833b54 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -527,7 +527,7 @@ void __i915_request_submit(struct i915_request *request)
>  
>  	seqno = timeline_get_seqno(&engine->timeline);
>  	GEM_BUG_ON(!seqno);
> -	GEM_BUG_ON(i915_seqno_passed(intel_engine_get_seqno(engine), seqno));
> +	GEM_BUG_ON(intel_engine_signaled(engine, seqno));
>  
>  	/* We may be recursing from the signal callback of another i915 fence */
>  	spin_lock_nested(&request->lock, SINGLE_DEPTH_NESTING);
> @@ -579,8 +579,7 @@ void __i915_request_unsubmit(struct i915_request *request)
>  	 */
>  	GEM_BUG_ON(!request->global_seqno);
>  	GEM_BUG_ON(request->global_seqno != engine->timeline.seqno);
> -	GEM_BUG_ON(i915_seqno_passed(intel_engine_get_seqno(engine),
> -				     request->global_seqno));
> +	GEM_BUG_ON(intel_engine_has_completed(engine, request->global_seqno));
>  	engine->timeline.seqno--;
>  
>  	/* We may be recursing from the signal callback of another i915 fence */
> @@ -1205,7 +1204,7 @@ static bool __i915_spin_request(const struct i915_request *rq,
>  	 * it is a fair assumption that it will not complete within our
>  	 * relatively short timeout.
>  	 */
> -	if (!i915_seqno_passed(intel_engine_get_seqno(engine), seqno - 1))
> +	if (!intel_engine_has_started(engine, seqno))
>  		return false;
>  
>  	/*
> @@ -1222,7 +1221,7 @@ static bool __i915_spin_request(const struct i915_request *rq,
>  	irq = READ_ONCE(engine->breadcrumbs.irq_count);
>  	timeout_us += local_clock_us(&cpu);
>  	do {
> -		if (i915_seqno_passed(intel_engine_get_seqno(engine), seqno))
> +		if (intel_engine_has_completed(engine, seqno))
>  			return seqno == i915_request_global_seqno(rq);
>  
>  		/*
> diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h
> index e1c9365dfefb..05888de2e045 100644
> --- a/drivers/gpu/drm/i915/i915_request.h
> +++ b/drivers/gpu/drm/i915/i915_request.h
> @@ -272,7 +272,10 @@ long i915_request_wait(struct i915_request *rq,
>  #define I915_WAIT_ALL		BIT(2) /* used by i915_gem_object_wait() */
>  #define I915_WAIT_FOR_IDLE_BOOST BIT(3)
>  
> -static inline u32 intel_engine_get_seqno(struct intel_engine_cs *engine);
> +static inline bool intel_engine_has_started(struct intel_engine_cs *engine,
> +					    u32 seqno);
> +static inline bool intel_engine_has_completed(struct intel_engine_cs *engine,
> +					    u32 seqno);
>  
>  /**
>   * Returns true if seq1 is later than seq2.
> @@ -282,11 +285,31 @@ static inline bool i915_seqno_passed(u32 seq1, u32 seq2)
>  	return (s32)(seq1 - seq2) >= 0;
>  }
>  
> +/**
> + * i915_request_started - check if the request has begun being executed
> + * @rq: the request
> + *
> + * Returns true if the request has been submitted to hardware, and the hardware
> + * has advanced passed the end of the previous request and so should be either
> + * currently processing the request (though it may be preempted and so
> + * not necessarily the next request to complete) or have completed the request.
> + */
> +static inline bool i915_request_started(const struct i915_request *rq)
> +{
> +	u32 seqno;
> +
> +	seqno = i915_request_global_seqno(rq);
> +	if (!seqno) /* not yet submitted to HW */
> +		return false;
> +
> +	return intel_engine_has_started(rq->engine, seqno);
> +}
> +
>  static inline bool
>  __i915_request_completed(const struct i915_request *rq, u32 seqno)
>  {
>  	GEM_BUG_ON(!seqno);
> -	return i915_seqno_passed(intel_engine_get_seqno(rq->engine), seqno) &&
> +	return intel_engine_has_completed(rq->engine, seqno) &&
>  		seqno == i915_request_global_seqno(rq);
>  }
>  
> @@ -301,18 +324,6 @@ static inline bool i915_request_completed(const struct i915_request *rq)
>  	return __i915_request_completed(rq, seqno);
>  }
>  
> -static inline bool i915_request_started(const struct i915_request *rq)
> -{
> -	u32 seqno;
> -
> -	seqno = i915_request_global_seqno(rq);
> -	if (!seqno)
> -		return false;
> -
> -	return i915_seqno_passed(intel_engine_get_seqno(rq->engine),
> -				 seqno - 1);
> -}
> -
>  static inline bool i915_sched_node_signaled(const struct i915_sched_node *node)
>  {
>  	const struct i915_request *rq =
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index 1db6ba7d926e..84bf8d827136 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -256,8 +256,7 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
>  	spin_unlock(&b->irq_lock);
>  
>  	rbtree_postorder_for_each_entry_safe(wait, n, &b->waiters, node) {
> -		GEM_BUG_ON(!i915_seqno_passed(intel_engine_get_seqno(engine),
> -					      wait->seqno));
> +		GEM_BUG_ON(!intel_engine_signaled(engine, wait->seqno));
>  		RB_CLEAR_NODE(&wait->node);
>  		wake_up_process(wait->tsk);
>  	}
> @@ -508,8 +507,7 @@ bool intel_engine_add_wait(struct intel_engine_cs *engine,
>  		return armed;
>  
>  	/* Make the caller recheck if its request has already started. */
> -	return i915_seqno_passed(intel_engine_get_seqno(engine),
> -				 wait->seqno - 1);
> +	return intel_engine_has_started(engine, wait->seqno);
>  }
>  
>  static inline bool chain_wakeup(struct rb_node *rb, int priority)
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 67c4fc5d737c..99d5a24219c1 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -968,8 +968,7 @@ bool intel_engine_is_idle(struct intel_engine_cs *engine)
>  		return true;
>  
>  	/* Any inflight/incomplete requests? */
> -	if (!i915_seqno_passed(intel_engine_get_seqno(engine),
> -			       intel_engine_last_submit(engine)))
> +	if (!intel_engine_signaled(engine, intel_engine_last_submit(engine)))
>  		return false;
>  
>  	if (I915_SELFTEST_ONLY(engine->breadcrumbs.mock))
> diff --git a/drivers/gpu/drm/i915/intel_hangcheck.c b/drivers/gpu/drm/i915/intel_hangcheck.c
> index 2fc7a0dd0df9..e26d05a46451 100644
> --- a/drivers/gpu/drm/i915/intel_hangcheck.c
> +++ b/drivers/gpu/drm/i915/intel_hangcheck.c
> @@ -142,7 +142,7 @@ static int semaphore_passed(struct intel_engine_cs *engine)
>  	if (signaller->hangcheck.deadlock >= I915_NUM_ENGINES)
>  		return -1;
>  
> -	if (i915_seqno_passed(intel_engine_get_seqno(signaller), seqno))
> +	if (intel_engine_signaled(signaller, seqno))
>  		return 1;
>  
>  	/* cursory check for an unkickable deadlock */
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 57f3787ed6ec..057b88c149f7 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -911,14 +911,10 @@ int intel_engine_stop_cs(struct intel_engine_cs *engine);
>  u64 intel_engine_get_active_head(const struct intel_engine_cs *engine);
>  u64 intel_engine_get_last_batch_head(const struct intel_engine_cs *engine);
>  
> -static inline u32 intel_engine_get_seqno(struct intel_engine_cs *engine)
> -{
> -	return intel_read_status_page(engine, I915_GEM_HWS_INDEX);
> -}
> -
>  static inline u32 intel_engine_last_submit(struct intel_engine_cs *engine)
>  {
> -	/* We are only peeking at the tail of the submit queue (and not the
> +	/*
> +	 * We are only peeking at the tail of the submit queue (and not the
>  	 * queue itself) in order to gain a hint as to the current active
>  	 * state of the engine. Callers are not expected to be taking
>  	 * engine->timeline->lock, nor are they expected to be concerned
> @@ -928,6 +924,30 @@ static inline u32 intel_engine_last_submit(struct intel_engine_cs *engine)
>  	return READ_ONCE(engine->timeline.seqno);
>  }
>  
> +static inline u32 intel_engine_get_seqno(struct intel_engine_cs *engine)
> +{
> +	return intel_read_status_page(engine, I915_GEM_HWS_INDEX);
> +}
> +
> +static inline bool intel_engine_signaled(struct intel_engine_cs *engine,
> +					 u32 seqno)
> +{
> +	GEM_BUG_ON(!seqno);
> +	return i915_seqno_passed(intel_engine_get_seqno(engine), seqno);
> +}
> +
> +static inline bool intel_engine_has_completed(struct intel_engine_cs *engine,
> +					      u32 seqno)
> +{
> +	return intel_engine_signaled(engine, seqno);
> +}
> +
> +static inline bool intel_engine_has_started(struct intel_engine_cs *engine,
> +					    u32 seqno)
> +{
> +	return intel_engine_signaled(engine, seqno - 1);
> +}
> +
>  void intel_engine_get_instdone(struct intel_engine_cs *engine,
>  			       struct intel_instdone *instdone);
>  
> -- 
> 2.18.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Pull seqno started checks together

[Chris Wilson]

Quoting Mika Kuoppala (2018-08-06 12:17:16)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > We have a few instances of checking seqno-1 to see if the HW has started
> > the request. Pull those together under a helper.
> >
> 
> Could you elaborate why you want both completed and signaled?

Purely on context. In some cases we are talking about signaling and so
signaled makes sense, in others we are at a higher level and talking
about requests being started or completed.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Pull seqno started checks together

[Chris Wilson]

We have a few instances of checking seqno-1 to see if the HW has started
the request. Pull those together under a helper.

v2: Pull the !seqno assertion higher, as we given seqno==1 we may indeed
check to see if we have started using seqno==0.

Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_request.c      |  9 +++---
 drivers/gpu/drm/i915/i915_request.h      | 39 +++++++++++++++---------
 drivers/gpu/drm/i915/intel_breadcrumbs.c |  6 ++--
 drivers/gpu/drm/i915/intel_engine_cs.c   |  3 +-
 drivers/gpu/drm/i915/intel_hangcheck.c   |  2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.h  | 33 ++++++++++++++++----
 6 files changed, 60 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 5c2c93cbab12..09ed48833b54 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -527,7 +527,7 @@ void __i915_request_submit(struct i915_request *request)
 
 	seqno = timeline_get_seqno(&engine->timeline);
 	GEM_BUG_ON(!seqno);
-	GEM_BUG_ON(i915_seqno_passed(intel_engine_get_seqno(engine), seqno));
+	GEM_BUG_ON(intel_engine_signaled(engine, seqno));
 
 	/* We may be recursing from the signal callback of another i915 fence */
 	spin_lock_nested(&request->lock, SINGLE_DEPTH_NESTING);
@@ -579,8 +579,7 @@ void __i915_request_unsubmit(struct i915_request *request)
 	 */
 	GEM_BUG_ON(!request->global_seqno);
 	GEM_BUG_ON(request->global_seqno != engine->timeline.seqno);
-	GEM_BUG_ON(i915_seqno_passed(intel_engine_get_seqno(engine),
-				     request->global_seqno));
+	GEM_BUG_ON(intel_engine_has_completed(engine, request->global_seqno));
 	engine->timeline.seqno--;
 
 	/* We may be recursing from the signal callback of another i915 fence */
@@ -1205,7 +1204,7 @@ static bool __i915_spin_request(const struct i915_request *rq,
 	 * it is a fair assumption that it will not complete within our
 	 * relatively short timeout.
 	 */
-	if (!i915_seqno_passed(intel_engine_get_seqno(engine), seqno - 1))
+	if (!intel_engine_has_started(engine, seqno))
 		return false;
 
 	/*
@@ -1222,7 +1221,7 @@ static bool __i915_spin_request(const struct i915_request *rq,
 	irq = READ_ONCE(engine->breadcrumbs.irq_count);
 	timeout_us += local_clock_us(&cpu);
 	do {
-		if (i915_seqno_passed(intel_engine_get_seqno(engine), seqno))
+		if (intel_engine_has_completed(engine, seqno))
 			return seqno == i915_request_global_seqno(rq);
 
 		/*
diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h
index e1c9365dfefb..05888de2e045 100644
--- a/drivers/gpu/drm/i915/i915_request.h
+++ b/drivers/gpu/drm/i915/i915_request.h
@@ -272,7 +272,10 @@ long i915_request_wait(struct i915_request *rq,
 #define I915_WAIT_ALL		BIT(2) /* used by i915_gem_object_wait() */
 #define I915_WAIT_FOR_IDLE_BOOST BIT(3)
 
-static inline u32 intel_engine_get_seqno(struct intel_engine_cs *engine);
+static inline bool intel_engine_has_started(struct intel_engine_cs *engine,
+					    u32 seqno);
+static inline bool intel_engine_has_completed(struct intel_engine_cs *engine,
+					    u32 seqno);
 
 /**
  * Returns true if seq1 is later than seq2.
@@ -282,11 +285,31 @@ static inline bool i915_seqno_passed(u32 seq1, u32 seq2)
 	return (s32)(seq1 - seq2) >= 0;
 }
 
+/**
+ * i915_request_started - check if the request has begun being executed
+ * @rq: the request
+ *
+ * Returns true if the request has been submitted to hardware, and the hardware
+ * has advanced passed the end of the previous request and so should be either
+ * currently processing the request (though it may be preempted and so
+ * not necessarily the next request to complete) or have completed the request.
+ */
+static inline bool i915_request_started(const struct i915_request *rq)
+{
+	u32 seqno;
+
+	seqno = i915_request_global_seqno(rq);
+	if (!seqno) /* not yet submitted to HW */
+		return false;
+
+	return intel_engine_has_started(rq->engine, seqno);
+}
+
 static inline bool
 __i915_request_completed(const struct i915_request *rq, u32 seqno)
 {
 	GEM_BUG_ON(!seqno);
-	return i915_seqno_passed(intel_engine_get_seqno(rq->engine), seqno) &&
+	return intel_engine_has_completed(rq->engine, seqno) &&
 		seqno == i915_request_global_seqno(rq);
 }
 
@@ -301,18 +324,6 @@ static inline bool i915_request_completed(const struct i915_request *rq)
 	return __i915_request_completed(rq, seqno);
 }
 
-static inline bool i915_request_started(const struct i915_request *rq)
-{
-	u32 seqno;
-
-	seqno = i915_request_global_seqno(rq);
-	if (!seqno)
-		return false;
-
-	return i915_seqno_passed(intel_engine_get_seqno(rq->engine),
-				 seqno - 1);
-}
-
 static inline bool i915_sched_node_signaled(const struct i915_sched_node *node)
 {
 	const struct i915_request *rq =
diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index 1db6ba7d926e..84bf8d827136 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -256,8 +256,7 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
 	spin_unlock(&b->irq_lock);
 
 	rbtree_postorder_for_each_entry_safe(wait, n, &b->waiters, node) {
-		GEM_BUG_ON(!i915_seqno_passed(intel_engine_get_seqno(engine),
-					      wait->seqno));
+		GEM_BUG_ON(!intel_engine_signaled(engine, wait->seqno));
 		RB_CLEAR_NODE(&wait->node);
 		wake_up_process(wait->tsk);
 	}
@@ -508,8 +507,7 @@ bool intel_engine_add_wait(struct intel_engine_cs *engine,
 		return armed;
 
 	/* Make the caller recheck if its request has already started. */
-	return i915_seqno_passed(intel_engine_get_seqno(engine),
-				 wait->seqno - 1);
+	return intel_engine_has_started(engine, wait->seqno);
 }
 
 static inline bool chain_wakeup(struct rb_node *rb, int priority)
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 67c4fc5d737c..99d5a24219c1 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -968,8 +968,7 @@ bool intel_engine_is_idle(struct intel_engine_cs *engine)
 		return true;
 
 	/* Any inflight/incomplete requests? */
-	if (!i915_seqno_passed(intel_engine_get_seqno(engine),
-			       intel_engine_last_submit(engine)))
+	if (!intel_engine_signaled(engine, intel_engine_last_submit(engine)))
 		return false;
 
 	if (I915_SELFTEST_ONLY(engine->breadcrumbs.mock))
diff --git a/drivers/gpu/drm/i915/intel_hangcheck.c b/drivers/gpu/drm/i915/intel_hangcheck.c
index 2fc7a0dd0df9..e26d05a46451 100644
--- a/drivers/gpu/drm/i915/intel_hangcheck.c
+++ b/drivers/gpu/drm/i915/intel_hangcheck.c
@@ -142,7 +142,7 @@ static int semaphore_passed(struct intel_engine_cs *engine)
 	if (signaller->hangcheck.deadlock >= I915_NUM_ENGINES)
 		return -1;
 
-	if (i915_seqno_passed(intel_engine_get_seqno(signaller), seqno))
+	if (intel_engine_signaled(signaller, seqno))
 		return 1;
 
 	/* cursory check for an unkickable deadlock */
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 57f3787ed6ec..70b2c6a7eb84 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -911,14 +911,10 @@ int intel_engine_stop_cs(struct intel_engine_cs *engine);
 u64 intel_engine_get_active_head(const struct intel_engine_cs *engine);
 u64 intel_engine_get_last_batch_head(const struct intel_engine_cs *engine);
 
-static inline u32 intel_engine_get_seqno(struct intel_engine_cs *engine)
-{
-	return intel_read_status_page(engine, I915_GEM_HWS_INDEX);
-}
-
 static inline u32 intel_engine_last_submit(struct intel_engine_cs *engine)
 {
-	/* We are only peeking at the tail of the submit queue (and not the
+	/*
+	 * We are only peeking at the tail of the submit queue (and not the
 	 * queue itself) in order to gain a hint as to the current active
 	 * state of the engine. Callers are not expected to be taking
 	 * engine->timeline->lock, nor are they expected to be concerned
@@ -928,6 +924,31 @@ static inline u32 intel_engine_last_submit(struct intel_engine_cs *engine)
 	return READ_ONCE(engine->timeline.seqno);
 }
 
+static inline u32 intel_engine_get_seqno(struct intel_engine_cs *engine)
+{
+	return intel_read_status_page(engine, I915_GEM_HWS_INDEX);
+}
+
+static inline bool intel_engine_signaled(struct intel_engine_cs *engine,
+					 u32 seqno)
+{
+	return i915_seqno_passed(intel_engine_get_seqno(engine), seqno);
+}
+
+static inline bool intel_engine_has_completed(struct intel_engine_cs *engine,
+					      u32 seqno)
+{
+	GEM_BUG_ON(!seqno);
+	return intel_engine_signaled(engine, seqno);
+}
+
+static inline bool intel_engine_has_started(struct intel_engine_cs *engine,
+					    u32 seqno)
+{
+	GEM_BUG_ON(!seqno);
+	return intel_engine_signaled(engine, seqno - 1);
+}
+
 void intel_engine_get_instdone(struct intel_engine_cs *engine,
 			       struct intel_instdone *instdone);
 
-- 
2.18.0

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

✗ Fi.CI.CHECKPATCH: warning for drm/i915: Pull seqno started checks together (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Pull seqno started checks together (rev2)
URL   : https://patchwork.freedesktop.org/series/47753/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
95cd3d8b15e9 drm/i915: Pull seqno started checks together
-:69: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#69: FILE: drivers/gpu/drm/i915/i915_request.h:278:
+static inline bool intel_engine_has_completed(struct intel_engine_cs *engine,
+					    u32 seqno);

total: 0 errors, 0 warnings, 1 checks, 176 lines checked

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

✓ Fi.CI.BAT: success for drm/i915: Pull seqno started checks together (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Pull seqno started checks together (rev2)
URL   : https://patchwork.freedesktop.org/series/47753/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4621 -> Patchwork_9857 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/47753/revisions/2/mbox/

== Known issues ==

  Here are the changes found in Patchwork_9857 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@debugfs_test@read_all_entries:
      {fi-icl-u}:         PASS -> DMESG-FAIL (fdo#107411)

    igt@drv_selftest@live_hangcheck:
      {fi-cfl-8109u}:     PASS -> DMESG-FAIL (fdo#106560)

    igt@gem_exec_reloc@basic-gtt-read-noreloc:
      {fi-icl-u}:         PASS -> DMESG-WARN (fdo#107411) +77

    igt@kms_frontbuffer_tracking@basic:
      fi-hsw-peppy:       PASS -> DMESG-FAIL (fdo#102614, fdo#106103)

    
    ==== Possible fixes ====

    igt@debugfs_test@read_all_entries:
      fi-snb-2520m:       INCOMPLETE (fdo#103713) -> PASS

    igt@kms_chamelium@dp-crc-fast:
      fi-kbl-7500u:       FAIL (fdo#103841) -> PASS

    igt@kms_frontbuffer_tracking@basic:
      {fi-byt-clapper}:   FAIL (fdo#103167) -> PASS

    
    ==== Warnings ====

    {igt@kms_psr@primary_page_flip}:
      fi-cnl-psr:         DMESG-WARN (fdo#107372) -> DMESG-FAIL (fdo#107372)

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#103841 https://bugs.freedesktop.org/show_bug.cgi?id=103841
  fdo#106103 https://bugs.freedesktop.org/show_bug.cgi?id=106103
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  fdo#107372 https://bugs.freedesktop.org/show_bug.cgi?id=107372
  fdo#107411 https://bugs.freedesktop.org/show_bug.cgi?id=107411


== Participating hosts (53 -> 48) ==

  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4621 -> Patchwork_9857

  CI_DRM_4621: b35c6b4ccdc1e9b386d5679282123099cf83adf1 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4587: 5d78c73d871525ec9caecd88ad7d9abe36637314 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9857: 95cd3d8b15e9d77959953082b62d6142cf65a0a3 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

95cd3d8b15e9 drm/i915: Pull seqno started checks together

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9857/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915: Pull seqno started checks together (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Pull seqno started checks together (rev2)
URL   : https://patchwork.freedesktop.org/series/47753/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4621_full -> Patchwork_9857_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_9857_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9857_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9857_full:

  === IGT changes ===

    ==== Warnings ====

    igt@pm_rc6_residency@rc6-accuracy:
      shard-kbl:          SKIP -> PASS

    
== Known issues ==

  Here are the changes found in Patchwork_9857_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_ppgtt@blt-vs-render-ctx0:
      shard-kbl:          PASS -> INCOMPLETE (fdo#106023, fdo#103665)

    igt@kms_atomic_transition@1x-modeset-transitions-nonblocking-fencing:
      shard-glk:          PASS -> FAIL (fdo#105703)

    igt@kms_cursor_crc@cursor-128x128-suspend:
      shard-glk:          PASS -> FAIL (fdo#103375)

    igt@kms_setmode@basic:
      shard-hsw:          PASS -> FAIL (fdo#99912)

    igt@perf@polling:
      shard-hsw:          PASS -> FAIL (fdo#102252)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_workarounds:
      shard-kbl:          DMESG-FAIL (fdo#107292) -> PASS

    igt@kms_cursor_crc@cursor-128x128-suspend:
      shard-kbl:          INCOMPLETE (fdo#103665) -> PASS

    igt@kms_flip@2x-flip-vs-expired-vblank:
      shard-glk:          FAIL (fdo#105363) -> PASS

    igt@kms_setmode@basic:
      shard-apl:          FAIL (fdo#99912) -> PASS

    
  fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
  fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105703 https://bugs.freedesktop.org/show_bug.cgi?id=105703
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#107292 https://bugs.freedesktop.org/show_bug.cgi?id=107292
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4621 -> Patchwork_9857

  CI_DRM_4621: b35c6b4ccdc1e9b386d5679282123099cf83adf1 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4587: 5d78c73d871525ec9caecd88ad7d9abe36637314 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9857: 95cd3d8b15e9d77959953082b62d6142cf65a0a3 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9857/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Pull seqno started checks together

[Mika Kuoppala]

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

> We have a few instances of checking seqno-1 to see if the HW has started
> the request. Pull those together under a helper.
>
> v2: Pull the !seqno assertion higher, as we given seqno==1 we may indeed
> check to see if we have started using seqno==0.
>
> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

He is very quiet. But for me too it all reads better.

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
-Mika
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Pull seqno started checks together

[Chris Wilson]

Quoting Mika Kuoppala (2018-08-07 10:09:48)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > We have a few instances of checking seqno-1 to see if the HW has started
> > the request. Pull those together under a helper.
> >
> > v2: Pull the !seqno assertion higher, as we given seqno==1 we may indeed
> > check to see if we have started using seqno==0.
> >
> > Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> He is very quiet. But for me too it all reads better.
> 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

And pushed, thanks for the review.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx