[Mesa-dev] [PATCH 07/40] i965/blorp: Use 8k chunk size for urb allocation

[Pohjolainen, Topi]

On Sun, Apr 17, 2016 at 10:46:54PM -0700, Kenneth Graunke wrote:
> On Saturday, April 16, 2016 4:42:35 PM PDT Topi Pohjolainen wrote:
> > Otherwise switch from blorp to compute failes. Note that this now
> > follows the normal i965 upload logic found in gen7_upload_urb().
> > 
> > Effectively vs_size changes from 2 to 1 and vs_start from 2 to 4.
> > 
> > Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> 
> How about this explanation instead?
> 
> Previously, we hardcoded "VS URB Starting Address" to 2 (in 8kB chunks),
> which meant VS URB data would start at an offset of 16kB.
> 
> However, on Haswell GT3 and Gen8+, we allocate the first 32kB for the
> push constant region.  This means that the PS push constant and VS URB
> data regions overlap, which can lead to corruption.
> 
> Cc: mesa-stable at lists.freedesktop.org

That is clearly better, thanks!

> 
> > ---
> >  src/mesa/drivers/dri/i965/gen7_blorp.cpp | 19 ++++++++++++++-----
> >  1 file changed, 14 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/gen7_blorp.cpp b/src/mesa/drivers/
> dri/i965/gen7_blorp.cpp
> > index 4debeb3..c1491ae 100644
> > --- a/src/mesa/drivers/dri/i965/gen7_blorp.cpp
> > +++ b/src/mesa/drivers/dri/i965/gen7_blorp.cpp
> > @@ -47,8 +47,17 @@
> >  static void
> >  gen7_blorp_emit_urb_config(struct brw_context *brw)
> >  {
> > +   /* URB allocations must be done in 8k chunks. */
> > +   const unsigned chunk_size_bytes = 8192;
> >     const unsigned urb_size =
> >        (brw->gen >= 8 || (brw->is_haswell && brw->gt == 3)) ? 32 : 16;
> > +   const unsigned push_constant_bytes = 1024 * urb_size;
> > +   const unsigned push_constant_chunks =
> > +      push_constant_bytes / chunk_size_bytes;
> > +   const unsigned vs_size = 1;
> 
> Changing vs_size from 2 to 1 seems like a separate change to me, and 
> I doubt that it's necessary to fix the corruption that you're seeing.
> 
> The amount of VBO data we're uploading is 96 bytes (3 vertices * 2
> attributes * 4 values * 4 bytes/float).  Each size is 512 bits = 64
> bytes.  So based on the VBO data...a VS size of 2 seems right.
> 
> 1 actually seems wrong, unless we don't need to account for the VUE
> header (at which point it's 48 bytes, and 48 < 64 so it fits in 1.)
> 
> Does it still work if you make this "const unsigned vs_size = 2;"?

It does. We really need to take the header into account as we include it into
the vertex buffer explicitly using software. Blorp disables vertex state and
therefore needs to submit the VUE as if it came from vertex state - that is
with the header. When vertices are provided to vertex state first than the
header is not needed.

I need to change patch 34 accordingly.

> 
> > +   const unsigned vs_start = push_constant_chunks;
> > +   const unsigned vs_chunks =
> > +      DIV_ROUND_UP(brw->urb.min_vs_entries * vs_size * 64, 
> chunk_size_bytes);
> >  
> >     gen7_emit_push_constant_state(brw,
> >                                   urb_size / 2 /* vs_size */,
> > @@ -59,17 +68,17 @@ gen7_blorp_emit_urb_config(struct brw_context *brw)
> >  
> >     gen7_emit_urb_state(brw,
> >                         brw->urb.min_vs_entries /* num_vs_entries */,
> > -                       2 /* vs_size */,
> > -                       2 /* vs_start */,
> > +                       vs_size,
> > +                       vs_start,
> >                         0 /* num_hs_entries */,
> >                         1 /* hs_size */,
> > -                       2 /* hs_start */,
> > +                       vs_start + vs_chunks /* hs_start */,
> >                         0 /* num_ds_entries */,
> >                         1 /* ds_size */,
> > -                       2 /* ds_start */,
> > +                       vs_start + vs_chunks /* ds_start */,
> >                         0 /* num_gs_entries */,
> >                         1 /* gs_size */,
> > -                       2 /* gs_start */);
> > +                       vs_start + vs_chunks /* gs_start */);
> >  }
> >  
> >  
> > 
>