[PATCH 1/4] drm/i915: Stop tracking timeline->inflight_seqnos

[Tvrtko Ursulin]

On 20/04/2018 12:35, Chris Wilson wrote:
> In commit 9b6586ae9f6b ("drm/i915: Keep a global seqno per-engine"), we
> moved from a global inflight counter to per-engine counters in the
> hope that will be easy to run concurrently in future. However, with the
> advent of the desire to move requests between engines, we do need a
> global counter to preserve the semantics that no engine wraps in the
> middle of a submit. (Although this semantic is now only required for gen7
> semaphore support, which only supports greater-then comparisons!)
> 
> References: 9b6586ae9f6b ("drm/i915: Keep a global seqno per-engine")
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c      |  5 ++---
>   drivers/gpu/drm/i915/i915_gem_timeline.h |  6 ------
>   drivers/gpu/drm/i915/i915_request.c      | 12 +++---------
>   drivers/gpu/drm/i915/intel_engine_cs.c   |  5 ++---
>   4 files changed, 7 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index e0274f41bc76..a8b3dba5505b 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1340,10 +1340,9 @@ static int i915_hangcheck_info(struct seq_file *m, void *unused)
>   		struct rb_node *rb;
>   
>   		seq_printf(m, "%s:\n", engine->name);
> -		seq_printf(m, "\tseqno = %x [current %x, last %x], inflight %d\n",
> +		seq_printf(m, "\tseqno = %x [current %x, last %x]\n",
>   			   engine->hangcheck.seqno, seqno[id],
> -			   intel_engine_last_submit(engine),
> -			   engine->timeline->inflight_seqnos);
> +			   intel_engine_last_submit(engine));
>   		seq_printf(m, "\twaiters? %s, fake irq active? %s, stalled? %s\n",
>   			   yesno(intel_engine_has_waiter(engine)),
>   			   yesno(test_bit(engine->id,
> diff --git a/drivers/gpu/drm/i915/i915_gem_timeline.h b/drivers/gpu/drm/i915/i915_gem_timeline.h
> index 33e01bf6aa36..6e82119e2cd8 100644
> --- a/drivers/gpu/drm/i915/i915_gem_timeline.h
> +++ b/drivers/gpu/drm/i915/i915_gem_timeline.h
> @@ -37,12 +37,6 @@ struct intel_timeline {
>   	u64 fence_context;
>   	u32 seqno;
>   
> -	/**
> -	 * Count of outstanding requests, from the time they are constructed
> -	 * to the moment they are retired. Loosely coupled to hardware.
> -	 */
> -	u32 inflight_seqnos;
> -
>   	spinlock_t lock;
>   
>   	/**
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index b692a9f7c357..1437538d5bfa 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -260,17 +260,14 @@ int i915_gem_set_global_seqno(struct drm_device *dev, u32 seqno)
>   static int reserve_engine(struct intel_engine_cs *engine)
>   {
>   	struct drm_i915_private *i915 = engine->i915;
> -	u32 active = ++engine->timeline->inflight_seqnos;
> -	u32 seqno = engine->timeline->seqno;
>   	int ret;
>   
>   	/* Reservation is fine until we need to wrap around */
> -	if (unlikely(add_overflows(seqno, active))) {
> +	if (unlikely(add_overflows(engine->timeline->seqno,
> +				   i915->gt.active_requests + 1))) {

So it will have to reset a bit earlier than before but I don't see it as 
a problem.

>   		ret = reset_all_global_seqno(i915, 0);
> -		if (ret) {
> -			engine->timeline->inflight_seqnos--;
> +		if (ret)
>   			return ret;
> -		}
>   	}
>   
>   	if (!i915->gt.active_requests++)
> @@ -285,9 +282,6 @@ static void unreserve_engine(struct intel_engine_cs *engine)
>   
>   	if (!--i915->gt.active_requests)
>   		i915_gem_park(i915);
> -
> -	GEM_BUG_ON(!engine->timeline->inflight_seqnos);
> -	engine->timeline->inflight_seqnos--;
>   }
>   
>   void i915_gem_retire_noop(struct i915_gem_active *active,
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index be608f7111f5..a55a849b81b6 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -1318,12 +1318,11 @@ void intel_engine_dump(struct intel_engine_cs *engine,
>   	if (i915_terminally_wedged(&engine->i915->gpu_error))
>   		drm_printf(m, "*** WEDGED ***\n");
>   
> -	drm_printf(m, "\tcurrent seqno %x, last %x, hangcheck %x [%d ms], inflight %d\n",
> +	drm_printf(m, "\tcurrent seqno %x, last %x, hangcheck %x [%d ms]\n",
>   		   intel_engine_get_seqno(engine),
>   		   intel_engine_last_submit(engine),
>   		   engine->hangcheck.seqno,
> -		   jiffies_to_msecs(jiffies - engine->hangcheck.action_timestamp),
> -		   engine->timeline->inflight_seqnos);
> +		   jiffies_to_msecs(jiffies - engine->hangcheck.action_timestamp));
>   	drm_printf(m, "\tReset count: %d (global %d)\n",
>   		   i915_reset_engine_count(error, engine),
>   		   i915_reset_count(error));
> 

Looks fine to me. I'll leave actual r-b for the real mailing list. Or if 
you want to copy it over:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko