<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Apr 5, 2017 at 11:18 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">On Wed, Apr 05, 2017 at 11:02:18AM -0700, Jason Ekstrand wrote: > On Wed, Apr 5, 2017 at 10:45 AM, Chris Wilson <div><div class="m_7071355497396330583h5">> <[1]<a href="mailto:chris@chris-wilson.co.uk" target="_blank">chris@chris-wilson.co.uk</a>> wrote: > > 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); > > 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. > > 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>It's the ordering though. Currently it is !query_is_available !busy get_reset_stats <-- slow return query_is_available. I'm arguing that you want !query_is_available !busy if (query_is_available) return SUCCESS return get_reset_stats <-- slow. with the observation that if the gpu completed the available before the hang, it is not affected by the unlikely hang. You really, really want to avoid get_reset_stats on low latency paths (for the next few months at least). </blockquote></div> </div><div class="gmail_extra">Ok, that makes more sense. </div></div>