[Mesa-dev] [PATCH 05/36] i965: old VS: use the VUE map to compute the URB entry size.

[Paul Berry]

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

> On Fri,  2 Sep 2011 09:06:44 -0700, Paul Berry <stereotype441 at gmail.com>
> wrote:
> > Previously, the old VS backend computed the URB entry size by adding
> > the number of vertex shader outputs to the size of the URB header.
> > This often produced a larger result than necessary, because some
> > vertex shader outputs are stored in the header, so they were being
> > double counted.  This patch changes the old VS backend to compute the
> > URB entry size directly from the number of slots in the VUE map.
>
> This kind of scares me, if the later stages with similar failure at
> computing the size might be trying to read too much data from a
> now-smaller VUE.  I'm not sure what happens in that case.  (hopefully
> the answer is "read garbage", but what if our vertex happens to be at
> the end of URB space?
>

Yeah, good point.  I don't know what the GPU would do if the vertex were at
the end of URB space and a later stage tried to read beyond the end.
Hopefully, as you say, the answer is "read garbage".  Fortunately, later
patches in the series adjust the later pipeline stages to compute their URB
read sizes based on the VUE map.  So by the time the whole patch series is
applied, we should be fine.  (In fact, we should be in far lower danger of
this kind of problem, because after the whole patch series, all URB sizes
are computed in the same way from the VUE map).

So, depending on how the hardware behaves when reading beyond the end of a
URB entry, there is a minor danger that this patch will introduce a
temporary regression that gets fixed by later patches in the series.  I'm on
the fence as to what to do about it.  On one hand, I could move this patch
to the end of the series and that would close the window for temporary
regressions.  On the other hand, that would mean that the VS-related changes
aren't grouped together anymore.  Advice?


>
> > ---
> >  src/mesa/drivers/dri/i965/brw_vs.h      |    1 -
> >  src/mesa/drivers/dri/i965/brw_vs_emit.c |   31
> +++++++------------------------
> >  2 files changed, 7 insertions(+), 25 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_vs.h
> b/src/mesa/drivers/dri/i965/brw_vs.h
> > index a02c06d..c31869c 100644
> > --- a/src/mesa/drivers/dri/i965/brw_vs.h
> > +++ b/src/mesa/drivers/dri/i965/brw_vs.h
> > @@ -65,7 +65,6 @@ struct brw_vs_compile {
> >
> >     struct brw_vue_map vue_map;
> >     GLuint first_output;
> > -   GLuint nr_outputs;
> >     GLuint last_scratch;
> >
> >     GLuint first_tmp;
> > diff --git a/src/mesa/drivers/dri/i965/brw_vs_emit.c
> b/src/mesa/drivers/dri/i965/brw_vs_emit.c
> > index e7667c8..7c430ce 100644
> > --- a/src/mesa/drivers/dri/i965/brw_vs_emit.c
> > +++ b/src/mesa/drivers/dri/i965/brw_vs_emit.c
> > @@ -402,36 +402,19 @@ static void brw_vs_alloc_regs( struct
> brw_vs_compile *c )
> >     /* The VS VUEs are shared by VF (outputting our inputs) and VS, so
> size
> >      * them to fit the biggest thing they need to.
> >      */
> > -   attributes_in_vue = MAX2(c->nr_outputs, c->nr_inputs);
> > +   attributes_in_vue = MAX2(c->vue_map.num_slots, c->nr_inputs);
>
> There's a subtle change here, that we're no longer counting header regs
> towards the size of the VF input.  Is that appropriate?  I think it is,
> because brw_vs_state.c has vs->thread3.urb_entry_read_offset = 0.
>
> This probably deserves mention in the commit message.
>

Good point.  I will make mention of this in the commit message.

BTW I don't think the *new* VS backend accounts for the number of inputs at
all when computing the URB entry size.  That's probably a bug (I believe it
would manifest itself for cases where there are a lot of vertex attributes
but not many varyings).  I kind of glazed over this detail when making the
patch series because I don't understand how VS inputs work yet.  Do you have
an opinion on this?


>
> > -   /* See emit_vertex_write() for where the VUE's overhead on top of the
> > -    * attributes comes from.
> > -    */
> > -   if (intel->gen >= 7) {
> > -      int header_regs = 2;
> > -      if (c->key.nr_userclip)
> > -      header_regs += 2;
> > -
> > -      /* Each attribute is 16 bytes (1 vec4), so dividing by 4 gives us
> the
> > -       * number of 64-byte (512-bit) units.
> > -       */
> > -      c->prog_data.urb_entry_size = (attributes_in_vue + header_regs +
> 3) / 4;
> > -   } else if (intel->gen == 6) {
> > -      int header_regs = 2;
> > -      if (c->key.nr_userclip)
> > -      header_regs += 2;
> > -
> > -      /* Each attribute is 16 bytes (1 vec4), so dividing by 8 gives us
> the
> > +   if (intel->gen == 6) {
> > +      /* Each attribute is 32 bytes (2 vec4s), so dividing by 8 gives us
> the
> >         * number of 128-byte (1024-bit) units.
> >         */
>
> This comment change here looks wrong to me.  32 bytes * 8 = 256 bytes.
> Same below.  An attribute is 1 vec4, and the urb_entry_size is for one
> vertex's URB entry, even though a register contains 2 vec4s which get
> output to two separate URB entries.
>

Oops, you're right.  This was a total brain failure; the comment was correct
as is.  Good catch.


>
> > -      c->prog_data.urb_entry_size = (attributes_in_vue + header_regs +
> 7) / 8;
> > -   } else if (intel->gen == 5)
> > +      c->prog_data.urb_entry_size = (attributes_in_vue + 7) / 8;
> > +   } else {
> >        /* Each attribute is 16 bytes (1 vec4), so dividing by 4 gives us
> the
> >         * number of 64-byte (512-bit) units.
> >         */
> > -      c->prog_data.urb_entry_size = (attributes_in_vue + 6 + 3) / 4;
> > -   else
> > -      c->prog_data.urb_entry_size = (attributes_in_vue + 2 + 3) / 4;
> > +      c->prog_data.urb_entry_size = (attributes_in_vue + 3) / 4;
> > +   }
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20110902/60969061/attachment-0001.htm>