Re: [PATCH 01/14] drm/i915: Keep a global seqno per-engine

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On to, 2017-02-02 at 15:12 +0000, Chris Wilson wrote:
> Replace the global device seqno with one for each engine, and account
> for in-flight seqno on each separately. This is consistent with
> dma-fence as each timeline has separate fence-contexts for each engine
> and a seqno is only ordered within a fence-context (i.e.  seqno do not
> need to be ordered wrt to other engines, just ordered within a single
> engine). This is required to enable request rewinding for preemption on
> individual engines (we have to rewind the global seqno to avoid
> overflow, and we do not have to rewind all engines just to preempt one.)
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

<SNIP>

> @@ -213,7 +213,9 @@ static void i915_gem_request_retire(struct drm_i915_gem_request *request)
>  	GEM_BUG_ON(!i915_sw_fence_signaled(&request->submit));
>  	GEM_BUG_ON(!i915_sw_fence_signaled(&request->execute));
>  	GEM_BUG_ON(!i915_gem_request_completed(request));
> +
>  	GEM_BUG_ON(!request->i915->gt.active_requests);
> +	GEM_BUG_ON(!request->engine->timeline->active_seqno);

active_seqnos to indicate it's a count.
 
> -static int reserve_global_seqno(struct drm_i915_private *i915)
> +static int reserve_global_seqno(struct intel_engine_cs *engine)
>  {
> -	u32 active_requests = ++i915->gt.active_requests;
> -	u32 seqno = atomic_read(&i915->gt.global_timeline.seqno);
> +	u32 active = ++engine->timeline->active_seqno;
> +	u32 seqno = engine->timeline->seqno;
>  	int ret;
>  
>  	/* Reservation is fine until we need to wrap around */
> -	if (likely(seqno + active_requests > seqno))
> +	if (likely(seqno + active > seqno))
>  		return 0;
> 

Make a comment here that we don't currently track the semaphores
waiting on specific engine, so we instead choose to wrap all of them.
 
> -	ret = i915_gem_init_global_seqno(i915, 0);
> +	ret = i915_gem_init_global_seqno(engine->i915, 0);

Also, we don't have a global seqno anymore, so the function name needs
updating. Maybe "init_{all_}global_seqnos" to make it clear.

>  	if (ret) {
> -		i915->gt.active_requests--;
> +		engine->timeline->active_seqno--;
>  		return ret;
>  	}
>  
>  	return 0;
>  }

<SNIP>

> @@ -889,16 +887,14 @@ void __i915_add_request(struct drm_i915_gem_request *request, bool flush_caches)
>  	list_add_tail(&request->link, &timeline->requests);
>  	spin_unlock_irq(&timeline->lock);
>  
> -	GEM_BUG_ON(i915_seqno_passed(timeline->last_submitted_seqno,
> -				     request->fence.seqno));
> -
> -	timeline->last_submitted_seqno = request->fence.seqno;
> +	GEM_BUG_ON(timeline->seqno != request->fence.seqno);
>  	i915_gem_active_set(&timeline->last_request, request);
>  
>  	list_add_tail(&request->ring_link, &ring->request_list);
>  	request->emitted_jiffies = jiffies;
>  
> -	i915_gem_mark_busy(engine);
> +	if (!request->i915->gt.active_requests++)
> +		i915_gem_mark_busy(engine);

This kinda hides the increment, I'd prefer to have it better in sight.

With the comment added and function renamed;

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx