<div dir="ltr">On 23 March 2013 10:59, Kenneth Graunke <<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>> wrote: <div class="gmail_extra"><div class="gmail_quote"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>On 03/21/2013 05:40 PM, Paul Berry wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> This patch modifies post-GS pipeline stages (transform feedback, clip, sf, fs) to refer to the VUE map through brw->vue_map_geom_out rather than brw->vs.prog_data->vue_map. This ensures that when geometry shader support is added, these pipeline stages will consult the geometry shader output VUE map when appropriate, rather than the vertex shader output VUE map. --- src/mesa/drivers/dri/i965/brw_clip.c | 7 +++---- src/mesa/drivers/dri/i965/brw_sf.c | 7 +++---- src/mesa/drivers/dri/i965/brw_state.h | 2 +- src/mesa/drivers/dri/i965/brw_wm.c | 6 +++--- src/mesa/drivers/dri/i965/gen6_sf_state.c | 10 +++++----- src/mesa/drivers/dri/i965/gen7_sf_state.c | 8 ++++---- src/mesa/drivers/dri/i965/gen7_sol_state.c | 14 +++++++------- 7 files changed, 26 insertions(+), 28 deletions(-) </blockquote> </div> I heartily approve of this patch :) It really untangles the mess of VS dependencies. Some comments below...<div> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> diff --git a/src/mesa/drivers/dri/i965/brw_clip.c b/src/mesa/drivers/dri/i965/brw_clip.c index e20f7c2..fa7e85d 100644 --- a/src/mesa/drivers/dri/i965/brw_clip.c +++ b/src/mesa/drivers/dri/i965/brw_clip.c @@ -69,7 +69,7 @@ static void compile_clip_prog( struct brw_context *brw, c.func.single_program_flow = 1; c.key = *key; </blockquote> > - c.vue_map = brw->vs.prog_data->vue_map; <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + c.vue_map = brw->vue_map_geom_out; /* nr_regs is the number of registers filled by reading data from the VUE. * This program accesses the entire VUE, so nr_regs needs to be the size of @@ -146,7 +146,7 @@ brw_upload_clip_prog(struct brw_context *brw) /* BRW_NEW_REDUCED_PRIMITIVE */ key.primitive = brw->intel.reduced_primitive; /* CACHE_NEW_VS_PROG (also part of VUE map) */ - key.attrs = brw->vs.prog_data->vue_map.slots_valid; + key.attrs = brw->vue_map_geom_out.slots_valid; </blockquote> </div> This comment is now wrong. It should be /* BRW_NEW_VUE_MAP_GEOM_OUT */ not /* CACHE_NEW_VS_PROG */.</blockquote><div> </div><div>Oops, you're right. I'll fix before pushing. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> /* _NEW_LIGHT */ key.do_flat_shading = (ctx->Light.ShadeModel == GL_FLAT); key.pv_first = (ctx->Light.ProvokingVertex == GL_FIRST_VERTEX_CONVENTION); @@ -258,8 +258,7 @@ const struct brw_tracked_state brw_clip_prog = { _NEW_TRANSFORM | _NEW_POLYGON | _NEW_BUFFERS), - .brw = (BRW_NEW_REDUCED_PRIMITIVE), - .cache = CACHE_NEW_VS_PROG </blockquote> </div> ...and I think this was actually the last use of CACHE_NEW_VS_PROG, so you can probably eliminate that.</blockquote><div> </div><div>Actually I did (note the "-" at the beginning of the line above). </div> <div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + .brw = (BRW_NEW_REDUCED_PRIMITIVE | BRW_NEW_VUE_MAP_GEOM_OUT) }, .emit = brw_upload_clip_prog }; diff --git a/src/mesa/drivers/dri/i965/brw_sf.c b/src/mesa/drivers/dri/i965/brw_sf.c index c8b7033..fc36961 100644 --- a/src/mesa/drivers/dri/i965/brw_sf.c +++ b/src/mesa/drivers/dri/i965/brw_sf.c @@ -65,7 +65,7 @@ static void compile_sf_prog( struct brw_context *brw, brw_init_compile(brw, &c.func, mem_ctx); c.key = *key; - c.vue_map = brw->vs.prog_data->vue_map; + c.vue_map = brw->vue_map_geom_out; if (c.key.do_point_coord) { /* * gl_PointCoord is a FS instead of VS builtin variable, thus it's @@ -145,7 +145,7 @@ brw_upload_sf_prog(struct brw_context *brw) /* Populate the key, noting state dependencies: */ /* CACHE_NEW_VS_PROG */ - key.attrs = brw->vs.prog_data->vue_map.slots_valid; </blockquote> </div> Ditto (wrong comment)</blockquote><div> </div><div>Fixed. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + key.attrs = brw->vue_map_geom_out.slots_valid; /* BRW_NEW_REDUCED_PRIMITIVE */ switch (brw->intel.reduced_primitive) { @@ -216,8 +216,7 @@ const struct brw_tracked_state brw_sf_prog = { .dirty = { .mesa = (_NEW_HINT | _NEW_LIGHT | _NEW_POLYGON | _NEW_POINT | _NEW_TRANSFORM | _NEW_BUFFERS | _NEW_PROGRAM), - .brw = (BRW_NEW_REDUCED_PRIMITIVE), - .cache = CACHE_NEW_VS_PROG </blockquote> </div> Ditto (CACHE_NEW_VS_PROG doesn't appear necessary)</blockquote><div> </div><div>Already deleted this one too. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + .brw = (BRW_NEW_REDUCED_PRIMITIVE | BRW_NEW_VUE_MAP_GEOM_OUT) }, .emit = brw_upload_sf_prog }; diff --git a/src/mesa/drivers/dri/i965/brw_state.h b/src/mesa/drivers/dri/i965/brw_state.h index 02ce57b..1f5e18a 100644 --- a/src/mesa/drivers/dri/i965/brw_state.h +++ b/src/mesa/drivers/dri/i965/brw_state.h @@ -227,7 +227,7 @@ void upload_default_color(struct brw_context *brw, /* gen6_sf_state.c */ uint32_t -get_attr_override(struct brw_vue_map *vue_map, int urb_entry_read_offset, +get_attr_override(const struct brw_vue_map *vue_map, int urb_entry_read_offset, int fs_attr, bool two_side_color, uint32_t *max_source_attr); #ifdef __cplusplus diff --git a/src/mesa/drivers/dri/i965/brw_wm.c b/src/mesa/drivers/dri/i965/brw_wm.c index e7e9ddc..6053f94 100644 --- a/src/mesa/drivers/dri/i965/brw_wm.c +++ b/src/mesa/drivers/dri/i965/brw_wm.c @@ -481,7 +481,7 @@ static void brw_wm_populate_key( struct brw_context *brw, /* CACHE_NEW_VS_PROG */ if (intel->gen < 6) - key->vp_outputs_written = brw->vs.prog_data->vue_map.slots_valid; + key->vp_outputs_written = brw->vue_map_geom_out.slots_valid; </blockquote> </div> Ditto (wrong comment)</blockquote><div> </div><div>Fixed. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> /* The unique fragment program ID */ key->program_string_id = fp->id; @@ -524,8 +524,8 @@ const struct brw_tracked_state brw_wm_prog = { _NEW_MULTISAMPLE), .brw = (BRW_NEW_FRAGMENT_PROGRAM | BRW_NEW_WM_INPUT_DIMENSIONS | - BRW_NEW_REDUCED_PRIMITIVE), - .cache = CACHE_NEW_VS_PROG, + BRW_NEW_REDUCED_PRIMITIVE | + BRW_NEW_VUE_MAP_GEOM_OUT) </blockquote> </div> Hey, you caught it in the rest of the cases :) I would like to see this series run through Piglit on pre-Gen6 just to be sure we haven't broken anything. I can do that, as I'm setting up my Ironlake for other reasons anyway...</blockquote><div> </div><div> I wouldn't say no to additional testing, but FWIW I have tested this series on Gen5, and there were no regressions :) </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> }, .emit = brw_upload_wm_prog }; diff --git a/src/mesa/drivers/dri/i965/gen6_sf_state.c b/src/mesa/drivers/dri/i965/gen6_sf_state.c index 7fe1dca..4ee7f56 100644 --- a/src/mesa/drivers/dri/i965/gen6_sf_state.c +++ b/src/mesa/drivers/dri/i965/gen6_sf_state.c @@ -53,7 +53,7 @@ * 256-bit increments. */ uint32_t -get_attr_override(struct brw_vue_map *vue_map, int urb_entry_read_offset, +get_attr_override(const struct brw_vue_map *vue_map, int urb_entry_read_offset, int fs_attr, bool two_side_color, uint32_t *max_source_attr) { if (fs_attr == VARYING_SLOT_POS) { @@ -312,9 +312,9 @@ upload_sf_state(struct brw_context *brw) */ assert(input_index < 16 || attr == input_index); - /* CACHE_NEW_VS_PROG | _NEW_LIGHT | _NEW_PROGRAM */ + /* BRW_NEW_VUE_MAP_GEOM_OUT | _NEW_LIGHT | _NEW_PROGRAM */ attr_overrides[input_index++] = - get_attr_override(&brw->vs.prog_data->vue_map, + get_attr_override(&brw->vue_map_geom_out, urb_entry_read_offset, attr, ctx->VertexProgram._TwoSideEnabled, &max_source_attr); @@ -370,8 +370,8 @@ const struct brw_tracked_state gen6_sf_state = { _NEW_POINT | _NEW_MULTISAMPLE), .brw = (BRW_NEW_CONTEXT | - BRW_NEW_FRAGMENT_PROGRAM), - .cache = CACHE_NEW_VS_PROG + BRW_NEW_FRAGMENT_PROGRAM | + BRW_NEW_VUE_MAP_GEOM_OUT) }, .emit = upload_sf_state, }; diff --git a/src/mesa/drivers/dri/i965/gen7_sf_state.c b/src/mesa/drivers/dri/i965/gen7_sf_state.c index 86809a1..5ebe6f2 100644 --- a/src/mesa/drivers/dri/i965/gen7_sf_state.c +++ b/src/mesa/drivers/dri/i965/gen7_sf_state.c @@ -100,9 +100,9 @@ upload_sbe_state(struct brw_context *brw) */ assert(input_index < 16 || attr == input_index); - /* CACHE_NEW_VS_PROG | _NEW_LIGHT | _NEW_PROGRAM */ + /* BRW_NEW_VUE_MAP_GEOM_OUT | _NEW_LIGHT | _NEW_PROGRAM */ attr_overrides[input_index++] = - get_attr_override(&brw->vs.prog_data->vue_map, + get_attr_override(&brw->vue_map_geom_out, urb_entry_read_offset, attr, ctx->VertexProgram._TwoSideEnabled, &max_source_attr); @@ -149,8 +149,8 @@ const struct brw_tracked_state gen7_sbe_state = { _NEW_POINT | _NEW_PROGRAM), .brw = (BRW_NEW_CONTEXT | - BRW_NEW_FRAGMENT_PROGRAM), - .cache = CACHE_NEW_VS_PROG + BRW_NEW_FRAGMENT_PROGRAM | + BRW_NEW_VUE_MAP_GEOM_OUT) }, .emit = upload_sbe_state, }; diff --git a/src/mesa/drivers/dri/i965/gen7_sol_state.c b/src/mesa/drivers/dri/i965/gen7_sol_state.c index b55fccc..1c71d0f 100644 --- a/src/mesa/drivers/dri/i965/gen7_sol_state.c +++ b/src/mesa/drivers/dri/i965/gen7_sol_state.c @@ -107,7 +107,7 @@ upload_3dstate_so_buffers(struct brw_context *brw) */ static void upload_3dstate_so_decl_list(struct brw_context *brw, - struct brw_vue_map *vue_map) + const struct brw_vue_map *vue_map) </blockquote> </div></div> Good call on the const.<div><div> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> { struct intel_context *intel = &brw->intel; struct gl_context *ctx = &intel->ctx; @@ -183,7 +183,7 @@ upload_3dstate_so_decl_list(struct brw_context *brw, static void upload_3dstate_streamout(struct brw_context *brw, bool active, - struct brw_vue_map *vue_map) + const struct brw_vue_map *vue_map) { struct intel_context *intel = &brw->intel; struct gl_context *ctx = &intel->ctx; @@ -241,8 +241,8 @@ upload_sol_state(struct brw_context *brw) if (active) { upload_3dstate_so_buffers(brw); - /* CACHE_NEW_VS_PROG */ - upload_3dstate_so_decl_list(brw, &brw->vs.prog_data->vue_map); + /* BRW_NEW_VUE_MAP_GEOM_OUT */ + upload_3dstate_so_decl_list(brw, &brw->vue_map_geom_out); intel->batch.needs_sol_reset = true; } @@ -252,7 +252,7 @@ upload_sol_state(struct brw_context *brw) * MMIO register updates (current performed by the kernel at each batch * emit). */ - upload_3dstate_streamout(brw, active, &brw->vs.prog_data->vue_map); + upload_3dstate_streamout(brw, active, &brw->vue_map_geom_out); } const struct brw_tracked_state gen7_sol_state = { @@ -261,8 +261,8 @@ const struct brw_tracked_state gen7_sol_state = { _NEW_LIGHT | _NEW_TRANSFORM_FEEDBACK), .brw = (BRW_NEW_BATCH | - BRW_NEW_VERTEX_PROGRAM), - .cache = CACHE_NEW_VS_PROG, + BRW_NEW_VERTEX_PROGRAM | + BRW_NEW_VUE_MAP_GEOM_OUT) }, .emit = upload_sol_state, }; </blockquote> </div></div></blockquote></div> </div></div>