[Mesa-dev] [PATCH v2 00/13] Engine parameter for instructions

Thu Nov 1 10:52:05 UTC 2018

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.lonnberg at 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.

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.

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.

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.

> 
> --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.lonnberg at 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 at lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > >
> >