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

[Michel Dänzer]

On 24.01.2015 11:56, Tom Stellard wrote:
> On Thu, Jan 22, 2015 at 11:27:32AM +0900, Michel Dänzer wrote:
>> On 21.01.2015 21:12, Marek Olšák wrote:
>>> We also had a case when the CPU accidentally corrupted shaders,
>>> because the shaders were mapped after textures and a CPU texture
>>> upload overflowed and overwrote shaders. I suppose we should have
>>> unmapped the shaders.
>>
>> Sounds like a good idea.
>>
>>
>> Tom, for now I suggest this solution, summarized from Marek's previous
>> descriptions:
>>
>> (At least) for shaders which have relocations, keep a copy of the
>> machine code in malloced memory. When the relocated values change,
>> update them in the malloced memory, allocate a new BO, map it, copy the
>> machine code from the malloced memory to the BO, replace any existing
>> shader BO with the new one and invalidate the shader state.
>>
> 
> Hi,
> 
> Attached is a WIP patch attempting to implement it this way.
> Unfortunately, I was unable to get it working, so I wanted to
> submit it for review in case someone can spot what I'm doing wrong.
> 
> You can find the broken code wrapped in #if 0 in the
> si_update_scratch_buffer() function in si_state_shaders.c
> 
> Based on the dmesg output and other tests I've done, it appears
> that the GPU is still executing the shader code from the old bo
> which does not contain the relocations.
> 
> The code in the #else branch works fine, but it updates the existing
> bo in place rather than creating a new one.
> 
> Any idea what I've done wrong?

Some more comments in addition to what I mentioned before:


> @@ -221,6 +222,11 @@ struct si_context {
>  	int			last_prim;
>  	int			last_multi_vgt_param;
>  	int			last_rast_prim;
> +
> +	/* Scratch buffer */
> +	boolean                 emit_scratch_reloc ;
> +	unsigned		scratch_waves;
> +	unsigned		spi_tmpring_size;
>  };
>  
>  /* si_blit.c */

No space before the semicolon.

Also, there's a few extraneous blank lines being added, e.g. at the
beginning of blocks or between statements where there already is a blank
line.


> +	/* The LLVM shader backend should be reporting aligned scratch_sizes. */
> +	assert((scratch_needed_size & ~0x3FF) == scratch_needed_size &&
> +		"scratch size should already be aligned correctly.");
> +
> +	sctx->spi_tmpring_size = S_0286E8_WAVES(sctx->scratch_waves) |
> +				S_0286E8_WAVESIZE(scratch_bytes_per_wave >> 10);

I think these are only necessary in the (scratch_needed_size >
current_scratch_buffer_size) case?

The (scratch_needed_size > current_scratch_buffer_size) case also needs
to set sctx->emit_scratch_reloc = true.


Other than that, the change looks good to me.


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