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

[John Harrison]

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