Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit f01a6f14 authored by Vishnu Nair's avatar Vishnu Nair
Browse files

[sf] Fix latch unsignaled issues

With LatchUnsignaledConfig::AutoSingleLayer flag, we should only
apply a transaction with an unsignaled fence if its the only transaction.
If we pass an unsignaled fence to HWC, HWC might miss presenting
the frame if the fence does not fire in time. If we apply another
transaction we may penalize the other transaction unfairly.

With LatchUnsignaledConfig::Always flag, respect backpressure flag
and take the opportunity to simplify the logic by treating the flag as
a signal to ignore the fence when applying transactions.

Test: surfaceflinger unit tests
Bug: 276229937
Change-Id: Ib5cd8205d62fdf8912b7f5c2cad312a01cae4300
parent f9c1b98e
Loading
Loading
Loading
Loading
+58 −43
Original line number Original line Diff line number Diff line
@@ -64,11 +64,61 @@ std::vector<TransactionState> TransactionHandler::flushTransactions() {
        transactionsPendingBarrier = flushPendingTransactionQueues(transactions, flushState);
        transactionsPendingBarrier = flushPendingTransactionQueues(transactions, flushState);
    } while (lastTransactionsPendingBarrier != transactionsPendingBarrier);
    } while (lastTransactionsPendingBarrier != transactionsPendingBarrier);


    applyUnsignaledBufferTransaction(transactions, flushState);

    mPendingTransactionCount.fetch_sub(transactions.size());
    mPendingTransactionCount.fetch_sub(transactions.size());
    ATRACE_INT("TransactionQueue", static_cast<int>(mPendingTransactionCount.load()));
    ATRACE_INT("TransactionQueue", static_cast<int>(mPendingTransactionCount.load()));
    return transactions;
    return transactions;
}
}


void TransactionHandler::applyUnsignaledBufferTransaction(
        std::vector<TransactionState>& transactions, TransactionFlushState& flushState) {
    // only apply an unsignaled buffer transaction if it's the first one
    if (!transactions.empty()) {
        return;
    }

    if (!flushState.queueWithUnsignaledBuffer) {
        return;
    }

    auto it = mPendingTransactionQueues.find(flushState.queueWithUnsignaledBuffer);
    LOG_ALWAYS_FATAL_IF(it == mPendingTransactionQueues.end(),
                        "Could not find queue with unsignaled buffer!");

    auto& queue = it->second;
    popTransactionFromPending(transactions, flushState, queue);
    if (queue.empty()) {
        it = mPendingTransactionQueues.erase(it);
    }
}

void TransactionHandler::popTransactionFromPending(std::vector<TransactionState>& transactions,
                                                   TransactionFlushState& flushState,
                                                   std::queue<TransactionState>& queue) {
    auto& transaction = queue.front();
    // Transaction is ready move it from the pending queue.
    flushState.firstTransaction = false;
    removeFromStalledTransactions(transaction.id);
    transactions.emplace_back(std::move(transaction));
    queue.pop();

    auto& readyToApplyTransaction = transactions.back();
    readyToApplyTransaction.traverseStatesWithBuffers([&](const layer_state_t& state) {
        const bool frameNumberChanged =
                state.bufferData->flags.test(BufferData::BufferDataChange::frameNumberChanged);
        if (frameNumberChanged) {
            flushState.bufferLayersReadyToPresent.emplace_or_replace(state.surface.get(),
                                                                     state.bufferData->frameNumber);
        } else {
            // Barrier function only used for BBQ which always includes a frame number.
            // This value only used for barrier logic.
            flushState.bufferLayersReadyToPresent
                    .emplace_or_replace(state.surface.get(), std::numeric_limits<uint64_t>::max());
        }
    });
}

