<div dir="ltr"><div><div>Ah, I see.<br>I was assuming that a necessary dirty bit had not been set.<br></div>But the right dirty bit had been set and was being ignored.<br></div>Watching the right dirty bit is going to look a bit different in 10.3 or 10.4.<br>It will need to use ".cache = CACHE_NEW_WM_PROG" in brw_texture_surfaces<br>instead of adding BRW_NEW_FS_PROG_DATA to the brw mask.<br><br></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Dec 22, 2014 at 8:09 PM, Kenneth Graunke <span dir="ltr"><<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Monday, December 22, 2014 05:22:06 PM Mike Stroyan wrote:<br>
> This patch fixes a problem I reported as<br>
> [Bug 87619] Changes to state such as render targets change fragment shader without marking it dirty.<br>
><br>
> I sent a test that demonstrates the problem to the piglit mailing list as<br>
> fbo: Changing mrt binding with same shader source<br>
<br>
</span>Thanks for tracking this down (and writing a test)!<br>
<span class=""><br>
> The root cause of problem is rather generic.<br>
> brw_upload_wm_prog() calls brw_search_cache() to find the right<br>
> fragment shader for a particular key from brw_wm_populate_key().<br>
> It does not set any dirty bit for changes to the shader.<br>
<br>
</span>It actually does. brw_search_cache() contains:<br>
<br>
if (item->offset != *inout_offset) {<br>
brw->state.dirty.brw |= (1 << cache_id);<br>
*inout_offset = item->offset;<br>
}<br>
<br>
(1 << cache_id) corresponds to the BRW_NEW_*_PROG_DATA dirty bits<br>
(formerly known as CACHE_NEW_*_PROG).<br>
<br>
Looking at a call of brw_search_cache, we see that inout_offset<br>
corresponds to brw->wm.base.prog_offset:<br>
<br>
if (!brw_search_cache(&brw->cache, BRW_CACHE_FS_PROG,<br>
&key, sizeof(key),<br>
&brw->wm.base.prog_offset, &brw->wm.prog_data)) {<br>
<br>
So, if brw->wm.base.prog_offset changes, we flag BRW_NEW_FS_PROG_DATA.<br>
In other words, whenever we search the cache, if we select a different<br>
cache entry than we were using on the previous draw, we flag<br>
BRW_NEW_*_PROG_DATA.<br>
<br>
I explained the difference between the two dirty bits in brw_context.h:<br>
<br>
/**<br>
* BRW_NEW_*_PROG_DATA and BRW_NEW_*_PROGRAM are similar, but distinct.<br>
*<br>
* BRW_NEW_*_PROGRAM relates to the gl_shader_program/gl_program structures.<br>
* When the currently bound shader program differs from the previous draw<br>
* call, these will be flagged. They cover brw->{stage}_program and<br>
* ctx->{Stage}Program->_Current.<br>
*<br>
* BRW_NEW_*_PROG_DATA is flagged when the effective shaders change, from a<br>
* driver perspective. Even if the same shader is bound at the API level,<br>
* we may need to switch between multiple versions of that shader to handle<br>
* changes in non-orthagonal state.<br>
*<br>
* Additionally, multiple shader programs may have identical vertex shaders<br>
* (for example), or compile down to the same code in the backend. We combine<br>
* those into a single program cache entry.<br>
*<br>
* BRW_NEW_*_PROG_DATA occurs when switching program cache entries, which<br>
* covers the brw_*_prog_data structures, and brw->*.prog_offset.<br>
*/<br>
<br>
Here, the problem was that brw_upload_texture_surfaces was referring to<br>
brw->wm.base.prog_data, but not listening to BRW_NEW_FS_PROG_DATA.<br>
I've sent a patch to fix that (and other similar failures in the area).<br>
<br>
I've been slowly migrating the state upload code to only use brw_*_prog_data<br>
and BRW_NEW_*_PROG_DATA, and stop looking at the Mesa program structures<br>
(covered by BRW_NEW_{VERTEX,GEOMETRY,FRAGMENT}_PROGRAM). In some cases we<br>
just refer to the core Mesa structures when the value doesn't change between<br>
NOS specializations (i.e. does the program ever read gl_FragCoord). I'd<br>
like to stop doing that - looking at the effective program makes more sense,<br>
simplifies the code, and makes it harder to botch things like this.<br>
<br>
I've got a few more patches toward that end.<br>
<div class="HOEnZb"><div class="h5"><br>
> There is a test in brw_upload_state() that checks for changes-<br>
><br>
> if (brw->fragment_program != ctx->FragmentProgram._Current) {<br>
> brw->fragment_program = ctx->FragmentProgram._Current;<br>
> brw->state.dirty.brw |= BRW_NEW_FRAGMENT_PROGRAM;<br>
> }<br>
><br>
> But that test is not looking for changes to NOS in the cache key.<br>
> It only sees more direct changes to the fragment program.<br>
><br>
> Setting BRW_NEW_FRAGMENT_PROGRAM in brw_upload_wm_prog() fixes the<br>
> particular program that I was debuggging and the piglit test I created.<br>
> But I wonder how many other cases occur. There are six other callers<br>
> of brw_search_cache() that may not be getting the right dirty bits<br>
> set when cache key changes.</div></div></blockquote></div><br><br clear="all"><br>-- <br><div class="gmail_signature"><br> Mike Stroyan - Software Architect<br> LunarG, Inc. - The Graphics Experts<br> Cell: (970) 219-7905<br> Email: Mike@LunarG.com<br> Website: <a href="http://www.lunarg.com">http://www.lunarg.com</a></div>
</div>