[Mesa-dev] [PATCH] i965/vs: Move compute_clip_distance() out of emit_urb_writes().

[Jason Ekstrand]

On Fri, Jun 26, 2015 at 3:56 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> Legacy user clipping (using gl_Position or gl_ClipVertex) is handled by
> turning those into the modern gl_ClipDistance equivalents.
>
> This is unnecessary in Core Profile: if user clipping is enabled, but
> the shader doesn't write the corresponding gl_ClipDistance entry,
> results are undefined.  Hence, it is also unnecessary for geometry
> shaders.
>
> This patch moves the call up to run_vs().  This is equivalent for VS,
> but removes the need to pass clip distances into emit_urb_writes().
>
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/mesa/drivers/dri/i965/brw_fs.cpp         |    4 +++-
>  src/mesa/drivers/dri/i965/brw_fs.h           |    2 +-
>  src/mesa/drivers/dri/i965/brw_fs_visitor.cpp |   16 +++++++++++-----
>  3 files changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
> index 4292aa6..8658554 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> @@ -3816,7 +3816,9 @@ fs_visitor::run_vs(gl_clip_plane *clip_planes)
>     if (failed)
>        return false;
>
> -   emit_urb_writes(clip_planes);
> +   compute_clip_distance(clip_planes);
> +
> +   emit_urb_writes();
>
>     if (shader_time_index >= 0)
>        emit_shader_time_end();
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h b/src/mesa/drivers/dri/i965/brw_fs.h
> index 243baf6..d08d438 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.h
> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
> @@ -271,7 +271,7 @@ public:
>                                   fs_reg src0_alpha, unsigned components,
>                                   unsigned exec_size, bool use_2nd_half = false);
>     void emit_fb_writes();
> -   void emit_urb_writes(gl_clip_plane *clip_planes);
> +   void emit_urb_writes();
>     void emit_cs_terminate();
>
>     void emit_barrier();
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> index 7074b5c..854e49b 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> @@ -1730,6 +1730,12 @@ fs_visitor::setup_uniform_clipplane_values(gl_clip_plane *clip_planes)
>     }
>  }
>
> +/**
> + * Lower legacy fixed-function and gl_ClipVertex clipping to clip distances.
> + *
> + * This does nothing if the shader uses gl_ClipDistance or user clipping is
> + * disabled altogether.
> + */
>  void fs_visitor::compute_clip_distance(gl_clip_plane *clip_planes)
>  {
>     struct brw_vue_prog_data *vue_prog_data =
> @@ -1737,6 +1743,10 @@ void fs_visitor::compute_clip_distance(gl_clip_plane *clip_planes)
>     const struct brw_vue_prog_key *key =
>        (const struct brw_vue_prog_key *) this->key;
>
> +   /* Bail unless some sort of legacy clipping is enabled */
> +   if (!key->userclip_active || prog->UsesClipDistanceOut)
> +      return;
> +

Any reason why you changed this from a conditional call to
compute_clip_distance to an early return?  I don't know that I care
much either way.

Thanks for making this less gross.

Reviewed-by: Jason Ekstrand <jason.ekstrand at intel.com>

>     /* From the GLSL 1.30 spec, section 7.1 (Vertex Shader Special Variables):
>      *
>      *     "If a linked set of shaders forming the vertex stage contains no
> @@ -1780,7 +1790,7 @@ void fs_visitor::compute_clip_distance(gl_clip_plane *clip_planes)
>  }
>
>  void
> -fs_visitor::emit_urb_writes(gl_clip_plane *clip_planes)
> +fs_visitor::emit_urb_writes()
>  {
>     int slot, urb_offset, length;
>     struct brw_vs_prog_data *vs_prog_data =
> @@ -1793,10 +1803,6 @@ fs_visitor::emit_urb_writes(gl_clip_plane *clip_planes)
>     bool flush;
>     fs_reg sources[8];
>
> -   /* Lower legacy ff and ClipVertex clipping to clip distances */
> -   if (key->base.userclip_active && !prog->UsesClipDistanceOut)
> -      compute_clip_distance(clip_planes);
> -
>     /* If we don't have any valid slots to write, just do a minimal urb write
>      * send to terminate the shader. */
>     if (vue_map->slots_valid == 0) {
> --
> 1.7.10.4
>