On Fri, Nov 13, 2015 at 05:53:38PM -0200, Paulo Zanoni wrote: > One of the problems with the current code is that it frees the CFB and > releases its drm_mm node as soon as we flip FBC's enable bit. This is > bad because after we disable FBC the hardware may still use the CFB > for the rest of the frame, so in theory we should only release the > drm_mm node one frame after we disable FBC. Otherwise, a stolen memory > allocation done right after an FBC disable may result in either > corrupted memory for the new owner of that memory region or corrupted > screen/underruns in case the new owner changes it while the hardware > is still reading it. This case is not exactly easy to reproduce since > we currently don't do a lot of stolen memory allocations, but I see > patches on the mailing list trying to expose stolen memory to user > space, so races will be possible. > > I thought about three different approaches to solve this, and they all > have downsides. > > The first approach would be to simply use multiple drm_mm nodes and > freeing the unused ones only after a frame has passed. The problem > with this approach is that since stolen memory is rather small, > there's a risk we just won't be able to allocate a new CFB from stolen > if the previous one was not freed yet. This could happen in case we > quickly disable FBC from pipe A and decide to enable it on pipe B, or > just if we change pipe A's fb stride while FBC is enabled. > > The second approach would be similar to the first one, but maintaining > a single drm_mm node and keeping track of when it can be reused. This > would remove the disadvantage of not having enough space for two > nodes, but would create the new problem where we may not be able to > enable FBC at the point intel_fbc_update() is called, so we would have > to add more code to retry updating FBC after the time has passed. And > that can quickly get too complex since we can get invalidate, flush, > disable and other calls in the middle of the wait. > > Both solutions above - and also the current code - have the problem > that we unnecessarily free+realloc FBC during invalidate+flush > operations even if the CFB size doesn't change. > > The third option would be to move the allocation/deallocation to > enable/disable. This makes sure that the pipe is always disabled when > we allocate/deallocate the CFB, so there's no risk that the FBC > hardware may read or write to the memory right after it is freed from > drm_mm. The downside is that it is possible for user space to change > the buffer stride without triggering a disable/enable - only > deactivate/activate -, so we'll have to handle this case somehow, even > though it is uncommon - see igt's kms_frontbuffer_tracking test, > fbc-stridechange subtest. It could be possible to implement a way to > free+alloc the CFB during said stride change, but it would involve a > lot of book-keeping - exactly as mentioned above - just for a rare > case, so for now I'll keep it simple and just deactivate FBC. Besides, > we may not even need to disable FBC since we do CFB over-allocation. It's not really that rare. Starting a fullscreen client that covers a single monitor in a multi-monitor setup will trigger a change in stride on one of the CRTC (the monitors will be flipped independently). Not that actually affects your argument, just presenting a use-case that exists in the wild today. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
