[Mesa-dev] [PATCH 00/10] i965/state: Merge cache and brw flags.

[Kenneth Graunke]

On Tuesday, November 25, 2014 09:25:35 AM Kristian Høgsberg wrote:
> On Tue, Nov 25, 2014 at 4:43 AM, Kenneth Graunke <kenneth at 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 <krh at bitplanet.net>

Thanks again!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141201/a0fca4ca/attachment.sig>