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

[Marek Olšák]

On Thu, Aug 15, 2013 at 9:33 AM, Michel Dänzer <michel at daenzer.net> wrote:
> 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?

Very likely. I'm using SI VERDE.

>
>>   If I set SH_KCACHE_ACTION_ENA before every draw call, only 2 contexts are needed.
>
> But that hurts performance?

I didn't test performance in that case, but presumably yes.

>
>
> 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.

I was thinking of that too. Alright, I'll add the flags.

Marek