On 23 March 2013 10:59, Kenneth Graunke <kenn...@whitecape.org> wrote:
> On 03/21/2013 05:40 PM, Paul Berry wrote:
>
>> 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(-)
>>
>
> I heartily approve of this patch :) It really untangles the mess of VS
> dependencies.
>
> Some comments below...
>
>
> 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;
>>
> > - c.vue_map = brw->vs.prog_data->vue_map;
>
>> + 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;
>>
>
> This comment is now wrong. It should be
> /* BRW_NEW_VUE_MAP_GEOM_OUT */ not /* CACHE_NEW_VS_PROG */.
Oops, you're right. I'll fix before pushing.
>
> /* _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
>>
>
> ...and I think this was actually the last use of CACHE_NEW_VS_PROG, so you
> can probably eliminate that.
Actually I did (note the "-" at the beginning of the line above).
>
>
> + .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;
>>
>
> Ditto (wrong comment)
Fixed.
>
>
> + 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
>>
>
> Ditto (CACHE_NEW_VS_PROG doesn't appear necessary)
Already deleted this one too.
>
>
> + .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;
>>
>
> Ditto (wrong comment)
Fixed.
>
>
>
>> /* 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)
>>
>
> 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...
I wouldn't say no to additional testing, but FWIW I have tested this series
on Gen5, and there were no regressions :)
>
>
> },
>> .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)
>>
>
> Good call on the const.
>
>
> {
>> 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,
>> };
>>
>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
