Quoting Chris Wilson (2019-01-15 09:14:17)
> Quoting John Harrison (2019-01-15 00:55:39)
> > On 1/7/2019 03:55, Chris Wilson wrote:
> > > Supplement the per-engine HWSP with a per-timeline HWSP. That is a
> > > per-request pointer through which we can check a local seqno,
> > > abstracting away the presumption of a global seqno. In this first step,
> > > we point each request back into the engine's HWSP so everything
> > > continues to work with the global timeline.
> > > ---
> > > +static inline u32 i915_request_hwsp(const struct i915_request *rq)
> > > +{
> > > + return READ_ONCE(*rq->hwsp_seqno);
> > > +}
> > > +
> > Shouldn't the function name have an _seqno as well? Just
> > 'i915_request_hwsp()' is fairly ambiguous, there could be many different
> > things stored in the HWSP.
>
> It's not even necessarily the HWSP! :)
>
> i915_request_hw_seqno() // dissatisfying
> -> i915_request_hwsp_seqno() // but rq only stores one element in HWSP!
> -> i915_request_hwsp()
>
> Was the evolution of names I chose.
>
> Of that mix, i915_request_hwsp_seqno(). hw_seqno just feels nondescript.
>
> i915_request_current_[hw]_seqno() maybe, but because we start with
> i915_request I find it confusing and expect the seqno to be tied to the
> request. So maybe just drop i915_request here, and go with something
> like hwsp_breadcrumb(), that just happens to take i915_request as a
> convenience.
Alternatively,
static inline u32 i915_request_hwsp(struct i915_request *rq, int index)
{
return READ_ONCE(rq->hwsp_seqno[index]);
}
And probably rename s/rq->hwsp_seqno/rq->hwsp/. That should compile away
the argument, but you'll still probably want a
static inline u32 i915_request_hwsp_seqno(struct i915_request *rq)
{
return i915_request_hwsp(rq, 0);
}
I can't win! But it does look more methodical.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
