[Mesa-dev] [PATCH] radeonsi: add flexible shader descriptor management and use it for sampler views

[Michel Dänzer]

On Don, 2013-08-15 at 05:25 +0200, Marek Olšák wrote:
> (This should be applied before MSAA, which will need to be rebased.)
> 
> It moves all sampler view descriptors to a buffer.
> It supports partial resource updates and it can also unbind resources
> (required for FMASK texturing).
> 
> The buffer contains all sampler view descriptors for one shader stage,
> represented as an array. On top of that, there are N arrays in the buffer,
> which are used to emulate context registers as implemented by the previous
> ASICs (each array is a context).
> 
> This uses the RCU synchronization approach to avoid read-after-write hazards
> as discussed in the thread:
> "radeonsi: add FMASK texture binding slots and resource setup"
> 
> CP DMA is used to clear the descriptors at context initialization and to copy
> the descriptors from one context to the next.
> 
> IMPORTANT:
>   128 resource contexts are needed, 64 doesn't work.

Hmm, I suppose this might depend on the specific GPU?

>   If I set SH_KCACHE_ACTION_ENA before every draw call, only 2 contexts are needed.

But that hurts performance?


The patch looks good to me, just a minor comment:


> +/* Emit a CP DMA packet to do a copy from one buffer to another.
> + * The size must fit in bits [20:0]. Notes:
> + *
> + * 1) Set sync to true if you want the 3D engine to wait until CP DMA is done.
> + *
> + * 2) Set raw_hazard_wait to true if the source data was used as a destination
> + *    in a previous CP DMA packet. It's for preventing a read-after-write hazard
> + *    between two CP DMA packets.
> + */
> +static void si_emit_cp_dma_copy_buffer(struct r600_context *rctx,
> +				       uint64_t dst_va, uint64_t src_va,
> +				       unsigned size,
> +				       bool sync, bool raw_hazard_wait)

[...]

> +	/* Copy the descriptors to a new context slot. */
> +	si_emit_cp_dma_copy_buffer(rctx,
> +				   va_base + new_context_id * desc->context_size,
> +				   va_base + desc->current_context_id * desc->context_size,
> +				   desc->context_size, true, false);

It's hard to remember / guess the meaning of such boolean parameters, so
it might be better to use self-explanatory flags instead.


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