On Tuesday, November 25, 2014 09:25:35 AM Kristian Høgsberg wrote: > On Tue, Nov 25, 2014 at 4:43 AM, Kenneth Graunke <kenn...@whitecape.org> > wrote: > > Hello, > > > > This series does some longstanding cleaning I've been meaning to do > > in the i965 state upload code. The distinction between BRW_NEW_* and > > CACHE_NEW_* flags has been pretty arbitrary for a while - 10/17 of > > them were for things we stopped caching years ago. So, I moved > > those to be BRW_NEW_* bits, and combined a bunch of redundant ones > > while I was at it. > > > > Patches 1-6 move non-cache-related things out of .cache, along with > > other tidying. This actually could save up to 160 bytes of memory > > per context (on 64-bit), because cache types have auxiliary compare > > and free function pointers...which weren't used at all for these. > > (I haven't actually measured this - just eliminated the fields). > > > > Patches 7-10 take it a step further, and kill off the "cache" bitset > > altogether. A while back, I was looking at callgrind graphs for Glamor, > > trying to reduce brw_state_upload costs. One of the places where I saw > > cycles being wasted was in check_state(), which sees if each atom needs > > to be emitted. Eliminating "cache" should eliminate 1/4 of the cycles > > spent there, and every little bit helps. > > > > I also like the new names - BRW_NEW_VERTEX_PROGRAM vs CACHE_NEW_VS_PROG > > was always confusing - which is which, and why should I use one or the > > other? BRW_NEW_VS_PROG_DATA is clearly tied to brw_vs_prog_data. > > Wow, nice, I like everything about this series. One of the most > confusing things about the state mechanism in mesa is the .cache > stuff, which certainly comes down to most of the items not being > cached.
Awesome, thanks!
> Also, BRW_NEW_PROG_DATA is a better name, but it doesn't as
> clearly suggest that the kernel start pointer (prog_offset) also
> changes. BRW_NEW_VS_BINARY, perhaps? The one comment I have is
> regarding this:
Yeah...that aspect of the new name isn't great. I think it's unlikely to
cause problems, though - only the atoms which emit 3DSTATE_VS/GS/PS/HS/DS need
to care about prog_offset, and those also have a ton of prog_data dependencies.
The thing I liked about BRW_NEW_*_PROG_DATA vs. BRW_NEW_*_PROGRAM is that
they're fairly distinct sounding, and the name is clearly associated with the
prog_data structure. So, when other atoms need to access prog_data, it's
obvious which bit to listen to (and we've gotten that backwards frequently).
To me, BRW_NEW_VS_BINARY and BRW_NEW_VERTEX_PROGRAM sound too similar, and I
suspect I'd have to ask "which one is which?" again. I took a quick poll
around the office and people seemed to like BRW_NEW_*_PROG_DATA.
So, I think I'll stick with these names for now, but if you feel strongly
about it, we can always rename it again :)
> + * BRW_NEW_*_PROG_DATA does not occur quite as often, and is a strict subset.
> + * Multiple shader programs may have identical vertex shaders (for example),
> + * or compile down to the same code in the backend. We combine those into
> + * a single program cache entry. BRW_NEW_*_PROG_DATA occurs when switching
> + * program cache entries, which covers the brw_*_prog_data structures, and
> + * brw->*.prog_offset.
>
> Isn't the main cause of BRW_NEW_*_PROG_DATA going to be switching to a
> different kernel/prog_data combination because of non-orthogonal state
> changes?
Oh man - thanks for pointing that out. That is indeed the primary cause
(beyond actually binding a different program), and that makes it *not* a strict
subset of BRW_NEW_*_PROGRAM. The only thing worse than a lack of comments is
comments that are wrong, and also point at the quux of the problem :)
Here's the revised one:
/**
* BRW_NEW_*_PROG_DATA and BRW_NEW_*_PROGRAM are similar, but distinct.
*
* BRW_NEW_*_PROGRAM relates to the gl_shader_program/gl_program structures.
* When the currently bound shader program differs from the previous draw
* call, these will be flagged. They cover brw->{stage}_program and
* ctx->{Stage}Program->_Current.
*
* BRW_NEW_*_PROG_DATA is flagged when the effective shaders change, from a
* driver perspective. Even if the same shader is bound at the API level,
* we may need to switch between multiple versions of that shader to handle
* changes in non-orthagonal state.
*
* Additionally, multiple shader programs may have identical vertex shaders
* (for example), or compile down to the same code in the backend. We combine
* those into a single program cache entry.
*
* BRW_NEW_*_PROG_DATA occurs when switching program cache entries, which
* covers the brw_*_prog_data structures, and brw->*.prog_offset.
*/
> For the series, enthusiastically
>
> Reviewed-by: Kristian Høgsberg <k...@bitplanet.net>
Thanks again!
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
