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