Re: [Mesa-dev] i965/blorp: Use flat vertex inputs instead of uniforms
On Jul 4, 2016 1:05 AM, "Pohjolainen, Topi" 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.pohjolai...@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]kenn...@whitecape.org> > > > CC: Jason Ekstrand <[3]ja...@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]ja...@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]ja...@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]ja...@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]ja...@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. Fortunate
Re: [Mesa-dev] i965/blorp: Use flat vertex inputs instead of uniforms
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.pohjolai...@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]kenn...@whitecape.org> > > CC: Jason Ekstrand <[3]ja...@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]ja...@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]ja...@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]ja...@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]ja...@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. > > > >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. > > > > >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]ja...@jlekstrand.net> > > > > src/mesa/drivers/dri/i965/brw_blorp.c | 18 +- > > src/mesa/drivers/dri/i965/brw_blorp.h
Re: [Mesa-dev] i965/blorp: Use flat vertex inputs instead of uniforms
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.pohjolai...@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]kenn...@whitecape.org> > CC: Jason Ekstrand <[3]ja...@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]ja...@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]ja...@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]ja...@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]ja...@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]ja...@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 ++--- >
Re: [Mesa-dev] i965/blorp: Use flat vertex inputs instead of uniforms
On Thu, Jun 23, 2016 at 12:16 PM, Topi Pohjolainen < topi.pohjolai...@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 > CC: Jason Ekstrand > > Topi Pohjolainen (18): > i965/draw: Expose vertex buffer state setup > i965: Unify vertex buffer setup > I like these two cleanups Reviewed-by: Jason Ekstrand > 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 > 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 > 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 > 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 > > 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 > > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] i965/blorp: Use flat vertex inputs instead of uniforms
I keep saying I'm going to review this "tomorrow" but I really mean it this time! I would have today but I spent the whole day arguing the finer points of surface layout, HiZ, fast clears, and color compression with Chad. It was a fun day! :-) --Jason On Jun 23, 2016 12:17 PM, "Topi Pohjolainen" 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 > CC: Jason Ekstrand > > Topi Pohjolainen (18): > i965/draw: Expose vertex buffer state setup > i965: Unify vertex buffer setup > i965/blorp: Split vertex data and element setup > i965/blorp: Use core vertex buffer state setup > i965/blorp: Rename push constants to inputs > i965/blorp: Share input slot between pixel kill and blend/scaled > i965/blorp: Load tranformation coordinates as vec4 > i965/blorp: Drop LOAD_UNIFORM macro > 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 > i965/urb: Allow blorp to record current settings > i965/blorp: Fix the size requirement for vertex elements > i965/blorp/gen7+: Stop trashing push constant allocation > i965/blorp/gen7+: Do not trigger push constant space reconfig > > 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 > > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] i965/blorp: Use flat vertex inputs instead of uniforms
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 CC: Jason Ekstrand Topi Pohjolainen (18): i965/draw: Expose vertex buffer state setup i965: Unify vertex buffer setup i965/blorp: Split vertex data and element setup i965/blorp: Use core vertex buffer state setup i965/blorp: Rename push constants to inputs i965/blorp: Share input slot between pixel kill and blend/scaled i965/blorp: Load tranformation coordinates as vec4 i965/blorp: Drop LOAD_UNIFORM macro 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 i965/urb: Allow blorp to record current settings i965/blorp: Fix the size requirement for vertex elements i965/blorp/gen7+: Stop trashing push constant allocation i965/blorp/gen7+: Do not trigger push constant space reconfig 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 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
