Re: [Mesa-dev] [PATCH 7/7] virgl: use hw-atomics instead of in-ssbo ones
Hi, One more small think here: > +int virgl_encode_set_hw_atomic_buffers(struct virgl_context *ctx, > + unsigned start_slot, unsigned count, > + const struct pipe_shader_buffer *buffers) > +{ > + int i; I believe that it will be better to use 'unsigned' type for 'i' here because the 'count' variable has 'unsigned' type. Please lat me know if I am incorrect. Regars, Andrii. On Fri, Aug 31, 2018 at 8:35 PM Gurchetan Singh wrote: > On Thu, Aug 30, 2018 at 6:41 AM Erik Faye-Lund > wrote: > > > > From: Tomeu Vizoso > > > > Emulating atomics on top of ssbos can lead to too small max SSBO count, > > so let's use the hw-atomics mechanism to expose atomic buffers instead. > > > > Signed-off-by: Erik Faye-Lund > > --- > > src/gallium/drivers/virgl/virgl_context.c | 37 ++ > > src/gallium/drivers/virgl/virgl_context.h | 2 ++ > > src/gallium/drivers/virgl/virgl_encode.c | 23 ++ > > src/gallium/drivers/virgl/virgl_encode.h | 3 ++ > > src/gallium/drivers/virgl/virgl_hw.h | 5 +++ > > src/gallium/drivers/virgl/virgl_protocol.h | 9 ++ > > src/gallium/drivers/virgl/virgl_screen.c | 14 +--- > > 7 files changed, 88 insertions(+), 5 deletions(-) > > > > diff --git a/src/gallium/drivers/virgl/virgl_context.c > b/src/gallium/drivers/virgl/virgl_context.c > > index edc03f5dcf..30cd0e4331 100644 > > --- a/src/gallium/drivers/virgl/virgl_context.c > > +++ b/src/gallium/drivers/virgl/virgl_context.c > > @@ -196,6 +196,19 @@ static void virgl_attach_res_shader_images(struct > virgl_context *vctx, > > } > > } > > > > +static void virgl_attach_res_atomic_buffers(struct virgl_context *vctx) > > +{ > > + struct virgl_winsys *vws = virgl_screen(vctx->base.screen)->vws; > > + struct virgl_resource *res; > > + unsigned i; > > + for (i = 0; i < PIPE_MAX_SHADER_BUFFERS; i++) { > > Why not PIPE_MAX_HW_ATOMIC_BUFFERS? > > > + res = virgl_resource(vctx->atomic_buffers[i]); > > + if (res) { > > + vws->emit_res(vws, vctx->cbuf, res->hw_res, FALSE); > > + } > > + } > > +} > > + > > /* > > * after flushing, the hw context still has a bunch of > > * resources bound, so we need to rebind those here. > > @@ -214,6 +227,7 @@ static void virgl_reemit_res(struct virgl_context > *vctx) > >virgl_attach_res_shader_buffers(vctx, shader_type); > >virgl_attach_res_shader_images(vctx, shader_type); > > } > > + virgl_attach_res_atomic_buffers(vctx); > > virgl_attach_res_vertex_buffers(vctx); > > virgl_attach_res_so_targets(vctx); > > } > > @@ -952,6 +966,28 @@ static void virgl_blit(struct pipe_context *ctx, > > blit); > > } > > > > +static void virgl_set_hw_atomic_buffers(struct pipe_context *ctx, > > + unsigned start_slot, > > + unsigned count, > > + const struct > pipe_shader_buffer *buffers) > > nit: mixing tabs and spaces > > > +{ > > + struct virgl_context *vctx = virgl_context(ctx); > > + > > + for (unsigned i = 0; i < count; i++) { > > + unsigned idx = start_slot + i; > > + > > + if (buffers) { > > + if (buffers[i].buffer) { > > +pipe_resource_reference(&vctx->atomic_buffers[idx], > > +buffers[i].buffer); > > +continue; > > + } > > + } > > + pipe_resource_reference(&vctx->atomic_buffers[idx], NULL); > > + } > > + virgl_encode_set_hw_atomic_buffers(vctx, start_slot, count, buffers); > > +} > > + > > static void virgl_set_shader_buffers(sunsignedtruct pipe_context *ctx, > > enum pipe_shader_type shader, > > unsigned start_slot, unsigned > count, > > @@ -1209,6 +1245,7 @@ struct pipe_context *virgl_context_create(struct > pipe_screen *pscreen, > > vctx->base.blit = virgl_blit; > > > > vctx->base.set_shader_buffers = virgl_set_shader_buffers; > > + vctx->base.set_hw_atomic_buffers = virgl_set_hw_atomic_buffers; > > vctx->base.set_shader_images = virgl_set_shader_images; > > vctx->base.memory_barrier = virgl_memory_barrier; > > > > diff --git a/src/gallium/drivers/virgl/virgl_context.h > b/src/gallium/drivers/virgl/virgl_context.h > > index 38d1f450e1..20988baa3c 100644 > > --- a/src/gallium/drivers/virgl/virgl_context.h > > +++ b/src/gallium/drivers/virgl/virgl_context.h > > @@ -75,6 +75,8 @@ struct virgl_context { > > int num_draws; > > struct list_head to_flush_bufs; > > > > + struct pipe_resource *atomic_buffers[PIPE_MAX_HW_ATOMIC_BUFFERS]; > > + > > struct primconvert_context *primconvert; > > uint32_t hw_sub_ctx_id; > > }; > > diff --git a/src/gallium/drivers/virgl/virgl_encode.c > b/src/gallium/drivers/virgl/virgl_encode.c > > index bcb14d8939..c9cc099061 100644 > > --- a
Re: [Mesa-dev] [PATCH 7/7] virgl: use hw-atomics instead of in-ssbo ones
On fr., aug. 31, 2018 at 7:35 PM, Gurchetan Singh wrote: On Thu, Aug 30, 2018 at 6:41 AM Erik Faye-Lund wrote: From: Tomeu Vizoso Emulating atomics on top of ssbos can lead to too small max SSBO count, so let's use the hw-atomics mechanism to expose atomic buffers instead. Signed-off-by: Erik Faye-Lund --- src/gallium/drivers/virgl/virgl_context.c | 37 ++ src/gallium/drivers/virgl/virgl_context.h | 2 ++ src/gallium/drivers/virgl/virgl_encode.c | 23 ++ src/gallium/drivers/virgl/virgl_encode.h | 3 ++ src/gallium/drivers/virgl/virgl_hw.h | 5 +++ src/gallium/drivers/virgl/virgl_protocol.h | 9 ++ src/gallium/drivers/virgl/virgl_screen.c | 14 +--- 7 files changed, 88 insertions(+), 5 deletions(-) diff --git a/src/gallium/drivers/virgl/virgl_context.c b/src/gallium/drivers/virgl/virgl_context.c index edc03f5dcf..30cd0e4331 100644 --- a/src/gallium/drivers/virgl/virgl_context.c +++ b/src/gallium/drivers/virgl/virgl_context.c @@ -196,6 +196,19 @@ static void virgl_attach_res_shader_images(struct virgl_context *vctx, } } +static void virgl_attach_res_atomic_buffers(struct virgl_context *vctx) +{ + struct virgl_winsys *vws = virgl_screen(vctx->base.screen)->vws; + struct virgl_resource *res; + unsigned i; + for (i = 0; i < PIPE_MAX_SHADER_BUFFERS; i++) { Why not PIPE_MAX_HW_ATOMIC_BUFFERS? Yeah, that seems better. Fixed locally. + res = virgl_resource(vctx->atomic_buffers[i]); + if (res) { + vws->emit_res(vws, vctx->cbuf, res->hw_res, FALSE); + } + } +} + /* * after flushing, the hw context still has a bunch of * resources bound, so we need to rebind those here. @@ -214,6 +227,7 @@ static void virgl_reemit_res(struct virgl_context *vctx) virgl_attach_res_shader_buffers(vctx, shader_type); virgl_attach_res_shader_images(vctx, shader_type); } + virgl_attach_res_atomic_buffers(vctx); virgl_attach_res_vertex_buffers(vctx); virgl_attach_res_so_targets(vctx); } @@ -952,6 +966,28 @@ static void virgl_blit(struct pipe_context *ctx, blit); } +static void virgl_set_hw_atomic_buffers(struct pipe_context *ctx, + unsigned start_slot, + unsigned count, + const struct pipe_shader_buffer *buffers) nit: mixing tabs and spaces Thanks, fixed locally. +{ + struct virgl_context *vctx = virgl_context(ctx); + + for (unsigned i = 0; i < count; i++) { + unsigned idx = start_slot + i; + + if (buffers) { + if (buffers[i].buffer) { +pipe_resource_reference(&vctx->atomic_buffers[idx], +buffers[i].buffer); +continue; + } + } + pipe_resource_reference(&vctx->atomic_buffers[idx], NULL); + } + virgl_encode_set_hw_atomic_buffers(vctx, start_slot, count, buffers); +} + static void virgl_set_shader_buffers(struct pipe_context *ctx, enum pipe_shader_type shader, unsigned start_slot, unsigned count, @@ -1209,6 +1245,7 @@ struct pipe_context *virgl_context_create(struct pipe_screen *pscreen, vctx->base.blit = virgl_blit; vctx->base.set_shader_buffers = virgl_set_shader_buffers; + vctx->base.set_hw_atomic_buffers = virgl_set_hw_atomic_buffers; vctx->base.set_shader_images = virgl_set_shader_images; vctx->base.memory_barrier = virgl_memory_barrier; diff --git a/src/gallium/drivers/virgl/virgl_context.h b/src/gallium/drivers/virgl/virgl_context.h index 38d1f450e1..20988baa3c 100644 --- a/src/gallium/drivers/virgl/virgl_context.h +++ b/src/gallium/drivers/virgl/virgl_context.h @@ -75,6 +75,8 @@ struct virgl_context { int num_draws; struct list_head to_flush_bufs; + struct pipe_resource *atomic_buffers[PIPE_MAX_HW_ATOMIC_BUFFERS]; + struct primconvert_context *primconvert; uint32_t hw_sub_ctx_id; }; diff --git a/src/gallium/drivers/virgl/virgl_encode.c b/src/gallium/drivers/virgl/virgl_encode.c index bcb14d8939..c9cc099061 100644 --- a/src/gallium/drivers/virgl/virgl_encode.c +++ b/src/gallium/drivers/virgl/virgl_encode.c @@ -958,6 +958,29 @@ int virgl_encode_set_shader_buffers(struct virgl_context *ctx, return 0; } +int virgl_encode_set_hw_atomic_buffers(struct virgl_context *ctx, + unsigned start_slot, unsigned count, + const struct pipe_shader_buffer *buffers) +{ + int i; + virgl_encoder_write_cmd_dword(ctx, VIRGL_CMD0(VIRGL_CCMD_SET_ATOMIC_BUFFERS, 0, VIRGL_SET_ATOMIC_BUFFER_SIZE(count))); + + virgl_encoder_write_dword(ctx->cbuf, start_slot); + for (i = 0; i < count; i++) { + if (buffers) { +
Re: [Mesa-dev] [PATCH 7/7] virgl: use hw-atomics instead of in-ssbo ones
On ma., sep. 3, 2018 at 11:50 AM, andrey simiklit wrote: Hi, One more small think here: > +int virgl_encode_set_hw_atomic_buffers(struct virgl_context *ctx, > + unsigned start_slot, unsigned count, > + const struct pipe_shader_buffer *buffers) > +{ > + int i; I believe that it will be better to use 'unsigned' type for 'i' here because the 'count' variable has 'unsigned' type. Please lat me know if I am incorrect. While I agree with you in principle, I'd rather keep it this way, and be consistent with what the surrounding code does (like virgl_encode_set_shader_buffers and virgl_encode_set_shader_images). These are already guaranteed never be big enough to require unsigned anyway. Regars, Andrii. On Fri, Aug 31, 2018 at 8:35 PM Gurchetan Singh wrote: On Thu, Aug 30, 2018 at 6:41 AM Erik Faye-Lund wrote: > > From: Tomeu Vizoso > > Emulating atomics on top of ssbos can lead to too small max SSBO count, > so let's use the hw-atomics mechanism to expose atomic buffers instead. > > Signed-off-by: Erik Faye-Lund > --- > src/gallium/drivers/virgl/virgl_context.c | 37 ++ > src/gallium/drivers/virgl/virgl_context.h | 2 ++ > src/gallium/drivers/virgl/virgl_encode.c | 23 ++ > src/gallium/drivers/virgl/virgl_encode.h | 3 ++ > src/gallium/drivers/virgl/virgl_hw.h | 5 +++ > src/gallium/drivers/virgl/virgl_protocol.h | 9 ++ > src/gallium/drivers/virgl/virgl_screen.c | 14 +--- > 7 files changed, 88 insertions(+), 5 deletions(-) > > diff --git a/src/gallium/drivers/virgl/virgl_context.c b/src/gallium/drivers/virgl/virgl_context.c > index edc03f5dcf..30cd0e4331 100644 > --- a/src/gallium/drivers/virgl/virgl_context.c > +++ b/src/gallium/drivers/virgl/virgl_context.c > @@ -196,6 +196,19 @@ static void virgl_attach_res_shader_images(struct virgl_context *vctx, > } > } > > +static void virgl_attach_res_atomic_buffers(struct virgl_context *vctx) > +{ > + struct virgl_winsys *vws = virgl_screen(vctx->base.screen)->vws; > + struct virgl_resource *res; > + unsigned i; > + for (i = 0; i < PIPE_MAX_SHADER_BUFFERS; i++) { Why not PIPE_MAX_HW_ATOMIC_BUFFERS? > + res = virgl_resource(vctx->atomic_buffers[i]); > + if (res) { > + vws->emit_res(vws, vctx->cbuf, res->hw_res, FALSE); > + } > + } > +} > + > /* > * after flushing, the hw context still has a bunch of > * resources bound, so we need to rebind those here. > @@ -214,6 +227,7 @@ static void virgl_reemit_res(struct virgl_context *vctx) >virgl_attach_res_shader_buffers(vctx, shader_type); >virgl_attach_res_shader_images(vctx, shader_type); > } > + virgl_attach_res_atomic_buffers(vctx); > virgl_attach_res_vertex_buffers(vctx); > virgl_attach_res_so_targets(vctx); > } > @@ -952,6 +966,28 @@ static void virgl_blit(struct pipe_context *ctx, > blit); > } > > +static void virgl_set_hw_atomic_buffers(struct pipe_context *ctx, > + unsigned start_slot, > + unsigned count, > + const struct pipe_shader_buffer *buffers) nit: mixing tabs and spaces > +{ > + struct virgl_context *vctx = virgl_context(ctx); > + > + for (unsigned i = 0; i < count; i++) { > + unsigned idx = start_slot + i; > + > + if (buffers) { > + if (buffers[i].buffer) { > +pipe_resource_reference(&vctx->atomic_buffers[idx], > +buffers[i].buffer); > +continue; > + } > + } > + pipe_resource_reference(&vctx->atomic_buffers[idx], NULL); > + } > + virgl_encode_set_hw_atomic_buffers(vctx, start_slot, count, buffers); > +} > + > static void virgl_set_shader_buffers(sunsignedtruct pipe_context *ctx, > enum pipe_shader_type shader, > unsigned start_slot, unsigned count, > @@ -1209,6 +1245,7 @@ struct pipe_context *virgl_context_create(struct pipe_screen *pscreen, > vctx->base.blit = virgl_blit; > > vctx->base.set_shader_buffers = virgl_set_shader_buffers; > + vctx->base.set_hw_atomic_buffers = virgl_set_hw_atomic_buffers; > vctx->base.set_shader_images = virgl_set_shader_images; > vctx->base.memory_barrier = virgl_memory_barrier; > > diff --git a/src/gallium/drivers/virgl/virgl_context.h b/src/gallium/drivers/virgl/virgl_context.h > index 38d1f450e1..20988baa3c 100644 > --- a/src/gallium/drivers/virgl/virgl_context.h > +++ b/src/gallium/drivers/virgl/virgl_context.h > @@ -75,6 +75,8 @@ struct virgl_context { > int num_draws; > struct list_head to_flush_bufs; > > + struct pipe_resource *atomic_buffers[PIPE_MAX_HW_ATOMIC_BUFFERS]; > + > struct primconvert_context *primconvert; > uint32_t hw_sub_ctx_id; > }
Re: [Mesa-dev] [PATCH 7/7] virgl: use hw-atomics instead of in-ssbo ones
On Thu, Aug 30, 2018 at 6:41 AM Erik Faye-Lund wrote: > > From: Tomeu Vizoso > > Emulating atomics on top of ssbos can lead to too small max SSBO count, > so let's use the hw-atomics mechanism to expose atomic buffers instead. > > Signed-off-by: Erik Faye-Lund > --- > src/gallium/drivers/virgl/virgl_context.c | 37 ++ > src/gallium/drivers/virgl/virgl_context.h | 2 ++ > src/gallium/drivers/virgl/virgl_encode.c | 23 ++ > src/gallium/drivers/virgl/virgl_encode.h | 3 ++ > src/gallium/drivers/virgl/virgl_hw.h | 5 +++ > src/gallium/drivers/virgl/virgl_protocol.h | 9 ++ > src/gallium/drivers/virgl/virgl_screen.c | 14 +--- > 7 files changed, 88 insertions(+), 5 deletions(-) > > diff --git a/src/gallium/drivers/virgl/virgl_context.c > b/src/gallium/drivers/virgl/virgl_context.c > index edc03f5dcf..30cd0e4331 100644 > --- a/src/gallium/drivers/virgl/virgl_context.c > +++ b/src/gallium/drivers/virgl/virgl_context.c > @@ -196,6 +196,19 @@ static void virgl_attach_res_shader_images(struct > virgl_context *vctx, > } > } > > +static void virgl_attach_res_atomic_buffers(struct virgl_context *vctx) > +{ > + struct virgl_winsys *vws = virgl_screen(vctx->base.screen)->vws; > + struct virgl_resource *res; > + unsigned i; > + for (i = 0; i < PIPE_MAX_SHADER_BUFFERS; i++) { Why not PIPE_MAX_HW_ATOMIC_BUFFERS? > + res = virgl_resource(vctx->atomic_buffers[i]); > + if (res) { > + vws->emit_res(vws, vctx->cbuf, res->hw_res, FALSE); > + } > + } > +} > + > /* > * after flushing, the hw context still has a bunch of > * resources bound, so we need to rebind those here. > @@ -214,6 +227,7 @@ static void virgl_reemit_res(struct virgl_context *vctx) >virgl_attach_res_shader_buffers(vctx, shader_type); >virgl_attach_res_shader_images(vctx, shader_type); > } > + virgl_attach_res_atomic_buffers(vctx); > virgl_attach_res_vertex_buffers(vctx); > virgl_attach_res_so_targets(vctx); > } > @@ -952,6 +966,28 @@ static void virgl_blit(struct pipe_context *ctx, > blit); > } > > +static void virgl_set_hw_atomic_buffers(struct pipe_context *ctx, > + unsigned start_slot, > + unsigned count, > + const struct pipe_shader_buffer > *buffers) nit: mixing tabs and spaces > +{ > + struct virgl_context *vctx = virgl_context(ctx); > + > + for (unsigned i = 0; i < count; i++) { > + unsigned idx = start_slot + i; > + > + if (buffers) { > + if (buffers[i].buffer) { > +pipe_resource_reference(&vctx->atomic_buffers[idx], > +buffers[i].buffer); > +continue; > + } > + } > + pipe_resource_reference(&vctx->atomic_buffers[idx], NULL); > + } > + virgl_encode_set_hw_atomic_buffers(vctx, start_slot, count, buffers); > +} > + > static void virgl_set_shader_buffers(struct pipe_context *ctx, > enum pipe_shader_type shader, > unsigned start_slot, unsigned count, > @@ -1209,6 +1245,7 @@ struct pipe_context *virgl_context_create(struct > pipe_screen *pscreen, > vctx->base.blit = virgl_blit; > > vctx->base.set_shader_buffers = virgl_set_shader_buffers; > + vctx->base.set_hw_atomic_buffers = virgl_set_hw_atomic_buffers; > vctx->base.set_shader_images = virgl_set_shader_images; > vctx->base.memory_barrier = virgl_memory_barrier; > > diff --git a/src/gallium/drivers/virgl/virgl_context.h > b/src/gallium/drivers/virgl/virgl_context.h > index 38d1f450e1..20988baa3c 100644 > --- a/src/gallium/drivers/virgl/virgl_context.h > +++ b/src/gallium/drivers/virgl/virgl_context.h > @@ -75,6 +75,8 @@ struct virgl_context { > int num_draws; > struct list_head to_flush_bufs; > > + struct pipe_resource *atomic_buffers[PIPE_MAX_HW_ATOMIC_BUFFERS]; > + > struct primconvert_context *primconvert; > uint32_t hw_sub_ctx_id; > }; > diff --git a/src/gallium/drivers/virgl/virgl_encode.c > b/src/gallium/drivers/virgl/virgl_encode.c > index bcb14d8939..c9cc099061 100644 > --- a/src/gallium/drivers/virgl/virgl_encode.c > +++ b/src/gallium/drivers/virgl/virgl_encode.c > @@ -958,6 +958,29 @@ int virgl_encode_set_shader_buffers(struct virgl_context > *ctx, > return 0; > } > > +int virgl_encode_set_hw_atomic_buffers(struct virgl_context *ctx, > + unsigned start_slot, unsigned count, > + const struct pipe_shader_buffer > *buffers) > +{ > + int i; > + virgl_encoder_write_cmd_dword(ctx, > VIRGL_CMD0(VIRGL_CCMD_SET_ATOMIC_BUFFERS, 0, > VIRGL_SET_ATOMIC_BUFFER_SIZE(count))); > + > + virgl_encoder_write_dword(ctx->cbuf, start_slot); > + for (i = 0; i < count; i++) { > + if (buffers) { > +struct virgl_resour