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

[Chris Wilson]

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.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre