[Intel-gfx] [PATCH 41/46] drm/i915: Introduce concept of per-timeline (context) HWSP

[Chris Wilson]

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