Reviewed-by: Chris Forbes <chr...@ijw.co.nz>
On Sat, Jun 27, 2015 at 11:31 AM, Kenneth Graunke <kenn...@whitecape.org> wrote:
> On Friday, June 26, 2015 04:17:39 PM Jason Ekstrand wrote:
>> On Fri, Jun 26, 2015 at 3:56 PM, Kenneth Graunke <kenn...@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 <kenn...@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.ekstr...@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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
