<div dir="auto"><div>Ping? Any further comments/feedback/reviews? <div class="gmail_extra"> <div class="gmail_quote">On Dec 5, 2016 11:22 AM, "Ilia Mirkin" <<a href="mailto:imirkin@alum.mit.edu">imirkin@alum.mit.edu</a>> wrote: <blockquote class="quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="elided-text">On Mon, Dec 5, 2016 at 11:11 AM, Robert Bragg <<a href="mailto:robert@sixbynine.org">robert@sixbynine.org</a>> wrote: > > > On Sun, Nov 27, 2016 at 7:23 PM, Ilia Mirkin <<a href="mailto:imirkin@alum.mit.edu">imirkin@alum.mit.edu</a>> wrote: >> >> The strategy is to just keep n anv_query_pool_slot entries per query >> instead of one. The available bit is only valid in the last one. >> >> Signed-off-by: Ilia Mirkin <<a href="mailto:imirkin@alum.mit.edu">imirkin@alum.mit.edu</a>> >> --- >> >> I think this is in a pretty good state now. I've tested both the direct >> and >> buffer paths with a hacked up cube application, and I'm seeing >> non-ridiculous >> values for the various counters, although I haven't 100% verified them for >> accuracy. >> >> This also implements the hsw/bdw workaround for dividing frag invocations >> by 4, >> copied from hsw_queryobj. I tested this on SKL and it seem to divide the >> values >> as expected. >> >> The cube patch I've been testing with is at >> <a href="http://paste.debian.net/899374/" rel="noreferrer" target="_blank">http://paste.debian.net/899374/</a> >> You can flip between copying to a buffer and explicit retrieval by >> commenting >> out the relevant function calls. >> >> src/intel/vulkan/anv_device.c | 2 +- >> src/intel/vulkan/anv_private.h | 4 + >> src/intel/vulkan/anv_query.c | 99 ++++++++++---- >> src/intel/vulkan/genX_cmd_buffer.c | 260 >> ++++++++++++++++++++++++++++++++----- >> 4 files changed, 308 insertions(+), 57 deletions(-) >> >> >> diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c >> index 99eb73c..7ad1970 100644 >> --- a/src/intel/vulkan/anv_device.c >> +++ b/src/intel/vulkan/anv_device.c >> @@ -427,7 +427,7 @@ void anv_GetPhysicalDeviceFeatures( >> .textureCompressionASTC_LDR = pdevice->info.gen >= 9, >> /* FINISHME CHV */ >> .textureCompressionBC = true, >> .occlusionQueryPrecise = true, >> - .pipelineStatisticsQuery = false, >> + .pipelineStatisticsQuery = true, >> .fragmentStoresAndAtomics = true, >> .shaderTessellationAndGeometryPointSize = true, >> .shaderImageGatherExtended = false, >> diff --git a/src/intel/vulkan/anv_private.h >> b/src/intel/vulkan/anv_private.h >> index 2fc543d..7271609 100644 >> --- a/src/intel/vulkan/anv_private.h >> +++ b/src/intel/vulkan/anv_private.h >> @@ -1763,6 +1763,8 @@ struct anv_render_pass { >> struct anv_subpass subpasses[0]; >> }; >> >> +#define ANV_PIPELINE_STATISTICS_COUNT 11 >> + >> struct anv_query_pool_slot { >> uint64_t begin; >> uint64_t end; >> @@ -1772,6 +1774,8 @@ struct anv_query_pool_slot { >> struct anv_query_pool { >> VkQueryType type; >> uint32_t slots; >> + uint32_t pipeline_statistics; >> + uint32_t slot_stride; >> struct anv_bo bo; >> }; >> >> diff --git a/src/intel/vulkan/anv_query.c b/src/intel/vulkan/anv_query.c >> index 293257b..dc00859 100644 >> --- a/src/intel/vulkan/anv_query.c >> +++ b/src/intel/vulkan/anv_query.c >> @@ -38,8 +38,10 @@ VkResult anv_CreateQueryPool( >> ANV_FROM_HANDLE(anv_device, device, _device); >> struct anv_query_pool *pool; >> VkResult result; >> - uint32_t slot_size; >> - uint64_t size; >> + uint32_t slot_size = sizeof(struct anv_query_pool_slot); >> + uint32_t slot_stride = 1; >> + uint64_t size = pCreateInfo->queryCount * slot_size; >> + uint32_t pipeline_statistics = 0; >> >> assert(pCreateInfo->sType == >> VK_STRUCTURE_TYPE_QUERY_POOL_CREATE_INFO); >> >> @@ -48,12 +50,16 @@ VkResult anv_CreateQueryPool( >> case VK_QUERY_TYPE_TIMESTAMP: >> break; >> case VK_QUERY_TYPE_PIPELINE_STATISTICS: >> - return VK_ERROR_INCOMPATIBLE_DRIVER; >> + pipeline_statistics = pCreateInfo->pipelineStatistics & >> + ((1 << ANV_PIPELINE_STATISTICS_COUNT) - 1); >> + slot_stride = _mesa_bitcount(pipeline_statistics); >> + size *= slot_stride; >> + break; >> default: >> assert(!"Invalid query type"); >> + return VK_ERROR_INCOMPATIBLE_DRIVER; >> } >> >> - slot_size = sizeof(struct anv_query_pool_slot); >> pool = vk_alloc2(&device->alloc, pAllocator, sizeof(*pool), 8, >> VK_SYSTEM_ALLOCATION_SCOPE_OBJECT); >> if (pool == NULL) >> @@ -61,8 +67,9 @@ VkResult anv_CreateQueryPool( >> >> pool->type = pCreateInfo->queryType; >> pool->slots = pCreateInfo->queryCount; >> + pool->pipeline_statistics = pipeline_statistics; >> + pool->slot_stride = slot_stride; >> >> - size = pCreateInfo->queryCount * slot_size; >> result = anv_bo_init_new(&pool->bo, device, size); >> if (result != VK_SUCCESS) >> goto fail; >> @@ -95,6 +102,27 @@ void anv_DestroyQueryPool( >> vk_free2(&device->alloc, pAllocator, pool); >> } >> >> +static void * >> +store_query_result(void *pData, VkQueryResultFlags flags, >> + uint64_t result, uint64_t available) >> +{ >> + if (flags & VK_QUERY_RESULT_64_BIT) { >> + uint64_t *dst = pData; >> + *dst++ = result; >> + if (flags & VK_QUERY_RESULT_WITH_AVAILABILITY_BIT) >> + *dst++ = available; >> + return dst; >> + } else { >> + uint32_t *dst = pData; >> + if (result > UINT32_MAX) >> + result = UINT32_MAX; >> + *dst++ = result; >> + if (flags & VK_QUERY_RESULT_WITH_AVAILABILITY_BIT) >> + *dst++ = available; >> + return dst; >> + } >> +} >> + >> VkResult anv_GetQueryPoolResults( >> VkDevice _device, >> VkQueryPool queryPool, >> @@ -112,6 +140,7 @@ VkResult anv_GetQueryPoolResults( >> int ret; >> >> assert(pool->type == VK_QUERY_TYPE_OCCLUSION || >> + pool->type == VK_QUERY_TYPE_PIPELINE_STATISTICS || >> pool->type == VK_QUERY_TYPE_TIMESTAMP); >> >> if (pData == NULL) >> @@ -129,14 +158,42 @@ VkResult anv_GetQueryPoolResults( >> void *data_end = pData + dataSize; >> struct anv_query_pool_slot *slot = pool->bo.map; >> >> - for (uint32_t i = 0; i < queryCount; i++) { >> + for (uint32_t i = 0; i < queryCount && pData < data_end; >> + i++, pData += stride) { >> + if (pool->type == VK_QUERY_TYPE_PIPELINE_STATISTICS) { >> + VkQueryResultFlags f = flags & >> ~VK_QUERY_RESULT_WITH_AVAILABILITY_BIT; >> + void *pos = pData; >> + uint32_t pipeline_statistics = pool->pipeline_statistics; >> + struct anv_query_pool_slot *base = >> + &slot[(firstQuery + i) * pool->slot_stride]; >> + >> + while (pipeline_statistics) { >> + uint32_t stat = u_bit_scan(&pipeline_statistics); >> + uint64_t result = base->end - base->begin; >> + >> + /* WaDividePSInvocationCountBy4:HSW,BDW */ >> + if ((device->info.gen == 8 || device->info.is_haswell) && >> + (1 << stat) == >> VK_QUERY_PIPELINE_STATISTIC_FRAGMENT_SHADER_INVOCATIONS_BIT) >> + result >>= 2; >> + >> + pos = store_query_result(pos, f, result, 0); >> + base++; >> + } >> + if (flags & VK_QUERY_RESULT_WITH_AVAILABILITY_BIT) { >> + base--; >> + if (flags & VK_QUERY_RESULT_64_BIT) >> + *(uint64_t *)pos = base->available; >> + else >> + *(uint32_t *)pos = base->available; >> + } >> + continue; >> + } >> + >> switch (pool->type) { >> case VK_QUERY_TYPE_OCCLUSION: { >> result = slot[firstQuery + i].end - slot[firstQuery + i].begin; >> break; >> } >> - case VK_QUERY_TYPE_PIPELINE_STATISTICS: >> - unreachable("pipeline stats not supported"); >> case VK_QUERY_TYPE_TIMESTAMP: { >> result = slot[firstQuery + i].begin; >> break; >> @@ -145,23 +202,7 @@ VkResult anv_GetQueryPoolResults( >> unreachable("invalid pool type"); >> } >> >> - if (flags & VK_QUERY_RESULT_64_BIT) { >> - uint64_t *dst = pData; >> - dst[0] = result; >> - if (flags & VK_QUERY_RESULT_WITH_AVAILABILITY_BIT) >> - dst[1] = slot[firstQuery + i].available; >> - } else { >> - uint32_t *dst = pData; >> - if (result > UINT32_MAX) >> - result = UINT32_MAX; >> - dst[0] = result; >> - if (flags & VK_QUERY_RESULT_WITH_AVAILABILITY_BIT) >> - dst[1] = slot[firstQuery + i].available; >> - } >> - >> - pData += stride; >> - if (pData >= data_end) >> - break; >> + store_query_result(pData, flags, result, slot[firstQuery + >> i].available); >> } >> >> return VK_SUCCESS; >> @@ -183,6 +224,14 @@ void anv_CmdResetQueryPool( >> slot[firstQuery + i].available = 0; >> break; >> } >> + case VK_QUERY_TYPE_PIPELINE_STATISTICS: { >> + struct anv_query_pool_slot *slot = pool->bo.map; >> + >> + slot = &slot[(firstQuery + i) * pool->slot_stride]; >> + for (uint32_t j = 0; j < pool->slot_stride; j++) >> + slot[j].available = 0; >> + break; >> + } >> default: >> assert(!"Invalid query type"); >> } >> diff --git a/src/intel/vulkan/genX_cmd_buffer.c >> b/src/intel/vulkan/genX_cmd_buffer.c >> index a965cd6..1369ac2 100644 >> --- a/src/intel/vulkan/genX_cmd_buffer.c >> +++ b/src/intel/vulkan/genX_cmd_buffer.c >> @@ -2272,6 +2272,50 @@ emit_query_availability(struct anv_cmd_buffer >> *cmd_buffer, >> } >> } >> >> +#define IA_VERTICES_COUNT 0x2310 >> +#define IA_PRIMITIVES_COUNT 0x2318 >> +#define VS_INVOCATION_COUNT 0x2320 >> +#define HS_INVOCATION_COUNT 0x2300 >> +#define DS_INVOCATION_COUNT 0x2308 >> +#define GS_INVOCATION_COUNT 0x2328 >> +#define GS_PRIMITIVES_COUNT 0x2330 >> +#define CL_INVOCATION_COUNT 0x2338 >> +#define CL_PRIMITIVES_COUNT 0x2340 >> +#define PS_INVOCATION_COUNT 0x2348 >> +#define CS_INVOCATION_COUNT 0x2290 >> + >> +static const uint32_t PIPELINE_STAT_TO_REG[] = { >> + IA_VERTICES_COUNT, >> + IA_PRIMITIVES_COUNT, >> + VS_INVOCATION_COUNT, >> + GS_INVOCATION_COUNT, >> + GS_PRIMITIVES_COUNT, >> + CL_INVOCATION_COUNT, >> + CL_PRIMITIVES_COUNT, >> + PS_INVOCATION_COUNT, >> + HS_INVOCATION_COUNT, >> + DS_INVOCATION_COUNT, >> + CS_INVOCATION_COUNT >> +}; >> + >> +static void >> +emit_pipeline_stat(struct anv_cmd_buffer *cmd_buffer, uint32_t stat, >> + struct anv_bo *bo, uint32_t offset) { >> + STATIC_ASSERT(ARRAY_SIZE(PIPELINE_STAT_TO_REG) == >> + ANV_PIPELINE_STATISTICS_COUNT); >> + >> + uint32_t reg = PIPELINE_STAT_TO_REG[stat]; >> + >> + anv_batch_emit(&cmd_buffer->batch, GENX(MI_STORE_REGISTER_MEM), lrm) { >> + lrm.RegisterAddress = reg, >> + lrm.MemoryAddress = (struct anv_address) { bo, offset }; >> + } >> + anv_batch_emit(&cmd_buffer->batch, GENX(MI_STORE_REGISTER_MEM), lrm) { >> + lrm.RegisterAddress = reg + 4, >> + lrm.MemoryAddress = (struct anv_address) { bo, offset + 4 }; >> + } >> +} >> + >> void genX(CmdBeginQuery)( >> VkCommandBuffer commandBuffer, >> VkQueryPool queryPool, >> @@ -2301,7 +2345,25 @@ void genX(CmdBeginQuery)( >> query * sizeof(struct anv_query_pool_slot)); >> break; >> >> - case VK_QUERY_TYPE_PIPELINE_STATISTICS: >> + case VK_QUERY_TYPE_PIPELINE_STATISTICS: { >> + uint32_t pipeline_statistics = pool->pipeline_statistics; >> + uint32_t slot_offset = query * pool->slot_stride * >> + sizeof(struct anv_query_pool_slot); >> + >> + /* TODO: This might only be necessary for certain stats */ >> + anv_batch_emit(&cmd_buffer->batch, GENX(PIPE_CONTROL), pc) { >> + pc.CommandStreamerStallEnable = true; >> + pc.StallAtPixelScoreboard = true; >> + } > > > I couldn't find a reference for needing this for workarounds, though I saw > the workaround Ben implemented for Gen9 + GT4 using a CS stall when querying > timestamps, which is handled separately. </div>Well, experimentally, I need it for the clipper and frag shader invocation counts to show up - otherwise they're always 0. This is on SKL GT2 with the vk-loader cube patched up as described. Perhaps the timing works out s.t. the counters all update outside of the begin/end "critical section" otherwise, so even though they're moving, the difference remains 0. <div class="quoted-text"> > > Depending on how strictly we consider that the queries should only measure > the commands they bracket then I think some stalling will be necessary to > serialize the work associated with a query and defer reading the end state > until after the relevant stages have completed their work. > > We aren't very precise about this in GL currently, but in Begin maybe we > should stall until everything >= the statistic-stage is idle and in End > stall until everything <= the statistic-stage is idle before reading (where > 'statistic-stage' here is the pipeline stage associated with the pipeline > statistic being queried (or respectively the min/max stage for a set)). > > For reference in my implementation of INTEL_performance_query facing this > same question, I'm currently just stalling before and after queries: > > <a href="https://github.com/rib/mesa/blob/wip/rib/oa-next/src/mesa/drivers/dri/i965/brw_performance_query.c#L994" rel="noreferrer" target="_blank">https://github.com/rib/mesa/blob/wip/rib/oa-next/src/mesa/drivers/dri/i965/brw_performance_query.c#L994</a> > <a href="https://github.com/rib/mesa/blob/wip/rib/oa-next/src/mesa/drivers/dri/i965/brw_performance_query.c#L1136" rel="noreferrer" target="_blank">https://github.com/rib/mesa/blob/wip/rib/oa-next/src/mesa/drivers/dri/i965/brw_performance_query.c#L1136</a> </div>So that's essentially what I'm doing here, I think. (And what the GL driver does.) brw_emit_mi_flush actually emits a lot more flushes than this, but I poked around and the main thing is to have a command streamer stall, which on earlier gens requires at least one other bit set. [Please note that I have no real familiarity with the hardware - I'm mostly just looking at what the GL driver does and copying it here with minor adjustments.] <div class="elided-text"> > >> + >> + while (pipeline_statistics) { >> + uint32_t stat = u_bit_scan(&pipeline_statistics); >> + >> + emit_pipeline_stat(cmd_buffer, stat, &pool->bo, slot_offset); >> + slot_offset += sizeof(struct anv_query_pool_slot); >> + } >> + break; >> + } >> default: >> unreachable(""); >> } >> @@ -2314,17 +2376,35 @@ void genX(CmdEndQuery)( >> { >> ANV_FROM_HANDLE(anv_cmd_buffer, cmd_buffer, commandBuffer); >> ANV_FROM_HANDLE(anv_query_pool, pool, queryPool); >> + uint32_t slot_offset = query * pool->slot_stride * >> + sizeof(struct anv_query_pool_slot); >> >> switch (pool->type) { >> case VK_QUERY_TYPE_OCCLUSION: >> - emit_ps_depth_count(cmd_buffer, &pool->bo, >> - query * sizeof(struct anv_query_pool_slot) + >> 8); >> + emit_ps_depth_count(cmd_buffer, &pool->bo, slot_offset + 8); >> + emit_query_availability(cmd_buffer, &pool->bo, slot_offset + 16); >> + break; >> + >> + case VK_QUERY_TYPE_PIPELINE_STATISTICS: { >> + uint32_t pipeline_statistics = pool->pipeline_statistics; >> + /* TODO: This might only be necessary for certain stats */ >> + anv_batch_emit(&cmd_buffer->batch, GENX(PIPE_CONTROL), pc) { >> + pc.CommandStreamerStallEnable = true; >> + pc.StallAtPixelScoreboard = true; >> + } >> + >> + while (pipeline_statistics) { >> + uint32_t stat = u_bit_scan(&pipeline_statistics); >> >> - emit_query_availability(cmd_buffer, &pool->bo, >> - query * sizeof(struct anv_query_pool_slot) >> + 16); >> + emit_pipeline_stat(cmd_buffer, stat, &pool->bo, slot_offset + >> 8); >> + slot_offset += sizeof(struct anv_query_pool_slot); >> + } >> + >> + slot_offset -= sizeof(struct anv_query_pool_slot); >> + emit_query_availability(cmd_buffer, &pool->bo, slot_offset + 16); >> break; >> + } >> >> - case VK_QUERY_TYPE_PIPELINE_STATISTICS: >> default: >> unreachable(""); >> } >> @@ -2421,6 +2501,31 @@ emit_load_alu_reg_u64(struct anv_batch *batch, >> uint32_t reg, >> } >> >> static void >> +emit_load_alu_reg_imm32(struct anv_batch *batch, uint32_t reg, uint32_t >> imm) >> +{ >> + anv_batch_emit(batch, GENX(MI_LOAD_REGISTER_IMM), lri) { >> + lri.RegisterOffset = reg; >> + lri.DataDWord = imm; >> + } >> +} >> + >> +static void >> +emit_load_alu_reg_imm64(struct anv_batch *batch, uint32_t reg, uint64_t >> imm) >> +{ >> + emit_load_alu_reg_imm32(batch, reg, (uint32_t)imm); >> + emit_load_alu_reg_imm32(batch, reg + 4, (uint32_t)(imm >> 32)); >> +} >> + >> +static void >> +emit_load_alu_reg_reg32(struct anv_batch *batch, uint32_t src, uint32_t >> dst) >> +{ >> + anv_batch_emit(batch, GENX(MI_LOAD_REGISTER_REG), lrr) { >> + lrr.SourceRegisterAddress = src; >> + lrr.DestinationRegisterAddress = dst; >> + } >> +} >> + >> +static uint32_t >> store_query_result(struct anv_batch *batch, uint32_t reg, >> struct anv_bo *bo, uint32_t offset, VkQueryResultFlags >> flags) >> { >> @@ -2434,9 +2539,88 @@ store_query_result(struct anv_batch *batch, >> uint32_t reg, >> srm.RegisterAddress = reg + 4; >> srm.MemoryAddress = (struct anv_address) { bo, offset + 4 }; >> } >> + >> + return offset + 8; >> + } >> + >> + return offset + 4; >> +} >> + >> +/* >> + * GPR0 = GPR0 & ((1ull << n) - 1); >> + */ >> +static void >> +keep_gpr0_lower_n_bits(struct anv_batch *batch, uint32_t n) >> +{ >> + assert(n < 64); >> + emit_load_alu_reg_imm64(batch, CS_GPR(1), (1ull << n) - 1); >> + >> + uint32_t *dw = anv_batch_emitn(batch, 5, GENX(MI_MATH)); >> + dw[1] = alu(OPCODE_LOAD, OPERAND_SRCA, OPERAND_R0); >> + dw[2] = alu(OPCODE_LOAD, OPERAND_SRCB, OPERAND_R1); >> + dw[3] = alu(OPCODE_AND, 0, 0); >> + dw[4] = alu(OPCODE_STORE, OPERAND_R0, OPERAND_ACCU); >> +} >> + >> +/* >> + * GPR0 = GPR0 << 30; >> + */ >> +static void >> +shl_gpr0_by_30_bits(struct anv_batch *batch) >> +{ >> + /* First we mask 34 bits of GPR0 to prevent overflow */ >> + keep_gpr0_lower_n_bits(batch, 34); >> + >> + const uint32_t outer_count = 5; >> + const uint32_t inner_count = 6; >> + STATIC_ASSERT(outer_count * inner_count == 30); >> + const uint32_t cmd_len = 1 + inner_count * 4; >> + >> + /* We'll emit 5 commands, each shifting GPR0 left by 6 bits, for a >> total of >> + * 30 left shifts. >> + */ >> + for (int o = 0; o < outer_count; o++) { >> + /* Submit one MI_MATH to shift left by 6 bits */ >> + uint32_t *dw = anv_batch_emitn(batch, cmd_len, GENX(MI_MATH)); >> + dw++; >> + for (int i = 0; i < inner_count; i++, dw += 4) { >> + dw[0] = alu(OPCODE_LOAD, OPERAND_SRCA, OPERAND_R0); >> + dw[1] = alu(OPCODE_LOAD, OPERAND_SRCB, OPERAND_R0); >> + dw[2] = alu(OPCODE_ADD, 0, 0); >> + dw[3] = alu(OPCODE_STORE, OPERAND_R0, OPERAND_ACCU); >> + } >> } >> } >> >> +/* >> + * GPR0 = GPR0 >> 2; >> + * >> + * Note that the upper 30 bits of GPR are lost! >> + */ >> +static void >> +shr_gpr0_by_2_bits(struct anv_batch *batch) >> +{ >> + shl_gpr0_by_30_bits(batch); >> + emit_load_alu_reg_reg32(batch, CS_GPR(0) + 4, CS_GPR(0)); >> + emit_load_alu_reg_imm32(batch, CS_GPR(0) + 4, 0); >> +} >> + >> +static void >> +compute_query_result(struct anv_batch *batch, struct anv_bo *bo, >> + uint32_t dst_reg, uint32_t offset) >> +{ >> + emit_load_alu_reg_u64(batch, CS_GPR(0), bo, offset); >> + emit_load_alu_reg_u64(batch, CS_GPR(1), bo, offset + 8); >> + >> + /* FIXME: We need to clamp the result for 32 bit. */ >> + >> + uint32_t *dw = anv_batch_emitn(batch, 5, GENX(MI_MATH)); >> + dw[1] = alu(OPCODE_LOAD, OPERAND_SRCA, OPERAND_R1); >> + dw[2] = alu(OPCODE_LOAD, OPERAND_SRCB, OPERAND_R0); >> + dw[3] = alu(OPCODE_SUB, 0, 0); >> + dw[4] = alu(OPCODE_STORE, dst_reg, OPERAND_ACCU); >> +} >> + >> void genX(CmdCopyQueryPoolResults)( >> VkCommandBuffer commandBuffer, >> VkQueryPool queryPool, >> @@ -2459,50 +2643,64 @@ void genX(CmdCopyQueryPoolResults)( >> } >> } >> >> - dst_offset = buffer->offset + destOffset; >> for (uint32_t i = 0; i < queryCount; i++) { >> - >> + dst_offset = buffer->offset + destOffset + destStride * i; >> slot_offset = (firstQuery + i) * sizeof(struct >> anv_query_pool_slot); >> switch (pool->type) { >> case VK_QUERY_TYPE_OCCLUSION: >> - emit_load_alu_reg_u64(&cmd_buffer->batch, >> - CS_GPR(0), &pool->bo, slot_offset); >> - emit_load_alu_reg_u64(&cmd_buffer->batch, >> - CS_GPR(1), &pool->bo, slot_offset + 8); >> - >> - /* FIXME: We need to clamp the result for 32 bit. */ >> - >> - uint32_t *dw = anv_batch_emitn(&cmd_buffer->batch, 5, >> GENX(MI_MATH)); >> - dw[1] = alu(OPCODE_LOAD, OPERAND_SRCA, OPERAND_R1); >> - dw[2] = alu(OPCODE_LOAD, OPERAND_SRCB, OPERAND_R0); >> - dw[3] = alu(OPCODE_SUB, 0, 0); >> - dw[4] = alu(OPCODE_STORE, OPERAND_R2, OPERAND_ACCU); >> + compute_query_result(&cmd_buffer->batch, &pool->bo, OPERAND_R2, >> + slot_offset); >> + dst_offset = store_query_result( >> + &cmd_buffer->batch, >> + CS_GPR(2), buffer->bo, dst_offset, flags); >> break; >> >> case VK_QUERY_TYPE_TIMESTAMP: >> emit_load_alu_reg_u64(&cmd_buffer->batch, >> CS_GPR(2), &pool->bo, slot_offset); >> + dst_offset = store_query_result( >> + &cmd_buffer->batch, >> + CS_GPR(2), buffer->bo, dst_offset, flags); >> break; >> >> + case VK_QUERY_TYPE_PIPELINE_STATISTICS: { >> + uint32_t pipeline_statistics = pool->pipeline_statistics; >> + >> + slot_offset *= pool->slot_stride; >> + while (pipeline_statistics) { >> + uint32_t stat = u_bit_scan(&pipeline_statistics); >> + >> + compute_query_result(&cmd_buffer->batch, &pool->bo, >> OPERAND_R0, >> + slot_offset); >> + >> + /* WaDividePSInvocationCountBy4:HSW,BDW */ >> + if ((cmd_buffer->device->info.gen == 8 || >> + cmd_buffer->device->info.is_haswell) && >> + (1 << stat) == >> VK_QUERY_PIPELINE_STATISTIC_FRAGMENT_SHADER_INVOCATIONS_BIT) { >> + shr_gpr0_by_2_bits(&cmd_buffer->batch); >> + } >> + dst_offset = store_query_result( >> + &cmd_buffer->batch, >> + CS_GPR(0), buffer->bo, dst_offset, flags); >> + slot_offset += sizeof(struct anv_query_pool_slot); >> + } >> + >> + /* Get the slot offset to where it's supposed to be for the >> + * availability bit. >> + */ >> + slot_offset -= sizeof(struct anv_query_pool_slot); >> + break; >> + } >> default: >> unreachable("unhandled query type"); >> } >> >> - store_query_result(&cmd_buffer->batch, >> - CS_GPR(2), buffer->bo, dst_offset, flags); >> - >> if (flags & VK_QUERY_RESULT_WITH_AVAILABILITY_BIT) { >> emit_load_alu_reg_u64(&cmd_buffer->batch, CS_GPR(0), >> &pool->bo, slot_offset + 16); >> - if (flags & VK_QUERY_RESULT_64_BIT) >> - store_query_result(&cmd_buffer->batch, >> - CS_GPR(0), buffer->bo, dst_offset + 8, >> flags); >> - else >> - store_query_result(&cmd_buffer->batch, >> - CS_GPR(0), buffer->bo, dst_offset + 4, >> flags); >> + store_query_result(&cmd_buffer->batch, >> + CS_GPR(0), buffer->bo, dst_offset, flags); >> } >> - >> - dst_offset += destStride; >> } >> } > > > I was a bit surprised to see us needing to go the same route as GL query > objects with using the command streamer's very limited ALU for fixing up > metrics on the gpu instead of doing that on the CPU if possible. > > My understanding had been that multiple HW vendors pushed back on the design > of these query apis to ensure they could deal with counter scaling and > normalization on the CPU without needing to support direct feedback into the > gpu. Reading the spec though it does unfortunately look like > vkCmdCopyQueryPoolResults implies scaling on the gpu :-/ </div>Yeah, that looks identical to ARB_query_buffer_object functionality, with very minor differences. <div class="quoted-text"> > > One thorny problem we have today at least with GL is that we don't scale > Skylake timestamps properly when using the ALU considering the extra > annoyance of scaling by a floating point factor of 83.3333. In the future > this scale factor might ideally not even be a constant if we can take > advantage of training done in the kernel to determine a more accurate > frequency (ref: > <a href="https://lists.freedesktop.org/archives/intel-gfx/2016-June/097272.html" rel="noreferrer" target="_blank">https://lists.freedesktop.org/archives/intel-gfx/2016-June/097272.html</a>) Also > for reference here, there's this patch that looks at fixing the timestamp > scaling done on the cpu for SKL: > <a href="https://patchwork.freedesktop.org/patch/118855/" rel="noreferrer" target="_blank">https://patchwork.freedesktop.org/patch/118855/</a> I understand you aren't > dealing with timestamp scaling here, but it seems like an a example of where > it's getting a bit out-of-hand using the ALU for this type of purpose. </div>I haven't checked, but I hope that the timestamp units can be in whatever you want. So you can hopefully get away without scaling. <div class="quoted-text"> > > Can we maybe at least consider the complexity trade-off from using a shader > instead of ALU - or consider if there's a legitimate way we're missing of > using the cpu here (though I suppose not). </div>Go ahead :) I was just copying the GL approach. Happy to rip out the MI_MATH stuff and make the feature IVB + SKL+ only for now if you all agree that the MI_MATH stuff is bad. IMHO the complexity of the MI_MATH logic is quite small for the / 4 case, but I don't feel like I'm particularly part of the decision committee. Cheers, -ilia </blockquote></div> </div></div></div>