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

[Kenneth Graunke]

On Friday, June 26, 2015 04:17:39 PM Jason Ekstrand wrote:
> 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>

I did it that way because compute_clip_distances() already prods at
brw_vue_prog_key, and run_vs() currently doesn't.  I would have had
to introduce key casts there.  I felt the unconditional call kept
run_vs() less cluttered, too.

Either way would work.
-------------- 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/20150626/35b9b62a/attachment-0001.sig>