[Intel-gfx] [PATCH 2/6] drm/i915: Avoid accessing request->timeline outside of its lifetime

[Tvrtko Ursulin]

On 31/10/2016 21:03, Chris Wilson wrote:
> On Mon, Oct 31, 2016 at 05:35:50PM +0000, Tvrtko Ursulin wrote:
>>
>> On 31/10/2016 10:26, Chris Wilson wrote:
>>> Whilst waiting on a request, we may do so without holding any locks or
>>> any guards beyond a reference to the request. In order to avoid taking
>>> locks within request deallocation, we drop references to its timeline
>>> (via the context and ppgtt) upon retirement. We should avoid chasing
>>
>> Couldn't find that there is a reference taken (or dropped) on the
>> timeline when stored in a request. It looks like a borrowed pointer
>> to me?
>
> The timeline is owned by the address space which is owned by either the
> context or the device. The request holds a reference on the context (and
> so indirectly onto the timeline, except for the device's which outlives
> the request) up until we retire the request. (Retiring holds
> struct_mutex so is earlier than freeing.)
>
>>> +static inline u32 intel_engine_last_submit(struct intel_engine_cs *engine)
>>> +{
>>> +	return READ_ONCE(engine->timeline->last_submitted_seqno);
>>> +}
>>> +
>>
>> Don't like that READ_ONCE gets sprinkled all over the place via call
>> sites. It should be extremely well defined and controlled from where
>> it is used. Otherwise it suggests READ_ONCE is not really
>> appropriate.
>
> It actually is appropriate. last_submitted_seqno is under the timeline
> spinlock, none of the callers take the appropriate guard. This is
> trying to document that these callers don't call about fully
> synchronising the last_submitted_seqno with the request list or last
> request pointer. And we don't care to go full seqlock, since we really
> are only peeking.

Ah my bad, I thought it was actually req->timeline->last_submitted_seqno.

In this case, assuming the seqno namespaces are correct:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko