On 1/15/2019 07:40, Chris Wilson wrote:
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.
My vote would be 'hwsp_breadcrumb()' or similar. As you say, the seqno
in the HWSP isn't actually tied to the request. Quite the opposite in
fact - you are generally comparing multiple requests' seqnos to the HWSP
seqno to see which have or have not completed. It should really be tied
to the timeline (or more accurately, to the context as that is what
dictates the timeline). The code is generally starting from a request
structure so it makes sense to have a shortcut via the request. But
logically, it should be req->timeline->hwsp[SEQNO]. Maybe even something
like i915_timeline_out_seqno(rq)? Or i915_timeline_done_seqno(rq)?
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);
}
Given that there is only a single per context element in the HWSP at
present, this version does seem overkill. It might be useful to move to
that later when there are more entries, if that ever happens. For now,
keep things simple I think.
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
