On Fri, Nov 2, 2018 at 6:05 AM Toni Lönnberg <toni.lonnb...@intel.com> wrote:
> On Fri, Nov 02, 2018 at 12:09:54AM -0500, Jason Ekstrand wrote: > > On Thu, Nov 1, 2018 at 5:51 AM Toni Lönnberg <toni.lonnb...@intel.com> > > wrote: > > > > > On Wed, Oct 31, 2018 at 01:18:11PM -0500, Jason Ekstrand wrote: > > > > On Wed, Oct 31, 2018 at 11:10 AM Toni Lönnberg < > toni.lonnb...@intel.com> > > > > wrote: > > > > > > > > > When we debug media or 3d+media workloads, we'd like to be able to > see > > > > > what is > > > > > in the aub dumps for those workloads. At the moment the decoder > can't > > > > > distinguish instructions which share the same opcode between the > render > > > > > and > > > > > video pipe, and thus aubinator outputs garbage on media > instructions. > > > > > > > > > > I was reluctant to make these changes into Mesa in the first place > > > since > > > > > the > > > > > work is related to media, not 3d, but as aubinator is now located > here, > > > > > here we > > > > > are. > > > > > > > > > > > > > That's fine. It's fine if these changres live in mesa. > > > > > > > > > > > > > As far as I can see, these are the options: > > > > > > > > > > 1. Put these in Mesa as aubinator now resides here instead of IGT. > > > > > a) Put all instruction definitions in the current genxml files, > be > > > it > > > > > with a > > > > > tag or an attribute, both methods have their advantages and > > > > > disadvantages. > > > > > b) Separate genxml files into common, render and video (and > > > blitter?) > > > > > > > > > > 2. Fork aubinator tools and related genxml infra to a completely > > > separate > > > > > project. > > > > > > > > > > If I have missed an option, feel free to suggest. > > > > > > > > > > > > > I wasn't suggesting we fork the tools and the XML. I was just > wondering > > > > whether we wanted to do separate sections or an attribute. I think > it > > > > should land in mesa either way. > > > > > > I'm not really a fan of any of the methods to be honest as each of them > > > have > > > disadvantages. > > > > > > Using the attribute makes it easier to add instructions which should be > > > handled > > > by a set of engines and not others but requires each instruction to > have > > > one, > > > except the common ones that are always shared which can be made to > default > > > to > > > all engines like in my version. > > > > > > The tag makes it easier to group things nicely but adds a new level to > the > > > xml > > > and requires duplicate definitions of the instruction if more than one > > > engine > > > uses that instruction but some engine doesn't. > > > > > > > Yeah, I think I'm convinced. > > > > > > > Splitting the genxml files into different ones for each engine also > > > organizes > > > the instructions nicely but has the same problem as the tag and > requires > > > loading > > > and parsing of multiple xml files. > > > > > > > I also experimented with splitting up the XML files. There are times > when > > this is convenient but I'm not sure it's actually all that useful. If we > > were to split it, we'd probably want to split it across other logical > lines > > like "MI" commands vs. "3DSTATE" commands and maybe a separate file for > > registers. At least for now, we may as well keep it all together. > > Splitting the files seems convenient but coherence is important because we > don't > want to end up with hundreds of xml files just for the sake of splitting > stuff. > So I'd keep them as they are unless at some point we find a convincing > reason to > split them apart. > > > > To make things more icky, there are the instructions, like MI_FLUSH_DW, > > > which > > > are shared between engines but might use some of the bits for different > > > things. > > > > > > > Yeah. When you mentioned this, I briefly considered the idea of putting > > engine tags on individual fields in that case. That may be getting a bit > > nuts; I certainly don't want to recreate the insanity that is the bxml. > > I haven't checked how many instructions there are that are shared but have > some > bits enabled for some engines and not for others, can't be that many. If > there > were a lot of those, it might make more sense adding an engine-attribute > to > fields but as of now, that's a bit overkill. And I'm with you here about > bspec > :) > > > > In the end, I just went with how Lionel saw it because it was as good > of > > > an > > > option as any, and pretty straight forward to implement, so.. mehhh. I > > > don't > > > know if you'd prefer one way or the other. > > > > > > > Works for me. I just wanted to make sure we'd thought it through before > > adding it. I know I certainly hadn't given it all that much thought in > my > > brief experiments. I think the attributes is the best solution for now. > > So we're good to go with the reviews? > Yes, if I understand the question correctly. That doesn't mean I have reviewed it; I haven't really looked at the patches in any detail whatsoever. It does mean that I'm happy with it in principal and all that's really left is someone to thoroughly double-check everything and make sure the right instructions got the right tags. --Jason > > --Jason > > > > > > > > > > > > > > --Jason > > > > > > > > > > > > > On Wed, Oct 31, 2018 at 09:20:39AM -0500, Jason Ekstrand wrote: > > > > > > Toni, > > > > > > > > > > > > I'm a bit curious where you're going with this. I started on a > > > similar > > > > > > project a couple of years ago: > > > > > > > > > > > > > > > > https://gitlab.freedesktop.org/jekstrand/mesa/commits/wip/genxml-engines > > > > > > > > > > > > Mine took a different (not necessarily better) approach of > > > surrounding > > > > > the > > > > > > instructions in an <engine> tag. I'm not sure if that's any > better > > > or > > > > > > worse than an attribute. > > > > > > > > > > > > At the time, I was planning to port over the blitter code to > genxml > > > and > > > > > get > > > > > > aubinator decoding blit streams. I canned the project because > there > > > are > > > > > > few enough differences in hardware generations for the blitter > to be > > > > > worth > > > > > > the re-compilation and I had better things to do. I've always > > > thought it > > > > > > would be good to support other engines for no other reason than > to > > > make > > > > > > aubinator for blits. It would also likely be useful to have if > we > > > wanted > > > > > > to start doing media in mesa for some reason. What's your > > > motivation? I > > > > > > ask because I can't really have an opinion on the approach > unless I > > > know > > > > > > where it's headed. > > > > > > > > > > > > --Jason > > > > > > > > > > > > On Wed, Oct 31, 2018 at 8:12 AM Toni Lönnberg < > > > toni.lonnb...@intel.com> > > > > > > wrote: > > > > > > > > > > > > > These patches add an engine parameter to the instructions > defined > > > in > > > > > the > > > > > > > genxml > > > > > > > files so that they can be distinguished when sending them to > > > different > > > > > > > engines. > > > > > > > By default, an instruction is defined to be used by all engines > > > and is > > > > > > > defined > > > > > > > for a specific engine by adding the parameter "engine" to the > > > > > definition. > > > > > > > Currently the supported engines are "render", "video" and > > > "blitter". > > > > > > > > > > > > > > v2: > > > > > > > > > > > > > > * gen_engine enum removed and replaced with use of > > > > > > > drm_i915_gem_engine_class > > > > > > > > > > > > > > * The current engine being used is now saved in the decoder > context > > > > > and is > > > > > > > not > > > > > > > being passed through gen_print_batch(). > > > > > > > > > > > > > > * Split the genxml changes into multiple patches > > > > > > > > > > > > > > Toni Lönnberg (13): > > > > > > > intel/decoder: tools: gen_engine to drm_i915_gem_engine_class > > > > > > > intel/decoder: Engine parameter for instructions > > > > > > > intel/decoder: tools: Use engine for decoding batch > instructions > > > > > > > intel/genxml: Add engine definition to render engine > instructions > > > > > > > (gen4) > > > > > > > intel/genxml: Add engine definition to render engine > instructions > > > > > > > (gen45) > > > > > > > intel/genxml: Add engine definition to render engine > instructions > > > > > > > (gen5) > > > > > > > intel/genxml: Add engine definition to render engine > instructions > > > > > > > (gen6) > > > > > > > intel/genxml: Add engine definition to render engine > instructions > > > > > > > (gen7) > > > > > > > intel/genxml: Add engine definition to render engine > instructions > > > > > > > (gen75) > > > > > > > intel/genxml: Add engine definition to render engine > instructions > > > > > > > (gen8) > > > > > > > intel/genxml: Add engine definition to render engine > instructions > > > > > > > (gen9) > > > > > > > intel/genxml: Add engine definition to render engine > instructions > > > > > > > (gen10) > > > > > > > intel/genxml: Add engine definition to render engine > instructions > > > > > > > (gen11) > > > > > > > > > > > > > > src/intel/common/gen_batch_decoder.c | 25 ++- > > > > > > > src/intel/common/gen_decoder.c | 18 +- > > > > > > > src/intel/common/gen_decoder.h | 11 +- > > > > > > > src/intel/genxml/gen10.xml | 206 > > > +++++++++++----------- > > > > > > > src/intel/genxml/gen11.xml | 208 > > > +++++++++++------------ > > > > > > > src/intel/genxml/gen4.xml | 36 ++-- > > > > > > > src/intel/genxml/gen45.xml | 38 ++--- > > > > > > > src/intel/genxml/gen5.xml | 44 ++--- > > > > > > > src/intel/genxml/gen6.xml | 94 +++++----- > > > > > > > src/intel/genxml/gen7.xml | 154 > ++++++++--------- > > > > > > > src/intel/genxml/gen75.xml | 184 > > > ++++++++++---------- > > > > > > > src/intel/genxml/gen8.xml | 202 > > > +++++++++++----------- > > > > > > > src/intel/genxml/gen9.xml | 208 > > > +++++++++++------------ > > > > > > > src/intel/tools/aub_read.c | 22 +-- > > > > > > > src/intel/tools/aub_read.h | 11 +- > > > > > > > src/intel/tools/aubinator.c | 8 +- > > > > > > > src/intel/tools/aubinator_error_decode.c | 16 ++ > > > > > > > 17 files changed, 763 insertions(+), 722 deletions(-) > > > > > > > > > > > > > > -- > > > > > > > 2.17.1 > > > > > > > > > > > > > > _______________________________________________ > > > > > > > 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 > >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev