[Mesa-dev] [PATCH 10/12] i965/gs: Remove the dependency on the VS VUE map.

[Kenneth Graunke]

On Saturday, August 29, 2015 02:24:04 AM Kenneth Graunke wrote:
> Because we only support geometry shaders in core profile, we can safely
> ignore any driver-extending of VS outputs.
> 
> Those are:
> - Legacy userclipping (doesn't exist in core profile)
> - Edgeflag copying (Gen4-5 only, no GS support)
> - Point coord replacement (Gen4-5 only, no GS support)
> - front/back color hacks (Gen4-5 only, no GS support)
> 
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/mesa/drivers/dri/i965/brw_gs.c      | 20 +++++++++++---------
>  src/mesa/drivers/dri/i965/brw_program.h |  2 --
>  2 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_gs.c b/src/mesa/drivers/dri/i965/brw_gs.c
> index 04d9f3f..6cb4263 100644
> --- a/src/mesa/drivers/dri/i965/brw_gs.c
> +++ b/src/mesa/drivers/dri/i965/brw_gs.c
> @@ -242,8 +242,18 @@ brw_codegen_gs_prog(struct brw_context *brw,
>     c.prog_data.output_topology =
>        get_hw_prim_for_gl_prim(gp->program.OutputType);
>  
> +   /* The GLSL linker will have already matched up GS inputs and the outputs
> +    * of prior stages.  The driver does extend VS outputs in some cases, but
> +    * only for legacy OpenGL or Gen4-5 hardware, neither of which offer
> +    * geometry shader support.  So we can safely ignore that.
> +    *
> +    * However, we need to ignore VARYING_SLOT_PRIMITIVE_ID, as it's not
> +    * written by previous stages and shows up via payload magic.
> +    */
> +   GLbitfield64 inputs_read =
> +      gp->program.Base.InputsRead & ~VARYING_BIT_PRIMITIVE_ID;
>     brw_compute_vue_map(brw->intelScreen->devinfo,
> -                       &c.input_vue_map, c.key.input_varyings);
> +                       &c.input_vue_map, inputs_read);

(summarizing a conversation on #intel-gfx...)

So, it turns out this doesn't work with separate shader objects, since
the linker doesn't dead code eliminate varyings.

If the VS has:

   layout(location = 2) out vec3 a;
   layout(location = 4) out vec3 b;
   layout(location = 3) out vec3 c;

and the GS has:

   layout(location = 2) in vec3 a[];
   layout(location = 4) in vec3 b[];

then by only looking at gp->InputsRead and not considering
vp->OutputsWritten, we fail to notice the extra unread variable 'c' at
location 3...in the middle.  The VUE map assigns slots contiguously,
so the VS output VUE map and the GS input VUE map don't match, and we
get bogus results.

So I'm going to have to NAK patch 10 and 12.  I've pushed the rest, as
they seem fine.

Our options at this point seem to be:
- Continue recompiling when the output signature changes (lame)
- Introduce a linker step that dead code eliminates varyings (sketchy)
- Make the VUE map code take locations into account, rather than
  assigning consecutive slots (at least when using SSO).  This would
  introduce holes, possibly wasting URB space, but I doubt SSO apps use
  really sparse locations, so it's probably not a big deal.

I'll have to look into that.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150903/9e0b505e/attachment.sig>