[Mesa-dev] [PATCH 2/2] r600g: simplify and fix flushing and synchronization v2

[Marek Olšák]

On Fri, Jul 20, 2012 at 1:34 AM, Jerome Glisse <j.glisse at gmail.com> wrote:
> On Thu, Jul 19, 2012 at 6:07 PM, Marek Olšák <maraeo at gmail.com> wrote:
>> I have these issues with the patch:
>>
>> 1) On GPUs without a vertex cache, you flush the texture cache every
>> draw operation. Are you kidding?
>
> Show me one app with perf regression due to that ? Or just go look at
> what fglrx is doing.

I don't believe that fglrx unconditionally emits SURFACE_SYNC with
TC_ACTION_ENA before every DRAW packet. I just don't buy that. It's
too stupid to be true. And considering that it wasn't needed before,
it's not needed now either. Please give me some other argument than
just fglrx.

>
>> 2) All colorbuffers / streamout buffers are flushed, even those which
>> are not enabled. E.g. instead of flushing only CB0 when there is only
>> one, this code flushes all of them. Why? This either needs an
>> explanation or it should only flush the buffers which are enabled
>> (like the old code did).
>
> fglrx + no perf regression ...

The "no perf regression" argument doesn't apply here, because it just
might not be the bottleneck now. I'm willing to step aside from this
one issue though.

>
>> 3) Please explain:
>> - why you added PS_PARTIAL_FLUSH in r600_texture_barrier and
>> r600_set_framebuffer_state.
>
> fglrx is doing something similar

But not exactly the same thing, right? So there's no reason for it to be there.

>
>> - why you added CACHE_FLUSH_AND_INV_EVENT in set_framebuffer_state for
>> R700 and evergreen.
>
> fglrx ...
>
>> - why you applied the CB flush workarounds meant for RV6xx to all R600
>> and R700 chipsets.
>
> fglrx ...
>
>> - why the streamout workaround for RV6xx (S_0085F0_DEST_BASE_0_ENA) is
>> applied to all R600, R700, and evergreen chipsets.
>
> didn't hurt thought fglrx is not using that at all but i did not
> wanted to remove it

Well, you didn't remove it. You added it for those other chipsets.
That's a difference. You don't even know what you did there, do you?
:) All the things I mentioned are either half-assed or added for no
reason. Fglrx might do all sorts of stupid things or for its own
reasons, but that doesn't mean it's automatically good for us. Besides
that, it's almost impossible to figure out why a CS was built up
exactly the way it was without access to the driver code and to its developers.

>
>> - why R600_CONTEXT_FLUSH_AND_INV emits SURFACE_SYNC on evergreen,
>> resulting in emission of SURFACE_SYNC twice in a row in most
>> situations.
>
> fglrx is doing that and without that lockup ...

Hm, now you're talking. So do you need:

FLUSH_AND_INV +
SURFACE_SYNC (COHER_CNTL = ~0)

or do you need:

FLUSH_AND_INV +
SURFACE_SYNC (COHER_CNTL = ~0) +
SURFACE_SYNC (COHER_CNTL = according to flags)

for it not to lock up?

>
>> Flushing has always worked without all the changes (1, 2, 3) mentioned
>> above, so please if you don't have a reasonable explanation, revert to
>> the old behavior.
>
> Well if you have a better solution please show me ...

I already showed you in the first reply. If you are unwilling to
change your patches even a little bit, I'll happily take them over
from you.

Marek