On Fri, 2 Sep 2011 17:40:39 -0700, Paul Berry <stereotype...@gmail.com> wrote:
Non-text part: multipart/alternative
> On 2 September 2011 15:46, Paul Berry <stereotype...@gmail.com> wrote:
>
> > On 2 September 2011 13:13, Eric Anholt <e...@anholt.net> wrote:
> >
> >> On Fri, 2 Sep 2011 09:06:48 -0700, Paul Berry <stereotype...@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 :)
pgpHK7Ojd4G7I.pgp
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
