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

[Eric Anholt]

On Fri, 2 Sep 2011 17:40:39 -0700, Paul Berry <stereotype441 at gmail.com> wrote:
Non-text part: multipart/alternative
> On 2 September 2011 15:46, Paul Berry <stereotype441 at gmail.com> wrote:
> 
> > 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.
> >
> 
> Hmm, after some further thought about this, I'm kind of bewildered.  The
> hardware docs definitely specify that clip distances belong in those slots,
> but I can't figure out what hardware settings would cause those clip
> distances to be used.  (After all, clipping occurs in a driver-supplied GPU
> program on Gen5, and the GPU program we supply definitely doesn't read from
> those slots).  But I think you made the right decision not to push any
> changes--since there's no measurable performance improvement, and we're not
> anticipating any future changes to Gen5 that would require reworking the VUE
> layout, there's no reason to move things around and risk a regression.
> 
> Considering my level of uncertainty about what actually is going on in the
> hardware, I think I'm not going to try to explain why we are setting aside
> space for clip distance with this somewhat weasely comment:
> 
>       /* Clip distance slots are set aside in gen5, but they are not used.
> It
>        * is not clear whether we actually need to set aside space for them,
>        * but the performance cost is negligible.
>        */

Now I can't find the text on how I thought gen5 worked either, so maybe
I just forgot over time.  That wording reflects what I think now :)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20110902/28f89a96/attachment.pgp>