<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">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.<br></div><div class="gmail_quote"><br></div><div class="gmail_quote">On Thu, Jun 14, 2018 at 7:52 PM, Keith Packard <span dir="ltr"><<a href="mailto:keithp@keithp.com" target="_blank">keithp@keithp.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Handle the case where the set of fences to wait for is not all of the<br>
same type by either waiting for them sequentially (waitAll), or<br>
polling them until the timer has expired (!waitAll). We hope the<br>
latter case is not common.<br>
<br>
While the current code makes sure that it always has fences of only<br>
one type, that will not be true when we add WSI fences. Split out this<br>
refactoring to make merging that clearer.<br>
<br>
v2: Adopt Jason Ekstrand's coding conventions<br>
<br>
Declare variables at first use, eliminate extra whitespace between<br>
types and names. Wrap lines to 80 columns.<br>
<br>
Suggested-by: Jason Ekstrand <<a href="mailto:jason.ekstrand@intel.com">jason.ekstrand@intel.com</a>><br>
<br>
Signed-off-by: Keith Packard <<a href="mailto:keithp@keithp.com">keithp@keithp.com</a>><br>
---<br>
src/intel/vulkan/anv_queue.c | 105 +++++++++++++++++++++++++++++-<wbr>-----<br>
1 file changed, 88 insertions(+), 17 deletions(-)<br>
<br>
diff --git a/src/intel/vulkan/anv_queue.c b/src/intel/vulkan/anv_queue.c<br>
index 6fe04a0a19d..8df99c84549 100644<br>
--- a/src/intel/vulkan/anv_queue.c<br>
+++ b/src/intel/vulkan/anv_queue.c<br>
@@ -459,12 +459,32 @@ gettime_ns(void)<br>
return (uint64_t)current.tv_sec * NSEC_PER_SEC + current.tv_nsec;<br>
}<br>
<br>
+static uint64_t anv_get_absolute_timeout(<wbr>uint64_t timeout)<br>
+{<br>
+ if (timeout == 0)<br>
+ return 0;<br>
+ uint64_t current_time = gettime_ns();<br>
+<br>
+ timeout = MIN2(INT64_MAX - current_time, timeout);<br>
+<br>
+ return (current_time + timeout);<br>
+}<br></blockquote><div><br></div><div>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.</div><div><br></div><div>That said, I think I just saw a bug in the old code which I'll point out below.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+static int64_t anv_get_relative_timeout(<wbr>uint64_t abs_timeout)<br>
+{<br>
+ uint64_t now = gettime_ns();<br>
+<br>
+ if (abs_timeout < now)<br>
+ return 0;<br>
+ return abs_timeout - now;<br>
+}<br>
+<br>
static VkResult<br>
anv_wait_for_syncobj_fences(<wbr>struct anv_device *device,<br>
uint32_t fenceCount,<br>
const VkFence *pFences,<br>
bool waitAll,<br>
- uint64_t _timeout)<br>
+ uint64_t abs_timeout_ns)<br>
{<br>
uint32_t *syncobjs = vk_zalloc(&device->alloc,<br>
sizeof(*syncobjs) * fenceCount, 8,<br>
@@ -484,19 +504,6 @@ anv_wait_for_syncobj_fences(<wbr>struct anv_device *device,<br>
syncobjs[i] = impl->syncobj;<br>
}<br>
<br>
- int64_t abs_timeout_ns = 0;<br>
- if (_timeout > 0) {<br>
- uint64_t current_ns = gettime_ns();<br>
-<br>
- /* Add but saturate to INT32_MAX */<br>
- if (current_ns + _timeout < current_ns)<br>
- abs_timeout_ns = INT64_MAX;<br>
- else if (current_ns + _timeout > INT64_MAX)<br></blockquote><div><br></div><div>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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- abs_timeout_ns = INT64_MAX;<br>
- else<br>
- abs_timeout_ns = current_ns + _timeout;<br>
- }<br>
-<br>
/* The gem_syncobj_wait ioctl may return early due to an inherent<br>
* limitation in the way it computes timeouts. Loop until we've actually<br>
* passed the timeout.<br>
@@ -665,6 +672,67 @@ done:<br>
return result;<br>
}<br>
<br>
+static VkResult<br>
+anv_wait_for_fences(struct anv_device *device,<br>
+ uint32_t fenceCount,<br>
+ const VkFence *pFences,<br>
+ bool waitAll,<br>
+ uint64_t abs_timeout)<br>
+{<br>
+ VkResult result = VK_SUCCESS;<br>
+<br>
+ if (fenceCount <= 1 || waitAll) {<br>
+ for (uint32_t i = 0; i < fenceCount; i++) {<br>
+ ANV_FROM_HANDLE(anv_fence, fence, pFences[i]);<br>
+ switch (fence->permanent.type) {<br>
+ case ANV_FENCE_TYPE_BO:<br>
+ result = anv_wait_for_bo_fences(<br>
+ device, 1, &pFences[i], true,<br>
+ anv_get_relative_timeout(abs_<wbr>timeout));<br>
+ break;<br>
+ case ANV_FENCE_TYPE_SYNCOBJ:<br>
+ result = anv_wait_for_syncobj_fences(<wbr>device, 1, &pFences[i],<br>
+ true, abs_timeout);<br>
+ break;<br>
+ case ANV_FENCE_TYPE_NONE:<br>
+ result = VK_SUCCESS;<br>
+ break;<br>
+ }<br>
+ if (result != VK_SUCCESS)<br>
+ return result;<br>
+ }<br>
+ } else {<br>
+ while (gettime_ns() < abs_timeout) {<br>
+ for (uint32_t i = 0; i < fenceCount; i++) {<br>
+ if (anv_wait_for_fences(device, 1, &pFences[i], true, 0) == VK_SUCCESS)<br>
+ return VK_SUCCESS;<br>
+ }<br>
+ }<br>
+ result = VK_TIMEOUT;<br>
+ }<br>
+ return result;<br>
+}<br>
+<br>
+static bool anv_all_fences_syncobj(uint32_<wbr>t fenceCount, const VkFence *pFences)<br>
+{<br>
+ for (uint32_t i = 0; i < fenceCount; ++i) {<br>
+ ANV_FROM_HANDLE(anv_fence, fence, pFences[i]);<br>
+ if (fence->permanent.type != ANV_FENCE_TYPE_SYNCOBJ)<br>
+ return false;<br>
+ }<br>
+ return true;<br>
+}<br>
+<br>
+static bool anv_all_fences_bo(uint32_t fenceCount, const VkFence *pFences)<br>
+{<br>
+ for (uint32_t i = 0; i < fenceCount; ++i) {<br>
+ ANV_FROM_HANDLE(anv_fence, fence, pFences[i]);<br>
+ if (fence->permanent.type != ANV_FENCE_TYPE_BO)<br>
+ return false;<br>
+ }<br>
+ return true;<br>
+}<br>
+<br>
VkResult anv_WaitForFences(<br>
VkDevice _device,<br>
uint32_t fenceCount,<br>
@@ -677,12 +745,15 @@ VkResult anv_WaitForFences(<br>
if (unlikely(device->lost))<br>
return VK_ERROR_DEVICE_LOST;<br>
<br>
- if (device->instance-><wbr>physicalDevice.has_syncobj_<wbr>wait) {<br>
+ if (anv_all_fences_syncobj(<wbr>fenceCount, pFences)) {<br>
return anv_wait_for_syncobj_fences(<wbr>device, fenceCount, pFences,<br>
- waitAll, timeout);<br>
- } else {<br>
+ waitAll, anv_get_absolute_timeout(<wbr>timeout));<br>
+ } else if (anv_all_fences_bo(fenceCount, pFences)) {<br>
return anv_wait_for_bo_fences(device, fenceCount, pFences,<br>
waitAll, timeout);<br>
+ } else {<br>
+ return anv_wait_for_fences(device, fenceCount, pFences,<br>
+ waitAll, anv_get_absolute_timeout(<wbr>timeout));<br>
}<br>
}<br>
<span class="HOEnZb"><font color="#888888"> <br>
-- <br>
2.17.1<br>
<br>
______________________________<wbr>_________________<br>
dri-devel mailing list<br>
<a href="mailto:dri-devel@lists.freedesktop.org">dri-devel@lists.freedesktop.<wbr>org</a><br>
<a href="https://lists.freedesktop.org/mailman/listinfo/dri-devel" rel="noreferrer" target="_blank">https://lists.freedesktop.org/<wbr>mailman/listinfo/dri-devel</a><br>
</font></span></blockquote></div><br></div></div>