[Intel-gfx] [PATCH 17/32] drm/i915: Remove the lazy_coherency parameter from request-completed?

[Chris Wilson]

On Mon, Jan 04, 2016 at 02:09:53PM +0000, Dave Gordon wrote:
> On 04/01/16 13:02, Dave Gordon wrote:
> >On 04/01/16 11:26, Chris Wilson wrote:
> >>On Mon, Jan 04, 2016 at 11:16:04AM +0000, Dave Gordon wrote:
> >>>On 14/12/15 15:11, Chris Wilson wrote:
> >>>>On Mon, Dec 14, 2015 at 02:59:30PM +0000, Tvrtko Ursulin wrote:
> >>>>>
> >>>>>Hi,
> >>>>>
> >>>>>On 11/12/15 11:33, Chris Wilson wrote:
> >>>>>>Now that we have split out the seqno-barrier from the
> >>>>>>engine->get_seqno() callback itself, we can move the users of the
> >>>>>>seqno-barrier to the required callsites simplifying the common
> >>>>>>code and
> >>>>>>making the required workaround handling much more explicit.
> >>>>>
> >>>>>What bothers me about this patch, and the one preceding it, is that
> >>>>>I don't see a tangible improvement for the programmer who still has
> >>>>>to know when to read the seqno and when to "read it harder, read for
> >>>>>real".
> >>>>
> >>>>In earlier patches, I called it irq_barrier.
> >>>>
> >>>>It's not reading it harder. It's just that there is a ordering issue
> >>>>with receiving an interrupt and the seqno write being visible.
> >>>>
> >>>>>Barrier in this sense has a relation to the state of things but
> >>>>>somehow feels too low level to me when used from the code. But to be
> >>>>>fair I am not sure how to better define it.
> >>>>>
> >>>>>Would ring->get_seqno paired with ring->read_seqno perhaps make
> >>>>>sense? Implementation for ring->read_seqno would just be a flush
> >>>>>followed by ring->get_seqno then. Or maybe keep the barrier and add
> >>>>>ring->read_seqno which would be ring->seqno_barrier +
> >>>>>ring_get_seqno?
> >>>>
> >>>>No.
> >>>>-Chris
> >>>
> >>>We could instead put the knowledge about whether and how to read
> >>>"for real" inside the read-the-seqno function. For example:
> >>
> >>You do appreciate the irony that you are on the reviewer list for patches
> >>that do that?
> >>
> >>http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=breadcrumbs&id=34409f2d965001d7d63f21a1c5339b07eed6af34
> >
> >No, I haven't got as far as that one, since it was posted over a week
> >after the message at the head of this thread. Anyway, I still can't see
> >in that patch anything equivalent to what I described above.
> 
> Oh, I spotted what you meant, but it's not in /that/ patch (which
> was a version of PATCH 15/32 (Slaughter the thundering
> i915_wait_request herd)
> it's in PATCH 19/32 (Check the CPU cached value of seqno after
> waking the waiter).
> 
> Even so, it's not at the same level of code structure; I was
> suggesting pushing it all the way down, because
> __i915_wait_request() and/or i915_gem_request_completed() aren't the
> only functions that use it.

No. I am arguing that there should be precisely one piece of code
responsible for seqno-vs-interrupt ordering. Everywhere else should not
have to worry about that interrupts may be asserted before the HWS write
is posted.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre