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 +
