Ah, I see.
I was assuming that a necessary dirty bit had not been set.
But the right dirty bit had been set and was being ignored.
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.
On Mon, Dec 22, 2014 at 8:09 PM, Kenneth Graunke <kenn...@whitecape.org>
wrote:
> 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.
>
> > 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.
>
--
Mike Stroyan - Software Architect
LunarG, Inc. - The Graphics Experts
Cell: (970) 219-7905
Email: m...@lunarg.com
Website: http://www.lunarg.com
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
