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

[Chris Wilson]

On Wed, Feb 15, 2017 at 09:49:41PM +0000, Chris Wilson wrote:
> > >-static int reserve_global_seqno(struct drm_i915_private *i915)
> > >+static int reserve_global_seqno(struct intel_engine_cs *engine)
> > 
> > Rename to reserve_engine_seqno?
> 
> Yes.
> 
> > >-	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))
> > 
> > Case for one of the overflows type helpers?
> 
> add_overflows
> 
> if (likely(!add_overflows(seqno, active))
> 
> > 
> > > 		return 0;
> > >
> > >-	ret = i915_gem_init_global_seqno(i915, 0);
> > >+	ret = i915_gem_init_global_seqno(engine->i915, 0);
> > 
> > i915_gem_init_engine_seqno(engine) ?
> 
> No. Still retains global scope, and definitely wants to keep that
> nuisance front and centre.
> 
> > > 	if (ret) {
> > >-		i915->gt.active_requests--;
> > >+		engine->timeline->active_seqno--;
> > 
> > It would be better for the active seqno count to be managed on the
> > same level for readability. By that I mean having the decrement in
> > add_request where it was incremented.
> 
> It's incremented in this function, so the unwind on error is here as
> well.

Ah, I guess you were referring to the decrement in request_alloc. Pulled
that out to unreserve_seqno() to match the call to reserve_seqno().
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre