[libcamera-devel] [PATCH v4 2/9] ipu3: Allow only one camera being started

[Laurent Pinchart]

Hi Harvey,

Thank you for the patch.

On Tue, Aug 02, 2022 at 10:29:36AM +0000, Harvey Yang via libcamera-devel wrote:
> From: Harvey Yang <chenghaoyang at chromium.org>
> 
> As we hardly have use cases/applications that need both cameras at the
> same time, this patch adds a rule that only one camera can be started
> one time. This also allows the following patches that use both imgus to
> process frames from one single camera.

While I think this is right for the IPU3, this is the wrong reason. The
fact that Chrome OS has little use for capturing from the two cameras at
the same time doesn't meant there are no use cases on other platforms.

The reason why I think this makes sense is that the two ImgU instances
are actually not independent, both need to be configured before any gets
started. That conflicts with how the libcamera API operates. I would
like to not suffer from this limitation though, but that would require
broader changes in the public API, I think it's fine to limit operation
to one camera at a time for now.

On a side note, when I select a camera in a video call in the Chrome
browser, I get a small preview of each available camera. Isn't this a
use case that is of interest to Chrome OS ?

Kieran told me that this works under Windows on a Surface Go 2.
Technically speaking, I don't know how we could implement it though, as
far as I understand, only one instance of the ImgU can operate in video
mode and guarantee real time operation, while the other instance needs
to operate in still image capture mode and will work in "batch" mode,
using the free processing time of the hardware that isn't reserved for
video mode.

> Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> ---
>  include/libcamera/internal/pipeline_handler.h |  5 ++++
>  src/libcamera/camera.cpp                      | 11 ++++++++
>  src/libcamera/pipeline/ipu3/ipu3.cpp          | 18 +++++++++++++
>  src/libcamera/pipeline_handler.cpp            | 26 +++++++++++++++++++
>  4 files changed, 60 insertions(+)

This patch should be split in two, with one patch that extends the
Camera and PipelineHandler API, and a second patch to implement it in
the IPU3 pipeline handler.

> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index c3e4c258..8277ceda 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -55,6 +55,9 @@ public:
>  	virtual int exportFrameBuffers(Camera *camera, Stream *stream,
>  				       std::vector<std::unique_ptr<FrameBuffer>> *buffers) = 0;
>  
> +	virtual bool acquire(Camera *camera);
> +	virtual void release(Camera *camera);
> +
>  	virtual int start(Camera *camera, const ControlList *controls) = 0;
>  	void stop(Camera *camera);
>  	bool hasPendingRequests(const Camera *camera) const;
> @@ -76,6 +79,8 @@ protected:
>  
>  	CameraManager *manager_;
>  
> +	std::set<Camera*> acquiredCameras_;

Space between Camera and *.

> +
>  private:
>  	void mediaDeviceDisconnected(MediaDevice *media);
>  	virtual void disconnect();
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 713543fd..a31f8769 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -826,6 +826,8 @@ int Camera::exportFrameBuffers(Stream *stream,
>   * \return 0 on success or a negative error code otherwise
>   * \retval -ENODEV The camera has been disconnected from the system
>   * \retval -EBUSY The camera is not free and can't be acquired by the caller
> + * \retval -EUSERS The maximum number of cameras that can be opened concurrently
> + * were opened already

That's not how acquire() should work. The purpose of acquire() is to
reserve a camera for usage by the application, preventing other
applications that use libcamera from doing the same. It's a simple
locking mechanism really. With this change, it won't be possible for an
application to acquire the front and back cameras. This will cause
issues, for instance, when switching between the front and back cameras
in an application, if the application wants to acquire the other camera
before releasing the current one, to make sure that, if the acquire
fails, the current camera can still be used. It also breaks the use case
of acquiring multiple cameras to switch between them while preventing
other applications from using any.

I'd like to see a design proposal for how the public API should evolve
to make mutual exclusion possible, while keeping use cases such as the
above supported (unless, of course, you show that those use cases
actually don't make sense, in which case we could reconsider them).

>   */
>  int Camera::acquire()
>  {
> @@ -845,6 +847,14 @@ int Camera::acquire()
>  		return -EBUSY;
>  	}
>  
> +	if (!d->pipe_->acquire(this)) {
> +		LOG(Camera, Info)
> +			<< "The maximum number of cameras that can be opened"
> +			   "concurrently were opened already";
> +		d->pipe_->unlock();
> +		return -EUSERS;
> +	}
> +
>  	d->setState(Private::CameraAcquired);
>  
>  	return 0;
> @@ -873,6 +883,7 @@ int Camera::release()
>  	if (ret < 0)
>  		return ret == -EACCES ? -EBUSY : ret;
>  
> +	d->pipe_->release(this);
>  	d->pipe_->unlock();
>  
>  	d->setState(Private::CameraAvailable);
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index fd989e61..091a40e1 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -141,6 +141,8 @@ public:
>  	int exportFrameBuffers(Camera *camera, Stream *stream,
>  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
>  
> +	bool acquire(Camera *camera) override;
> +
>  	int start(Camera *camera, const ControlList *controls) override;
>  	void stopDevice(Camera *camera) override;
>  
> @@ -763,8 +765,24 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)
>  	return 0;
>  }
>  
> +bool PipelineHandlerIPU3::acquire(Camera *camera) {

The curly brace should be on the next line.

> +	/*
> +	 * Enforce that only a single camera can be used at a time to use both
> +	 * ImgUs on the camera, so that StillCapture stream can adopt another
> +	 * set of configuration.
> +	 */
> +	if (!acquiredCameras_.empty())

Given that the set can only contain a single camera, you could replace
it with just a pointer.

> +		return false;
> +
> +	return PipelineHandler::acquire(camera);
> +
> +}
> +
>  int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlList *controls)
>  {
> +	if (acquiredCameras_.empty() || *acquiredCameras_.begin() != camera)
> +		return -EUSERS;
> +
>  	IPU3CameraData *data = cameraData(camera);
>  	CIO2Device *cio2 = &data->cio2_;
>  	ImgUDevice *imgu = data->imgu_;
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 7ebd76ad..fff9ee59 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -270,6 +270,32 @@ void PipelineHandler::unlock()
>   * otherwise
>   */
>  
> +/**
> + * \fn PipelineHandler::acquire()
> + * \brief Check if \a camera can be acquired and supported with the current
> + * pipeline handler usage. If \a camera has already been acquired (by the same
> + * or another process), return false.
> + * \param[in] camera The camera
> + */
> +bool PipelineHandler::acquire(Camera *camera)
> +{
> +	if (acquiredCameras_.find(camera) != acquiredCameras_.end())
> +		return false;
> +
> +	acquiredCameras_.emplace(camera);
> +	return true;
> +}
> +
> +/**
> + * \fn PipelineHandler::release()
> + * \brief Release exclusive access to \a camera
> + * \param[in] camera The camera
> + */
> +void PipelineHandler::release(Camera *camera)
> +{
> +	acquiredCameras_.erase(camera);
> +}
> +
>  /**
>   * \fn PipelineHandler::start()
>   * \brief Start capturing from a group of streams

-- 
Regards,

Laurent Pinchart