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

[Paul Berry]

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.
       */

...Unless you can suggest a better phrasing :)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20110902/21ddbf72/attachment.htm>