[Mesa-dev] [PATCH v2 00/13] Engine parameter for instructions
Toni Lönnberg
toni.lonnberg at intel.com
Fri Nov 2 15:02:12 UTC 2018
On Fri, Nov 02, 2018 at 09:47:11AM -0500, Jason Ekstrand wrote:
> On Fri, Nov 2, 2018 at 6:05 AM Toni Lönnberg <toni.lonnberg at 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.lonnberg at 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.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.
> > > >
> > >
> > > 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.
That's what I meant :) That we can continue with the reviews regarding
correctness, rather than talking about the style of implementation.
> --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.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
> > > > > > > >
> > > > > >
> > > >
> >
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
> >
More information about the mesa-dev
mailing list