On Jul 4, 2016 1:05 AM, "Pohjolainen, Topi" <<a href="mailto:topi.pohjolainen@intel.com">topi.pohjolainen@intel.com</a>> 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]<a href="mailto:topi.pohjolainen@intel.com">topi.pohjolainen@intel.com</a>> 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]<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> > > > CC: Jason Ekstrand <[3]<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> > > > 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]<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> > > > > > > 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]<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> > > > > > > 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]<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> > > > > > > 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]<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> > > > > > > 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 <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> > > > > > > > > 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]<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> > > > > > > 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:<a href="mailto:topi.pohjolainen@intel.com">topi.pohjolainen@intel.com</a> > > > 2. mailto:<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a> > > > 3. mailto:<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a> > > > 4. mailto:<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a> > > > 5. mailto:<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a> > > > 6. mailto:<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a> > > > 7. mailto:<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a> > > > 8. mailto:<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a> > > _______________________________________________ > > mesa-dev mailing list > ><a href="mailto:mesa-dev@lists.freedesktop.org"> mesa-dev@lists.freedesktop.org</a> > ><a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev"> https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a>