<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Apr 5, 2017 at 10:45 AM, Chris Wilson <<a href="mailto:chris@chris-wilson.co.uk" target="_blank">chris@chris-wilson.co.uk</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Wed, Apr 05, 2017 at 10:28:53AM -0700, Jason Ekstrand wrote: > Before, we were just looking at whether or not the user wanted us to > wait and waiting on the BO. Some clients, such as the Serious engine, > use a single query pool for hundreds of individual query results where > the writes for those queries may be split across several command > buffers. In this scenario, the individual query we're looking for may > become available long before the BO is idle so waiting on the query pool > BO to be finished is wasteful. This commit makes us instead busy-loop on > each query until it's available. > > This significantly reduces pipeline bubbles and improves performance of > The Talos Principle on medium settings (where the GPU isn't overloaded > with drawing) by around 20% on my SkyLake gt4. > --- > src/intel/vulkan/genX_query.c | 52 ++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 46 insertions(+), 6 deletions(-) > > diff --git a/src/intel/vulkan/genX_query.c b/src/intel/vulkan/genX_query.c > index 7ea9404..0d303a6 100644 > --- a/src/intel/vulkan/genX_query.c > +++ b/src/intel/vulkan/genX_query.c > @@ -131,6 +131,44 @@ cpu_write_query_result(void *dst_slot, VkQueryResultFlags flags, > } > } > > +static bool > +query_is_available(struct anv_device *device, volatile uint64_t *slot) > +{ > + if (!device->info.has_llc) > + __builtin_ia32_clflush(slot); > + > + return slot[0]; > +} > + > +static VkResult > +wait_for_available(struct anv_device *device, > + struct anv_query_pool *pool, uint64_t *slot) > +{ > + while (true) { > + if (query_is_available(device, slot)) > + return VK_SUCCESS; > + > + VkResult result = anv_device_bo_busy(device, &pool->bo); </div></div>Ah, but you can use the simpler check here because you follow up with a query_is_available() so you know whether or not the hang clobbered the result. If the query is not available but the bo is idle, you might then went to check for a reset in case it was due to a lost device. GEM_BUSY is lockless, but GEM_RESET_STATS currently takes the big struct_mutex and so has non-deterministic and often quite large latencies. </blockquote></div> </div><div class="gmail_extra">anv_device_bo_busy does that for us. :-) In particular, it queries GEM_RESET_STATS but only if the BO is not busy. This way, so long as the BO is busy, we only do GEM_BUSY but as soon as it is not busy we do a single GEM_RESET_STATS and return VK_ERROR_DEVICE_LOST if we've gotten a hang. The theory (the same for wait) is that, so long as the BO is still busy, we either haven't hung or we haven't figured out that we've hung. We could, in theory have two batches where the first one hangs and the second is ok, so this isn't quite true. However, the important thing is to never return VK_SUCCESS to the user when results are invalid. Both anv_device_wait and anv_device_bo_busy only do an early return of VK_NOT_READY or VK_TIMEOUT and don't return VK_SUCCESS until after they've called GEM_RESET_STATS to ensuring that we haven't hung. </div></div>