On Tue, May 2, 2017 at 6:03 AM, Iago Toral <ito...@igalia.com> wrote: > On Mon, 2017-05-01 at 13:54 -0700, Matt Turner wrote: >> Which will allow us to print validation errors found in shader >> assembly >> in GPU hang error states. >> --- >> src/intel/tools/disasm.c | 71 +++++++++++++++++++++++++++++--------- >> ---------- >> 1 file changed, 43 insertions(+), 28 deletions(-) >> >> diff --git a/src/intel/tools/disasm.c b/src/intel/tools/disasm.c >> index 62256d2..361885b 100644 >> --- a/src/intel/tools/disasm.c >> +++ b/src/intel/tools/disasm.c >> @@ -43,52 +43,67 @@ is_send(uint32_t opcode) >> opcode == BRW_OPCODE_SENDSC ); >> } >> >> -void >> -gen_disasm_disassemble(struct gen_disasm *disasm, void *assembly, >> - int start, FILE *out) >> +static int >> +gen_disasm_find_end(struct gen_disasm *disasm, void *assembly, int >> start) >> { >> struct gen_device_info *devinfo = &disasm->devinfo; >> - bool dump_hex = false; >> int offset = start; >> >> /* This loop exits when send-with-EOT or when opcode is 0 */ >> while (true) { >> brw_inst *insn = assembly + offset; >> - brw_inst uncompacted; >> - bool compacted = brw_inst_cmpt_control(devinfo, insn); >> - if (0) >> - fprintf(out, "0x%08x: ", offset); >> - >> - if (compacted) { >> - brw_compact_inst *compacted = (void *)insn; >> - if (dump_hex) { >> - fprintf(out, "0x%08x 0x%08x ", >> - ((uint32_t *)insn)[1], >> - ((uint32_t *)insn)[0]); >> - } >> - >> - brw_uncompact_instruction(devinfo, &uncompacted, >> compacted); >> - insn = &uncompacted; >> + >> + if (brw_inst_cmpt_control(devinfo, insn)) { >> offset += 8; >> } else { >> - if (dump_hex) { >> - fprintf(out, "0x%08x 0x%08x 0x%08x 0x%08x ", >> - ((uint32_t *)insn)[3], >> - ((uint32_t *)insn)[2], >> - ((uint32_t *)insn)[1], >> - ((uint32_t *)insn)[0]); >> - } >> offset += 16; >> } >> >> - brw_disassemble_inst(out, devinfo, insn, compacted); >> - >> /* Simplistic, but efficient way to terminate disasm */ >> uint32_t opcode = brw_inst_opcode(devinfo, insn); >> if (opcode == 0 || (is_send(opcode) && brw_inst_eot(devinfo, >> insn))) { >> break; >> } >> } >> + >> + return offset; >> +} >> + >> +void >> +gen_disasm_disassemble(struct gen_disasm *disasm, void *assembly, >> + int start, FILE *out) >> +{ >> + struct gen_device_info *devinfo = &disasm->devinfo; >> + int end = gen_disasm_find_end(disasm, assembly, start); >> + >> + /* Make a dummy annotation structure that >> brw_validate_instructions >> + * can work from. >> + */ >> + struct annotation_info annotation_info = { >> + .ann_count = 1, >> + .ann_size = 2, >> + }; >> + annotation_info.mem_ctx = ralloc_context(NULL); >> + annotation_info.ann = rzalloc_array(annotation_info.mem_ctx, >> + struct annotation, >> + annotation_info.ann_size); >> + annotation_info.ann[0].offset = start; >> + annotation_info.ann[1].offset = end; >> + brw_validate_instructions(devinfo, assembly, start, end, >> &annotation_info); >> + struct annotation *annotation = annotation_info.ann; >> + >> + for (int i = 0; i < annotation_info.ann_count; i++) { >> + int start_offset = annotation[i].offset; >> + int end_offset = annotation[i + 1].offset; > > I was going to say that this code seems to overflow when i > == annotation_info.ann_count - 1, but then I noticed that there are > similar loops in other parts of the code and that you initialized > ann_count to just 1 even though you have two initial annotations, so I > guess this is how this is supposed to work, right?
Yeah, exactly. I'm not sure it's a good way to handle it though. To be honest, I had a hard time remembering how it was supposed to work and I wrote the annotation system. I think a linked list would be a lot more natural than the array/count. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev