[Mesa-dev] i965/blorp: Use flat vertex inputs instead of uniforms

Mon Jul 4 15:29:27 UTC 2016

On Jul 4, 2016 1:05 AM, "Pohjolainen, Topi" <topi.pohjolainen at intel.com>
wrote:
>
> On Mon, Jul 04, 2016 at 10:10:03AM +0300, Pohjolainen, Topi wrote:
> > On Fri, Jul 01, 2016 at 01:34:41PM -0700, Jason Ekstrand wrote:
> > >    On Thu, Jun 23, 2016 at 12:16 PM, Topi Pohjolainen
> > >    <[1]topi.pohjolainen at intel.com> wrote:
> > >
> > >      In addition to the actual vertex coordinates blorp will get
another
> > >      vertex input buffer providing the constants that are until now
> > >      provided as uniforms. This will remove the need to configure push
> > >      constants and their allocation in the pipeline.
> > >      First three patches refactor the vertex data setup for blorp. The
> > >      existing logic in blorp already supports all gens (gen6-gen9). I
> > >      chose to change the core upload logic accordingly and simply use
> > >      that
> > >      for blorp.
> > >      Patches 5-8 pack the constants in blorp programs into vec4s. By
> > >      default compiler puts input variables two full hardware registers
> > >      apart. Having them in vec4s drops the need to repack them.
> > >      Last four patches take actual advantage of the change by dropping
> > >      unnecessary pipeline reconfiguration.
> > >      CC: Kenneth Graunke <[2]kenneth at whitecape.org>
> > >      CC: Jason Ekstrand <[3]jason at jlekstrand.net>
> > >      Topi Pohjolainen (18):
> > >        i965/draw: Expose vertex buffer state setup
> > >        i965: Unify vertex buffer setup
> > >
> > >    I like these two cleanups
> > >    Reviewed-by: Jason Ekstrand <[4]jason at jlekstrand.net>
> > >
> > >        i965/blorp: Split vertex data and element setup
> > >        i965/blorp: Use core vertex buffer state setup
> > >        i965/blorp: Rename push constants to inputs
> > >
> > >    Reviewed-by: Jason Ekstrand <[5]jason at jlekstrand.net>
> > >
> > >        i965/blorp: Share input slot between pixel kill and
blend/scaled
> > >
> > >    I'm not sure this patch is needed and it does seem to complicate
> > >    things.  I'm not 100% set on that; feel free to argue for it. :-)
See
> > >    the inline comments for details.
> > >
> > >        i965/blorp: Load tranformation coordinates as vec4
> > >
> > >    Reviewed-by: Jason Ekstrand <[6]jason at jlekstrand.net>
> > >
> > >        i965/blorp: Drop LOAD_UNIFORM macro
> > >
> > >    I'm not sure if we want to drop the macro.  As I mentioned inline
on
> > >    the next patch, using offsetof is kind-of nice which is why I made
it
> > >    in the first place.  I'm not that set on offsetof but I think it
merits
> > >    a little discusison.
> > >
> > >        i965/blorp: Store input read mask
> > >        i965/blorp: Add support for flat input buffer
> > >        i965/blorp: Tell vertex fetcher about flat inputs
> > >        i965/blorp: Prepare for more than two vertex attributes
> > >        i965/blorp: Use flat inputs instead of uniforms
> > >        i965/blorp: Remove support for push constants
> > >
> > >    Thanks for working on this!  The above 6 are
> > >    Reviewed-by: Jason Ekstrand <[7]jason at jlekstrand.net>
> > >
> > >        i965/urb: Allow blorp to record current settings
> > >        i965/blorp: Fix the size requirement for vertex elements
>
> And just realized substantial mistake in my rebasing and re-organisizing.
> This patch is needed on its own before flat inputs can be even enabled.
> Otherwise the vertex element size is configured too small for blits
requiring
> more than one vec4 worth of constants.

Right.  I wondered about that...

> > >
> > >    I'm not sure about these two...  They add a lot of coupling between
> > >    blorp and the driver that's going to make it hard to share things
> > >    between GL and Vulkan.  What benchmark did they help and by how
much?
> > >    Also, are they required for the next two?  If not, let's put them
last
> > >    in the series so that we can get the rest of it pushed as soon as
the
> > >    comments on patch 6 are sorted.
> >
> > Push constant space configuration is about how the space reserved for
push
> > constant in the urb allocation is divided between the stages. In the urb
> > configuration we always reserve the same (hw gen based) fixed amount of
space
> > for push constants regardless which one, blorp or the core i965 pipeline
> > upload does the configuration.
> > Therefore we can safely skip the push constant space re-config even if
blorp
> > continues to trash the urb allocation. Both urb allocators (blorp and
core)
> > reserve the same total amount of push constant space.
> >
> > I haven't measured separately what is the performance impact. I
understood
> > from Ken that urb re-configuration is rather heavy though.
>
> And a word about the direct coupling between blorp and gl-specific upload
-
> gen7_blorp.c::gen7_blorp_emit_urb_config() already cobbles
> brw->ctx.NewDriverState meaning we need some sort customization between
vulkan
> and gl driver.

Ugh...

I just went and looked at it again.  It shouldn't be too hard to factor out
later.  Fortunately, the only data passed between blorp and the driver is
the VS URB size.  Go ahead.

Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>

> >
> > >
> > >        i965/blorp/gen7+: Stop trashing push constant allocation
> > >        i965/blorp/gen7+: Do not trigger push constant space reconfig
> > >
> > >    Yes please!
> > >    Reviewed-by: Jason Ekstrand <[8]jason at jlekstrand.net>
> > >
> > >       src/mesa/drivers/dri/i965/brw_blorp.c         |  18 +-
> > >       src/mesa/drivers/dri/i965/brw_blorp.h         |  81 ++++++---
> > >       src/mesa/drivers/dri/i965/brw_blorp_blit.cpp  | 135
+++++++++------
> > >       src/mesa/drivers/dri/i965/brw_blorp_clear.cpp |  15 +-
> > >       src/mesa/drivers/dri/i965/brw_context.h       |  12 +-
> > >       src/mesa/drivers/dri/i965/brw_draw.h          |  13 ++
> > >       src/mesa/drivers/dri/i965/brw_draw_upload.c   |  74 +++++---
> > >       src/mesa/drivers/dri/i965/gen6_blorp.c        | 240
> > >      +++++++++++++-------------
> > >       src/mesa/drivers/dri/i965/gen7_blorp.c        | 153
> > >      ++++------------
> > >       src/mesa/drivers/dri/i965/gen7_urb.c          |  93 +++++-----
> > >       src/mesa/drivers/dri/i965/gen8_blorp.c        |  86 ++-------
> > >       src/mesa/drivers/dri/i965/gen8_draw_upload.c  |  41 ++---
> > >       12 files changed, 448 insertions(+), 513 deletions(-)
> > >      --
> > >      2.5.5
> > >
> > > References
> > >
> > >    1. mailto:topi.pohjolainen at intel.com
> > >    2. mailto:kenneth at whitecape.org
> > >    3. mailto:jason at jlekstrand.net
> > >    4. mailto:jason at jlekstrand.net
> > >    5. mailto:jason at jlekstrand.net
> > >    6. mailto:jason at jlekstrand.net
> > >    7. mailto:jason at jlekstrand.net
> > >    8. mailto:jason at jlekstrand.net
> > _______________________________________________
> > 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/20160704/18b92838/attachment.html>