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

Commit cb7dd008 authored by Yiwei Zhang's avatar Yiwei Zhang
Browse files

TimeStats: fix a racing case 

setPostTime is plumbed without BufferQueue lock, and it's called at the
end of queueBuffer. If the addAndGetFrameTimestamps is delayed by
queueBuffer back pressure (this could happen on special SF/BQ configs),
setAcquireFence will be called with a out-of-bound index internally.

So this change will tweak the order of the back Pressure logic to make
the post time correct when queueBuffer back pressure happens as well as
guarding the TimeStats more strictly.

Bug: 130515827
Test: all SurfaceFlinger tests
Change-Id: I2eace6d8693cc647284de0f33e7b58a6bf79eaaf
parent c4f335d6

libs/gui/BufferQueueProducer.cpp

+8 −8
Original line number Diff line number Diff line
@@ -1007,14 +1007,6 @@ status_t BufferQueueProducer::queueBuffer(int slot, 
        mCallbackCondition.notify_all();

    }



    // Wait without lock held

    if (connectedApi == NATIVE_WINDOW_API_EGL) {

        // Waiting here allows for two full buffers to be queued but not a

        // third. In the event that frames take varying time, this makes a

        // small trade-off in favor of latency rather than throughput.

        lastQueuedFence->waitForever("Throttling EGL Production");

    }



    // Update and get FrameEventHistory.

    nsecs_t postedTime = systemTime(SYSTEM_TIME_MONOTONIC);

    NewFrameEventsEntry newFrameEventsEntry = {
@@ -1026,6 +1018,14 @@ status_t BufferQueueProducer::queueBuffer(int slot, 
    addAndGetFrameTimestamps(&newFrameEventsEntry,

            getFrameTimestamps ? &output->frameTimestamps : nullptr);



    // Wait without lock held

    if (connectedApi == NATIVE_WINDOW_API_EGL) {

        // Waiting here allows for two full buffers to be queued but not a

        // third. In the event that frames take varying time, this makes a

        // small trade-off in favor of latency rather than throughput.

        lastQueuedFence->waitForever("Throttling EGL Production");

    }



    return NO_ERROR;

}

services/surfaceflinger/TimeStats/TimeStats.cpp

+18 −0
Original line number Diff line number Diff line
@@ -291,6 +291,9 @@ void TimeStats::setLatchTime(int32_t layerID, uint64_t frameNumber, nsecs_t latc 
    std::lock_guard<std::mutex> lock(mMutex);

    if (!mTimeStatsTracker.count(layerID)) return;

    LayerRecord& layerRecord = mTimeStatsTracker[layerID];

    if (layerRecord.waitData < 0 ||

        layerRecord.waitData >= static_cast<int32_t>(layerRecord.timeRecords.size()))

        return;

    TimeRecord& timeRecord = layerRecord.timeRecords[layerRecord.waitData];

    if (timeRecord.frameTime.frameNumber == frameNumber) {

        timeRecord.frameTime.latchTime = latchTime;
@@ -306,6 +309,9 @@ void TimeStats::setDesiredTime(int32_t layerID, uint64_t frameNumber, nsecs_t de 
    std::lock_guard<std::mutex> lock(mMutex);

    if (!mTimeStatsTracker.count(layerID)) return;

    LayerRecord& layerRecord = mTimeStatsTracker[layerID];

    if (layerRecord.waitData < 0 ||

        layerRecord.waitData >= static_cast<int32_t>(layerRecord.timeRecords.size()))

        return;

    TimeRecord& timeRecord = layerRecord.timeRecords[layerRecord.waitData];

    if (timeRecord.frameTime.frameNumber == frameNumber) {

        timeRecord.frameTime.desiredTime = desiredTime;
@@ -321,6 +327,9 @@ void TimeStats::setAcquireTime(int32_t layerID, uint64_t frameNumber, nsecs_t ac 
    std::lock_guard<std::mutex> lock(mMutex);

    if (!mTimeStatsTracker.count(layerID)) return;

    LayerRecord& layerRecord = mTimeStatsTracker[layerID];

    if (layerRecord.waitData < 0 ||

        layerRecord.waitData >= static_cast<int32_t>(layerRecord.timeRecords.size()))

        return;

    TimeRecord& timeRecord = layerRecord.timeRecords[layerRecord.waitData];

    if (timeRecord.frameTime.frameNumber == frameNumber) {

        timeRecord.frameTime.acquireTime = acquireTime;
@@ -338,6 +347,9 @@ void TimeStats::setAcquireFence(int32_t layerID, uint64_t frameNumber, 
    std::lock_guard<std::mutex> lock(mMutex);

    if (!mTimeStatsTracker.count(layerID)) return;

    LayerRecord& layerRecord = mTimeStatsTracker[layerID];

    if (layerRecord.waitData < 0 ||

        layerRecord.waitData >= static_cast<int32_t>(layerRecord.timeRecords.size()))

        return;

    TimeRecord& timeRecord = layerRecord.timeRecords[layerRecord.waitData];

    if (timeRecord.frameTime.frameNumber == frameNumber) {

        timeRecord.acquireFence = acquireFence;
@@ -353,6 +365,9 @@ void TimeStats::setPresentTime(int32_t layerID, uint64_t frameNumber, nsecs_t pr 
    std::lock_guard<std::mutex> lock(mMutex);

    if (!mTimeStatsTracker.count(layerID)) return;

    LayerRecord& layerRecord = mTimeStatsTracker[layerID];

    if (layerRecord.waitData < 0 ||

        layerRecord.waitData >= static_cast<int32_t>(layerRecord.timeRecords.size()))

        return;

    TimeRecord& timeRecord = layerRecord.timeRecords[layerRecord.waitData];

    if (timeRecord.frameTime.frameNumber == frameNumber) {

        timeRecord.frameTime.presentTime = presentTime;
@@ -374,6 +389,9 @@ void TimeStats::setPresentFence(int32_t layerID, uint64_t frameNumber, 
    std::lock_guard<std::mutex> lock(mMutex);

    if (!mTimeStatsTracker.count(layerID)) return;

    LayerRecord& layerRecord = mTimeStatsTracker[layerID];

    if (layerRecord.waitData < 0 ||

        layerRecord.waitData >= static_cast<int32_t>(layerRecord.timeRecords.size()))

        return;

    TimeRecord& timeRecord = layerRecord.timeRecords[layerRecord.waitData];

    if (timeRecord.frameTime.frameNumber == frameNumber) {

        timeRecord.presentFence = presentFence;