[Mesa-dev] [PATCH 5/5] radeonsi: Enable VGPR spilling for all shader types v5

[Michel Dänzer]

On 28.01.2015 04:05, Tom Stellard wrote:
> v2:
>   - Only emit write SPI_TMPRING_SIZE once per packet.
>   - Use context global scratch buffer.
> 
> v3:
>   - Patch shaders using WRITE_DATA packet instead of map/unmap.
>   - Emit ICACHE_FLUSH, CS_PARTIAL_FLUSH, PS_PARTIAL_FLUSH, and
>     VS_PARTIAL_FLUSH when patching shaders.
> 
> v4:
>   - Code cleanups.
>   - Remove unnecessary multiplies.
> 
> v5:
>   - Patch shaders in system memory and re-upload to vram.

[...]

> @@ -158,6 +159,12 @@ 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.  Using the wrong value here can result in
> +	 * GPU lockups, but the maximum value seems to always work.
> +	 */

Missing 'to' in "I'm not sure how to compute".


> @@ -571,6 +571,21 @@ void si_draw_vbo(struct pipe_context *ctx, const struct pipe_draw_info *info)
>  	if (sctx->b.flags)
>  		sctx->atoms.s.cache_flush->dirty = true;
>  
> +
> +	if (sctx->emit_scratch_reloc) {

Extraneous blank line before the if statement.


> +	/* Update the shader state to use the new shader bo. */
> +        si_shader_init_pm4_state(shader);

Indentation of the function call looks off.


> +static unsigned si_get_max_scratch_bytes_per_wave(struct si_context *sctx)
> +{
> +
> +	return MAX3(si_get_scratch_buffer_bytes_per_wave(sctx, sctx->ps_shader),
> +			si_get_scratch_buffer_bytes_per_wave(sctx, sctx->gs_shader),
> +			si_get_scratch_buffer_bytes_per_wave(sctx, sctx->vs_shader));
> +}

[...]

> +	if (scratch_needed_size > 0) {
> +
> +		if (scratch_needed_size > current_scratch_buffer_size) {
> +
> +			/* Create a bigger scratch buffer */

Extraneous blank lines at the beginning of the blocks.


> +		/* Update the shaders, so they are using the latest scratch.  The
> +		 * scratch buffer may have been changed since these shaders were
> +		 * last used, so we still need to try to update them, even if
> +		 * they require scratch buffers smaller than the current size.
> +		 */
> +		if (si_update_scratch_buffer(sctx, sctx->ps_shader)) {
> +			si_pm4_bind_state(sctx, ps, sctx->ps_shader->current->pm4);
> +			sctx->emitted.named.ps = NULL;
> +		}
> +		if (si_update_scratch_buffer(sctx, sctx->gs_shader)) {
> +			si_pm4_bind_state(sctx, gs, sctx->gs_shader->current->pm4);
> +			sctx->emitted.named.gs = NULL;
> +		}
> +		if (si_update_scratch_buffer(sctx, sctx->vs_shader)) {
> +			si_pm4_bind_state(sctx, vs, sctx->vs_shader->current->pm4);
> +			sctx->emitted.named.vs = NULL;
> +		}

Setting the emitted states to NULL isn't necessary. I think Marek made
that suggestion based on the assumption that the PM4 states would be
modified instead of replaced by new ones, but I don't think that's
practical.


In patch 4, I spotted a typo: 'beofre'


Other than that, the series is

Reviewed-by: Michel Dänzer <michel.daenzer at amd.com>


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