[Mesa-dev] Fwd: [PATCH 09/36] i965: new VS: move clip distance computation (GEN5+) to a separate function.

[Paul Berry]

Whoops, forgot to copy the list on this reply.

---------- Forwarded message ----------
From: Paul Berry <stereotype441 at gmail.com>
Date: 2 September 2011 15:46
Subject: Re: [PATCH 09/36] i965: new VS: move clip distance computation
(GEN5+) to a separate function.
To: Eric Anholt <eric at anholt.net>


On 2 September 2011 13:13, Eric Anholt <eric at anholt.net> wrote:

> On Fri,  2 Sep 2011 09:06:48 -0700, Paul Berry <stereotype441 at gmail.com>
> wrote:
> > ---
> >  src/mesa/drivers/dri/i965/brw_vec4.h           |    1 +
> >  src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp |   32
> ++++++++++++++---------
> >  2 files changed, 20 insertions(+), 13 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_vec4.h
> b/src/mesa/drivers/dri/i965/brw_vec4.h
> > index 8c613bd..01313ec 100644
> > --- a/src/mesa/drivers/dri/i965/brw_vec4.h
> > +++ b/src/mesa/drivers/dri/i965/brw_vec4.h
> > @@ -465,6 +465,7 @@ public:
> >
> >     void emit_ndc_computation();
> >     void emit_psiz_and_flags(struct brw_reg reg);
> > +   void emit_clip_distances(struct brw_reg reg, int offset);
> >     int emit_vue_header_gen6(int header_mrf);
> >     int emit_vue_header_gen4(int header_mrf);
> >     void emit_urb_writes(void);
> > diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> > index bd8878a..374cf8a 100644
> > --- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> > @@ -1789,6 +1789,21 @@ vec4_visitor::emit_psiz_and_flags(struct brw_reg
> reg)
> >     }
> >  }
> >
> > +void
> > +vec4_visitor::emit_clip_distances(struct brw_reg reg, int offset)
> > +{
> > +   if (intel->gen < 6)
> > +      /* Clip distance slots are set aside in gen5 because the hardware
> > +       * requires them to be, but they are not used. */
> > +      return;
>
> Style consistency nits for the day: I like to see braces for if
> statements with a multi-line then case.  Also, before your patch series
> there were only 15 instances of cuddling the "*/" onto the last line of
> a multi-line comment in our driver.
>

Ah, yes. I figured I wouldn't be able to write 36 patches without failing to
follow style conventions somewhere.  Fortunately for me, these are
conventions that I (a) didn't know about, and (b) don't mind following.
I'll fix this and update docs/devinfo.html accordingly.


>
> And now for some actual review: The hardware doesn't actually require
> the clip distance slots on gen5, we just fail to set up the hardware to
> not use them.  I made a patch series at one point to do that, but given
> that I couldn't measure a performance difference and I was already
> living in fear of our VUE setup, I never pushed it.
>

Yeah, I kind of suspected something like that was the case.  I'll rewrite
the comment to clarify this.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20110902/51c434ba/attachment.html>