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

[Chris Wilson]

On Mon, Jan 04, 2016 at 01:02:25PM +0000, 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.
> 
> >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
> 
> So, we could instead have a per-engine flag which says, interrupt
> has happened, next reader should expect an update (and try hard to
> see one)?

Go back and read. That patch adds the commentary that should explain
what needs to be done and where, continue on in the series you can see
just that micro-optimisation along with the required barriers.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre