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

[Kristian Høgsberg]

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.  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:

+ * 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?

For the series, enthusiastically

Reviewed-by: Kristian Høgsberg <krh at bitplanet.net>


> No regressions on 965, GM45, Ironlake, Sandybridge GT1/2, Ivybridge GT1/2,
> or Haswell GT3e.  I really should check Broadwell before pushing, but
> haven't yet.
>
> This is available as the 'state-kill-cache' branch of my tree.
> It depends on the ddx/ddy cleanups I sent yesterday.
>
> --Ken
>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev