[libcamera-devel] [PATCH v4 32/32] libcamera: camera: Remove the prepared state
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Jan 12 14:42:48 CET 2020
Hi Niklas,
Thank you for the patch.
On Sun, Jan 12, 2020 at 02:02:12AM +0100, Niklas Söderlund wrote:
> With the FrameBuffer rework completed there is no reason to keep the
> camera prepared state around as buffer allocations are now decoupled
> from the camera state. Remove the camera state simplifying the API.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> include/libcamera/camera.h | 4 -
> src/android/camera_device.cpp | 11 +--
> src/cam/capture.cpp | 8 --
> src/libcamera/camera.cpp | 100 +++++-------------------
> src/libcamera/framebuffer_allocator.cpp | 5 +-
> src/libcamera/pipeline_handler.cpp | 5 ++
> src/qcam/main_window.cpp | 9 ---
> src/v4l2/v4l2_camera.cpp | 5 --
> test/camera/buffer_import.cpp | 10 ---
> test/camera/capture.cpp | 10 ---
> test/camera/statemachine.cpp | 83 --------------------
> 11 files changed, 27 insertions(+), 223 deletions(-)
>
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index 28655bec9ebc89ce..6597ade83288a170 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -91,9 +91,6 @@ public:
> std::unique_ptr<CameraConfiguration> generateConfiguration(const StreamRoles &roles);
> int configure(CameraConfiguration *config);
>
> - int allocateBuffers();
> - int freeBuffers();
> -
> Request *createRequest(uint64_t cookie = 0);
> int queueRequest(Request *request);
>
> @@ -105,7 +102,6 @@ private:
> CameraAvailable,
> CameraAcquired,
> CameraConfigured,
> - CameraPrepared,
> CameraRunning,
> };
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 49321db07a2c93d5..a98fd744f5347432 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -77,8 +77,6 @@ int CameraDevice::open()
> void CameraDevice::close()
> {
> camera_->stop();
> -
> - camera_->freeBuffers();
> camera_->release();
>
> running_ = false;
> @@ -690,16 +688,9 @@ void CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reque
>
> /* Start the camera if that's the first request we handle. */
> if (!running_) {
> - int ret = camera_->allocateBuffers();
> - if (ret) {
> - LOG(HAL, Error) << "Failed to allocate buffers";
> - return;
> - }
> -
> - ret = camera_->start();
> + int ret = camera_->start();
> if (ret) {
> LOG(HAL, Error) << "Failed to start camera";
> - camera_->freeBuffers();
> return;
> }
>
> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> index 738fa1c267eb6e36..7d970f991d3aaf1a 100644
> --- a/src/cam/capture.cpp
> +++ b/src/cam/capture.cpp
> @@ -42,12 +42,6 @@ int Capture::run(EventLoop *loop, const OptionsParser::Options &options)
> return ret;
> }
>
> - ret = camera_->allocateBuffers();
> - if (ret) {
> - std::cerr << "Failed to allocate buffers" << std::endl;
> - return ret;
> - }
> -
> camera_->requestCompleted.connect(this, &Capture::requestComplete);
>
> if (options.isSet(OptFile)) {
> @@ -67,8 +61,6 @@ int Capture::run(EventLoop *loop, const OptionsParser::Options &options)
> writer_ = nullptr;
> }
>
> - camera_->freeBuffers();
> -
> delete allocator;
>
> return ret;
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index f3a7578d0834a9d6..79a5f994f9bbc8c1 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -275,15 +275,13 @@ std::size_t CameraConfiguration::size() const
> * \section camera_operation Operating the Camera
> *
> * An application needs to perform a sequence of operations on a camera before
> - * it is ready to process requests. The camera needs to be acquired, configured
> - * and resources allocated or imported to prepare the camera for capture. Once
> - * started the camera can process requests until it is stopped. When an
> - * application is done with a camera all resources allocated need to be freed
> - * and the camera released.
> + * it is ready to process requests. The camera needs to be acquired and
> + * configured to prepare the camera for capture. Once started the camera can
> + * process requests until it is stopped. When an application is done with a
> + * camera, the camera needs to be released.
> *
> * An application may start and stop a camera multiple times as long as it is
> - * not released. The camera may also be reconfigured provided that all
> - * resources allocated are freed prior to the reconfiguration.
> + * not released. The camera may also be reconfigured.
> *
> * \subsection Camera States
> *
> @@ -297,7 +295,6 @@ std::size_t CameraConfiguration::size() const
> * node [shape = doublecircle ]; Available;
> * node [shape = circle ]; Acquired;
> * node [shape = circle ]; Configured;
> - * node [shape = circle ]; Prepared;
> * node [shape = circle ]; Running;
> *
> * Available -> Available [label = "release()"];
> @@ -307,14 +304,10 @@ std::size_t CameraConfiguration::size() const
> * Acquired -> Configured [label = "configure()"];
> *
> * Configured -> Available [label = "release()"];
> - * Configured -> Configured [label = "configure()"];
> - * Configured -> Prepared [label = "allocateBuffers()"];
> + * Configured -> Configured [label = "configure(), createRequest()"];
> + * Configured -> Running [label = "start()"];
> *
> - * Prepared -> Configured [label = "freeBuffers()"];
> - * Prepared -> Prepared [label = "createRequest()"];
> - * Prepared -> Running [label = "start()"];
> - *
> - * Running -> Prepared [label = "stop()"];
> + * Running -> Configured [label = "stop()"];
> * Running -> Running [label = "createRequest(), queueRequest()"];
> * }
> * \enddot
> @@ -330,19 +323,14 @@ std::size_t CameraConfiguration::size() const
> * Configured state.
> *
> * \subsubsection Configured
> - * The camera is configured and ready for the application to prepare it with
> - * resources. The camera may be reconfigured multiple times until resources
> - * are provided and the state progresses to Prepared.
> - *
> - * \subsubsection Prepared
> - * The camera has been configured and provided with resources and is ready to be
> - * started. The application may free the camera's resources to get back to the
> - * Configured state or start() it to progress to the Running state.
> + * The camera is configured and ready to be started. The application may
> + * release() the camera and to get back to the Available state or start()
> + * it to progress to the Running state.
> *
> * \subsubsection Running
> * The camera is running and ready to process requests queued by the
> * application. The camera remains in this state until it is stopped and moved
> - * to the Prepared state.
> + * to the Configured state.
> */
>
> /**
> @@ -420,7 +408,6 @@ static const char *const camera_state_names[] = {
> "Available",
> "Acquired",
> "Configured",
> - "Prepared",
> "Running",
> };
>
> @@ -465,8 +452,6 @@ bool Camera::stateIs(State state) const
> *
> * \todo Deal with pending requests if the camera is disconnected in a
> * running state.
> - * \todo Update comment about Running state when importing buffers as well as
> - * allocating them are supported.
> */
> void Camera::disconnect()
> {
> @@ -474,11 +459,11 @@ void Camera::disconnect()
>
> /*
> * If the camera was running when the hardware was removed force the
> - * state to Prepared to allow applications to call freeBuffers() and
> - * release() before deleting the camera.
> + * state to Configured state to allow applications to free resources
> + * and call release() before deleting the camera.
> */
> if (state_ == CameraRunning)
> - state_ = CameraPrepared;
> + state_ = CameraConfigured;
>
> disconnected_ = true;
> disconnected.emit(this);
> @@ -702,53 +687,6 @@ int Camera::configure(CameraConfiguration *config)
> return 0;
> }
>
> -/**
> - * \brief Allocate buffers for all configured streams
> - *
> - * This function affects the state of the camera, see \ref camera_operation.
> - *
> - * \return 0 on success or a negative error code otherwise
> - * \retval -ENODEV The camera has been disconnected from the system
> - * \retval -EACCES The camera is not in a state where buffers can be allocated
> - * \retval -EINVAL The configuration is not valid
> - */
> -int Camera::allocateBuffers()
> -{
> - if (disconnected_)
> - return -ENODEV;
> -
> - if (!stateIs(CameraConfigured))
> - return -EACCES;
> -
> - if (activeStreams_.empty()) {
> - LOG(Camera, Error)
> - << "Can't allocate buffers without streams";
> - return -EINVAL;
> - }
> -
> - state_ = CameraPrepared;
> -
> - return 0;
> -}
> -
> -/**
> - * \brief Release all buffers from allocated pools in each stream
> - *
> - * This function affects the state of the camera, see \ref camera_operation.
> - *
> - * \return 0 on success or a negative error code otherwise
> - * \retval -EACCES The camera is not in a state where buffers can be freed
> - */
> -int Camera::freeBuffers()
> -{
> - if (!stateIs(CameraPrepared))
> - return -EACCES;
> -
> - state_ = CameraConfigured;
> -
> - return 0;
> -}
> -
> /**
> * \brief Create a request object for the camera
> * \param[in] cookie Opaque cookie for application use
> @@ -764,14 +702,14 @@ int Camera::freeBuffers()
> * The ownership of the returned request is passed to the caller, which is
> * responsible for either queueing the request or deleting it.
> *
> - * This function shall only be called when the camera is in the Prepared
> + * This function shall only be called when the camera is in the Configured
> * or Running state, see \ref camera_operation.
> *
> * \return A pointer to the newly created request, or nullptr on error
> */
> Request *Camera::createRequest(uint64_t cookie)
> {
> - if (disconnected_ || !stateBetween(CameraPrepared, CameraRunning))
> + if (disconnected_ || !stateBetween(CameraConfigured, CameraRunning))
> return nullptr;
>
> return new Request(this, cookie);
> @@ -842,7 +780,7 @@ int Camera::start()
> if (disconnected_)
> return -ENODEV;
>
> - if (!stateIs(CameraPrepared))
> + if (!stateIs(CameraConfigured))
> return -EACCES;
>
> LOG(Camera, Debug) << "Starting capture";
> @@ -885,7 +823,7 @@ int Camera::stop()
>
> LOG(Camera, Debug) << "Stopping capture";
>
> - state_ = CameraPrepared;
> + state_ = CameraConfigured;
>
> pipe_->stop(this);
>
> diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp
> index 57789b24e9615fa6..207a13bd841da2b8 100644
> --- a/src/libcamera/framebuffer_allocator.cpp
> +++ b/src/libcamera/framebuffer_allocator.cpp
> @@ -116,8 +116,7 @@ FrameBufferAllocator::~FrameBufferAllocator()
> */
> int FrameBufferAllocator::allocate(Stream *stream)
> {
> - if (camera_->state_ != Camera::CameraConfigured &&
> - camera_->state_ != Camera::CameraPrepared) {
> + if (camera_->state_ != Camera::CameraConfigured) {
> LOG(Allocator, Error)
> << "Camera must be in the configured state to allocate buffers";
> return -EACCES;
> @@ -163,7 +162,7 @@ int FrameBufferAllocator::allocate(Stream *stream)
> */
> int FrameBufferAllocator::free(Stream *stream)
> {
> - if (camera_->state_ != Camera::CameraConfigured && camera_->state_ != Camera::CameraPrepared) {
> + if (camera_->state_ != Camera::CameraConfigured) {
> LOG(Allocator, Error)
> << "Camera must be in the configured state to free buffers";
> return -EACCES;
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 2fd65b468b9cd84f..669097f609ab7168 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -301,6 +301,11 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera)
> * suitable to be added to a Request for the stream, and shall be mappable to
> * the CPU through their associated dmabufs with mmap().
> *
> + * The method may only be called after the Camera has been configured and before
> + * it gets started, or after it gets stopped. It shall be called only for
> + * streams that are part of the active camera configuration, and at most once
> + * per stream until buffers for the stream are freed with freeFrameBuffers().
> + *
Ah, that's where the documentation went :-) You just need to move it to
the right patch. All the rest is fine here.
> * exportFrameBuffers() shall also allocate all other resources required by
> * the pipeline handler for the stream to prepare for starting the Camera. This
> * responsibility is shared with importFrameBuffers(), and one and only one of
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index 047bf15ea7c188f7..1d9c756f147a59f7 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -172,13 +172,6 @@ int MainWindow::startCapture()
>
> adjustSize();
>
> - ret = camera_->allocateBuffers();
> - if (ret) {
> - std::cerr << "Failed to allocate buffers"
> - << std::endl;
> - return ret;
> - }
> -
> ret = allocator_->allocate(stream);
> if (ret < 0) {
> std::cerr << "Failed to allocate capture buffers" << std::endl;
> @@ -244,7 +237,6 @@ error:
> }
> mappedBuffers_.clear();
>
> - camera_->freeBuffers();
> return ret;
> }
>
> @@ -264,7 +256,6 @@ void MainWindow::stopCapture()
> }
> mappedBuffers_.clear();
>
> - camera_->freeBuffers();
> isCapturing_ = false;
>
> config_.reset();
> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> index 10db15d6276d9bf3..44cb4e7c551be759 100644
> --- a/src/v4l2/v4l2_camera.cpp
> +++ b/src/v4l2/v4l2_camera.cpp
> @@ -121,10 +121,6 @@ int V4L2Camera::configure(StreamConfiguration *streamConfigOut,
>
> int V4L2Camera::allocBuffers(unsigned int count)
> {
> - int ret = camera_->allocateBuffers();
> - if (ret)
> - return ret == -EACCES ? -EBUSY : ret;
> -
> Stream *stream = *camera_->streams().begin();
>
> return bufferAllocator_->allocate(stream);
> @@ -134,7 +130,6 @@ void V4L2Camera::freeBuffers()
> {
> Stream *stream = *camera_->streams().begin();
> bufferAllocator_->free(stream);
> - camera_->freeBuffers();
> }
>
> FileDescriptor V4L2Camera::getBufferFd(unsigned int index)
> diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp
> index f506d1b221e568ad..e7048335e0317703 100644
> --- a/test/camera/buffer_import.cpp
> +++ b/test/camera/buffer_import.cpp
> @@ -174,11 +174,6 @@ protected:
> return TestFail;
> }
>
> - if (camera_->allocateBuffers()) {
> - std::cout << "Failed to allocate buffers" << std::endl;
> - return TestFail;
> - }
> -
> Stream *stream = cfg.stream();
>
> BufferSource source;
> @@ -244,11 +239,6 @@ protected:
> return TestFail;
> }
>
> - if (camera_->freeBuffers()) {
> - std::cout << "Failed to free buffers" << std::endl;
> - return TestFail;
> - }
> -
> return TestPass;
> }
>
> diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> index de879ee4eb1420a6..b304d59c1c2aa9e2 100644
> --- a/test/camera/capture.cpp
> +++ b/test/camera/capture.cpp
> @@ -87,11 +87,6 @@ protected:
> return TestFail;
> }
>
> - if (camera_->allocateBuffers()) {
> - cout << "Failed to allocate buffers" << endl;
> - return TestFail;
> - }
> -
> Stream *stream = cfg.stream();
>
> int ret = allocator_->allocate(stream);
> @@ -158,11 +153,6 @@ protected:
> return TestFail;
> }
>
> - if (camera_->freeBuffers()) {
> - cout << "Failed to free buffers" << endl;
> - return TestFail;
> - }
> -
> return TestPass;
> }
>
> diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp
> index f3a7ca7c32a5ec97..20541b3e4752dc81 100644
> --- a/test/camera/statemachine.cpp
> +++ b/test/camera/statemachine.cpp
> @@ -29,12 +29,6 @@ protected:
> if (camera_->configure(defconf_.get()) != -EACCES)
> return TestFail;
>
> - if (camera_->allocateBuffers() != -EACCES)
> - return TestFail;
> -
> - if (camera_->freeBuffers() != -EACCES)
> - return TestFail;
> -
> if (camera_->createRequest())
> return TestFail;
>
> @@ -65,12 +59,6 @@ protected:
> if (camera_->acquire() != -EBUSY)
> return TestFail;
>
> - if (camera_->allocateBuffers() != -EACCES)
> - return TestFail;
> -
> - if (camera_->freeBuffers() != -EACCES)
> - return TestFail;
> -
> if (camera_->createRequest())
> return TestFail;
>
> @@ -103,57 +91,6 @@ protected:
> if (camera_->acquire() != -EBUSY)
> return TestFail;
>
> - if (camera_->freeBuffers() != -EACCES)
> - return TestFail;
> -
> - if (camera_->createRequest())
> - return TestFail;
> -
> - Request request(camera_.get());
> - if (camera_->queueRequest(&request) != -EACCES)
> - return TestFail;
> -
> - if (camera_->start() != -EACCES)
> - return TestFail;
> -
> - if (camera_->stop() != -EACCES)
> - return TestFail;
> -
> - /* Test operations which should pass. */
> - if (camera_->configure(defconf_.get()))
> - return TestFail;
> -
> - /* Test valid state transitions, end in Prepared state. */
> - if (camera_->release())
> - return TestFail;
> -
> - if (camera_->acquire())
> - return TestFail;
> -
> - if (camera_->configure(defconf_.get()))
> - return TestFail;
> -
> - if (camera_->allocateBuffers())
> - return TestFail;
> -
> - return TestPass;
> - }
> -
> - int testPrepared()
> - {
> - /* Test operations which should fail. */
> - if (camera_->acquire() != -EBUSY)
> - return TestFail;
> -
> - if (camera_->release() != -EBUSY)
> - return TestFail;
> -
> - if (camera_->configure(defconf_.get()) != -EACCES)
> - return TestFail;
> -
> - if (camera_->allocateBuffers() != -EACCES)
> - return TestFail;
> -
> Request request1(camera_.get());
> if (camera_->queueRequest(&request1) != -EACCES)
> return TestFail;
> @@ -170,9 +107,6 @@ protected:
> delete request2;
>
> /* Test valid state transitions, end in Running state. */
> - if (camera_->freeBuffers())
> - return TestFail;
> -
> if (camera_->release())
> return TestFail;
>
> @@ -182,9 +116,6 @@ protected:
> if (camera_->configure(defconf_.get()))
> return TestFail;
>
> - if (camera_->allocateBuffers())
> - return TestFail;
> -
> /* Use internally allocated buffers. */
> allocator_ = FrameBufferAllocator::create(camera_);
> Stream *stream = *camera_->streams().begin();
> @@ -209,12 +140,6 @@ protected:
> if (camera_->configure(defconf_.get()) != -EACCES)
> return TestFail;
>
> - if (camera_->allocateBuffers() != -EACCES)
> - return TestFail;
> -
> - if (camera_->freeBuffers() != -EACCES)
> - return TestFail;
> -
> if (camera_->start() != -EACCES)
> return TestFail;
>
> @@ -236,9 +161,6 @@ protected:
>
> delete allocator_;
>
> - if (camera_->freeBuffers())
> - return TestFail;
> -
> if (camera_->release())
> return TestFail;
>
> @@ -276,11 +198,6 @@ protected:
> return TestFail;
> }
>
> - if (testPrepared() != TestPass) {
> - cout << "State machine in Prepared state failed" << endl;
> - return TestFail;
> - }
> -
> if (testRuning() != TestPass) {
> cout << "State machine in Running state failed" << endl;
> return TestFail;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list