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.
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
