[Mesa-dev] [PATCH 1/1] i965: Make sure the shadow buffers have enough space

[Kenneth Graunke]

On Friday, April 13, 2018 2:08:40 PM PDT James Xiong wrote:
> On Fri, 13 Apr 2018 13:51:02 -0700
> Kenneth Graunke <kenneth at whitecape.org> wrote:
> 
> > On Friday, April 13, 2018 1:35:45 PM PDT Kenneth Graunke wrote:
> > > On Monday, April 9, 2018 4:06:16 PM PDT James Xiong wrote:  
> > > > From: "Xiong, James" <james.xiong at intel.com>
> > > > 
> > > > On non-LLC platforms, we malloc shadow batch/state buffers
> > > > of the same sizes as our batch/state buffers' GEM allocations.
> > > > However the buffer allocator reuses similar-sized gem objects,
> > > > it returns a buffer larger than we asked for in some cases
> > > > and we end up with smaller shadow buffers. If we utilize the
> > > > full-size of the over-allocated batch/state buffers, we may wind
> > > > up accessing beyond the bounds of the shadow buffers and cause
> > > > segmentation fault and/or memory corruption.  
> > > 
> > > Oh, good catch!  We do indeed malloc too little if the bufmgr
> Thank you for taking time to review, Kenneth.
> > >   
> > > > A few examples:
> > > >      case            batch                  state
> > > >                  request bo   shadow     request bo  shadow
> > > > init        0    20K     20K  20K        16K     16K 16K
> > > > grow_buffer 1    30K     32K  30K        24K     24K 24K
> > > > grow_buffer 2    48K     48K  48K        36K     40K 36K
> > > > grow_buffer 3    72K     80K  72K        60K     64K 60K
> > > > grow_buffer 4    120K    128K 120K       -       -   -
> > > > 
> > > > batch #1, #3, #4; state #2 and #3 are problematic. We can change
> > > > the order to allocate the bo first, then allocate the shadow
> > > > buffer using the bo's size so that the shadow buffer have at
> > > > least an equivalent size of the gem allocation.
> > > > 
> > > > Another problem: even though the state/batch buffer could grow,
> > > > when checking if it runs out space, we always compare with the
> > > > initial batch/state sizes. To utilize the entire buffers, change
> > > > to compare with the actual sizes.  
> > > 
> > > This is actually intentional.  Our goal is to flush batches when the
> > > amount of commands or state reaches those thresholds.  Occasionally,
> > > we'll be in the middle of emitting a draw, and unable to stop.  In
> > > that case, we grow the batch and keep going.  But after that, we're
> > > beyond our original target, so we flush next time.  We don't want
> > > to grow without bounds...it's meant more for emergencies, or if
> > > we've badly estimated the size of the draw call.
> I am not sure I get it. Let me give an example: the state buffer
> gets grown once from 16K to 24K in brw_state_batch(), the used_size
> becomes 20K, then brw_require_statebuffer_space(1024) gets called to
> ask for 1K space, with the original logical, it compares the used size
> with 16K and flush the batch even though the state buffer still has 4K
> space available?

Yes, the idea is to flush at around 16K of state.  If we happen to be in
the middle of a draw and run out of space, we'll grow to 24K.  Once it's
over 16K, we flush as soon as we can.

We'd like to be fairly consistent on our batch size.  Running larger
batches can lead to differences in performance, and large batches can
lead to degradation in the interactivity of the system (especially on
GPUs without preemption).

The hope is to grow once, at most.  If we check against the BO size, we
might grow repeatedly, which would lead to really large batches and
things would get out of hand.

> > > 
> > > I've sent a simpler patch which I think should hopefully fix your
> > > bug: https://patchwork.freedesktop.org/patch/217107/  
> > 
> > Lionel noticed that I botched that patch.  Here's an actual one:
> > 
> > https://patchwork.freedesktop.org/patch/217108/
> Yes it will fix the existing bug. However the assumption here is
> that the init allocation size will NOT be rounded up as it happens to
> be the bucket size.
> I am working on an optimization to improve memory usage(that's how
> I find out this bug), this assumption is no longer true. Essentially the
> bufmgr could return a buffer with the same or larger size whether it is
> same as the bucket's or not. Anyway I guess I can send the fix
> later along with the optimization patches.

Ah, that's a good point.  Your patch also tries to use the BO size
for the initial malloc as well, which is a good idea...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180413/2d55a7c9/attachment.sig>