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

[Dave Gordon]

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.

.Dave.