[Mesa-dev] [PATCH 4/6] radeonsi: fix printing vertex buffer descriptors into ddebug reports

[Nicolai Hähnle]

On 01.08.2017 00:43, Marek Olšák wrote:
> From: Marek Olšák <marek.olsak at amd.com>
> 
> ---
>   src/gallium/drivers/radeonsi/si_debug.c       | 3 +++
>   src/gallium/drivers/radeonsi/si_descriptors.c | 7 ++++++-
>   2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/src/gallium/drivers/radeonsi/si_debug.c b/src/gallium/drivers/radeonsi/si_debug.c
> index 06dea61..7c8a0fe 100644
> --- a/src/gallium/drivers/radeonsi/si_debug.c
> +++ b/src/gallium/drivers/radeonsi/si_debug.c
> @@ -385,20 +385,23 @@ typedef unsigned (*slot_remap_func)(unsigned);
>   static void si_dump_descriptor_list(struct si_descriptors *desc,
>   				    const char *shader_name,
>   				    const char *elem_name,
>   				    unsigned element_dw_size,
>   				    unsigned num_elements,
>   				    slot_remap_func slot_remap,
>   				    FILE *f)
>   {
>   	unsigned i, j;
>   
> +	if (!desc->list)
> +		return;
> +
>   	for (i = 0; i < num_elements; i++) {
>   		unsigned dw_offset = slot_remap(i) * element_dw_size;
>   		uint32_t *gpu_ptr = desc->gpu_list ? desc->gpu_list : desc->list;
>   		const char *list_note = desc->gpu_list ? "GPU list" : "CPU list";
>   		uint32_t *cpu_list = desc->list + dw_offset;
>   		uint32_t *gpu_list = gpu_ptr + dw_offset;
>   
>   		fprintf(f, COLOR_GREEN "%s%s slot %u (%s):" COLOR_RESET "\n",
>   			shader_name, elem_name, i, list_note);
>   
> diff --git a/src/gallium/drivers/radeonsi/si_descriptors.c b/src/gallium/drivers/radeonsi/si_descriptors.c
> index b080562..4de6086 100644
> --- a/src/gallium/drivers/radeonsi/si_descriptors.c
> +++ b/src/gallium/drivers/radeonsi/si_descriptors.c
> @@ -1094,20 +1094,21 @@ bool si_upload_vertex_buffer_descriptors(struct si_context *sctx)
>   	 * the fine-grained upload path.
>   	 */
>   	u_upload_alloc(sctx->b.b.const_uploader, 0,
>   		       desc_list_byte_size,
>   		       si_optimal_tcc_alignment(sctx, desc_list_byte_size),
>   		       (unsigned*)&desc->buffer_offset,
>   		       (struct pipe_resource**)&desc->buffer, (void**)&ptr);
>   	if (!desc->buffer)
>   		return false;
>   
> +	desc->list = ptr;
>   	radeon_add_to_buffer_list(&sctx->b, &sctx->b.gfx,
>   			      desc->buffer, RADEON_USAGE_READ,
>   			      RADEON_PRIO_DESCRIPTORS);
>   
>   	assert(count <= SI_MAX_ATTRIBS);
>   
>   	for (i = 0; i < count; i++) {
>   		struct pipe_vertex_buffer *vb;
>   		struct r600_resource *rbuffer;
>   		unsigned offset;
> @@ -2827,20 +2828,22 @@ void si_init_all_descriptors(struct si_context *sctx)
>   				 SI_SGPR_RW_BUFFERS,
>   				 /* The second set of usage/priority is used by
>   				  * const buffers in RW buffer slots. */
>   				 RADEON_USAGE_READWRITE, RADEON_USAGE_READ,
>   				 RADEON_PRIO_SHADER_RINGS, RADEON_PRIO_CONST_BUFFER,
>   				 &ce_offset);
>   	sctx->descriptors[SI_DESCS_RW_BUFFERS].num_active_slots = SI_NUM_RW_BUFFERS;
>   
>   	si_init_descriptors(sctx, &sctx->vertex_buffers, SI_SGPR_VERTEX_BUFFERS,
>   			    4, SI_NUM_VERTEX_BUFFERS, 0, 0, NULL);
> +	FREE(sctx->vertex_buffers.list); /* not used */
> +	sctx->vertex_buffers.list = NULL;
>   
>   	sctx->descriptors_dirty = u_bit_consecutive(0, SI_NUM_DESCS);
>   	sctx->total_ce_ram_allocated = ce_offset;
>   
>   	if (sctx->b.chip_class >= GFX9)
>   		assert(ce_offset <= 4096);
>   	else
>   		assert(ce_offset <= 32768);
>   
>   	/* Set pipe_context functions. */
> @@ -2940,21 +2943,23 @@ void si_release_all_descriptors(struct si_context *sctx)
>   		si_release_sampler_views(&sctx->samplers[i].views);
>   		si_release_image_views(&sctx->images[i]);
>   	}
>   	si_release_buffer_resources(&sctx->rw_buffers,
>   				    &sctx->descriptors[SI_DESCS_RW_BUFFERS]);
>   	for (i = 0; i < SI_NUM_VERTEX_BUFFERS; i++)
>   		pipe_vertex_buffer_unreference(&sctx->vertex_buffer[i]);
>   
>   	for (i = 0; i < SI_NUM_DESCS; ++i)
>   		si_release_descriptors(&sctx->descriptors[i]);
> -	si_release_descriptors(&sctx->vertex_buffers);
> +
> +	/* Only one member of si_descriptors needs to be freed: */
> +	r600_resource_reference(&sctx->vertex_buffers.buffer, NULL);

I have a mild preference to instead do

    sctx->vertex_buffers.list = NULL; /* points into a mapped buffer */
    si_release_descriptors(&sctx->vertex_buffers);

so that if we ever add more members that need to be released, we don't 
have to worry about updating this.

Either way, patches 2-4:

Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>


>   }
>   
>   void si_all_descriptors_begin_new_cs(struct si_context *sctx)
>   {
>   	int i;
>   
>   	for (i = 0; i < SI_NUM_SHADERS; i++) {
>   		si_buffer_resources_begin_new_cs(sctx, &sctx->const_and_shader_buffers[i]);
>   		si_sampler_views_begin_new_cs(sctx, &sctx->samplers[i].views);
>   		si_image_views_begin_new_cs(sctx, &sctx->images[i]);
> 


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.