TransactionHandler::TransactionReadiness TransactionHandler::applyFilters(
TransactionHandler::TransactionReadiness TransactionHandler::applyFilters(
        TransactionFlushState& flushState) {
        TransactionFlushState& flushState) {
    auto ready = TransactionReadiness::Ready;
    auto ready = TransactionReadiness::Ready;
@@ -79,8 +129,7 @@ TransactionHandler::TransactionReadiness TransactionHandler::applyFilters(
            case TransactionReadiness::NotReadyBarrier:
            case TransactionReadiness::NotReadyBarrier:
                return perFilterReady;
                return perFilterReady;


            case TransactionReadiness::ReadyUnsignaled:
            case TransactionReadiness::NotReadyUnsignaled:
            case TransactionReadiness::ReadyUnsignaledSingle:
                // If one of the filters allows latching an unsignaled buffer, latch this ready
                // If one of the filters allows latching an unsignaled buffer, latch this ready
                // state.
                // state.
                ready = perFilterReady;
                ready = perFilterReady;
@@ -97,17 +146,7 @@ int TransactionHandler::flushPendingTransactionQueues(std::vector<TransactionSta
    int transactionsPendingBarrier = 0;
    int transactionsPendingBarrier = 0;
    auto it = mPendingTransactionQueues.begin();
    auto it = mPendingTransactionQueues.begin();
    while (it != mPendingTransactionQueues.end()) {
    while (it != mPendingTransactionQueues.end()) {
        auto& queue = it->second;
        auto& [applyToken, queue] = *it;
        IBinder* queueToken = it->first.get();

        // if we have already flushed a transaction with an unsignaled buffer then stop queue
        // processing
        if (std::find(flushState.queuesWithUnsignaledBuffers.begin(),
                      flushState.queuesWithUnsignaledBuffers.end(),
                      queueToken) != flushState.queuesWithUnsignaledBuffers.end()) {
            continue;
        }

        while (!queue.empty()) {
        while (!queue.empty()) {
            auto& transaction = queue.front();
            auto& transaction = queue.front();
            flushState.transaction = &transaction;
            flushState.transaction = &transaction;
@@ -117,38 +156,14 @@ int TransactionHandler::flushPendingTransactionQueues(std::vector<TransactionSta
                break;
                break;
            } else if (ready == TransactionReadiness::NotReady) {
            } else if (ready == TransactionReadiness::NotReady) {
                break;
                break;
            }
            } else if (ready == TransactionReadiness::NotReadyUnsignaled) {

                // We maybe able to latch this transaction if it's the only transaction
            // Transaction is ready move it from the pending queue.
                // ready to be applied.
            flushState.firstTransaction = false;
                flushState.queueWithUnsignaledBuffer = applyToken;
            removeFromStalledTransactions(transaction.id);
            transactions.emplace_back(std::move(transaction));
            queue.pop();

            // If the buffer is unsignaled, then we don't want to signal other transactions using
            // the buffer as a barrier.
            auto& readyToApplyTransaction = transactions.back();
            if (ready == TransactionReadiness::Ready) {
                readyToApplyTransaction.traverseStatesWithBuffers([&](const layer_state_t& state) {
                    const bool frameNumberChanged = state.bufferData->flags.test(
                            BufferData::BufferDataChange::frameNumberChanged);
                    if (frameNumberChanged) {
                        flushState.bufferLayersReadyToPresent
                                .emplace_or_replace(state.surface.get(),
                                                    state.bufferData->frameNumber);
                    } else {
                        // Barrier function only used for BBQ which always includes a frame number.
                        // This value only used for barrier logic.
                        flushState.bufferLayersReadyToPresent
                                .emplace_or_replace(state.surface.get(),
                                                    std::numeric_limits<uint64_t>::max());
                    }
                });
            } else if (ready == TransactionReadiness::ReadyUnsignaledSingle) {
                // Track queues with a flushed unsingaled buffer.
                flushState.queuesWithUnsignaledBuffers.emplace_back(queueToken);
                break;
                break;
            }
            }
            // ready == TransactionReadiness::Ready
            popTransactionFromPending(transactions, flushState, queue);
        }
        }


        if (queue.empty()) {
        if (queue.empty()) {
+13 −4
Original line number Original line Diff line number Diff line
@@ -40,14 +40,20 @@ public:
        // Layer handles that have transactions with buffers that are ready to be applied.
        // Layer handles that have transactions with buffers that are ready to be applied.
        ftl::SmallMap<IBinder* /* binder address */, uint64_t /* framenumber */, 15>
        ftl::SmallMap<IBinder* /* binder address */, uint64_t /* framenumber */, 15>
                bufferLayersReadyToPresent = {};
                bufferLayersReadyToPresent = {};
        ftl::SmallVector<IBinder* /* queueToken */, 15> queuesWithUnsignaledBuffers;
        // Tracks the queue with an unsignaled buffer. This is used to handle
        // LatchUnsignaledConfig::AutoSingleLayer to ensure we only apply an unsignaled buffer
        // if it's the only transaction that is ready to be applied.
        sp<IBinder> queueWithUnsignaledBuffer = nullptr;
    };
    };
    enum class TransactionReadiness {
    enum class TransactionReadiness {
        // Transaction is ready to be applied
        Ready,
        // Transaction has unmet conditions (fence, present time, etc) and cannot be applied.
        NotReady,
        NotReady,
        // Transaction is waiting on a barrier (another buffer to be latched first)
        NotReadyBarrier,
        NotReadyBarrier,
        Ready,
        // Transaction has an unsignaled fence but can be applied if it's the only transaction
        ReadyUnsignaled,
        NotReadyUnsignaled,
        ReadyUnsignaledSingle,
    };
    };
    using TransactionFilter = std::function<TransactionReadiness(const TransactionFlushState&)>;
    using TransactionFilter = std::function<TransactionReadiness(const TransactionFlushState&)>;


@@ -64,6 +70,9 @@ private:
    friend class ::android::TestableSurfaceFlinger;
    friend class ::android::TestableSurfaceFlinger;


    int flushPendingTransactionQueues(std::vector<TransactionState>&, TransactionFlushState&);
    int flushPendingTransactionQueues(std::vector<TransactionState>&, TransactionFlushState&);
    void applyUnsignaledBufferTransaction(std::vector<TransactionState>&, TransactionFlushState&);
    void popTransactionFromPending(std::vector<TransactionState>&, TransactionFlushState&,
                                   std::queue<TransactionState>&);
    TransactionReadiness applyFilters(TransactionFlushState&);
    TransactionReadiness applyFilters(TransactionFlushState&);
    std::unordered_map<sp<IBinder>, std::queue<TransactionState>, IListenerHash>
    std::unordered_map<sp<IBinder>, std::queue<TransactionState>, IListenerHash>
            mPendingTransactionQueues;
            mPendingTransactionQueues;
+14 −15
Original line number Original line Diff line number Diff line
@@ -4266,20 +4266,23 @@ TransactionHandler::TransactionReadiness SurfaceFlinger::transactionReadyBufferC
            return TraverseBuffersReturnValues::STOP_TRAVERSAL;
            return TraverseBuffersReturnValues::STOP_TRAVERSAL;
        }
        }


        // ignore the acquire fence if LatchUnsignaledConfig::Always is set.
        const bool checkAcquireFence = enableLatchUnsignaledConfig != LatchUnsignaledConfig::Always;
        const bool acquireFenceAvailable = s.bufferData &&
                s.bufferData->flags.test(BufferData::BufferDataChange::fenceChanged) &&
                s.bufferData->acquireFence;
        const bool fenceSignaled = !checkAcquireFence || !acquireFenceAvailable ||
                s.bufferData->acquireFence->getStatus() != Fence::Status::Unsignaled;
        if (!fenceSignaled) {
            // check fence status
            // check fence status
        const bool allowLatchUnsignaled = shouldLatchUnsignaled(layer, s, transaction.states.size(),
            const bool allowLatchUnsignaled =
                    shouldLatchUnsignaled(layer, s, transaction.states.size(),
                                          flushState.firstTransaction);
                                          flushState.firstTransaction);
            ATRACE_FORMAT("%s allowLatchUnsignaled=%s", layer->getName().c_str(),
            ATRACE_FORMAT("%s allowLatchUnsignaled=%s", layer->getName().c_str(),
                          allowLatchUnsignaled ? "true" : "false");
                          allowLatchUnsignaled ? "true" : "false");

            if (allowLatchUnsignaled) {
        const bool acquireFenceChanged = s.bufferData &&
                ready = TransactionReadiness::NotReadyUnsignaled;
                s.bufferData->flags.test(BufferData::BufferDataChange::fenceChanged) &&
            } else {
                s.bufferData->acquireFence;
        const bool fenceSignaled =
                (!acquireFenceChanged ||
                 s.bufferData->acquireFence->getStatus() != Fence::Status::Unsignaled);
        if (!fenceSignaled) {
            if (!allowLatchUnsignaled) {
                ready = TransactionReadiness::NotReady;
                ready = TransactionReadiness::NotReady;
                auto& listener = s.bufferData->releaseBufferListener;
                auto& listener = s.bufferData->releaseBufferListener;
                if (listener &&
                if (listener &&
@@ -4292,10 +4295,6 @@ TransactionHandler::TransactionReadiness SurfaceFlinger::transactionReadyBufferC
                }
                }
                return TraverseBuffersReturnValues::STOP_TRAVERSAL;
                return TraverseBuffersReturnValues::STOP_TRAVERSAL;
            }
            }

            ready = enableLatchUnsignaledConfig == LatchUnsignaledConfig::AutoSingleLayer
                    ? TransactionReadiness::ReadyUnsignaledSingle
                    : TransactionReadiness::ReadyUnsignaled;
        }
        }
        return TraverseBuffersReturnValues::CONTINUE_TRAVERSAL;
        return TraverseBuffersReturnValues::CONTINUE_TRAVERSAL;
    });
    });
