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

Commit ccf37d74 authored by Dominik Laskowski's avatar Dominik Laskowski
Browse files

SF: Fix thread safety for scheduler callbacks 

SurfaceFlinger::setRefreshRateTo, called from the Scheduler callbacks,
reads HWC and SF state without locking mStateLock, concurrently with
writes from the main thread. This CL registers ResetIdleTimerCallback
per EventThreadConnection, and locks mStateLock for connections used
off the main thread. Note that ExpiredTimerCallback locks mStateLock
unconditionally, since it is always called from the IdleTimer thread.

This CL also adds a thread annotation to setRefreshRateTo, and refactors
it accordingly.

Bug: 123715322
Test: libsurfaceflinger_unittest
Test: Boot with scheduler enabled
Change-Id: Id62c48ae22da38f292ffc18e8731a1c49a0a083c
parent 846c8337

services/surfaceflinger/Scheduler/EventThread.cpp

+13 −13
Original line number Diff line number Diff line
@@ -100,8 +100,10 @@ DisplayEventReceiver::Event makeVSync(PhysicalDisplayId displayId, nsecs_t times 
} // namespace



EventThreadConnection::EventThreadConnection(EventThread* eventThread,

                                             ResyncCallback resyncCallback)

                                             ResyncCallback resyncCallback,

                                             ResetIdleTimerCallback resetIdleTimerCallback)

      : resyncCallback(std::move(resyncCallback)),

        resetIdleTimerCallback(std::move(resetIdleTimerCallback)),

        mEventThread(eventThread),

        mChannel(gui::BitTube::DefaultSize) {}
@@ -147,22 +149,18 @@ EventThread::~EventThread() = default; 
namespace impl {



EventThread::EventThread(std::unique_ptr<VSyncSource> src,

                         const InterceptVSyncsCallback& interceptVSyncsCallback,

                         const ResetIdleTimerCallback& resetIdleTimerCallback,

                         const char* threadName)

      : EventThread(nullptr, std::move(src), interceptVSyncsCallback, threadName) {

    mResetIdleTimer = resetIdleTimerCallback;

}

                         InterceptVSyncsCallback interceptVSyncsCallback, const char* threadName)

      : EventThread(nullptr, std::move(src), std::move(interceptVSyncsCallback), threadName) {}



EventThread::EventThread(VSyncSource* src, InterceptVSyncsCallback interceptVSyncsCallback,

                         const char* threadName)

      : EventThread(src, nullptr, interceptVSyncsCallback, threadName) {}

      : EventThread(src, nullptr, std::move(interceptVSyncsCallback), threadName) {}



