[PATCH 4/7] anv: Support wait for heterogeneous list of fences [v2]
Jason Ekstrand
jason at jlekstrand.net
Tue Jun 19 23:26:04 UTC 2018
I still don't really like this but I agree that this code really should not
be getting hit very often so it's probably not too bad. Maybe one day
we'll be able to drop the non-syncobj paths entirely. Wouldn't that be
nice. In the mean time, this is probably fine. I did see one issue below
with time conversions that should be fixed though.
On Thu, Jun 14, 2018 at 7:52 PM, Keith Packard <keithp at keithp.com> wrote:
> Handle the case where the set of fences to wait for is not all of the
> same type by either waiting for them sequentially (waitAll), or
> polling them until the timer has expired (!waitAll). We hope the
> latter case is not common.
>
> While the current code makes sure that it always has fences of only
> one type, that will not be true when we add WSI fences. Split out this
> refactoring to make merging that clearer.
>
> v2: Adopt Jason Ekstrand's coding conventions
>
> Declare variables at first use, eliminate extra whitespace between
> types and names. Wrap lines to 80 columns.
>
> Suggested-by: Jason Ekstrand <jason.ekstrand at intel.com>
>
> Signed-off-by: Keith Packard <keithp at keithp.com>
> ---
> src/intel/vulkan/anv_queue.c | 105 +++++++++++++++++++++++++++++------
> 1 file changed, 88 insertions(+), 17 deletions(-)
>
> diff --git a/src/intel/vulkan/anv_queue.c b/src/intel/vulkan/anv_queue.c
> index 6fe04a0a19d..8df99c84549 100644
> --- a/src/intel/vulkan/anv_queue.c
> +++ b/src/intel/vulkan/anv_queue.c
> @@ -459,12 +459,32 @@ gettime_ns(void)
> return (uint64_t)current.tv_sec * NSEC_PER_SEC + current.tv_nsec;
> }
>
> +static uint64_t anv_get_absolute_timeout(uint64_t timeout)
> +{
> + if (timeout == 0)
> + return 0;
> + uint64_t current_time = gettime_ns();
> +
> + timeout = MIN2(INT64_MAX - current_time, timeout);
> +
> + return (current_time + timeout);
> +}
>
This does not have the same behavior as the code it replaces. In
particular, the old code saturates at INT64_MAX whereas this code does not
do so properly anymore. If UINT64_MAX gets passed into timeout, I believe
that will be implicitly casted to signed and the MIN will yield -1 which
gets casted back to unsigned and you get UINT64_MAX again. This is a
problem because the value we pass into the kernel ioctls is signed. The
behavior of the kernel *should* be infinite when timeout < 0 but, on some
older kernels, -1 is treated as 0 and you get no timeout.
That said, I think I just saw a bug in the old code which I'll point out
below.
> +
> +static int64_t anv_get_relative_timeout(uint64_t abs_timeout)
> +{
> + uint64_t now = gettime_ns();
> +
> + if (abs_timeout < now)
> + return 0;
> + return abs_timeout - now;
> +}
> +
> static VkResult
> anv_wait_for_syncobj_fences(struct anv_device *device,
> uint32_t fenceCount,
> const VkFence *pFences,
> bool waitAll,
> - uint64_t _timeout)
> + uint64_t abs_timeout_ns)
> {
> uint32_t *syncobjs = vk_zalloc(&device->alloc,
> sizeof(*syncobjs) * fenceCount, 8,
> @@ -484,19 +504,6 @@ anv_wait_for_syncobj_fences(struct anv_device
> *device,
> syncobjs[i] = impl->syncobj;
> }
>
> - int64_t abs_timeout_ns = 0;
> - if (_timeout > 0) {
> - uint64_t current_ns = gettime_ns();
> -
> - /* Add but saturate to INT32_MAX */
> - if (current_ns + _timeout < current_ns)
> - abs_timeout_ns = INT64_MAX;
> - else if (current_ns + _timeout > INT64_MAX)
>
I suspect we need to cast INT64_MAX to a uint64_t here to ensure we don't
accidentally get an implicit conversion to signed.
> - abs_timeout_ns = INT64_MAX;
> - else
> - abs_timeout_ns = current_ns + _timeout;
> - }
> -
> /* The gem_syncobj_wait ioctl may return early due to an inherent
> * limitation in the way it computes timeouts. Loop until we've
> actually
> * passed the timeout.
> @@ -665,6 +672,67 @@ done:
> return result;
> }
>
> +static VkResult
> +anv_wait_for_fences(struct anv_device *device,
> + uint32_t fenceCount,
> + const VkFence *pFences,
> + bool waitAll,
> + uint64_t abs_timeout)
> +{
> + VkResult result = VK_SUCCESS;
> +
> + if (fenceCount <= 1 || waitAll) {
> + for (uint32_t i = 0; i < fenceCount; i++) {
> + ANV_FROM_HANDLE(anv_fence, fence, pFences[i]);
> + switch (fence->permanent.type) {
> + case ANV_FENCE_TYPE_BO:
> + result = anv_wait_for_bo_fences(
> + device, 1, &pFences[i], true,
> + anv_get_relative_timeout(abs_timeout));
> + break;
> + case ANV_FENCE_TYPE_SYNCOBJ:
> + result = anv_wait_for_syncobj_fences(device, 1, &pFences[i],
> + true, abs_timeout);
> + break;
> + case ANV_FENCE_TYPE_NONE:
> + result = VK_SUCCESS;
> + break;
> + }
> + if (result != VK_SUCCESS)
> + return result;
> + }
> + } else {
> + while (gettime_ns() < abs_timeout) {
> + for (uint32_t i = 0; i < fenceCount; i++) {
> + if (anv_wait_for_fences(device, 1, &pFences[i], true, 0) ==
> VK_SUCCESS)
> + return VK_SUCCESS;
> + }
> + }
> + result = VK_TIMEOUT;
> + }
> + return result;
> +}
> +
> +static bool anv_all_fences_syncobj(uint32_t fenceCount, const VkFence
> *pFences)
> +{
> + for (uint32_t i = 0; i < fenceCount; ++i) {
> + ANV_FROM_HANDLE(anv_fence, fence, pFences[i]);
> + if (fence->permanent.type != ANV_FENCE_TYPE_SYNCOBJ)
> + return false;
> + }
> + return true;
> +}
> +
> +static bool anv_all_fences_bo(uint32_t fenceCount, const VkFence *pFences)
> +{
> + for (uint32_t i = 0; i < fenceCount; ++i) {
> + ANV_FROM_HANDLE(anv_fence, fence, pFences[i]);
> + if (fence->permanent.type != ANV_FENCE_TYPE_BO)
> + return false;
> + }
> + return true;
> +}
> +
> VkResult anv_WaitForFences(
> VkDevice _device,
> uint32_t fenceCount,
> @@ -677,12 +745,15 @@ VkResult anv_WaitForFences(
> if (unlikely(device->lost))
> return VK_ERROR_DEVICE_LOST;
>
> - if (device->instance->physicalDevice.has_syncobj_wait) {
> + if (anv_all_fences_syncobj(fenceCount, pFences)) {
> return anv_wait_for_syncobj_fences(device, fenceCount, pFences,
> - waitAll, timeout);
> - } else {
> + waitAll,
> anv_get_absolute_timeout(timeout));
> + } else if (anv_all_fences_bo(fenceCount, pFences)) {
> return anv_wait_for_bo_fences(device, fenceCount, pFences,
> waitAll, timeout);
> + } else {
> + return anv_wait_for_fences(device, fenceCount, pFences,
> + waitAll, anv_get_absolute_timeout(
> timeout));
> }
> }
>
> --
> 2.17.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20180619/c1c556d1/attachment.html>
More information about the dri-devel
mailing list