+5 −0
Original line number Original line Diff line number Diff line
@@ -174,10 +174,15 @@ enum class LatchUnsignaledConfig {
    // Latch unsignaled is permitted when a single layer is updated in a frame,
    // Latch unsignaled is permitted when a single layer is updated in a frame,
    // and the update includes just a buffer update (i.e. no sync transactions
    // and the update includes just a buffer update (i.e. no sync transactions
    // or geometry changes).
    // or geometry changes).
    // Latch unsignaled is also only permitted when a single transaction is ready
    // to be applied. If we pass an unsignaled fence to HWC, HWC might miss presenting
    // the frame if the fence does not fire in time. If we apply another transaction,
    // we may penalize the other transaction unfairly.
    AutoSingleLayer,
    AutoSingleLayer,


    // All buffers are latched unsignaled. This behaviour is discouraged as it
    // All buffers are latched unsignaled. This behaviour is discouraged as it
    // can break sync transactions, stall the display and cause undesired side effects.
    // can break sync transactions, stall the display and cause undesired side effects.
    // This is equivalent to ignoring the acquire fence when applying transactions.
    Always,
    Always,
};
};


+81 −3
Original line number Original line Diff line number Diff line
@@ -294,7 +294,8 @@ public:
        return fence;
        return fence;
    }
    }


    ComposerState createComposerState(int layerId, sp<Fence> fence, uint64_t what) {
    ComposerState createComposerState(int layerId, sp<Fence> fence, uint64_t what,
                                      std::optional<sp<IBinder>> layerHandle = std::nullopt) {
        ComposerState state;
        ComposerState state;
        state.state.bufferData =
        state.state.bufferData =
                std::make_shared<fake::BufferData>(/* bufferId */ 123L, /* width */ 1,
                std::make_shared<fake::BufferData>(/* bufferId */ 123L, /* width */ 1,
@@ -302,15 +303,20 @@ public:
                                                   /* outUsage */ 0);
                                                   /* outUsage */ 0);
        state.state.bufferData->acquireFence = std::move(fence);
        state.state.bufferData->acquireFence = std::move(fence);
        state.state.layerId = layerId;
        state.state.layerId = layerId;
        state.state.surface =
        state.state.surface = layerHandle.value_or(
                sp<Layer>::make(LayerCreationArgs(mFlinger.flinger(), nullptr, "TestLayer", 0, {}))
                sp<Layer>::make(LayerCreationArgs(mFlinger.flinger(), nullptr, "TestLayer", 0, {}))
                        ->getHandle();
                        ->getHandle());
        state.state.bufferData->flags = BufferData::BufferDataChange::fenceChanged;
        state.state.bufferData->flags = BufferData::BufferDataChange::fenceChanged;


        state.state.what = what;
        state.state.what = what;
        if (what & layer_state_t::eCropChanged) {
        if (what & layer_state_t::eCropChanged) {
            state.state.crop = Rect(1, 2, 3, 4);
            state.state.crop = Rect(1, 2, 3, 4);
        }
        }
        if (what & layer_state_t::eFlagsChanged) {
            state.state.flags = layer_state_t::eEnableBackpressure;
            state.state.mask = layer_state_t::eEnableBackpressure;
        }

        return state;
        return state;
    }
    }


