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 > > <[email protected]> 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 <[email protected]> > > > --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 > [email protected] > https://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
