I'm not that familiar with this code yet, so take this review with a grain of salt, but it looks good to me.
Reviewed-by: Rafael Antognolli <rafael.antogno...@intel.com> Just a few comments below but nothing really important. On Tue, Aug 07, 2018 at 06:35:21PM +0100, Lionel Landwerlin wrote: > Signed-off-by: Lionel Landwerlin <lionel.g.landwer...@intel.com> > --- > src/intel/tools/aubinator_viewer.h | 26 ++++ > src/intel/tools/aubinator_viewer_decoder.cpp | 150 ++++++++++++++++--- > 2 files changed, 153 insertions(+), 23 deletions(-) > > diff --git a/src/intel/tools/aubinator_viewer.h > b/src/intel/tools/aubinator_viewer.h > index 2d89d9cf658..4a030efc0d0 100644 > --- a/src/intel/tools/aubinator_viewer.h > +++ b/src/intel/tools/aubinator_viewer.h > @@ -33,12 +33,35 @@ struct aub_viewer_decode_cfg { > show_dwords(true) {} > }; > > +enum aub_decode_stage { > + AUB_DECODE_STAGE_VS, > + AUB_DECODE_STAGE_HS, > + AUB_DECODE_STAGE_DS, > + AUB_DECODE_STAGE_GS, > + AUB_DECODE_STAGE_PS, > + AUB_DECODE_STAGE_CS, > + AUB_DECODE_N_STAGE, > +}; > + > +struct aub_decode_urb_stage_state { > + uint32_t start; > + uint32_t size; > + uint32_t n_entries; > + > + uint32_t const_rd_length; > + uint32_t rd_offset; > + uint32_t rd_length; > + uint32_t wr_offset; > + uint32_t wr_length; > +}; > + > struct aub_viewer_decode_ctx { > struct gen_batch_decode_bo (*get_bo)(void *user_data, uint64_t address); > unsigned (*get_state_size)(void *user_data, > uint32_t offset_from_dynamic_state_base_addr); > > void (*display_shader)(void *user_data, const char *shader_desc, uint64_t > address); > + void (*display_urb)(void *user_data, const struct > aub_decode_urb_stage_state *stages); > void (*edit_address)(void *user_data, uint64_t address, uint32_t length); > > void *user_data; > @@ -53,6 +76,9 @@ struct aub_viewer_decode_ctx { > uint64_t dynamic_base; > uint64_t instruction_base; > > + enum aub_decode_stage stage; > + uint32_t end_urb_offset; > + struct aub_decode_urb_stage_state urb_stages[AUB_DECODE_N_STAGE]; > }; > > void aub_viewer_decode_ctx_init(struct aub_viewer_decode_ctx *ctx, > diff --git a/src/intel/tools/aubinator_viewer_decoder.cpp > b/src/intel/tools/aubinator_viewer_decoder.cpp > index a2ea3ba4a64..273bc2da376 100644 > --- a/src/intel/tools/aubinator_viewer_decoder.cpp > +++ b/src/intel/tools/aubinator_viewer_decoder.cpp > @@ -695,38 +695,125 @@ decode_load_register_imm(struct aub_viewer_decode_ctx > *ctx, > } > } > > +static void > +decode_3dprimitive(struct aub_viewer_decode_ctx *ctx, > + struct gen_group *inst, > + const uint32_t *p) > +{ > + if (ctx->display_urb) { > + if (ImGui::Button("Show URB")) > + ctx->display_urb(ctx->user_data, ctx->urb_stages); > + } > +} > + > +static void > +handle_urb(struct aub_viewer_decode_ctx *ctx, > + struct gen_group *inst, > + const uint32_t *p) > +{ > + struct gen_field_iterator iter; > + gen_field_iterator_init(&iter, inst, p, 0, false); > + while (gen_field_iterator_next(&iter)) { > + if (strstr(iter.name, "URB Starting Address")) { > + ctx->urb_stages[ctx->stage].start = iter.raw_value * 8192; > + } else if (strstr(iter.name, "URB Entry Allocation Size")) { > + ctx->urb_stages[ctx->stage].size = (iter.raw_value + 1) * 64; > + } else if (strstr(iter.name, "Number of URB Entries")) { > + ctx->urb_stages[ctx->stage].n_entries = iter.raw_value; > + } > + } > + > + ctx->end_urb_offset = MAX2(ctx->urb_stages[ctx->stage].start + > + ctx->urb_stages[ctx->stage].n_entries * > + ctx->urb_stages[ctx->stage].size, > + ctx->end_urb_offset); > +} > + > +static void > +handle_urb_read(struct aub_viewer_decode_ctx *ctx, > + struct gen_group *inst, > + const uint32_t *p) > +{ > + struct gen_field_iterator iter; > + gen_field_iterator_init(&iter, inst, p, 0, false); > + while (gen_field_iterator_next(&iter)) { > + /* Workaround the "Force * URB Entry Read Length" fields */ > + if (iter.end_bit - iter.start_bit < 2) > + continue; > + > + if (strstr(iter.name, "URB Entry Read Offset")) { > + ctx->urb_stages[ctx->stage].rd_offset = iter.raw_value * 32; > + } else if (strstr(iter.name, "URB Entry Read Length")) { > + ctx->urb_stages[ctx->stage].rd_length = iter.raw_value * 32; > + } else if (strstr(iter.name, "URB Entry Output Read Offset")) { > + ctx->urb_stages[ctx->stage].wr_offset = iter.raw_value * 32; > + } else if (strstr(iter.name, "URB Entry Output Length")) { > + ctx->urb_stages[ctx->stage].wr_length = iter.raw_value * 32; > + } > + } > +} > + > +static void > +handle_urb_constant(struct aub_viewer_decode_ctx *ctx, > + struct gen_group *inst, > + const uint32_t *p) > +{ > + struct gen_group *body = > + gen_spec_find_struct(ctx->spec, "3DSTATE_CONSTANT_BODY"); > + > + struct gen_field_iterator outer; > + gen_field_iterator_init(&outer, inst, p, 0, false); > + while (gen_field_iterator_next(&outer)) { > + if (outer.struct_desc != body) > + continue; > + > + struct gen_field_iterator iter; > + gen_field_iterator_init(&iter, body, &outer.p[outer.start_bit / 32], > + 0, false); > + > + ctx->urb_stages[ctx->stage].const_rd_length = 0; > + while (gen_field_iterator_next(&iter)) { > + int idx; > + if (sscanf(iter.name, "Read Length[%d]", &idx) == 1) { > + ctx->urb_stages[ctx->stage].const_rd_length += iter.raw_value * > 32; > + } > + } > + } > +} > + > struct custom_decoder { > const char *cmd_name; > void (*decode)(struct aub_viewer_decode_ctx *ctx, > struct gen_group *inst, > const uint32_t *p); > + enum aub_decode_stage stage; > } display_decoders[] = { > { "STATE_BASE_ADDRESS", handle_state_base_address }, > { "MEDIA_INTERFACE_DESCRIPTOR_LOAD", > handle_media_interface_descriptor_load }, > { "3DSTATE_VERTEX_BUFFERS", handle_3dstate_vertex_buffers }, > { "3DSTATE_INDEX_BUFFER", handle_3dstate_index_buffer }, > - { "3DSTATE_VS", decode_single_ksp }, > - { "3DSTATE_GS", decode_single_ksp }, > - { "3DSTATE_DS", decode_single_ksp }, > - { "3DSTATE_HS", decode_single_ksp }, > - { "3DSTATE_PS", decode_ps_kernels }, > - { "3DSTATE_CONSTANT_VS", decode_3dstate_constant }, > - { "3DSTATE_CONSTANT_GS", decode_3dstate_constant }, > - { "3DSTATE_CONSTANT_PS", decode_3dstate_constant }, > - { "3DSTATE_CONSTANT_HS", decode_3dstate_constant }, > - { "3DSTATE_CONSTANT_DS", decode_3dstate_constant }, > - > - { "3DSTATE_BINDING_TABLE_POINTERS_VS", > decode_3dstate_binding_table_pointers }, > - { "3DSTATE_BINDING_TABLE_POINTERS_HS", > decode_3dstate_binding_table_pointers }, > - { "3DSTATE_BINDING_TABLE_POINTERS_DS", > decode_3dstate_binding_table_pointers }, > - { "3DSTATE_BINDING_TABLE_POINTERS_GS", > decode_3dstate_binding_table_pointers }, > - { "3DSTATE_BINDING_TABLE_POINTERS_PS", > decode_3dstate_binding_table_pointers }, > - > - { "3DSTATE_SAMPLER_STATE_POINTERS_VS", > decode_3dstate_sampler_state_pointers }, > - { "3DSTATE_SAMPLER_STATE_POINTERS_HS", > decode_3dstate_sampler_state_pointers }, > - { "3DSTATE_SAMPLER_STATE_POINTERS_DS", > decode_3dstate_sampler_state_pointers }, > - { "3DSTATE_SAMPLER_STATE_POINTERS_GS", > decode_3dstate_sampler_state_pointers }, > - { "3DSTATE_SAMPLER_STATE_POINTERS_PS", > decode_3dstate_sampler_state_pointers }, > + { "3DSTATE_VS", decode_single_ksp, AUB_DECODE_STAGE_VS, }, > + { "3DSTATE_GS", decode_single_ksp, AUB_DECODE_STAGE_GS, }, > + { "3DSTATE_DS", decode_single_ksp, AUB_DECODE_STAGE_DS, }, > + { "3DSTATE_HS", decode_single_ksp, AUB_DECODE_STAGE_HS, }, > + { "3DSTATE_PS", decode_ps_kernels, AUB_DECODE_STAGE_PS, }, > + { "3DSTATE_CONSTANT_VS", decode_3dstate_constant, AUB_DECODE_STAGE_VS, }, > + { "3DSTATE_CONSTANT_GS", decode_3dstate_constant, AUB_DECODE_STAGE_GS, }, > + { "3DSTATE_CONSTANT_DS", decode_3dstate_constant, AUB_DECODE_STAGE_DS, }, > + { "3DSTATE_CONSTANT_HS", decode_3dstate_constant, AUB_DECODE_STAGE_HS, }, > + { "3DSTATE_CONSTANT_PS", decode_3dstate_constant, AUB_DECODE_STAGE_PS, }, > + > + { "3DSTATE_BINDING_TABLE_POINTERS_VS", > decode_3dstate_binding_table_pointers, AUB_DECODE_STAGE_VS, }, > + { "3DSTATE_BINDING_TABLE_POINTERS_GS", > decode_3dstate_binding_table_pointers, AUB_DECODE_STAGE_GS, }, > + { "3DSTATE_BINDING_TABLE_POINTERS_HS", > decode_3dstate_binding_table_pointers, AUB_DECODE_STAGE_HS, }, > + { "3DSTATE_BINDING_TABLE_POINTERS_DS", > decode_3dstate_binding_table_pointers, AUB_DECODE_STAGE_DS, }, > + { "3DSTATE_BINDING_TABLE_POINTERS_PS", > decode_3dstate_binding_table_pointers, AUB_DECODE_STAGE_PS, }, > + > + { "3DSTATE_SAMPLER_STATE_POINTERS_VS", > decode_3dstate_sampler_state_pointers, AUB_DECODE_STAGE_VS, }, > + { "3DSTATE_SAMPLER_STATE_POINTERS_GS", > decode_3dstate_sampler_state_pointers, AUB_DECODE_STAGE_GS, }, > + { "3DSTATE_SAMPLER_STATE_POINTERS_DS", > decode_3dstate_sampler_state_pointers, AUB_DECODE_STAGE_DS, }, > + { "3DSTATE_SAMPLER_STATE_POINTERS_HS", > decode_3dstate_sampler_state_pointers, AUB_DECODE_STAGE_HS, }, > + { "3DSTATE_SAMPLER_STATE_POINTERS_PS", > decode_3dstate_sampler_state_pointers, AUB_DECODE_STAGE_PS, }, > { "3DSTATE_SAMPLER_STATE_POINTERS", > decode_3dstate_sampler_state_pointers_gen6 }, > > { "3DSTATE_VIEWPORT_STATE_POINTERS_CC", > decode_3dstate_viewport_state_pointers_cc }, > @@ -734,11 +821,26 @@ struct custom_decoder { > { "3DSTATE_BLEND_STATE_POINTERS", decode_3dstate_blend_state_pointers }, > { "3DSTATE_CC_STATE_POINTERS", decode_3dstate_cc_state_pointers }, > { "3DSTATE_SCISSOR_STATE_POINTERS", decode_3dstate_scissor_state_pointers > }, > - { "MI_LOAD_REGISTER_IMM", decode_load_register_imm } > + { "MI_LOAD_REGISTER_IMM", decode_load_register_imm }, > + { "3DPRIMITIVE", decode_3dprimitive }, > }; > > struct custom_decoder info_decoders[] = { > { "STATE_BASE_ADDRESS", handle_state_base_address }, > + { "3DSTATE_URB_VS", handle_urb, AUB_DECODE_STAGE_VS, }, > + { "3DSTATE_URB_GS", handle_urb, AUB_DECODE_STAGE_GS, }, > + { "3DSTATE_URB_DS", handle_urb, AUB_DECODE_STAGE_DS, }, > + { "3DSTATE_URB_HS", handle_urb, AUB_DECODE_STAGE_HS, }, > + { "3DSTATE_VS", handle_urb_read, AUB_DECODE_STAGE_VS, }, > + { "3DSTATE_GS", handle_urb_read, AUB_DECODE_STAGE_GS, }, > + { "3DSTATE_DS", handle_urb_read, AUB_DECODE_STAGE_DS, }, > + { "3DSTATE_HS", handle_urb_read, AUB_DECODE_STAGE_HS, }, > + { "3DSTATE_PS", handle_urb_read, AUB_DECODE_STAGE_PS, }, > + { "3DSTATE_CONSTANT_VS", handle_urb_constant, AUB_DECODE_STAGE_VS, }, > + { "3DSTATE_CONSTANT_GS", handle_urb_constant, AUB_DECODE_STAGE_GS, }, > + { "3DSTATE_CONSTANT_DS", handle_urb_constant, AUB_DECODE_STAGE_DS, }, > + { "3DSTATE_CONSTANT_HS", handle_urb_constant, AUB_DECODE_STAGE_HS, }, > + { "3DSTATE_CONSTANT_PS", handle_urb_constant, AUB_DECODE_STAGE_PS, }, > }; > > static inline uint64_t > @@ -790,6 +892,7 @@ aub_viewer_render_batch(struct aub_viewer_decode_ctx *ctx, > > for (unsigned i = 0; i < ARRAY_SIZE(info_decoders); i++) { > if (strcmp(inst_name, info_decoders[i].cmd_name) == 0) { > + ctx->stage = info_decoders[i].stage; > info_decoders[i].decode(ctx, inst, p); > break; Looks like you run the info_decoders before the display decoders (even though some of them decode the same type of instructions) because you want to first do a pass storing all the information required for the display_decoders, right? And in this case, you also want to store the stage we are at, so when we run the info_decoders that information is available. Also, is there a chance we use these same info decoders for storing other information than urb related stuff? If so, maybe we should just call them handle_3ds_gs, handle_3ds_constant_vs, etc... Or something along those lines. But of course we could just rename them in the future. > } > @@ -804,6 +907,7 @@ aub_viewer_render_batch(struct aub_viewer_decode_ctx *ctx, > > for (unsigned i = 0; i < ARRAY_SIZE(display_decoders); i++) { > if (strcmp(inst_name, display_decoders[i].cmd_name) == 0) { > + ctx->stage = display_decoders[i].stage; > display_decoders[i].decode(ctx, inst, p); > break; > } > -- > 2.18.0 > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev