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