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

[Mike Stroyan]

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 <kenneth at 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: Mike at LunarG.com
 Website: http://www.lunarg.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141223/5b1bee91/attachment-0001.html>