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

[Laurent Pinchart]

On Wed, Aug 03, 2022 at 08:52:01AM +0300, Laurent Pinchart via libcamera-devel wrote:
> 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).

Another point I forgot to mention, don't we need an API to let
applications query mutual exclusion constraints between cameras ?

> >   */
> >  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