EventThread::EventThread(VSyncSource* src, std::unique_ptr<VSyncSource> uniqueSrc,

                         InterceptVSyncsCallback interceptVSyncsCallback, const char* threadName)

      : mVSyncSource(src),

        mVSyncSourceUnique(std::move(uniqueSrc)),

        mInterceptVSyncsCallback(interceptVSyncsCallback),

        mInterceptVSyncsCallback(std::move(interceptVSyncsCallback)),

        mThreadName(threadName) {

    if (src == nullptr) {

        mVSyncSource = mVSyncSourceUnique.get();
@@ -205,8 +203,10 @@ void EventThread::setPhaseOffset(nsecs_t phaseOffset) { 
    mVSyncSource->setPhaseOffset(phaseOffset);

}



sp<EventThreadConnection> EventThread::createEventConnection(ResyncCallback resyncCallback) const {

    return new EventThreadConnection(const_cast<EventThread*>(this), std::move(resyncCallback));

sp<EventThreadConnection> EventThread::createEventConnection(

        ResyncCallback resyncCallback, ResetIdleTimerCallback resetIdleTimerCallback) const {

    return new EventThreadConnection(const_cast<EventThread*>(this), std::move(resyncCallback),

                                     std::move(resetIdleTimerCallback));

}



status_t EventThread::registerDisplayEventConnection(const sp<EventThreadConnection>& connection) {
@@ -249,9 +249,9 @@ void EventThread::setVsyncRate(uint32_t rate, const sp<EventThreadConnection>& c 
}



void EventThread::requestNextVsync(const sp<EventThreadConnection>& connection, bool reset) {

    if (mResetIdleTimer && reset) {

    if (connection->resetIdleTimerCallback && reset) {

        ATRACE_NAME("resetIdleTimer");

        mResetIdleTimer();

        connection->resetIdleTimerCallback();

    }



    if (connection->resyncCallback) {

services/surfaceflinger/Scheduler/EventThread.h

+9 −13
Original line number Diff line number Diff line
@@ -45,6 +45,7 @@ class SurfaceFlinger; 
// ---------------------------------------------------------------------------



using ResyncCallback = std::function<void()>;

using ResetIdleTimerCallback = std::function<void()>;



enum class VSyncRequest {

    None = -1,
@@ -69,7 +70,7 @@ public: 


class EventThreadConnection : public BnDisplayEventConnection {

public:

    EventThreadConnection(EventThread* eventThread, ResyncCallback resyncCallback);

    EventThreadConnection(EventThread*, ResyncCallback, ResetIdleTimerCallback);

    virtual ~EventThreadConnection();



    virtual status_t postEvent(const DisplayEventReceiver::Event& event);
@@ -83,6 +84,7 @@ public: 


    // Called in response to requestNextVsync.

    const ResyncCallback resyncCallback;

    const ResetIdleTimerCallback resetIdleTimerCallback;



    VSyncRequest vsyncRequest = VSyncRequest::None;
@@ -96,8 +98,8 @@ class EventThread { 
public:

    virtual ~EventThread();



    virtual sp<EventThreadConnection> createEventConnection(

            ResyncCallback resyncCallback) const = 0;

    virtual sp<EventThreadConnection> createEventConnection(ResyncCallback,

                                                            ResetIdleTimerCallback) const = 0;



    // called before the screen is turned off from main thread

    virtual void onScreenReleased() = 0;
@@ -124,17 +126,14 @@ namespace impl { 
class EventThread : public android::EventThread, private VSyncSource::Callback {

public:

    using InterceptVSyncsCallback = std::function<void(nsecs_t)>;

    using ResetIdleTimerCallback = std::function<void()>;



    // TODO(b/113612090): Once the Scheduler is complete this constructor will become obsolete.

    EventThread(VSyncSource* src, InterceptVSyncsCallback interceptVSyncsCallback,

                const char* threadName);

    EventThread(std::unique_ptr<VSyncSource> src,

                const InterceptVSyncsCallback& interceptVSyncsCallback,

                const ResetIdleTimerCallback& resetIdleTimerCallback, const char* threadName);

    EventThread(VSyncSource*, InterceptVSyncsCallback, const char* threadName);

    EventThread(std::unique_ptr<VSyncSource>, InterceptVSyncsCallback, const char* threadName);

    ~EventThread();



    sp<EventThreadConnection> createEventConnection(ResyncCallback resyncCallback) const override;

    sp<EventThreadConnection> createEventConnection(ResyncCallback,

                                                    ResetIdleTimerCallback) const override;



    status_t registerDisplayEventConnection(const sp<EventThreadConnection>& connection) override;

    void setVsyncRate(uint32_t rate, const sp<EventThreadConnection>& connection) override;
@@ -220,9 +219,6 @@ private: 
    State mState GUARDED_BY(mMutex) = State::Idle;



    static const char* toCString(State);



    // Callback that resets the idle timer when the next vsync is received.

    ResetIdleTimerCallback mResetIdleTimer;

};



// ---------------------------------------------------------------------------

services/surfaceflinger/Scheduler/MessageQueue.cpp

+2 −1
Original line number Diff line number Diff line
@@ -96,7 +96,8 @@ void MessageQueue::setEventThread(android::EventThread* eventThread, 
    }



    mEventThread = eventThread;

    mEvents = eventThread->createEventConnection(std::move(resyncCallback));

    mEvents =

            eventThread->createEventConnection(std::move(resyncCallback), ResetIdleTimerCallback());

    mEvents->stealReceiveChannel(&mEventTube);

    mLooper->addFd(mEventTube.getFd(), 0, Looper::EVENT_INPUT, MessageQueue::cb_eventReceiver,

                   this);

services/surfaceflinger/Scheduler/Scheduler.cpp

+27 −19
Original line number Diff line number Diff line
@@ -87,6 +87,7 @@ Scheduler::~Scheduler() { 


sp<Scheduler::ConnectionHandle> Scheduler::createConnection(

        const char* connectionName, int64_t phaseOffsetNs, ResyncCallback resyncCallback,

        ResetIdleTimerCallback resetIdleTimerCallback,

        impl::EventThread::InterceptVSyncsCallback interceptCallback) {

    const int64_t id = sNextId++;

    ALOGV("Creating a connection handle with ID: %" PRId64 "\n", id);
@@ -94,12 +95,14 @@ sp<Scheduler::ConnectionHandle> Scheduler::createConnection( 
    std::unique_ptr<EventThread> eventThread =

            makeEventThread(connectionName, mPrimaryDispSync.get(), phaseOffsetNs,

                            std::move(interceptCallback));

    auto connection = std::make_unique<Connection>(new ConnectionHandle(id),

                                                   eventThread->createEventConnection(

                                                           std::move(resyncCallback)),

                                                   std::move(eventThread));



    mConnections.insert(std::make_pair(id, std::move(connection)));

    auto eventThreadConnection =

            createConnectionInternal(eventThread.get(), std::move(resyncCallback),

                                     std::move(resetIdleTimerCallback));

    mConnections.emplace(id,

                         std::make_unique<Connection>(new ConnectionHandle(id),

                                                      eventThreadConnection,

                                                      std::move(eventThread)));

    return mConnections[id]->handle;

}
@@ -109,14 +112,29 @@ std::unique_ptr<EventThread> Scheduler::makeEventThread( 
    std::unique_ptr<VSyncSource> eventThreadSource =

            std::make_unique<DispSyncSource>(dispSync, phaseOffsetNs, true, connectionName);

    return std::make_unique<impl::EventThread>(std::move(eventThreadSource),

                                               std::move(interceptCallback),

                                               [this] { resetIdleTimer(); }, connectionName);

                                               std::move(interceptCallback), connectionName);

}



sp<EventThreadConnection> Scheduler::createConnectionInternal(

        EventThread* eventThread, ResyncCallback&& resyncCallback,

        ResetIdleTimerCallback&& resetIdleTimerCallback) {

    return eventThread->createEventConnection(std::move(resyncCallback),

                                              [this,

                                               resetIdleTimerCallback =

                                                       std::move(resetIdleTimerCallback)] {

                                                  resetIdleTimer();

                                                  if (resetIdleTimerCallback) {

                                                      resetIdleTimerCallback();

                                                  }

                                              });

}



sp<IDisplayEventConnection> Scheduler::createDisplayEventConnection(

        const sp<Scheduler::ConnectionHandle>& handle, ResyncCallback resyncCallback) {

        const sp<Scheduler::ConnectionHandle>& handle, ResyncCallback resyncCallback,

        ResetIdleTimerCallback resetIdleTimerCallback) {

    RETURN_VALUE_IF_INVALID(nullptr);

    return mConnections[handle->id]->thread->createEventConnection(std::move(resyncCallback));

    return createConnectionInternal(mConnections[handle->id]->thread.get(),

                                    std::move(resyncCallback), std::move(resetIdleTimerCallback));

}



EventThread* Scheduler::getEventThread(const sp<Scheduler::ConnectionHandle>& handle) {
@@ -241,11 +259,6 @@ void Scheduler::setExpiredIdleTimerCallback(const ExpiredIdleTimerCallback& expi 
    mExpiredTimerCallback = expiredTimerCallback;

}



void Scheduler::setResetIdleTimerCallback(const ResetIdleTimerCallback& resetTimerCallback) {

    std::lock_guard<std::mutex> lock(mCallbackLock);

    mResetTimerCallback = resetTimerCallback;

}



void Scheduler::updateFrameSkipping(const int64_t skipCount) {

    ATRACE_INT("FrameSkipCount", skipCount);

    if (mSkipCount != skipCount) {
@@ -336,11 +349,6 @@ void Scheduler::resetIdleTimer() { 
        mIdleTimer->reset();

        ATRACE_INT("ExpiredIdleTimer", 0);

    }



    std::lock_guard<std::mutex> lock(mCallbackLock);

    if (mResetTimerCallback) {

        mResetTimerCallback();

    }

}



void Scheduler::expiredTimerCallback() {

services/surfaceflinger/Scheduler/Scheduler.h

+9 −8
Original line number Diff line number Diff line
@@ -37,7 +37,6 @@ class EventControlThread; 
class Scheduler {

public:

    using ExpiredIdleTimerCallback = std::function<void()>;

    using ResetIdleTimerCallback = std::function<void()>;



    // Enum to indicate whether to start the transaction early, or at vsync time.

    enum class TransactionStart { EARLY, NORMAL };
@@ -72,12 +71,13 @@ public: 
    virtual ~Scheduler();



    /** Creates an EventThread connection. */

    sp<ConnectionHandle> createConnection(

            const char* connectionName, int64_t phaseOffsetNs, ResyncCallback resyncCallback,

            impl::EventThread::InterceptVSyncsCallback interceptCallback);

    sp<ConnectionHandle> createConnection(const char* connectionName, int64_t phaseOffsetNs,

                                          ResyncCallback, ResetIdleTimerCallback,

                                          impl::EventThread::InterceptVSyncsCallback);



    sp<IDisplayEventConnection> createDisplayEventConnection(const sp<ConnectionHandle>& handle,

                                                             ResyncCallback resyncCallback);

                                                             ResyncCallback,

                                                             ResetIdleTimerCallback);



    // Getter methods.

    EventThread* getEventThread(const sp<ConnectionHandle>& handle);
@@ -116,8 +116,6 @@ public: 
    void incrementFrameCounter();

    // Callback that gets invoked once the idle timer expires.

    void setExpiredIdleTimerCallback(const ExpiredIdleTimerCallback& expiredTimerCallback);

    // Callback that gets invoked once the idle timer is reset.

    void setResetIdleTimerCallback(const ResetIdleTimerCallback& resetTimerCallback);

    // Returns relevant information about Scheduler for dumpsys purposes.

    std::string doDump();
@@ -127,6 +125,10 @@ protected: 
            impl::EventThread::InterceptVSyncsCallback interceptCallback);



private:

    // Creates a connection on the given EventThread and forwards the given callbacks.

    sp<EventThreadConnection> createConnectionInternal(EventThread*, ResyncCallback&&,

                                                       ResetIdleTimerCallback&&);



    nsecs_t calculateAverage() const;

    void updateFrameSkipping(const int64_t skipCount);

    // Collects the statistical mean (average) and median between timestamp
@@ -184,7 +186,6 @@ private: 


    std::mutex mCallbackLock;

    ExpiredIdleTimerCallback mExpiredTimerCallback GUARDED_BY(mCallbackLock);

    ResetIdleTimerCallback mResetTimerCallback GUARDED_BY(mCallbackLock);

};



} // namespace android