[Mesa-dev] [PATCH 03/18] anv/allocator: Convert the state stream to pull from a state pool
Juan A. Suarez Romero
jasuarez at igalia.com
Wed May 3 11:51:38 UTC 2017
On Fri, 2017-04-28 at 10:31 +0200, Juan A. Suarez Romero wrote:
> On Thu, 2017-04-27 at 20:30 -0700, Jason Ekstrand wrote:
> > On Wed, Apr 26, 2017 at 9:04 AM, Juan A. Suarez Romero <jasuarez at igalia.com> wrote:
> > > On Wed, 2017-04-26 at 07:35 -0700, Jason Ekstrand wrote:
> > > > ---
> > > > src/intel/vulkan/anv_allocator.c | 71 +++++++++++++++++------------------
> > > > src/intel/vulkan/anv_cmd_buffer.c | 8 ++--
> > > > src/intel/vulkan/anv_descriptor_set.c | 4 +-
> > > > src/intel/vulkan/anv_private.h | 21 ++++++-----
> > > > 4 files changed, 52 insertions(+), 52 deletions(-)
> > > >
> > > > diff --git a/src/intel/vulkan/anv_allocator.c b/src/intel/vulkan/anv_allocator.c
> > > > index d93d4c9..1c94d1b 100644
> > > > --- a/src/intel/vulkan/anv_allocator.c
> > > > +++ b/src/intel/vulkan/anv_allocator.c
> > > > @@ -736,14 +736,12 @@ anv_state_pool_free(struct anv_state_pool *pool, struct anv_state state)
> > > > anv_state_pool_free_no_vg(pool, state);
> > > > }
> > > >
> > > > -#define NULL_BLOCK 1
> > > > struct anv_state_stream_block {
> > > > + struct anv_state block;
> > > > +
> > > > /* The next block */
> > > > struct anv_state_stream_block *next;
> > > >
> > > > - /* The offset into the block pool at which this block starts */
> > > > - uint32_t offset;
> > > > -
> > > > #ifdef HAVE_VALGRIND
> > > > /* A pointer to the first user-allocated thing in this block. This is
> > > > * what valgrind sees as the start of the block.
> > > > @@ -757,16 +755,18 @@ struct anv_state_stream_block {
> > > > */
> > > > void
> > > > anv_state_stream_init(struct anv_state_stream *stream,
> > > > - struct anv_block_pool *block_pool)
> > > > + struct anv_state_pool *state_pool,
> > > > + uint32_t block_size)
> > > > {
> > > > - stream->block_pool = block_pool;
> > > > - stream->block = NULL;
> > > > + stream->state_pool = state_pool;
> > > > + stream->block_size = block_size;
> >
> > There's a bug here where I don't initialize stream->block. It's fixed locally.
> >
> > > > - /* Ensure that next + whatever > end. This way the first call to
> > > > + stream->block_list = NULL;
> > > > +
> > > > + /* Ensure that next + whatever > block_size. This way the first call to
> > > > * state_stream_alloc fetches a new block.
> > > > */
> > > > - stream->next = 1;
> > > > - stream->end = 0;
> > > > + stream->next = block_size;
> > > >
> > > > VG(VALGRIND_CREATE_MEMPOOL(stream, 0, false));
> > > > }
> > > > @@ -774,14 +774,12 @@ anv_state_stream_init(struct anv_state_stream *stream,
> > > > void
> > > > anv_state_stream_finish(struct anv_state_stream *stream)
> > > > {
> > > > - VG(const uint32_t block_size = stream->block_pool->block_size);
> > > > -
> > > > - struct anv_state_stream_block *next = stream->block;
> > > > + struct anv_state_stream_block *next = stream->block_list;
> > > > while (next != NULL) {
> > > > struct anv_state_stream_block sb = VG_NOACCESS_READ(next);
> > > > VG(VALGRIND_MEMPOOL_FREE(stream, sb._vg_ptr));
> > > > - VG(VALGRIND_MAKE_MEM_UNDEFINED(next, block_size));
> > > > - anv_block_pool_free(stream->block_pool, sb.offset);
> > > > + VG(VALGRIND_MAKE_MEM_UNDEFINED(next, stream->block_size));
> > > > + anv_state_pool_free_no_vg(stream->state_pool, sb.block);
> > > > next = sb.next;
> > > > }
> > > >
> > > > @@ -795,35 +793,36 @@ anv_state_stream_alloc(struct anv_state_stream *stream,
> > > > if (size == 0)
> > > > return ANV_STATE_NULL;
> > > >
> > > > - struct anv_state_stream_block *sb = stream->block;
> > > > + uint32_t offset = align_u32(stream->next, alignment);
> > > > + if (offset + size > stream->block_size) {
> > > > + stream->block = anv_state_pool_alloc_no_vg(stream->state_pool,
> > > > + stream->block_size,
> > > > + stream->block_size);
> > >
> > >
> > > Why are we calling anv_state_pool_alloc_no_vg() with block_size twice?
> > > Shouldn't we use alignment as the last parameter?
> >
> > Actually, what we really want here is page alignment. All alignments passed into the allocator will be no more than the page size and we can't guarantee an alignment larger than that anyway. I've locally changed this to pass PAGE_SIZE to state_pool_alloc_no_vg. How's that sound?
> >
>
> Sounds good. Thanks.
>
With the fix, Reviewed-by: Juan A. Suarez Romero <jasuarez at igalia.com>
>
> > --Jason
> >
> > > >
> > > > - struct anv_state state;
> > > > -
> > > > - state.offset = align_u32(stream->next, alignment);
> > > > - if (state.offset + size > stream->end) {
> > > > - uint32_t block = anv_block_pool_alloc(stream->block_pool);
> > > > - sb = stream->block_pool->map + block;
> > > > + struct anv_state_stream_block *sb = stream->block.map;
> > > > + VG_NOACCESS_WRITE(&sb->block, stream->block);
> > > > + VG_NOACCESS_WRITE(&sb->next, stream->block_list);
> > > > + stream->block_list = sb;
> > > > + VG_NOACCESS_WRITE(&sb->_vg_ptr, NULL);
> > > >
> > > > - VG(VALGRIND_MAKE_MEM_UNDEFINED(sb, sizeof(*sb)));
> > > > - sb->next = stream->block;
> > > > - sb->offset = block;
> > > > - VG(sb->_vg_ptr = NULL);
> > > > - VG(VALGRIND_MAKE_MEM_NOACCESS(sb, stream->block_pool->block_size));
> > > > + VG(VALGRIND_MAKE_MEM_NOACCESS(stream->block.map, stream->block_size));
> > > >
> > > > - stream->block = sb;
> > > > - stream->start = block;
> > > > - stream->next = block + sizeof(*sb);
> > > > - stream->end = block + stream->block_pool->block_size;
> > > > + /* Reset back to the start plus space for the header */
> > > > + stream->next = sizeof(*sb);
> > > >
> > > > - state.offset = align_u32(stream->next, alignment);
> > > > - assert(state.offset + size <= stream->end);
> > > > + offset = align_u32(stream->next, alignment);
> > > > + assert(offset + size <= stream->block_size);
> > > > }
> > > >
> > > > - assert(state.offset > stream->start);
> > > > - state.map = (void *)sb + (state.offset - stream->start);
> > > > + struct anv_state state = stream->block;
> > > > + state.offset += offset;
> > > > state.alloc_size = size;
> > > > + state.map += offset;
> > > > +
> > > > + stream->next = offset + size;
> > > >
> > > > #ifdef HAVE_VALGRIND
> > > > + struct anv_state_stream_block *sb = stream->block_list;
> > > > void *vg_ptr = VG_NOACCESS_READ(&sb->_vg_ptr);
> > > > if (vg_ptr == NULL) {
> > > > vg_ptr = state.map;
> > > > @@ -839,8 +838,6 @@ anv_state_stream_alloc(struct anv_state_stream *stream,
> > > > }
> > > > #endif
> > > >
> > > > - stream->next = state.offset + size;
> > > > -
> > > > return state;
> > > > }
> > > >
> > > > diff --git a/src/intel/vulkan/anv_cmd_buffer.c b/src/intel/vulkan/anv_cmd_buffer.c
> > > > index c65eba2..120b864 100644
> > > > --- a/src/intel/vulkan/anv_cmd_buffer.c
> > > > +++ b/src/intel/vulkan/anv_cmd_buffer.c
> > > > @@ -212,9 +212,9 @@ static VkResult anv_create_cmd_buffer(
> > > > goto fail;
> > > >
> > > > anv_state_stream_init(&cmd_buffer->surface_state_stream,
> > > > - &device->surface_state_block_pool);
> > > > + &device->surface_state_pool, 4096);
> > > > anv_state_stream_init(&cmd_buffer->dynamic_state_stream,
> > > > - &device->dynamic_state_block_pool);
> > > > + &device->dynamic_state_pool, 16384);
> > > >
> > > > memset(&cmd_buffer->state.push_descriptor, 0,
> > > > sizeof(cmd_buffer->state.push_descriptor));
> > > > @@ -306,11 +306,11 @@ anv_cmd_buffer_reset(struct anv_cmd_buffer *cmd_buffer)
> > > >
> > > > anv_state_stream_finish(&cmd_buffer->surface_state_stream);
> > > > anv_state_stream_init(&cmd_buffer->surface_state_stream,
> > > > - &cmd_buffer->device->surface_state_block_pool);
> > > > + &cmd_buffer->device->surface_state_pool, 4096);
> > > >
> > > > anv_state_stream_finish(&cmd_buffer->dynamic_state_stream);
> > > > anv_state_stream_init(&cmd_buffer->dynamic_state_stream,
> > > > - &cmd_buffer->device->dynamic_state_block_pool);
> > > > + &cmd_buffer->device->dynamic_state_pool, 16384);
> > > > return VK_SUCCESS;
> > > > }
> > > >
> > > > diff --git a/src/intel/vulkan/anv_descriptor_set.c b/src/intel/vulkan/anv_descriptor_set.c
> > > > index 4797c1e..4b58b0b 100644
> > > > --- a/src/intel/vulkan/anv_descriptor_set.c
> > > > +++ b/src/intel/vulkan/anv_descriptor_set.c
> > > > @@ -345,7 +345,7 @@ VkResult anv_CreateDescriptorPool(
> > > > pool->free_list = EMPTY;
> > > >
> > > > anv_state_stream_init(&pool->surface_state_stream,
> > > > - &device->surface_state_block_pool);
> > > > + &device->surface_state_pool, 4096);
> > > > pool->surface_state_free_list = NULL;
> > > >
> > > > *pDescriptorPool = anv_descriptor_pool_to_handle(pool);
> > > > @@ -380,7 +380,7 @@ VkResult anv_ResetDescriptorPool(
> > > > pool->free_list = EMPTY;
> > > > anv_state_stream_finish(&pool->surface_state_stream);
> > > > anv_state_stream_init(&pool->surface_state_stream,
> > > > - &device->surface_state_block_pool);
> > > > + &device->surface_state_pool, 4096);
> > > > pool->surface_state_free_list = NULL;
> > > >
> > > > return VK_SUCCESS;
> > > > diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
> > > > index 6ee8f54..216c6eb 100644
> > > > --- a/src/intel/vulkan/anv_private.h
> > > > +++ b/src/intel/vulkan/anv_private.h
> > > > @@ -511,17 +511,19 @@ struct anv_state_pool {
> > > > struct anv_state_stream_block;
> > > >
> > > > struct anv_state_stream {
> > > > - struct anv_block_pool *block_pool;
> > > > + struct anv_state_pool *state_pool;
> > > > +
> > > > + /* The size of blocks to allocate from the state pool */
> > > > + uint32_t block_size;
> > > >
> > > > - /* The current working block */
> > > > - struct anv_state_stream_block *block;
> > > > + /* Current block we're allocating from */
> > > > + struct anv_state block;
> > > >
> > > > - /* Offset at which the current block starts */
> > > > - uint32_t start;
> > > > - /* Offset at which to allocate the next state */
> > > > + /* Offset into the current block at which to allocate the next state */
> > > > uint32_t next;
> > > > - /* Offset at which the current block ends */
> > > > - uint32_t end;
> > > > +
> > > > + /* List of all blocks allocated from this pool */
> > > > + struct anv_state_stream_block *block_list;
> > > > };
> > > >
> > > > #define CACHELINE_SIZE 64
> > > > @@ -566,7 +568,8 @@ struct anv_state anv_state_pool_alloc(struct anv_state_pool *pool,
> > > > size_t state_size, size_t alignment);
> > > > void anv_state_pool_free(struct anv_state_pool *pool, struct anv_state state);
> > > > void anv_state_stream_init(struct anv_state_stream *stream,
> > > > - struct anv_block_pool *block_pool);
> > > > + struct anv_state_pool *state_pool,
> > > > + uint32_t block_size);
> > > > void anv_state_stream_finish(struct anv_state_stream *stream);
> > > > struct anv_state anv_state_stream_alloc(struct anv_state_stream *stream,
> > > > uint32_t size, uint32_t alignment);
> >
> >
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list