@@ -601,6 +607,41 @@ TEST_F(LatchUnsignaledAutoSingleLayerTest, DontLatchUnsignaledWhenEarlyOffset) {
    setTransactionStates({unsignaledTransaction}, kExpectedTransactionsPending);
    setTransactionStates({unsignaledTransaction}, kExpectedTransactionsPending);
}
}


TEST_F(LatchUnsignaledAutoSingleLayerTest, UnsignaledNotAppliedWhenThereAreSignaled_SignaledFirst) {
    const sp<IBinder> kApplyToken1 =
            IInterface::asBinder(TransactionCompletedListener::getIInstance());
    const sp<IBinder> kApplyToken2 = sp<BBinder>::make();
    const sp<IBinder> kApplyToken3 = sp<BBinder>::make();
    const auto kLayerId1 = 1;
    const auto kLayerId2 = 2;
    const auto kExpectedTransactionsPending = 1u;

    const auto signaledTransaction =
            createTransactionInfo(kApplyToken1,
                                  {
                                          createComposerState(kLayerId1,
                                                              fence(Fence::Status::Signaled),
                                                              layer_state_t::eBufferChanged),
                                  });
    const auto signaledTransaction2 =
            createTransactionInfo(kApplyToken2,
                                  {
                                          createComposerState(kLayerId1,
                                                              fence(Fence::Status::Signaled),
                                                              layer_state_t::eBufferChanged),
                                  });
    const auto unsignaledTransaction =
            createTransactionInfo(kApplyToken3,
                                  {
                                          createComposerState(kLayerId2,
                                                              fence(Fence::Status::Unsignaled),
                                                              layer_state_t::eBufferChanged),
                                  });

    setTransactionStates({signaledTransaction, signaledTransaction2, unsignaledTransaction},
                         kExpectedTransactionsPending);
}

