<html><head></head><body><div>On Wed, 2017-04-05 at 09:31 -0700, Jason Ekstrand wrote:</div><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Apr 5, 2017 at 12:24 AM, Iago Toral <span dir="ltr"><<a href="mailto:itoral@igalia.com" target="_blank">itoral@igalia.com</a>></span> wrote:<br><blockquote type="cite"><span class="">On Tue, 2017-04-04 at 19:21 -0700, Jason Ekstrand wrote:<br>
> Before, we were just looking at whether or not the user wanted us to<br>
> wait and waiting on the BO. This instead makes us busy-loop on each<br>
> query until it's available. This reduces some of the pipeline<br>
> bubbles<br>
> we were getting and improves performance of The Talos Principle on<br>
> medium settings (where the GPU isn't overloaded) by around 20% on my<br>
> SkyLake gt4.<br>
<br>
</span>Do you think busy looping is a good strategy in general? In scenarios<br>
where the GPU is busier, isn't this expected to have a negative effect<br>
on performance?<br></blockquote><div><br></div><div>The reason why this gets better performance is not because it's a busy wait rather than asking the kernel to wait. It's because it lets us wait on a single query rather than on the whole buffer. Talos packs hundreds of queries into a single pool and the uses of that pool may be split across multiple command buffers. Waiting for the whole BO causes us to basically wait for the GPU to be idle in order to get the one query result we want.<br></div></div></div></div></blockquote><div><br></div><div>Ok, that makes a lot more sense :)</div><div><br></div><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote type="cite"><div><div class="h5">
> ---<br>
> src/intel/vulkan/genX_query.c | 72<br>
> ++++++++++++++++++++++++++++++<wbr>+++++++++----<br>
> 1 file changed, 66 insertions(+), 6 deletions(-)<br>
><br>
> diff --git a/src/intel/vulkan/genX_query.<wbr>c<br>
> b/src/intel/vulkan/genX_query.<wbr>c<br>
> index 7ea9404..ebf99d2 100644<br>
> --- a/src/intel/vulkan/genX_query.<wbr>c<br>
> +++ b/src/intel/vulkan/genX_query.<wbr>c<br>
> @@ -131,6 +131,64 @@ cpu_write_query_result(void *dst_slot,<br>
> VkQueryResultFlags flags,<br>
> }<br>
> }<br>
> <br>
> +#define NSEC_PER_SEC 1000000000<br>
> +<br>
> +static bool<br>
> +query_is_available(struct anv_device *device, uint64_t *slot)<br>
> +{<br>
> + if (!device->info.has_llc)<br>
> + __builtin_ia32_clflush(<wbr>slot);<br>
> +<br>
> + return slot[0];<br>
> +}<br>
> +<br>
> +static VkResult<br>
> +wait_for_available(struct anv_device *device,<br>
> + struct anv_query_pool *pool, uint64_t *slot)<br>
> +{<br>
> + while (true) {<br>
> + struct timespec start;<br>
> + clock_gettime(CLOCK_<wbr>MONOTONIC, &start);<br>
> +<br>
> + while (true) {<br>
> + if (!device->info.has_llc)<br>
> + __builtin_ia32_<wbr>clflush(slot);<br>
> +<br>
> + if (query_is_available(device, slot))<br>
> + return VK_SUCCESS;<br>
> +<br>
> + struct timespec current;<br>
> + clock_gettime(CLOCK_<wbr>MONOTONIC, ¤t);<br>
> +<br>
> + if (current.tv_nsec < start.tv_nsec) {<br>
> + current.tv_nsec += NSEC_PER_SEC;<br>
> + current.tv_sec -= 1;<br>
> + }<br>
> +<br>
> + /* If we've been looping for more than 1 ms, break out of<br>
> the busy<br>
> + * loop and ask the kernel if the buffer is actually busy.<br>
> + */<br>
> + if (current.tv_sec > start.tv_sec ||<br>
<br>
</div></div>This should be && instead of ||, right?<span class=""><br></span><br></blockquote><div><br></div><div>No. Thanks to the subtraction above, if current.tv_sec > start.tv_sec, then it's been at least one whole second.<br></div></div></div></div></blockquote><div><br></div><div>Right, apparently I can't read properly any more...</div><div><br></div><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote type="cite"><span class="">
> + current.tv_nsec - start.tv_nsec > 1000000)<br>
> + break;<br>
> + }<br>
> +<br>
> + VkResult result = anv_device_wait(device, &pool->bo, 0);<br>
> + switch (result) {<br>
> + case VK_SUCCESS:<br>
> + /* The BO is no longer busy. If we haven't seen<br>
> availability yet,<br>
> + * then we never will.<br>
> + */<br>
> + return query_is_available(device, slot) ? VK_SUCCESS :<br>
> VK_NOT_READY;<br>
<br>
</span>By the comment above it sounds like returning VK_NOT_READY isn't what<br>
we want here. If we don't expect the query to ever be available at this<br>
point, returning VK_NOT_READY will only prompt the application to call<br>
us again... shouldn't we return an error in this case?<br><div class="HOEnZb"><div class="h5"></div></div><br></blockquote><div><br></div><div>This case shouldn't happen. If the client is using the API correctly, all of the query results it's trying to get should already have Begin/End submitted to the GPU. However, if the client does something bogus, I don't want to land them in an infinite busy-loop. Returning VK_NOT_READY seemed reasonable, but we could make it some error or other.<br></div></div></div></div></blockquote><div><br></div><div>If this can only happen in the case of incorrect API usage I guess it doesn't really matter much.</div><div><br></div><div>Reviewed-by: Iago Toral Quiroga <<a href="itoral@igalia.com>">itoral@igalia.com></a></div><div><br></div><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote type="cite"><div class="HOEnZb"><div class="h5">
> + case VK_TIMEOUT:<br>
> + /* The BO is still busy, keep waiting. */<br>
> + continue;<br>
> + default:<br>
> + return result;<br>
> + }<br>
> + }<br>
> +}<br>
> +<br>
> VkResult genX(GetQueryPoolResults)(<br>
> VkDevice <wbr> _device,<br>
> VkQueryPool <wbr> queryPool,<br>
> @@ -154,12 +212,6 @@ VkResult genX(GetQueryPoolResults)(<br>
> if (pData == NULL)<br>
> return VK_SUCCESS;<br>
> <br>
> - if (flags & VK_QUERY_RESULT_WAIT_BIT) {<br>
> - VkResult result = anv_device_wait(device, &pool->bo,<br>
> INT64_MAX);<br>
> - if (result != VK_SUCCESS)<br>
> - return result;<br>
> - }<br>
> -<br>
> void *data_end = pData + dataSize;<br>
> <br>
> if (!device->info.has_llc) {<br>
> @@ -176,6 +228,14 @@ VkResult genX(GetQueryPoolResults)(<br>
> /* Availability is always at the start of the slot */<br>
> bool available = slot[0];<br>
> <br>
> + if (!available && (flags & VK_QUERY_RESULT_WAIT_BIT)) {<br>
> + status = wait_for_available(device, pool, slot);<br>
> + if (status != VK_SUCCESS)<br>
> + return status;<br>
> +<br>
> + available = true;<br>
> + }<br>
> +<br>
> /* From the Vulkan 1.0.42 spec:<br>
> *<br>
> * "If VK_QUERY_RESULT_WAIT_BIT and<br>
> VK_QUERY_RESULT_PARTIAL_BIT are</div></div><br></blockquote></div><br></div></div>
</blockquote></body></html>