| Message ID | 20210521133054.274502-2-nfraprado@collabora.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Nícolas, Thank you for the patch. On Fri, May 21, 2021 at 10:30:50AM -0300, Nícolas F. R. A. Prado wrote: > Make Camera::stop() idempotent so that it can be called in any state and > consecutive times. When called in any state other than CameraRunning, it > is a no-op. This simplifies the cleanup path for applications. > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > --- > Changes in v4: > - Thanks to Jacopo: > - Added \todo in Camera::stop() > > No changes in v3 > > Changes in v2: > - Suggested by Kieran: > - Add new isRunning() function instead of relying on isAccessAllowed() > - Suggested by Laurent: > - Make stop()'s description clearer > > src/libcamera/camera.cpp | 24 +++++++++++++++++++++--- > 1 file changed, 21 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 1340c266cc5f..be0428ecbd46 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -347,6 +347,7 @@ public: > const std::set<Stream *> &streams); > ~Private(); > > + bool isRunning() const; > int isAccessAllowed(State state, bool allowDisconnected = false, > const char *from = __builtin_FUNCTION()) const; > int isAccessAllowed(State low, State high, > @@ -388,6 +389,15 @@ static const char *const camera_state_names[] = { > "Running", > }; > > +bool Camera::Private::isRunning() const > +{ > + State currentState = state_.load(std::memory_order_acquire); > + if (currentState == CameraRunning) > + return true; > + > + return false; You could write this return state_.load(std::memory_order_acquire) == CameraRunning; If that's the only change needed (and assuming you agree) I can handle this when applying the series. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > +} > + > int Camera::Private::isAccessAllowed(State state, bool allowDisconnected, > const char *from) const > { > @@ -1061,9 +1071,10 @@ int Camera::start(const ControlList *controls) > * This method stops capturing and processing requests immediately. All pending > * requests are cancelled and complete synchronously in an error state. > * > - * \context This function may only be called when the camera is in the Running > - * state as defined in \ref camera_operation, and shall be synchronized by the > - * caller with other functions that affect the camera state. > + * \context This function may be called in any camera state as defined in \ref > + * camera_operation, and shall be synchronized by the caller with other > + * functions that affect the camera state. If called when the camera isn't > + * running, it is a no-op. > * > * \return 0 on success or a negative error code otherwise > * \retval -ENODEV The camera has been disconnected from the system > @@ -1073,6 +1084,13 @@ int Camera::stop() > { > Private *const d = LIBCAMERA_D_PTR(); > > + /* > + * \todo Make calling stop() when not in 'Running' part of the state > + * machine rather than take this shortcut > + */ > + if (!d->isRunning()) > + return 0; > + > int ret = d->isAccessAllowed(Private::CameraRunning); > if (ret < 0) > return ret;
Hi Laurent, Em 2021-05-23 17:33, Laurent Pinchart escreveu: > Hi Nícolas, > > Thank you for the patch. > > On Fri, May 21, 2021 at 10:30:50AM -0300, Nícolas F. R. A. Prado wrote: > > Make Camera::stop() idempotent so that it can be called in any state and > > consecutive times. When called in any state other than CameraRunning, it > > is a no-op. This simplifies the cleanup path for applications. > > > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > > --- > > Changes in v4: > > - Thanks to Jacopo: > > - Added \todo in Camera::stop() > > > > No changes in v3 > > > > Changes in v2: > > - Suggested by Kieran: > > - Add new isRunning() function instead of relying on isAccessAllowed() > > - Suggested by Laurent: > > - Make stop()'s description clearer > > > > src/libcamera/camera.cpp | 24 +++++++++++++++++++++--- > > 1 file changed, 21 insertions(+), 3 deletions(-) > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > index 1340c266cc5f..be0428ecbd46 100644 > > --- a/src/libcamera/camera.cpp > > +++ b/src/libcamera/camera.cpp > > @@ -347,6 +347,7 @@ public: > > const std::set<Stream *> &streams); > > ~Private(); > > > > + bool isRunning() const; > > int isAccessAllowed(State state, bool allowDisconnected = false, > > const char *from = __builtin_FUNCTION()) const; > > int isAccessAllowed(State low, State high, > > @@ -388,6 +389,15 @@ static const char *const camera_state_names[] = { > > "Running", > > }; > > > > +bool Camera::Private::isRunning() const > > +{ > > + State currentState = state_.load(std::memory_order_acquire); > > + if (currentState == CameraRunning) > > + return true; > > + > > + return false; > > You could write this > > return state_.load(std::memory_order_acquire) == CameraRunning; > > If that's the only change needed (and assuming you agree) I can handle > this when applying the series. No need, I'll apply this for v5. Thanks, Nícolas > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > +} > > + > > int Camera::Private::isAccessAllowed(State state, bool allowDisconnected, > > const char *from) const > > { > > @@ -1061,9 +1071,10 @@ int Camera::start(const ControlList *controls) > > * This method stops capturing and processing requests immediately. All pending > > * requests are cancelled and complete synchronously in an error state. > > * > > - * \context This function may only be called when the camera is in the Running > > - * state as defined in \ref camera_operation, and shall be synchronized by the > > - * caller with other functions that affect the camera state. > > + * \context This function may be called in any camera state as defined in \ref > > + * camera_operation, and shall be synchronized by the caller with other > > + * functions that affect the camera state. If called when the camera isn't > > + * running, it is a no-op. > > * > > * \return 0 on success or a negative error code otherwise > > * \retval -ENODEV The camera has been disconnected from the system > > @@ -1073,6 +1084,13 @@ int Camera::stop() > > { > > Private *const d = LIBCAMERA_D_PTR(); > > > > + /* > > + * \todo Make calling stop() when not in 'Running' part of the state > > + * machine rather than take this shortcut > > + */ > > + if (!d->isRunning()) > > + return 0; > > + > > int ret = d->isAccessAllowed(Private::CameraRunning); > > if (ret < 0) > > return ret; > > -- > Regards, > > Laurent Pinchart > > -- > To unsubscribe, send mail to kernel-unsubscribe@lists.collabora.co.uk.
diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 1340c266cc5f..be0428ecbd46 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -347,6 +347,7 @@ public: const std::set<Stream *> &streams); ~Private(); + bool isRunning() const; int isAccessAllowed(State state, bool allowDisconnected = false, const char *from = __builtin_FUNCTION()) const; int isAccessAllowed(State low, State high, @@ -388,6 +389,15 @@ static const char *const camera_state_names[] = { "Running", }; +bool Camera::Private::isRunning() const +{ + State currentState = state_.load(std::memory_order_acquire); + if (currentState == CameraRunning) + return true; + + return false; +} + int Camera::Private::isAccessAllowed(State state, bool allowDisconnected, const char *from) const { @@ -1061,9 +1071,10 @@ int Camera::start(const ControlList *controls) * This method stops capturing and processing requests immediately. All pending * requests are cancelled and complete synchronously in an error state. * - * \context This function may only be called when the camera is in the Running - * state as defined in \ref camera_operation, and shall be synchronized by the - * caller with other functions that affect the camera state. + * \context This function may be called in any camera state as defined in \ref + * camera_operation, and shall be synchronized by the caller with other + * functions that affect the camera state. If called when the camera isn't + * running, it is a no-op. * * \return 0 on success or a negative error code otherwise * \retval -ENODEV The camera has been disconnected from the system @@ -1073,6 +1084,13 @@ int Camera::stop() { Private *const d = LIBCAMERA_D_PTR(); + /* + * \todo Make calling stop() when not in 'Running' part of the state + * machine rather than take this shortcut + */ + if (!d->isRunning()) + return 0; + int ret = d->isAccessAllowed(Private::CameraRunning); if (ret < 0) return ret;
Make Camera::stop() idempotent so that it can be called in any state and consecutive times. When called in any state other than CameraRunning, it is a no-op. This simplifies the cleanup path for applications. Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> --- Changes in v4: - Thanks to Jacopo: - Added \todo in Camera::stop() No changes in v3 Changes in v2: - Suggested by Kieran: - Add new isRunning() function instead of relying on isAccessAllowed() - Suggested by Laurent: - Make stop()'s description clearer src/libcamera/camera.cpp | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-)