class LatchUnsignaledDisabledTest : public LatchUnsignaledTest {
class LatchUnsignaledDisabledTest : public LatchUnsignaledTest {
public:
public:
    void SetUp() override {
    void SetUp() override {
@@ -943,6 +984,43 @@ TEST_F(LatchUnsignaledAlwaysTest, Flush_RemovesUnsignaledFromTheQueue) {
                         kExpectedTransactionsPending);
                         kExpectedTransactionsPending);
}
}


TEST_F(LatchUnsignaledAlwaysTest, RespectsBackPressureFlag) {
    const sp<IBinder> kApplyToken1 =
            IInterface::asBinder(TransactionCompletedListener::getIInstance());
    const sp<IBinder> kApplyToken2 = sp<BBinder>::make();
    const auto kLayerId1 = 1;
    const auto kExpectedTransactionsPending = 1u;
    auto layer =
            sp<Layer>::make(LayerCreationArgs(mFlinger.flinger(), nullptr, "TestLayer", 0, {}));
    auto layerHandle = layer->getHandle();
    const auto setBackPressureFlagTransaction =
            createTransactionInfo(kApplyToken1,
                                  {createComposerState(kLayerId1, fence(Fence::Status::Unsignaled),
                                                       layer_state_t::eBufferChanged |
                                                               layer_state_t::eFlagsChanged,
                                                       {layerHandle})});
    setTransactionStates({setBackPressureFlagTransaction}, 0u);

    const auto unsignaledTransaction =
            createTransactionInfo(kApplyToken1,
                                  {
                                          createComposerState(kLayerId1,
                                                              fence(Fence::Status::Unsignaled),
                                                              layer_state_t::eBufferChanged,
                                                              {layerHandle}),
                                  });
    const auto unsignaledTransaction2 =
            createTransactionInfo(kApplyToken1,
                                  {
                                          createComposerState(kLayerId1,
                                                              fence(Fence::Status::Unsignaled),
                                                              layer_state_t::eBufferChanged,
                                                              {layerHandle}),
                                  });
    setTransactionStates({unsignaledTransaction, unsignaledTransaction2},
                         kExpectedTransactionsPending);
}

TEST_F(LatchUnsignaledAlwaysTest, LatchUnsignaledWhenEarlyOffset) {
TEST_F(LatchUnsignaledAlwaysTest, LatchUnsignaledWhenEarlyOffset) {
    const sp<IBinder> kApplyToken =
    const sp<IBinder> kApplyToken =
            IInterface::asBinder(TransactionCompletedListener::getIInstance());
            IInterface::asBinder(TransactionCompletedListener::getIInstance());