On Friday, April 13, 2018 2:08:40 PM PDT James Xiong wrote: > On Fri, 13 Apr 2018 13:51:02 -0700 > Kenneth Graunke <kenn...@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.xi...@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...
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list firstname.lastname@example.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev