[Mesa-dev] [PATCH 3/3] anv: Rework fences

[Jason Ekstrand]

On Thu, Nov 3, 2016 at 2:53 AM, Chris Wilson <chris at chris-wilson.co.uk>
wrote:

> On Wed, Nov 02, 2016 at 05:15:52PM -0700, Jason Ekstrand wrote:
> > @@ -1562,22 +1580,98 @@ VkResult anv_WaitForFences(
> >      * best we can do is to clamp the timeout to INT64_MAX.  This limits
> the
> >      * maximum timeout from 584 years to 292 years - likely not a big
> deal.
> >      */
> > -   if (timeout > INT64_MAX)
> > -      timeout = INT64_MAX;
> > -
> > -   int64_t t = timeout;
> > +   int64_t timeout = MIN2(_timeout, INT64_MAX);
>
> > +      if (pending_fences && !signaled_fences) {
> > +         /* If we've hit this then someone decided to vkWaitForFences
> before
> > +          * they've actually submitted any of them to a queue.  This is
> a
> > +          * fairly pessimal case, so it's ok to lock here and use a
> standard
> > +          * pthreads condition variable.
> > +          */
> > +         pthread_mutex_lock(&device->mutex);
> > +
> > +         /* It's possible that some of the fences have changed state
> since the
> > +          * last time we checked.  Now that we have the lock, check for
> > +          * pending fences again and don't wait if it's changed.
> > +          */
> > +         uint32_t now_pending_fences = 0;
> > +         for (uint32_t i = 0; i < fenceCount; i++) {
> > +            ANV_FROM_HANDLE(anv_fence, fence, pFences[i]);
> > +            if (fence->state == ANV_FENCE_STATE_RESET)
> > +               now_pending_fences++;
> > +         }
> > +         assert(now_pending_fences <= pending_fences);
> > +
> > +         bool timeout = false;
>
> Shadowed timeout...
>

Drp...


> > +         if (now_pending_fences == pending_fences) {
> > +            struct timeval before;
> > +            gettimeofday(&before, NULL);
> > +
> > +            struct timespec ts = {
> > +               .tv_nsec = timeout % 1000000000,
> > +               .tv_sec = timeout / 1000000000,
> > +            };
> > +            pthread_cond_timedwait(&device->queue_submit,
> &device->mutex, &ts);
> > +
> > +            struct timeval after;
> > +            gettimeofday(&after, NULL);
> > +            uint64_t time_elapsed =
> > +               ((uint64_t)after.tv_sec * 1000000000 + after.tv_usec *
> 1000) -
> > +               ((uint64_t)before.tv_sec * 1000000000 + before.tv_usec *
> 1000);
> > +
> > +            if (time_elapsed > timeout) {
> > +               pthread_mutex_unlock(&device->mutex);
> > +               return VK_TIMEOUT;
> > +            }
> > +
> > +            timeout -= time_elapsed;
>
> Adjusts the bool and not the int64_t nanosecond counter.
>
> Looks ok, the only question is whether that should be a queue mutex
> rather than a device mutex for submitting. (Are fences local to a
> queue? Is the shared relocation state local to a queue or global...)
>

Fences get submitted on a queue so you could claim that a queue mutex is
better.  However, this case should never happen in practice but only really
in CTS tests (why would you wait on a fence you haven't submitted?) so
using a more global mutex isn't a huge deal.  Also, we only have one queue
per device so it works out the same.

Incidentally, I realized as I was working on some WSI stuff that I did my
timedwait all wrong here.  The time value you provide to
pthread_cond_timedwait is supposed to be an absolute time rather than a
delta relative to the call.  I'll get that sorted and send a v2.

--Jason
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20161103/6d3adaad/attachment-0001.html>