[Mesa-dev] [PATCH v2 00/13] Engine parameter for instructions
Jason Ekstrand
jason at jlekstrand.net
Fri Nov 2 14:47:11 UTC 2018
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.
--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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181102/6b6f5782/attachment-0001.html>
More information about the mesa-dev
mailing list