[Mesa-dev] [PATCH v2 0/2] *** Aubinator tool for Intel Gen platforms ***

[Gandikota, Sirisha]

Thanks for the review Ken. Will do the needful as suggested.

-Sirisha

-----Original Message-----
From: Kenneth Graunke [mailto:kenneth at whitecape.org] 
Sent: Tuesday, August 23, 2016 9:52 PM
To: mesa-dev at lists.freedesktop.org
Cc: Gandikota, Sirisha <sirisha.gandikota at intel.com>
Subject: Re: [Mesa-dev] [PATCH v2 0/2] *** Aubinator tool for Intel Gen platforms ***

On Friday, August 19, 2016 12:13:23 PM PDT Sirisha Gandikota wrote:
> This is a patch series for adding the aubinator tool to the codebase.

Hi Sirisha,

I've pushed your patches to import the tool.  I also pushed a third commit that fixes some minor style nitpicks - I figured it would be easier to just fix it up rather than to point out every instance of whitespace changes in a big long email.  Here's the patch, if you want to see what I did:

https://cgit.freedesktop.org/mesa/mesa/commit/?id=e7530bfcd6acdc8f8984820445c4b41602952298

While reviewing the code, I did find some things I'd like to see
changed:

1. Fix a compiler warning:

   decoder.c:460:12: warning: assignment discards ‘const’ qualifier from
   pointer target type [-Wdiscarded-qualifiers] 

   You can just mark the gen_field_iterator::p pointer as const.

2. Simplify gen_disasm_create()'s devinfo handling.

   You can just do gd->devinfo = *brw_get_device_info(pciid) to
   copy the whole struct, rather than just a couple fields.

3. Simplify print_dword_val().

   You don't need the float/dword union - iter->p[f->start / 32] 
   is already a uint32_t, which is what you want for the %08x
   printf formatter.  You may as well use it directly.

4. Make gen_disasm_disassemble handle split sends.

   Skylake adds new SENDS and SENDSC opcodes, which you should
   handle in the send-with-EOT check.  Maybe make an is_send()
   helper that checks if the opcode is SEND/SENDC/SENDS/SENDSC?

5. Bogus "end" parameter in gen_disasm_disassemble()?

   The loop pretends to loop over instructions from "start" to "end",
   but the callers always pass 8192 for end, which is some huge bogus
   value.  The real loop termination condition is send-with-EOT or 0.
   Maybe just drop the bogus "end" parameter entirely?

6. Fix decoding of values that span two DWords.

   64-bit fields (such as most pointers on Gen8+) aren't currently
   decoded correctly.  gen_field_iterator_next seems to walk one
   DWord at a time, sets v.dw, and then passes it to field().

   So, even though field() takes a uint64_t, we're passing it a
   uint32_t (which gets promoted, so the top 32 bits will always
   be zero).  This seems pretty bogus...

   If a field's (end - start) > 32, you probably want to read a
   whole uint64_t and pass that to field().  Might need to % 64
   instead of % 32.  I'll let you figure it out :)

Could you send a new patch series to address these?  All but the last should be trivial to fix.  Now that the tool has landed, it'd be nice to see each change as a separate commit - let's not squash things from now on.

Thanks!
--Ken