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

[Kenneth Graunke]

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 rounds up
> the BO size.  Thanks for finding this!
> 
> > 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'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/
-------------- 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/a306a50f/attachment.sig>