GitLab

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 - 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 on 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.

Note from Chris: "Starting a fullscreen client that covers a single
monitor in a multi-monitor setup will trigger a change in stride on
one of the CRTCs (the monitors will be flipped independently).". It
shouldn't be a huge problem if we lose FBC on multi-monitor setups
since these setups already have problems reaching deep PC states
anyway.

v2: Rebase after changing the patch order.
v3:
  - Remove references to the stride change case being "uncommon" and
    paste Chris' example.
  - Rebase after a change in a previous patch.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/