[Mesa-dev] [PATCH] radeonsi: Enable VGPR spilling for all shader types v3

[Michel Dänzer]

On 17.01.2015 02:54, Tom Stellard wrote:
> @@ -158,6 +159,11 @@ static struct pipe_context *si_create_context(struct pipe_screen *screen, void *
>  				     sctx->null_const_buf.buffer->width0, 0, false);
>  	}
>  
> +	/* XXX: This is the maximum value allowed.  I'm not sure how compute
> +	 * this for non-cs shaders.
> +	 */
> +	sctx->scratch_waves = 32 * sscreen->b.info.max_compute_units;

Spelling: 'how to compute'

What would be the impact of this value being off?


> +static void si_update_scratch_buffer(struct si_context *sctx,
> +				    struct si_shader_selector *sel)
> +{
> +	struct si_shader *shader;
> +	unsigned scratch_bytes;
> +
> +	if (!sel) {
> +		return;
> +	}

No need for curly braces around single statements.


> +	shader = sel->current;
> +	scratch_bytes =	shader->scratch_bytes_per_wave *
> +					sctx->scratch_waves;

The multiplied value could be saved in the shader struct.


> +static void si_emit_spi_tmpring_state(struct si_context *sctx)
> +{
> +	struct radeon_winsys_cs *cs = sctx->b.rings.gfx.cs;
> +	unsigned current_scratch_buffer_size =
> +		si_get_current_scratch_buffer_size(sctx);
> +	unsigned scratch_needed_size =
> +		si_get_max_scratch_size_needed(sctx);
> +	unsigned scratch_bytes_per_wave = scratch_needed_size /
> +		sctx->scratch_waves;
> +
> +	if (scratch_needed_size > current_scratch_buffer_size) {
> +		/* Create a bigger scratch buffer */
> +		struct r600_resource *new_scratch_buffer =
> +				si_resource_create_custom(&sctx->screen->b.b,
> +                                PIPE_USAGE_DEFAULT, scratch_needed_size);
> +
> +		pipe_resource_reference(
> +				(struct pipe_resource**)&sctx->scratch_buffer,
> +				&new_scratch_buffer->b.b);

I think this will leak the scratch buffers: pipe_resource_reference increments new_scratch_buffer's reference count to 2, so next time
it can only be decreased to 1. You need to do it like this:

		pipe_resource_reference(
				(struct pipe_resource**)&sctx->scratch_buffer,
				NULL);

		sctx->scratch_buffer =
			si_resource_create_custom(&sctx->screen->b.b,
                               PIPE_USAGE_DEFAULT, scratch_needed_size);


> +	/* Update the shaders, so they are using the latest scratch buffer. */
> +	si_update_scratch_buffer(sctx, sctx->ps_shader);
> +	si_update_scratch_buffer(sctx, sctx->gs_shader);
> +	si_update_scratch_buffer(sctx, sctx->vs_shader);

These should only be called after allocating a new scratch buffer, then you can remove the corresponding check in si_update_scratch_buffer.


> +	r600_write_context_reg(cs, R_0286E8_SPI_TMPRING_SIZE,
> +				S_0286E8_WAVES(sctx->scratch_waves) |
> +				S_0286E8_WAVESIZE(scratch_bytes_per_wave >> 10));
> +
> +	if (scratch_needed_size > 0) {
> +		r600_context_bo_reloc(&sctx->b, &sctx->b.rings.gfx,
> +			sctx->scratch_buffer, RADEON_USAGE_READWRITE,
> +			RADEON_PRIO_SHADER_RESOURCE_RW);
> +	}
> +}

These should only be done after allocating a new scratch buffer, and once for the first draw in each command stream. This will probably require some kind of flag in the context to remember whether these have been done yet for the current command stream, which is reset in si_begin_new_cs.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer