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

[Pohjolainen, Topi]

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

> 
>        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