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

Commit 183f0563 authored by Ruben Brunk's avatar Ruben Brunk
Browse files

Fix deadlock conditions in Camera3Device. 

Potential deadlock conditions this addresses, include:
- Not waking up waiting threads for several situations where
  the status had been updated.
- Not waking up all waiting thread when status had been updated
  (only one thread was awoken due to use of signal).
- Threads clear status transitions before other waiting threads
  have a chance to examine them.

Bug: 22448586
Change-Id: I53ba669d333a83d2bfa1ca3170d34acc6d8fe6e3
parent 4327df2a

services/camera/libcameraservice/device3/Camera3Device.cpp

+43 −20
Original line number Original line Diff line number Diff line
@@ -60,6 +60,7 @@ Camera3Device::Camera3Device(int id): 
        mIsConstrainedHighSpeedConfiguration(false),

        mIsConstrainedHighSpeedConfiguration(false),

        mHal3Device(NULL),

        mHal3Device(NULL),

        mStatus(STATUS_UNINITIALIZED),

        mStatus(STATUS_UNINITIALIZED),

        mStatusWaiters(0),

        mUsePartialResult(false),

        mUsePartialResult(false),

        mNumPartialResults(1),

        mNumPartialResults(1),

        mNextResultFrameNumber(0),

        mNextResultFrameNumber(0),
@@ -191,7 +192,8 @@ status_t Camera3Device::initialize(CameraModule *module) 
    mDeviceVersion = device->common.version;

    mDeviceVersion = device->common.version;

    mDeviceInfo = info.static_camera_characteristics;

    mDeviceInfo = info.static_camera_characteristics;

    mHal3Device = device;

    mHal3Device = device;

    mStatus = STATUS_UNCONFIGURED;



    internalUpdateStatusLocked(STATUS_UNCONFIGURED);

    mNextStreamId = 0;

    mNextStreamId = 0;

    mDummyStreamId = NO_STREAM;

    mDummyStreamId = NO_STREAM;

    mNeedConfig = true;

    mNeedConfig = true;
@@ -296,7 +298,7 @@ status_t Camera3Device::disconnect() { 
            mHal3Device = NULL;

            mHal3Device = NULL;

        }

        }





        mStatus = STATUS_UNINITIALIZED;

        internalUpdateStatusLocked(STATUS_UNINITIALIZED);

    }

    }





    ALOGV("%s: X", __FUNCTION__);

    ALOGV("%s: X", __FUNCTION__);
@@ -1166,6 +1168,13 @@ status_t Camera3Device::waitUntilDrainedLocked() { 
    return res;

    return res;

}

}







void Camera3Device::internalUpdateStatusLocked(Status status) {

    mStatus = status;

    mRecentStatusUpdates.add(mStatus);

    mStatusChanged.broadcast();

}



// Pause to reconfigure

// Pause to reconfigure

