On Fri, Jul 20, 2012 at 1:34 AM, Jerome Glisse <j.gli...@gmail.com> wrote: > On Thu, Jul 19, 2012 at 6:07 PM, Marek Olšák <mar...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
