Re: [Mesa-dev] [PATCH 11/17] anv: Separate compute and graphics descriptor sets
On Mon, Jan 15, 2018 at 10:47:08AM -0800, Jason Ekstrand wrote:
> On Mon, Jan 15, 2018 at 7:11 AM, Pohjolainen, Topi <
> topi.pohjolai...@gmail.com> wrote:
>
> > On Fri, Dec 15, 2017 at 05:09:09PM -0800, Jason Ekstrand wrote:
> > > The Vulkan spec says:
> > >
> > > "pipelineBindPoint is a VkPipelineBindPoint indicating whether the
> > > descriptors will be used by graphics pipelines or compute pipelines.
> > > There is a separate set of bind points for each of graphics and
> > > compute, so binding one does not disturb the other."
> > >
> > > Up until now, we've been ignoring the pipeline bind point and had just
> > > one bind point for everything. This commit separates things out into
> > > separate bind points.
> > >
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102897
> > > ---
> > > src/intel/vulkan/anv_cmd_buffer.c | 65
> > ++-
> > > src/intel/vulkan/anv_descriptor_set.c | 2 ++
> > > src/intel/vulkan/anv_private.h| 11 +++---
> > > src/intel/vulkan/genX_cmd_buffer.c| 24 +++--
> > > 4 files changed, 70 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/src/intel/vulkan/anv_cmd_buffer.c
> > b/src/intel/vulkan/anv_cmd_buffer.c
> > > index 636f515..9720e7e 100644
> > > --- a/src/intel/vulkan/anv_cmd_buffer.c
> > > +++ b/src/intel/vulkan/anv_cmd_buffer.c
> > > @@ -124,12 +124,20 @@ anv_cmd_state_init(struct anv_cmd_buffer
> > *cmd_buffer)
> > > }
> > >
> > > static void
> > > +anv_cmd_pipeline_state_finish(struct anv_cmd_buffer *cmd_buffer,
> > > + struct anv_cmd_pipeline_state *pipe_state)
> > > +{
> > > + for (uint32_t i = 0; i < ARRAY_SIZE(pipe_state->push_descriptors);
> > i++)
> > > + vk_free(_buffer->pool->alloc, pipe_state->push_descriptors[
> > i]);
> > > +}
> > > +
> > > +static void
> > > anv_cmd_state_finish(struct anv_cmd_buffer *cmd_buffer)
> > > {
> > > struct anv_cmd_state *state = _buffer->state;
> > >
> > > - for (uint32_t i = 0; i < ARRAY_SIZE(state->push_descriptors); i++)
> > > - vk_free(_buffer->pool->alloc, state->push_descriptors[i]);
> > > + anv_cmd_pipeline_state_finish(cmd_buffer, >gfx.base);
> > > + anv_cmd_pipeline_state_finish(cmd_buffer, >compute.base);
> > >
> > > for (uint32_t i = 0; i < MESA_SHADER_STAGES; i++)
> > >vk_free(_buffer->pool->alloc, state->push_constants[i]);
> > > @@ -495,6 +503,7 @@ void anv_CmdSetStencilReference(
> > >
> > > static void
> > > anv_cmd_buffer_bind_descriptor_set(struct anv_cmd_buffer *cmd_buffer,
> > > + VkPipelineBindPoint bind_point,
> > > struct anv_pipeline_layout *layout,
> > > uint32_t set_index,
> > > struct anv_descriptor_set *set,
> > > @@ -504,7 +513,14 @@ anv_cmd_buffer_bind_descriptor_set(struct
> > anv_cmd_buffer *cmd_buffer,
> > > struct anv_descriptor_set_layout *set_layout =
> > >layout->set[set_index].layout;
> > >
> > > - cmd_buffer->state.descriptors[set_index] = set;
> > > + struct anv_cmd_pipeline_state *pipe_state;
> > > + if (bind_point == VK_PIPELINE_BIND_POINT_COMPUTE) {
> > > + pipe_state = _buffer->state.compute.base;
> > > + } else {
> > > + assert(bind_point == VK_PIPELINE_BIND_POINT_GRAPHICS);
> > > + pipe_state = _buffer->state.gfx.base;
> > > + }
> > > + pipe_state->descriptors[set_index] = set;
> > >
> > > if (dynamic_offsets) {
> > >if (set_layout->dynamic_offset_count > 0) {
> > > @@ -514,9 +530,9 @@ anv_cmd_buffer_bind_descriptor_set(struct
> > anv_cmd_buffer *cmd_buffer,
> > > /* Assert that everything is in range */
> > > assert(set_layout->dynamic_offset_count <=
> > *dynamic_offset_count);
> > > assert(dynamic_offset_start + set_layout->dynamic_offset_count
> > <=
> > > -ARRAY_SIZE(cmd_buffer->state.dynamic_offsets));
> > > +ARRAY_SIZE(pipe_state->dynamic_offsets));
> > >
> > > - typed_memcpy(_buffer->state.dynamic_offsets[dynamic_
> > offset_start],
> > > + typed_memcpy(_state->dynamic_offsets[dynamic_
> > offset_start],
> > >*dynamic_offsets, set_layout->dynamic_offset_
> > count);
> > >
> > > *dynamic_offsets += set_layout->dynamic_offset_count;
> > > @@ -524,7 +540,13 @@ anv_cmd_buffer_bind_descriptor_set(struct
> > anv_cmd_buffer *cmd_buffer,
> > >}
> > > }
> > >
> > > - cmd_buffer->state.descriptors_dirty |= set_layout->shader_stages;
> > > + if (bind_point == VK_PIPELINE_BIND_POINT_COMPUTE) {
> > > + cmd_buffer->state.descriptors_dirty |=
> > VK_SHADER_STAGE_COMPUTE_BIT;
> > > + } else {
> > > + assert(bind_point == VK_PIPELINE_BIND_POINT_GRAPHICS);
> > > + cmd_buffer->state.descriptors_dirty |=
> > > + set_layout->shader_stages & VK_SHADER_STAGE_ALL_GRAPHICS;
> >
> > Should we put () around
Re: [Mesa-dev] [PATCH 11/17] anv: Separate compute and graphics descriptor sets
On Mon, Jan 15, 2018 at 7:11 AM, Pohjolainen, Topi <
topi.pohjolai...@gmail.com> wrote:
> On Fri, Dec 15, 2017 at 05:09:09PM -0800, Jason Ekstrand wrote:
> > The Vulkan spec says:
> >
> > "pipelineBindPoint is a VkPipelineBindPoint indicating whether the
> > descriptors will be used by graphics pipelines or compute pipelines.
> > There is a separate set of bind points for each of graphics and
> > compute, so binding one does not disturb the other."
> >
> > Up until now, we've been ignoring the pipeline bind point and had just
> > one bind point for everything. This commit separates things out into
> > separate bind points.
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102897
> > ---
> > src/intel/vulkan/anv_cmd_buffer.c | 65
> ++-
> > src/intel/vulkan/anv_descriptor_set.c | 2 ++
> > src/intel/vulkan/anv_private.h| 11 +++---
> > src/intel/vulkan/genX_cmd_buffer.c| 24 +++--
> > 4 files changed, 70 insertions(+), 32 deletions(-)
> >
> > diff --git a/src/intel/vulkan/anv_cmd_buffer.c
> b/src/intel/vulkan/anv_cmd_buffer.c
> > index 636f515..9720e7e 100644
> > --- a/src/intel/vulkan/anv_cmd_buffer.c
> > +++ b/src/intel/vulkan/anv_cmd_buffer.c
> > @@ -124,12 +124,20 @@ anv_cmd_state_init(struct anv_cmd_buffer
> *cmd_buffer)
> > }
> >
> > static void
> > +anv_cmd_pipeline_state_finish(struct anv_cmd_buffer *cmd_buffer,
> > + struct anv_cmd_pipeline_state *pipe_state)
> > +{
> > + for (uint32_t i = 0; i < ARRAY_SIZE(pipe_state->push_descriptors);
> i++)
> > + vk_free(_buffer->pool->alloc, pipe_state->push_descriptors[
> i]);
> > +}
> > +
> > +static void
> > anv_cmd_state_finish(struct anv_cmd_buffer *cmd_buffer)
> > {
> > struct anv_cmd_state *state = _buffer->state;
> >
> > - for (uint32_t i = 0; i < ARRAY_SIZE(state->push_descriptors); i++)
> > - vk_free(_buffer->pool->alloc, state->push_descriptors[i]);
> > + anv_cmd_pipeline_state_finish(cmd_buffer, >gfx.base);
> > + anv_cmd_pipeline_state_finish(cmd_buffer, >compute.base);
> >
> > for (uint32_t i = 0; i < MESA_SHADER_STAGES; i++)
> >vk_free(_buffer->pool->alloc, state->push_constants[i]);
> > @@ -495,6 +503,7 @@ void anv_CmdSetStencilReference(
> >
> > static void
> > anv_cmd_buffer_bind_descriptor_set(struct anv_cmd_buffer *cmd_buffer,
> > + VkPipelineBindPoint bind_point,
> > struct anv_pipeline_layout *layout,
> > uint32_t set_index,
> > struct anv_descriptor_set *set,
> > @@ -504,7 +513,14 @@ anv_cmd_buffer_bind_descriptor_set(struct
> anv_cmd_buffer *cmd_buffer,
> > struct anv_descriptor_set_layout *set_layout =
> >layout->set[set_index].layout;
> >
> > - cmd_buffer->state.descriptors[set_index] = set;
> > + struct anv_cmd_pipeline_state *pipe_state;
> > + if (bind_point == VK_PIPELINE_BIND_POINT_COMPUTE) {
> > + pipe_state = _buffer->state.compute.base;
> > + } else {
> > + assert(bind_point == VK_PIPELINE_BIND_POINT_GRAPHICS);
> > + pipe_state = _buffer->state.gfx.base;
> > + }
> > + pipe_state->descriptors[set_index] = set;
> >
> > if (dynamic_offsets) {
> >if (set_layout->dynamic_offset_count > 0) {
> > @@ -514,9 +530,9 @@ anv_cmd_buffer_bind_descriptor_set(struct
> anv_cmd_buffer *cmd_buffer,
> > /* Assert that everything is in range */
> > assert(set_layout->dynamic_offset_count <=
> *dynamic_offset_count);
> > assert(dynamic_offset_start + set_layout->dynamic_offset_count
> <=
> > -ARRAY_SIZE(cmd_buffer->state.dynamic_offsets));
> > +ARRAY_SIZE(pipe_state->dynamic_offsets));
> >
> > - typed_memcpy(_buffer->state.dynamic_offsets[dynamic_
> offset_start],
> > + typed_memcpy(_state->dynamic_offsets[dynamic_
> offset_start],
> >*dynamic_offsets, set_layout->dynamic_offset_
> count);
> >
> > *dynamic_offsets += set_layout->dynamic_offset_count;
> > @@ -524,7 +540,13 @@ anv_cmd_buffer_bind_descriptor_set(struct
> anv_cmd_buffer *cmd_buffer,
> >}
> > }
> >
> > - cmd_buffer->state.descriptors_dirty |= set_layout->shader_stages;
> > + if (bind_point == VK_PIPELINE_BIND_POINT_COMPUTE) {
> > + cmd_buffer->state.descriptors_dirty |=
> VK_SHADER_STAGE_COMPUTE_BIT;
> > + } else {
> > + assert(bind_point == VK_PIPELINE_BIND_POINT_GRAPHICS);
> > + cmd_buffer->state.descriptors_dirty |=
> > + set_layout->shader_stages & VK_SHADER_STAGE_ALL_GRAPHICS;
>
> Should we put () around the right hand side? We seem to be using that
> elsewhere.
>
Meh. I think it's fairly obvious what's going on. I do sometimes insist
on wraping == statements in () because a = b == c is something I find hard
to read but a = b & c seems obvious to me.
> > + }
> > }
> >
> > void
Re: [Mesa-dev] [PATCH 11/17] anv: Separate compute and graphics descriptor sets
On Fri, Dec 15, 2017 at 05:09:09PM -0800, Jason Ekstrand wrote:
> The Vulkan spec says:
>
> "pipelineBindPoint is a VkPipelineBindPoint indicating whether the
> descriptors will be used by graphics pipelines or compute pipelines.
> There is a separate set of bind points for each of graphics and
> compute, so binding one does not disturb the other."
>
> Up until now, we've been ignoring the pipeline bind point and had just
> one bind point for everything. This commit separates things out into
> separate bind points.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102897
> ---
> src/intel/vulkan/anv_cmd_buffer.c | 65
> ++-
> src/intel/vulkan/anv_descriptor_set.c | 2 ++
> src/intel/vulkan/anv_private.h| 11 +++---
> src/intel/vulkan/genX_cmd_buffer.c| 24 +++--
> 4 files changed, 70 insertions(+), 32 deletions(-)
>
> diff --git a/src/intel/vulkan/anv_cmd_buffer.c
> b/src/intel/vulkan/anv_cmd_buffer.c
> index 636f515..9720e7e 100644
> --- a/src/intel/vulkan/anv_cmd_buffer.c
> +++ b/src/intel/vulkan/anv_cmd_buffer.c
> @@ -124,12 +124,20 @@ anv_cmd_state_init(struct anv_cmd_buffer *cmd_buffer)
> }
>
> static void
> +anv_cmd_pipeline_state_finish(struct anv_cmd_buffer *cmd_buffer,
> + struct anv_cmd_pipeline_state *pipe_state)
> +{
> + for (uint32_t i = 0; i < ARRAY_SIZE(pipe_state->push_descriptors); i++)
> + vk_free(_buffer->pool->alloc, pipe_state->push_descriptors[i]);
> +}
> +
> +static void
> anv_cmd_state_finish(struct anv_cmd_buffer *cmd_buffer)
> {
> struct anv_cmd_state *state = _buffer->state;
>
> - for (uint32_t i = 0; i < ARRAY_SIZE(state->push_descriptors); i++)
> - vk_free(_buffer->pool->alloc, state->push_descriptors[i]);
> + anv_cmd_pipeline_state_finish(cmd_buffer, >gfx.base);
> + anv_cmd_pipeline_state_finish(cmd_buffer, >compute.base);
>
> for (uint32_t i = 0; i < MESA_SHADER_STAGES; i++)
>vk_free(_buffer->pool->alloc, state->push_constants[i]);
> @@ -495,6 +503,7 @@ void anv_CmdSetStencilReference(
>
> static void
> anv_cmd_buffer_bind_descriptor_set(struct anv_cmd_buffer *cmd_buffer,
> + VkPipelineBindPoint bind_point,
> struct anv_pipeline_layout *layout,
> uint32_t set_index,
> struct anv_descriptor_set *set,
> @@ -504,7 +513,14 @@ anv_cmd_buffer_bind_descriptor_set(struct anv_cmd_buffer
> *cmd_buffer,
> struct anv_descriptor_set_layout *set_layout =
>layout->set[set_index].layout;
>
> - cmd_buffer->state.descriptors[set_index] = set;
> + struct anv_cmd_pipeline_state *pipe_state;
> + if (bind_point == VK_PIPELINE_BIND_POINT_COMPUTE) {
> + pipe_state = _buffer->state.compute.base;
> + } else {
> + assert(bind_point == VK_PIPELINE_BIND_POINT_GRAPHICS);
> + pipe_state = _buffer->state.gfx.base;
> + }
> + pipe_state->descriptors[set_index] = set;
>
> if (dynamic_offsets) {
>if (set_layout->dynamic_offset_count > 0) {
> @@ -514,9 +530,9 @@ anv_cmd_buffer_bind_descriptor_set(struct anv_cmd_buffer
> *cmd_buffer,
> /* Assert that everything is in range */
> assert(set_layout->dynamic_offset_count <= *dynamic_offset_count);
> assert(dynamic_offset_start + set_layout->dynamic_offset_count <=
> -ARRAY_SIZE(cmd_buffer->state.dynamic_offsets));
> +ARRAY_SIZE(pipe_state->dynamic_offsets));
>
> -
> typed_memcpy(_buffer->state.dynamic_offsets[dynamic_offset_start],
> + typed_memcpy(_state->dynamic_offsets[dynamic_offset_start],
>*dynamic_offsets, set_layout->dynamic_offset_count);
>
> *dynamic_offsets += set_layout->dynamic_offset_count;
> @@ -524,7 +540,13 @@ anv_cmd_buffer_bind_descriptor_set(struct anv_cmd_buffer
> *cmd_buffer,
>}
> }
>
> - cmd_buffer->state.descriptors_dirty |= set_layout->shader_stages;
> + if (bind_point == VK_PIPELINE_BIND_POINT_COMPUTE) {
> + cmd_buffer->state.descriptors_dirty |= VK_SHADER_STAGE_COMPUTE_BIT;
> + } else {
> + assert(bind_point == VK_PIPELINE_BIND_POINT_GRAPHICS);
> + cmd_buffer->state.descriptors_dirty |=
> + set_layout->shader_stages & VK_SHADER_STAGE_ALL_GRAPHICS;
Should we put () around the right hand side? We seem to be using that
elsewhere.
> + }
> }
>
> void anv_CmdBindDescriptorSets(
> @@ -544,8 +566,8 @@ void anv_CmdBindDescriptorSets(
>
> for (uint32_t i = 0; i < descriptorSetCount; i++) {
>ANV_FROM_HANDLE(anv_descriptor_set, set, pDescriptorSets[i]);
> - anv_cmd_buffer_bind_descriptor_set(cmd_buffer, layout,
> - firstSet + i, set,
> + anv_cmd_buffer_bind_descriptor_set(cmd_buffer, pipelineBindPoint,
> + layout, firstSet +
[Mesa-dev] [PATCH 11/17] anv: Separate compute and graphics descriptor sets
The Vulkan spec says:
"pipelineBindPoint is a VkPipelineBindPoint indicating whether the
descriptors will be used by graphics pipelines or compute pipelines.
There is a separate set of bind points for each of graphics and
compute, so binding one does not disturb the other."
Up until now, we've been ignoring the pipeline bind point and had just
one bind point for everything. This commit separates things out into
separate bind points.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102897
---
src/intel/vulkan/anv_cmd_buffer.c | 65 ++-
src/intel/vulkan/anv_descriptor_set.c | 2 ++
src/intel/vulkan/anv_private.h| 11 +++---
src/intel/vulkan/genX_cmd_buffer.c| 24 +++--
4 files changed, 70 insertions(+), 32 deletions(-)
diff --git a/src/intel/vulkan/anv_cmd_buffer.c
b/src/intel/vulkan/anv_cmd_buffer.c
index 636f515..9720e7e 100644
--- a/src/intel/vulkan/anv_cmd_buffer.c
+++ b/src/intel/vulkan/anv_cmd_buffer.c
@@ -124,12 +124,20 @@ anv_cmd_state_init(struct anv_cmd_buffer *cmd_buffer)
}
static void
+anv_cmd_pipeline_state_finish(struct anv_cmd_buffer *cmd_buffer,
+ struct anv_cmd_pipeline_state *pipe_state)
+{
+ for (uint32_t i = 0; i < ARRAY_SIZE(pipe_state->push_descriptors); i++)
+ vk_free(_buffer->pool->alloc, pipe_state->push_descriptors[i]);
+}
+
+static void
anv_cmd_state_finish(struct anv_cmd_buffer *cmd_buffer)
{
struct anv_cmd_state *state = _buffer->state;
- for (uint32_t i = 0; i < ARRAY_SIZE(state->push_descriptors); i++)
- vk_free(_buffer->pool->alloc, state->push_descriptors[i]);
+ anv_cmd_pipeline_state_finish(cmd_buffer, >gfx.base);
+ anv_cmd_pipeline_state_finish(cmd_buffer, >compute.base);
for (uint32_t i = 0; i < MESA_SHADER_STAGES; i++)
vk_free(_buffer->pool->alloc, state->push_constants[i]);
@@ -495,6 +503,7 @@ void anv_CmdSetStencilReference(
static void
anv_cmd_buffer_bind_descriptor_set(struct anv_cmd_buffer *cmd_buffer,
+ VkPipelineBindPoint bind_point,
struct anv_pipeline_layout *layout,
uint32_t set_index,
struct anv_descriptor_set *set,
@@ -504,7 +513,14 @@ anv_cmd_buffer_bind_descriptor_set(struct anv_cmd_buffer
*cmd_buffer,
struct anv_descriptor_set_layout *set_layout =
layout->set[set_index].layout;
- cmd_buffer->state.descriptors[set_index] = set;
+ struct anv_cmd_pipeline_state *pipe_state;
+ if (bind_point == VK_PIPELINE_BIND_POINT_COMPUTE) {
+ pipe_state = _buffer->state.compute.base;
+ } else {
+ assert(bind_point == VK_PIPELINE_BIND_POINT_GRAPHICS);
+ pipe_state = _buffer->state.gfx.base;
+ }
+ pipe_state->descriptors[set_index] = set;
if (dynamic_offsets) {
if (set_layout->dynamic_offset_count > 0) {
@@ -514,9 +530,9 @@ anv_cmd_buffer_bind_descriptor_set(struct anv_cmd_buffer
*cmd_buffer,
/* Assert that everything is in range */
assert(set_layout->dynamic_offset_count <= *dynamic_offset_count);
assert(dynamic_offset_start + set_layout->dynamic_offset_count <=
-ARRAY_SIZE(cmd_buffer->state.dynamic_offsets));
+ARRAY_SIZE(pipe_state->dynamic_offsets));
- typed_memcpy(_buffer->state.dynamic_offsets[dynamic_offset_start],
+ typed_memcpy(_state->dynamic_offsets[dynamic_offset_start],
*dynamic_offsets, set_layout->dynamic_offset_count);
*dynamic_offsets += set_layout->dynamic_offset_count;
@@ -524,7 +540,13 @@ anv_cmd_buffer_bind_descriptor_set(struct anv_cmd_buffer
*cmd_buffer,
}
}
- cmd_buffer->state.descriptors_dirty |= set_layout->shader_stages;
+ if (bind_point == VK_PIPELINE_BIND_POINT_COMPUTE) {
+ cmd_buffer->state.descriptors_dirty |= VK_SHADER_STAGE_COMPUTE_BIT;
+ } else {
+ assert(bind_point == VK_PIPELINE_BIND_POINT_GRAPHICS);
+ cmd_buffer->state.descriptors_dirty |=
+ set_layout->shader_stages & VK_SHADER_STAGE_ALL_GRAPHICS;
+ }
}
void anv_CmdBindDescriptorSets(
@@ -544,8 +566,8 @@ void anv_CmdBindDescriptorSets(
for (uint32_t i = 0; i < descriptorSetCount; i++) {
ANV_FROM_HANDLE(anv_descriptor_set, set, pDescriptorSets[i]);
- anv_cmd_buffer_bind_descriptor_set(cmd_buffer, layout,
- firstSet + i, set,
+ anv_cmd_buffer_bind_descriptor_set(cmd_buffer, pipelineBindPoint,
+ layout, firstSet + i, set,
,
);
}
@@ -851,10 +873,19 @@ anv_cmd_buffer_get_depth_stencil_view(const struct
anv_cmd_buffer *cmd_buffer)
static struct anv_push_descriptor_set *
anv_cmd_buffer_get_push_descriptor_set(struct anv_cmd_buffer *cmd_buffer,
+
