[Mesa-stable] [Mesa-dev] [PATCH] radeonsi: Save and restore entire CE RAM.

[Bas Nieuwenhuizen]

On Wed, Jul 27, 2016 at 3:33 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> On 6 June 2016 at 16:46, Bas Nieuwenhuizen <bas at basnieuwenhuizen.nl> wrote:
>> On Mon, Jun 6, 2016 at 5:14 PM, Nicolai Hähnle <nhaehnle at gmail.com> wrote:
>>> On 06.06.2016 00:28, Bas Nieuwenhuizen wrote:
>>>>
>>>> This fixes a problem with the CE preamble and restoring only stuff in the
>>>> preamble when needed.
>>>>
>>>> To illustrate suppose we have two graphics IB's 1 and 2, which  are
>>>> submitted in
>>>> that order. Furthermore suppose IB 1 does not use CE ram, but IB 2 does,
>>>> and we
>>>> have a context switch at the start of IB 1, but not between IB 1 and IB 2.
>>>>
>>>> The old code put the CE RAM loads in the preamble of IB 2. As the preamble
>>>> of
>>>> IB 1 does not have the loads and the preamble of IB 2 does not get
>>>> executed, the
>>>> old values are not load into CE RAM.
>>>>
>>>> Fix this by always restoring the entire CE RAM.
>>>
>>>
>>> Nice catch!
>>>
>>> Have you considered restoring from the desc->buffers instead? The
>>> double-dump seems a bit redundant. But maybe it's easier this way...
>>>
>>> Also, do we really need to dump the entire CE RAM? After initializing the
>>> descriptor sets, we should know exactly how much we need...
>>
>> We can do both.
>>
>> However, wrt using the desc->buffers, note that this results in 21
>> loads (and adding potentially multiple buffers to the CS bo list)
>> while the current approach is 1 load
>> and 1 store. Furthermore  the CE is <= 15% busy in pretty much all
>> games I tried, as measured by polling CP_STAT.CE_BUSY. As such I
>> haven't really considered optimizing this GPU side and I'm not really
>> sure if it is worth it in this case to do a bit more work on the CPU
>> to avoid some work on the GPU.
>>
>> Though I'm probably overthinking this given how small an impact the
>> whole CE stuff had on performance and we're now talking about a per IB
>> cost instead of per draw.
>>
> Gents, double-checking:
> The patch has been superseded (with
> 54f755fa0fda14c578022767bcef2f27b2e89707?), correct ?

Correct for master. For 12.0 it has been superseded by
28294573c737ff762b1c3f8d58bc78ddd010feca.

- Bas

>
> Thanks
> Emil