Re: [PATCH 2/4] wined3d: Use the default queue index for resource fencing.

[Jan Sikorski]

Hi,
> On 23 Feb 2022, at 17:01, Stefan Dösinger  wrote:
> - while (InterlockedCompareExchange(&resource->access_count, 0, 0))
> - /* A resource is considered busy between queueing a command that reads it and the execution of that
> -
> * command. We use the head and tail pointer of the default CS queue for tracking the access time.
> -
> * We can't track commands on the map queue this way. If a map command is handled asynchronously the
> -
> * resource fencing needs to be handled some other way.
> -
> *
> -
> * The queue head and tail will wrap around when the 32 bit ULONG is exhausted. We therefore need to
> -
> * handle a few cases:
> -
> *
> -
> * A...access_time in the resource
> -
> * H...queue write head
> -
> * T...queue read tail
> -
> *
> -
> * Case 1:
> -
> * |.....T------A-----H..........|
> -
> * The resource is busy because the access time is between head and tail. No wrap-around has happened.
> -
> *
> -
> * Case 2:
> -
> * |..A.....T---------H..........|
> -
> * The resource is idle, the last command using it has been executed.
> -
> *
> -
> * Case 3:
> -
> * |........T---------H.....A....|
> -
> * The resource is idle, the last command using it has been executed and the head and tail have since
> -
> * wrapped around.
> -
> *
> -
> * Case 4:
> -
> * |--A---H.................T----|
> -
> * Resource is busy, HEAD has wrapped around, tail not yet. Note that Head < Tail
> -
> *
> -
> * Case 5:
> -
> * |------H....A............T----|
> -
> * Resource is idle. Head has wrapped around, tail not yet.
> -
> *
> -
> * Case 6:
> -
> *          A
> -
> *          T
> -
> * |........H....................|
> -
> *

There’s a case 7,  |--H.................T--A--|,  which I don’t think is handled correctly (it goes to the case 5 path).
I think it could handled uniformly with a slightly simpler condition like greater_wrap(A,T) && greater_wrap(H,A) => busy, where greater_wrap(x, y) = (x - y) < UINT_MAX/2.
That assumes that head and tail are close together relative to the range size, but it seems reasonable to me.
> -
> * Queue is empty, resource therefore idle.
> -
> *
> -
> * It is possible that a resource has not been used for a long time and is idle, but the head and
> -
> * tail wrapped around in such a way that the previously set access time falls between head and tail.
> -
> * In this case we will incorrectly wait for the resource. Because we use the entire 32 bits of the
> -
> * counters and not just the bits needed to address the actual queue memory, this should happen rarely.
> -
> * If it turns out to be a problem we can switch to 64 bit counters or attempt to somehow mark the
> -
> * access time of resources invalid. CS packets are at least 4 byte aligned, so we could use the lower
> -
> * 2 bits in access_time for such a marker.
> -
> *
> -
> * Note that the access time is set before the command is submitted, so we have to wait until the
> -
> * tail is bigger than access_time, not equal. */
> - access_time = resource->access_time;
> - head = cs->queue[WINED3D_CS_QUEUE_DEFAULT].head;
> - while (1)
> - {
> -
>    tail = *(volatile ULONG *)&cs->queue[WINED3D_CS_QUEUE_DEFAULT].tail;
> -
>    if (head == tail) /* Case 6, queue empty. */
> -
>        break;
> -
> -
>    if (head > tail)
> -
>    {
> -
>        if (access_time >= head || access_time < tail) /* Case 2, 3. */
> -
>            break;
> -
>    }
> -
>    else if (access_time > tail || access_time <= head) /* Case 5. */
> -
>    {
> -
>        break;
> -
>    }
> -
>    /* Case 1, 4 - busy, wait a little. */
>   YieldProcessor();
> - }
>
> }

- Jan