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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
