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

[Fredrik Höglund]

On Thursday 03 November 2016, Jason Ekstrand wrote:
> 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.

Imagine that you have a worker thread that writes data to a buffer,
and then records a command buffer that reads the data. It then creates
a fence, and hands the command buffer and the fence over to the main
thread, which submits them to a queue.  At some point in the future the
worker thread needs to know that it is safe to write over the data in
the buffer, and/or re-record the command buffer, so it waits for the
fence.  When the worker thread does this it doesn't know if the main
thread has submitted the fence to the queue yet.

> 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
>