On Thursday, February 12, 2015 05:38:09 PM Carl Worth wrote: > Across the board of the various generations, the intial few atoms in > all of the atom lists are basically the same, (performing uploads for > the various programs). The only difference is that prior to gen6 > there's an ff_gs upload in place of the later gs upload. > > In this commit, instead of using the atom lists for this program state > upload, we add a new function brw_upload_programs that performs the > same state checks and calls the various upload functions as necessary. > > This commit is intended to have no functional change. The motivation > is that future code, (such as the shader cache), wants to have a > single function within which to perform various operations before and > after program upload, (with some local variables holding state across > the upload). > --- > > In this revision of the patch, I've changed things so that the lists > of flags remain in the same files they were in before, (near the code > performing the operations that the flags control). So I think this > aspect is a bit cleaner. > > I'm still not a huge fan of the resulting conditional expressions, > (and the number of parentheses involved in them). It would not be too > hard to imagine one of those getting edited incorrectly. > > So feedback would be most welcome on improving those. > > Maybe: > > int dirty = 0; > > dirty |= (brw->state.dirty.mesa & (FLAG | FLAG2 | FLAG3); > dirty |= (brw->state.dirty.brw & (BRW_FLAG | BRW_FLAG2); > > if (dirty == 0) > return; > > ? > > src/mesa/drivers/dri/i965/brw_ff_gs.c | 21 +++++++------ > src/mesa/drivers/dri/i965/brw_ff_gs.h | 3 ++ > src/mesa/drivers/dri/i965/brw_gs.c | 23 ++++++-------- > src/mesa/drivers/dri/i965/brw_gs.h | 5 ++++ > src/mesa/drivers/dri/i965/brw_state_upload.c | 35 ++++++++++++---------- > src/mesa/drivers/dri/i965/brw_vs.c | 32 +++++++++----------- > src/mesa/drivers/dri/i965/brw_vs.h | 3 ++ > src/mesa/drivers/dri/i965/brw_wm.c | 45 > +++++++++++++--------------- > src/mesa/drivers/dri/i965/brw_wm.h | 3 ++ > 9 files changed, 86 insertions(+), 84 deletions(-) > > diff --git a/src/mesa/drivers/dri/i965/brw_ff_gs.c > b/src/mesa/drivers/dri/i965/brw_ff_gs.c > index 653c4b6..fd68aa3 100644 > --- a/src/mesa/drivers/dri/i965/brw_ff_gs.c > +++ b/src/mesa/drivers/dri/i965/brw_ff_gs.c > @@ -221,10 +221,19 @@ static void populate_key(struct brw_context *brw, > > /* Calculate interpolants for triangle and line rasterization. > */ > -static void > +void > brw_upload_ff_gs_prog(struct brw_context *brw) > { > struct brw_ff_gs_prog_key key; > + > + if (((brw->state.dirty.mesa & _NEW_LIGHT) | > + (brw->state.dirty.brw & (BRW_NEW_PRIMITIVE | > + BRW_NEW_TRANSFORM_FEEDBACK | > + BRW_NEW_VS_PROG_DATA))) == 0) > + { > + return; > + } > + > /* Populate the key: > */ > populate_key(brw, &key); > @@ -247,13 +256,3 @@ void gen6_brw_upload_ff_gs_prog(struct brw_context *brw) > { > brw_upload_ff_gs_prog(brw); > } > - > -const struct brw_tracked_state brw_ff_gs_prog = { > - .dirty = { > - .mesa = _NEW_LIGHT, > - .brw = BRW_NEW_PRIMITIVE | > - BRW_NEW_TRANSFORM_FEEDBACK | > - BRW_NEW_VS_PROG_DATA, > - }, > - .emit = brw_upload_ff_gs_prog > -}; > diff --git a/src/mesa/drivers/dri/i965/brw_ff_gs.h > b/src/mesa/drivers/dri/i965/brw_ff_gs.h > index a538948..e4afdab 100644 > --- a/src/mesa/drivers/dri/i965/brw_ff_gs.h > +++ b/src/mesa/drivers/dri/i965/brw_ff_gs.h > @@ -112,4 +112,7 @@ void gen6_sol_program(struct brw_ff_gs_compile *c, > unsigned num_verts, bool check_edge_flag); > void gen6_brw_upload_ff_gs_prog(struct brw_context *brw); > > +void > +brw_upload_ff_gs_prog(struct brw_context *brw); > + > #endif > diff --git a/src/mesa/drivers/dri/i965/brw_gs.c > b/src/mesa/drivers/dri/i965/brw_gs.c > index c7ebe5f..0fa06bb 100644 > --- a/src/mesa/drivers/dri/i965/brw_gs.c > +++ b/src/mesa/drivers/dri/i965/brw_gs.c > @@ -292,8 +292,7 @@ do_gs_prog(struct brw_context *brw, > return true; > } > > - > -static void > +void > brw_upload_gs_prog(struct brw_context *brw) > { > struct gl_context *ctx = &brw->ctx; > @@ -303,6 +302,14 @@ brw_upload_gs_prog(struct brw_context *brw) > struct brw_geometry_program *gp = > (struct brw_geometry_program *) brw->geometry_program; > > + if (((brw->state.dirty.mesa & _NEW_TEXTURE) | > + (brw->state.dirty.brw & (BRW_NEW_GEOMETRY_PROGRAM | > + BRW_NEW_TRANSFORM_FEEDBACK | > + BRW_NEW_VUE_MAP_VS))) == 0) > + { > + return; > + } > + > if (gp == NULL) { > /* No geometry shader. Vertex data just passes straight through. */ > if (brw->state.dirty.brw & BRW_NEW_VUE_MAP_VS) { > @@ -358,18 +365,6 @@ brw_upload_gs_prog(struct brw_context *brw) > } > } > > - > -const struct brw_tracked_state brw_gs_prog = { > - .dirty = { > - .mesa = _NEW_TEXTURE, > - .brw = BRW_NEW_GEOMETRY_PROGRAM | > - BRW_NEW_TRANSFORM_FEEDBACK | > - BRW_NEW_VUE_MAP_VS, > - }, > - .emit = brw_upload_gs_prog > -}; > - > - > bool > brw_gs_precompile(struct gl_context *ctx, > struct gl_shader_program *shader_prog, > diff --git a/src/mesa/drivers/dri/i965/brw_gs.h > b/src/mesa/drivers/dri/i965/brw_gs.h > index 5a15fa9..5f7c437 100644 > --- a/src/mesa/drivers/dri/i965/brw_gs.h > +++ b/src/mesa/drivers/dri/i965/brw_gs.h > @@ -26,6 +26,8 @@ > > #include <stdbool.h> > > +#include "brw_context.h" > + > #ifdef __cplusplus > extern "C" { > #endif > @@ -36,6 +38,9 @@ struct gl_program; > > bool brw_gs_prog_data_compare(const void *a, const void *b); > > +void > +brw_upload_gs_prog(struct brw_context *brw); > + > #ifdef __cplusplus > } /* extern "C" */ > #endif > diff --git a/src/mesa/drivers/dri/i965/brw_state_upload.c > b/src/mesa/drivers/dri/i965/brw_state_upload.c > index 52d96f4..049bdbee 100644 > --- a/src/mesa/drivers/dri/i965/brw_state_upload.c > +++ b/src/mesa/drivers/dri/i965/brw_state_upload.c > @@ -36,17 +36,17 @@ > #include "drivers/common/meta.h" > #include "intel_batchbuffer.h" > #include "intel_buffers.h" > +#include "brw_vs.h" > +#include "brw_ff_gs.h" > +#include "brw_gs.h" > +#include "brw_wm.h" > > static const struct brw_tracked_state *gen4_atoms[] = > { > - &brw_vs_prog, /* must do before GS prog, state base address. */ > - &brw_ff_gs_prog, /* must do before state base address */ > - > &brw_interpolation_map, > > &brw_clip_prog, /* must do before state base address */ > &brw_sf_prog, /* must do before state base address */ > - &brw_wm_prog, /* must do before state base address */ > > /* Once all the programs are done, we know how large urb entry > * sizes need to be and can decide if we need to change the urb > @@ -107,10 +107,6 @@ static const struct brw_tracked_state *gen4_atoms[] = > > static const struct brw_tracked_state *gen6_atoms[] = > { > - &brw_vs_prog, /* must do before state base address */ > - &brw_gs_prog, /* must do before state base address */ > - &brw_wm_prog, /* must do before state base address */ > - > &gen6_clip_vp, > &gen6_sf_vp, > > @@ -180,10 +176,6 @@ static const struct brw_tracked_state *gen6_atoms[] = > > static const struct brw_tracked_state *gen7_atoms[] = > { > - &brw_vs_prog, > - &brw_gs_prog, > - &brw_wm_prog, > - > /* Command packets: */ > > /* must do before binding table pointers, cc state ptrs */ > @@ -256,10 +248,6 @@ static const struct brw_tracked_state *gen7_atoms[] = > > static const struct brw_tracked_state *gen8_atoms[] = > { > - &brw_vs_prog, > - &brw_gs_prog, > - &brw_wm_prog, > - > /* Command packets: */ > &gen8_state_base_address, > > @@ -562,6 +550,19 @@ brw_print_dirty_count(struct dirty_bit_map *bit_map) > } > } > > +static void > +brw_upload_programs(struct brw_context *brw) > +{ > + brw_upload_vs_prog(brw); > + > + if (brw->gen < 6) > + brw_upload_ff_gs_prog(brw); > + else > + brw_upload_gs_prog(brw); > + > + brw_upload_wm_prog(brw); > +} > + > /*********************************************************************** > * Emit all state: > */ > @@ -612,6 +613,8 @@ void brw_upload_state(struct brw_context *brw) > if ((state->mesa | state->brw) == 0) > return; > > + brw_upload_programs(brw); > + > if (unlikely(INTEL_DEBUG)) { > /* Debug version which enforces various sanity checks on the > * state flags which are generated and checked to help ensure > diff --git a/src/mesa/drivers/dri/i965/brw_vs.c > b/src/mesa/drivers/dri/i965/brw_vs.c > index 2d56b74..a7d2988 100644 > --- a/src/mesa/drivers/dri/i965/brw_vs.c > +++ b/src/mesa/drivers/dri/i965/brw_vs.c > @@ -409,8 +409,8 @@ brw_setup_vue_key_clip_info(struct brw_context *brw, > } > } > > - > -static void brw_upload_vs_prog(struct brw_context *brw) > +void > +brw_upload_vs_prog(struct brw_context *brw) > { > struct gl_context *ctx = &brw->ctx; > struct brw_vs_prog_key key; > @@ -420,6 +420,18 @@ static void brw_upload_vs_prog(struct brw_context *brw) > struct gl_program *prog = (struct gl_program *) brw->vertex_program; > int i; > > + if (((brw->state.dirty.mesa & (_NEW_BUFFERS | > + _NEW_LIGHT | > + _NEW_POINT | > + _NEW_POLYGON | > + _NEW_TEXTURE | > + _NEW_TRANSFORM)) | > + (brw->state.dirty.brw & (BRW_NEW_VERTEX_PROGRAM | > + BRW_NEW_VS_ATTRIB_WORKAROUNDS))) == 0) > + { > + return; > + } > + > memset(&key, 0, sizeof(key)); > > /* Just upload the program verbatim for now. Always send it all > @@ -482,22 +494,6 @@ static void brw_upload_vs_prog(struct brw_context *brw) > } > } > > -/* See brw_vs.c: > - */ > -const struct brw_tracked_state brw_vs_prog = { > - .dirty = { > - .mesa = _NEW_BUFFERS | > - _NEW_LIGHT | > - _NEW_POINT | > - _NEW_POLYGON | > - _NEW_TEXTURE | > - _NEW_TRANSFORM, > - .brw = BRW_NEW_VERTEX_PROGRAM | > - BRW_NEW_VS_ATTRIB_WORKAROUNDS, > - }, > - .emit = brw_upload_vs_prog > -}; > - > bool > brw_vs_precompile(struct gl_context *ctx, > struct gl_shader_program *shader_prog, > diff --git a/src/mesa/drivers/dri/i965/brw_vs.h > b/src/mesa/drivers/dri/i965/brw_vs.h > index 93c5389..bad0f07 100644 > --- a/src/mesa/drivers/dri/i965/brw_vs.h > +++ b/src/mesa/drivers/dri/i965/brw_vs.h > @@ -72,6 +72,9 @@ void brw_vs_debug_recompile(struct brw_context *brw, > const struct brw_vs_prog_key *key); > bool brw_vs_prog_data_compare(const void *a, const void *b); > > +void > +brw_upload_vs_prog(struct brw_context *brw); > + > #ifdef __cplusplus > } /* extern "C" */ > > diff --git a/src/mesa/drivers/dri/i965/brw_wm.c > b/src/mesa/drivers/dri/i965/brw_wm.c > index e7939f0..eace7f9 100644 > --- a/src/mesa/drivers/dri/i965/brw_wm.c > +++ b/src/mesa/drivers/dri/i965/brw_wm.c > @@ -582,8 +582,7 @@ static void brw_wm_populate_key( struct brw_context *brw, > key->program_string_id = fp->id; > } > > - > -static void > +void > brw_upload_wm_prog(struct brw_context *brw) > { > struct gl_context *ctx = &brw->ctx; > @@ -591,6 +590,25 @@ brw_upload_wm_prog(struct brw_context *brw) > struct brw_fragment_program *fp = (struct brw_fragment_program *) > brw->fragment_program; >
What about putting this in brw_state.h: static inline state_dirty(struct brw_context *brw, uint32_t mesa_bits, uint64_t brw_bits) { return (brw->state.dirty.mesa & mesa_bits) | (brw->state.dirty.brw & brw_bits); } and then writing this as: if (!state_dirty(brw, _NEW_COLOR | _NEW_DEPTH | _NEW_FRAG_CLAMP | _NEW_HINT | _NEW_LIGHT | _NEW_LINE | _NEW_MULTISAMPLE | _NEW_POLYGON | _NEW_STENCIL | _NEW_TEXTURE, BRW_NEW_FRAGMENT_PROGRAM | BRW_NEW_REDUCED_PRIMITIVE | BRW_NEW_STATS_WM | BRW_NEW_VUE_MAP_GEOM_OUT)) return; I definitely like how the dirty flag list remains close to the code that relies on them - in fact, it's even closer now. With that suggested change, this patch is: Reviewed-by: Kenneth Graunke <kenn...@whitecape.org> I could potentially see us moving to this approach in general - instead of a gen7_atoms list, we could just have a gen7_upload_state() function that calls everything directly, rather than via a function pointer list. This could be more efficient, and writing a function directly might give us a bit more flexibility to pass data between pieces without having to store everything in brw_context awkwardly. I'm not suggesting you do extra work - just saying, I like this :) Jordan may have thoughts as well. > + if (((brw->state.dirty.mesa & (_NEW_BUFFERS | > + _NEW_COLOR | > + _NEW_DEPTH | > + _NEW_FRAG_CLAMP | > + _NEW_HINT | > + _NEW_LIGHT | > + _NEW_LINE | > + _NEW_MULTISAMPLE | > + _NEW_POLYGON | > + _NEW_STENCIL | > + _NEW_TEXTURE)) | > + (brw->state.dirty.brw & (BRW_NEW_FRAGMENT_PROGRAM | > + BRW_NEW_REDUCED_PRIMITIVE | > + BRW_NEW_STATS_WM | > + BRW_NEW_VUE_MAP_GEOM_OUT))) == 0) > + { > + return; > + } > + > brw_wm_populate_key(brw, &key); > > if (!brw_search_cache(&brw->cache, BRW_CACHE_FS_PROG, > @@ -603,26 +621,3 @@ brw_upload_wm_prog(struct brw_context *brw) > } > brw->wm.base.prog_data = &brw->wm.prog_data->base; > } > - > - > -const struct brw_tracked_state brw_wm_prog = { > - .dirty = { > - .mesa = _NEW_BUFFERS | > - _NEW_COLOR | > - _NEW_DEPTH | > - _NEW_FRAG_CLAMP | > - _NEW_HINT | > - _NEW_LIGHT | > - _NEW_LINE | > - _NEW_MULTISAMPLE | > - _NEW_POLYGON | > - _NEW_STENCIL | > - _NEW_TEXTURE, > - .brw = BRW_NEW_FRAGMENT_PROGRAM | > - BRW_NEW_REDUCED_PRIMITIVE | > - BRW_NEW_STATS_WM | > - BRW_NEW_VUE_MAP_GEOM_OUT, > - }, > - .emit = brw_upload_wm_prog > -}; > - > diff --git a/src/mesa/drivers/dri/i965/brw_wm.h > b/src/mesa/drivers/dri/i965/brw_wm.h > index a12c7d4..f54530f 100644 > --- a/src/mesa/drivers/dri/i965/brw_wm.h > +++ b/src/mesa/drivers/dri/i965/brw_wm.h > @@ -83,4 +83,7 @@ void brw_wm_debug_recompile(struct brw_context *brw, > const struct brw_wm_prog_key *key); > bool brw_wm_prog_data_compare(const void *a, const void *b); > > +void > +brw_upload_wm_prog(struct brw_context *brw); > + > #endif >
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