[Mesa-dev] [PATCH] i965: Set dirty bit for NOS fragment shader change

[Kenneth Graunke]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141222/010e0b2f/attachment.sig>