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

[Jason Ekstrand]

On Thu, Jun 23, 2016 at 12:16 PM, Topi Pohjolainen <
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 <kenneth at whitecape.org>
> CC: Jason Ekstrand <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 <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 <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 <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 <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.


>   i965/blorp/gen7+: Stop trashing push constant allocation
>   i965/blorp/gen7+: Do not trigger push constant space reconfig
>

Yes please!

Reviewed-by: Jason Ekstrand <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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160701/8017899b/attachment.html>