[Mesa-dev] [PATCH 08/15] i965: Reserve space for "Vertex Count" in GS outputs.

[Kenneth Graunke]

On 12/01/2013 08:14 AM, Paul Berry wrote:
> On 30 November 2013 13:14, Kenneth Graunke <kenneth at whitecape.org
> <mailto:kenneth at whitecape.org>> wrote:
> 
>     On 11/30/2013 10:29 AM, Paul Berry wrote:
>     > On 12 November 2013 17:51, Kenneth Graunke <kenneth at whitecape.org
>     <mailto:kenneth at whitecape.org>
>     > <mailto:kenneth at whitecape.org <mailto:kenneth at whitecape.org>>> wrote:
>     >
>     >     Signed-off-by: Kenneth Graunke <kenneth at whitecape.org
>     <mailto:kenneth at whitecape.org>
>     >     <mailto:kenneth at whitecape.org <mailto:kenneth at whitecape.org>>>
>     >     ---
>     >      src/mesa/drivers/dri/i965/brw_vec4_gs.c | 6 ++++++
>     >      1 file changed, 6 insertions(+)
>     >
>     >     diff --git a/src/mesa/drivers/dri/i965/brw_vec4_gs.c
>     >     b/src/mesa/drivers/dri/i965/brw_vec4_gs.c
>     >     index b52d646..e802c1e 100644
>     >     --- a/src/mesa/drivers/dri/i965/brw_vec4_gs.c
>     >     +++ b/src/mesa/drivers/dri/i965/brw_vec4_gs.c
>     >     @@ -194,6 +194,12 @@ do_gs_prog(struct brw_context *brw,
>     >            c.prog_data.output_vertex_size_hwords * 32 *
>     >     gp->program.VerticesOut;
>     >         output_size_bytes += 32 *
>     >     c.prog_data.control_data_header_size_hwords;
>     >
>     >     +   /* Broadwell stores "Vertex Count" as a full 8 DWord (32 byte)
>     >     URB output,
>     >     +    * which comes before the conttrol header.
>     >     +    */
>     >     +   if (brw->gen >= 8)
>     >     +      output_size_bytes += 32;
>     >     +
>     >         assert(output_size_bytes >= 1);
>     >         if (output_size_bytes > GEN7_MAX_GS_URB_ENTRY_SIZE_BYTES)
>     >            return false;
>     >     --
>     >     1.8.3.2
>     >
>     >
>     > It looks like changes also need to be made to
>     > vec4_gs_visitor::emit_urb_write_opcode() and
>     > vec4_gs_visitor::emit_control_data_bits() to offset the vertex
>     data and
>     > control data by 32 bytes, and to vec4_gs_visitor::emit_thread_end() to
>     > cause the appropriate data to be written to the vertex count.  Are
>     those
>     > changes elsewhere in the series?
> 
>     I handled that in the gen8_vec4_generator class (introduced in patch
>     12):
> 
>     The generate_urb_write() function reserves space:
> 
>     void
>     gen8_vec4_generator::generate_urb_write(vec4_instruction *ir, bool vs)
>     {
>        ...
> 
>        int global_offset = ir->offset;
>        /* The GS needs to increment Global Offset by 1 to make room for
>     the extra
>         * "Vertex Count" payload at the beginning.
>         */
>        if (!vs)
>           ++global_offset;
> 
>        ...
>     }
> 
>     and generate_gs_thread_end() does a SEND with EOT that writes Vertex
>     Count
>     at offset 0.
> 
>     It seems to work pretty well.  Sorry for the awkward split in the
>     patch series.
> 
>     --Ken
> 
> 
> Oh, I see.  I have to admit I'm really not a fan of this approach.  I
> worry that future maintainers are going to make the same assumption that
> I made (that the offset used by vec4_gs_visitor is the actual offset
> that's going to be used by the generated instruction), and they're going
> to get really confused.  It seems to me that it's not very much more
> effort to just use the correct offset in vec4_gs_visitor in the first place.

You're right, it was actually trivial to do it your way, and I do like
ir->offset being the real value.  This was just what I cobbled together
in my mad dash to get GS working. :)

The new version of this code implements your suggestion.  (It's on
origin/broadwell now, but will probably hit the list soon...)