[Intel-gfx] [PATCH 06/12] drm/i915: alloc/free the FBC CFB during enable/disable

[Chris Wilson]

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