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

[Chris Wilson]

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

There is just one place that we need the extra work, after the
interrupt. All other places only care about the current value of the
seqno in the CPU cache.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre