[PATCH 3/3] drm/amdgpu: busywait KIQ register accessing v2

[Ding, Pixel]

Thanks. In logically, it can’t handle the extreme case that the seq almost catches up with wait_seq in wrap round where we can’t use this trick, however I understand it can’t happen in reality.

I will test and modify.

— 
Sincerely Yours,
Pixel







On 17/10/2017, 4:59 PM, "Koenig, Christian" <Christian.Koenig at amd.com> wrote:

>Am 17.10.2017 um 10:38 schrieb Ding, Pixel:
>> [SNIP]
>>>> +		if (seq >= wait_seq && wait_seq >= last_seq)
>>>> +			break;
>>>> +		if (seq <= last_seq &&
>>>> +		    (wait_seq <= seq || wait_seq >= last_seq))
>>>> +			break;
>>> Why not just "(int32_t)(wait_seq - seq) > 0" ? IIRC that should be
>>> sufficient for a wrap around test.
>>> [pixel] seq could be larger (executed) or smaller (not yet) than wait_seq even without a wrap round, I think you mean "(int32_t)(last_seq - seq) > 0”
>>> it actually can know there’s a wrap around, but it still doesn't know if the waited seq is in the range between or not, we still need other conditions. Code here is to identify cases to break as:
>> ===last_seq=====wait_seq====seq===
>> ===wait_seq==seq========last_seq==
>> ==seq==========last_seq==wait_seq=
>>
>> Does it make sense?
>
>No, let me explain a bit more. The full code I had in mind is this:
>
>do {
>     seq = amdgpu_fence_read(ring)
>} while ((int32_t)(wait_seq - seq) > 0);
>
>This should handle the following cases:
>1. wait_seq is larger than seq, in this case we still need to wait.
>
>(wait_seq - seq) is larger than zero and the loop continues.
>
>2. wait_seq is smaller than seq, in this case the we can stop waiting.
>
>(wait_seq -seq) is smaller or equal to zero and the loop stops.
>
>3. wait_seq is much smaller than seq because of a wrap around, in this 
>case we still need to wait.
>
>(wait_seq - seq) is larger than zero because the substraction wraps 
>around the numeric range as well and the look will continue.
>
>4. wait_seq is much larger than seq because of a wrap around, in this 
>case we can stop waiting.
>
>(wait_seq - seq) is smaller than zero because the cast to a signed 
>number makes it negative and the loop stops.
>
>This is rather common code for wrap around testing and you don't need to 
>fiddle with last_seq at all here.
>
>If you want you can also use the __dma_fence_is_later() helper function 
>which implements exactly that test as well.
>
>Regards,
>Christian.