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@xxxxxxxxxxxxxxxxxx>
<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@xxxxxxxxxxxxxxx>
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