status_t Camera3Device::internalPauseAndWaitLocked() {

status_t Camera3Device::internalPauseAndWaitLocked() {

    mRequestThread->setPaused(true);

    mRequestThread->setPaused(true);
@@ -1196,23 +1205,40 @@ status_t Camera3Device::internalResumeLocked() { 
    return OK;

    return OK;

}

}





status_t Camera3Device::waitUntilStateThenRelock(bool active,

status_t Camera3Device::waitUntilStateThenRelock(bool active, nsecs_t timeout) {

        nsecs_t timeout) {

    status_t res = OK;

    status_t res = OK;

    if (active == (mStatus == STATUS_ACTIVE)) {



        // Desired state already reached

    size_t startIndex = 0;

        return res;

    if (mStatusWaiters == 0) {

        // Clear the list of recent statuses if there are no existing threads waiting on updates to

        // this status list

        mRecentStatusUpdates.clear();

    } else {

        // If other threads are waiting on updates to this status list, set the position of the

        // first element that this list will check rather than clearing the list.

        startIndex = mRecentStatusUpdates.size();

    }

    }





    mStatusWaiters++;



    bool stateSeen = false;

    bool stateSeen = false;

    do {

    do {

        mRecentStatusUpdates.clear();

        if (active == (mStatus == STATUS_ACTIVE)) {

            // Desired state is current

            break;

        }





        res = mStatusChanged.waitRelative(mLock, timeout);

        res = mStatusChanged.waitRelative(mLock, timeout);

        if (res != OK) break;

        if (res != OK) break;





        // Check state change history during wait

        // This is impossible, but if not, could result in subtle deadlocks and invalid state

        for (size_t i = 0; i < mRecentStatusUpdates.size(); i++) {

        // transitions.

        LOG_ALWAYS_FATAL_IF(startIndex > mRecentStatusUpdates.size(),

                "%s: Skipping status updates in Camera3Device, may result in deadlock.",

                __FUNCTION__);



        // Encountered desired state since we began waiting

        for (size_t i = startIndex; i < mRecentStatusUpdates.size(); i++) {

            if (active == (mRecentStatusUpdates[i] == STATUS_ACTIVE) ) {

            if (active == (mRecentStatusUpdates[i] == STATUS_ACTIVE) ) {

                stateSeen = true;

                stateSeen = true;

                break;

                break;
@@ -1220,6 +1246,8 @@ status_t Camera3Device::waitUntilStateThenRelock(bool active, 
        }

        }

    } while (!stateSeen);

    } while (!stateSeen);





    mStatusWaiters--;



    return res;

    return res;

}

}
@@ -1461,9 +1489,7 @@ void Camera3Device::notifyStatus(bool idle) { 
        }

        }

        ALOGV("%s: Camera %d: Now %s", __FUNCTION__, mId,

        ALOGV("%s: Camera %d: Now %s", __FUNCTION__, mId,

                idle ? "idle" : "active");

                idle ? "idle" : "active");

        mStatus = idle ? STATUS_CONFIGURED : STATUS_ACTIVE;

        internalUpdateStatusLocked(idle ? STATUS_CONFIGURED : STATUS_ACTIVE);

        mRecentStatusUpdates.add(mStatus);

        mStatusChanged.signal();





        // Skip notifying listener if we're doing some user-transparent

        // Skip notifying listener if we're doing some user-transparent

        // state changes

        // state changes
@@ -1672,7 +1698,7 @@ status_t Camera3Device::configureStreamsLocked() { 




        // Return state to that at start of call, so that future configures

        // Return state to that at start of call, so that future configures

        // properly clean things up

        // properly clean things up

        mStatus = STATUS_UNCONFIGURED;

        internalUpdateStatusLocked(STATUS_UNCONFIGURED);

        mNeedConfig = true;

        mNeedConfig = true;





        ALOGV("%s: Camera %d: Stream configuration failed", __FUNCTION__, mId);

        ALOGV("%s: Camera %d: Stream configuration failed", __FUNCTION__, mId);
@@ -1719,11 +1745,8 @@ status_t Camera3Device::configureStreamsLocked() { 




    mNeedConfig = false;

    mNeedConfig = false;





    if (mDummyStreamId == NO_STREAM) {

    internalUpdateStatusLocked((mDummyStreamId == NO_STREAM) ?

        mStatus = STATUS_CONFIGURED;

            STATUS_CONFIGURED : STATUS_UNCONFIGURED);

    } else {

        mStatus = STATUS_UNCONFIGURED;

    }





    ALOGV("%s: Camera %d: Stream configuration complete", __FUNCTION__, mId);

    ALOGV("%s: Camera %d: Stream configuration complete", __FUNCTION__, mId);
@@ -1831,7 +1854,7 @@ void Camera3Device::setErrorStateLockedV(const char *fmt, va_list args) { 
    mErrorCause = errorCause;

    mErrorCause = errorCause;





    mRequestThread->setPaused(true);

    mRequestThread->setPaused(true);

    mStatus = STATUS_ERROR;

    internalUpdateStatusLocked(STATUS_ERROR);





    // Notify upstream about a device error

    // Notify upstream about a device error

    if (mListener != NULL) {

    if (mListener != NULL) {

services/camera/libcameraservice/device3/Camera3Device.h

+11 −0
Original line number Original line Diff line number Diff line
@@ -205,7 +205,11 @@ class Camera3Device : 
        STATUS_CONFIGURED,

        STATUS_CONFIGURED,

        STATUS_ACTIVE

        STATUS_ACTIVE

    }                          mStatus;

    }                          mStatus;



    // Only clear mRecentStatusUpdates, mStatusWaiters from waitUntilStateThenRelock

    Vector<Status>             mRecentStatusUpdates;

    Vector<Status>             mRecentStatusUpdates;

    int                        mStatusWaiters;



    Condition                  mStatusChanged;

    Condition                  mStatusChanged;





    // Tracking cause of fatal errors when in STATUS_ERROR

    // Tracking cause of fatal errors when in STATUS_ERROR
@@ -275,6 +279,13 @@ class Camera3Device : 
     */

     */

    virtual CameraMetadata getLatestRequestLocked();

    virtual CameraMetadata getLatestRequestLocked();





    /**

     * Update the current device status and wake all waiting threads.

     *

     * Must be called with mLock held.

     */

    void internalUpdateStatusLocked(Status status);



    /**

    /**

     * Pause processing and flush everything, but don't tell the clients.

     * Pause processing and flush everything, but don't tell the clients.

     * This is for reconfiguring outputs transparently when according to the

     * This is for reconfiguring outputs transparently when according to the