[Mesa-dev] [PATCH 08/15] i965/gen6+: Remove VUE map dependency on userclip_active.

[Paul Berry]

On 9 September 2013 13:24, Ian Romanick <idr at freedesktop.org> wrote:

> On 09/03/2013 06:18 PM, Paul Berry wrote:
> > Previously, on Gen6+, we laid out the vertex (or geometry) shader VUE
> > map differently depending whether user clipping was active.  If it was
> > active, we put the clip distances in slots 2 and 3 (where the clipper
> > expects them); if it was inactive, we assigned them in the order of
> > the gl_varying_slot enum.
> >
> > This made for unnecessary recompiles, since turning clipping on/off
> > for a shader that used gl_ClipDistance might rearrange the varyings.
> > It also required extra bookkeeping, since it required the user
> > clipping flag to be provided to brw_compute_vue_map() as a parameter.
> >
> > With this patch, we always put clip distances at in slots 2 and 3 if
> > they are written to.  do_vs_prog() and do_gs_prog() are responsible
> > for ensuring that clip distances are written to when user clipping is
> > enabled (as do_vs_prog() previously did for gen4-5).
> >
> > This makes the only input to brw_compute_vue_map() a bitfield of which
> > varyings the shader writes to, a fact that we'll take advantage of in
> > forthcoming patches.
> > ---
> >  src/mesa/drivers/dri/i965/brw_context.h |  2 +-
> >  src/mesa/drivers/dri/i965/brw_vec4_gs.c | 15 ++++++++++++---
> >  src/mesa/drivers/dri/i965/brw_vs.c      | 26 +++++++++++++-------------
> >  3 files changed, 26 insertions(+), 17 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_context.h
> b/src/mesa/drivers/dri/i965/brw_context.h
> > index 167ed4a..0c1fd9e 100644
> > --- a/src/mesa/drivers/dri/i965/brw_context.h
> > +++ b/src/mesa/drivers/dri/i965/brw_context.h
> > @@ -436,7 +436,7 @@ static inline GLuint brw_varying_to_offset(struct
> brw_vue_map *vue_map,
> >  }
> >
> >  void brw_compute_vue_map(struct brw_context *brw, struct brw_vue_map
> *vue_map,
> > -                         GLbitfield64 slots_valid, bool
> userclip_active);
> > +                         GLbitfield64 slots_valid);
> >
> >
> >  /**
> > diff --git a/src/mesa/drivers/dri/i965/brw_vec4_gs.c
> b/src/mesa/drivers/dri/i965/brw_vec4_gs.c
> > index 7ab03ac..94c4017 100644
> > --- a/src/mesa/drivers/dri/i965/brw_vec4_gs.c
> > +++ b/src/mesa/drivers/dri/i965/brw_vec4_gs.c
> > @@ -62,9 +62,18 @@ do_gs_prog(struct brw_context *brw,
> >     c.prog_data.base.param = rzalloc_array(NULL, const float *,
> param_count);
> >     c.prog_data.base.pull_param = rzalloc_array(NULL, const float *,
> param_count);
> >
> > -   brw_compute_vue_map(brw, &c.prog_data.base.vue_map,
> > -                       gp->program.Base.OutputsWritten,
> > -                       c.key.base.userclip_active);
> > +   GLbitfield64 outputs_written = gp->program.Base.OutputsWritten;
> > +
> > +   /* In order for legacy clipping to work, we need to populate the clip
> > +    * distance varying slots whenever clipping is enabled, even if the
> vertex
> > +    * shader doesn't write to gl_ClipDistance.
> > +    */
> > +   if (c.key.base.userclip_active) {
> > +      outputs_written |= BITFIELD64_BIT(VARYING_SLOT_CLIP_DIST0);
> > +      outputs_written |= BITFIELD64_BIT(VARYING_SLOT_CLIP_DIST1);
> > +   }
> > +
> > +   brw_compute_vue_map(brw, &c.prog_data.base.vue_map, outputs_written);
>
> Having both of the callers of brw_compute_vue_map do this same dance
> before calling seems weird.  I want to understand this better... The VS
> and GS code adds clip distance as a written output because fixed
> function hardware is going to read it when user clipping is enabled.
> This then gets captured in the vue_map... so when the next patch adds
> another caller to brw_compute_vue_map, it doesn't need to know that user
> clipping was enabled (vs. gl_ClipDistance being written).  Yeah?
>

Correct.  The geometry shader doesn't care why the VS chose to include clip
distance in the VUE map.  It just needs to know what's in the VUE map so
that it knows where to look for each of its inputs.  Effectively the call
to brw_compute_vue_map that is added in patch 9 is just rebuilding an
already-known VUE map so that we don't have to waste space in the GS
program key storing the whole map.

I agree with you that the code duplication isn't great (in fact there's a
lot of code duplication between do_gs_prog and do_vs_prog, and that doesn't
thrill me), but I think the benefit in this case is that it makes for a
clean separation of concerns between deciding what varyings need to be in
the VUE (which is the job of do_{vs,gs}_prog) and deciding what order they
should go in (which is the job of brw_compute_vue_map).  We might, for
instance, decide in the future that as an optimization, do_vs_prog will
only add clip distances to the VUE map if user clipping is enabled AND
there's no GS.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130910/526d133c/attachment-0001.html>