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@xxxxxxxxx>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
