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

16901 diff mbox series

From: Harvey Yang <chenghaoyang@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.

Signed-off-by: Harvey Yang <chenghaoyang@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(+)

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@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@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

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@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@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

Hi Laurent,

Thanks for your comments!

On Wed, Aug 3, 2022 at 1:58 PM Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> 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@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.
> >


Before I update the commit message, let's go through the issues below first
:)


>
> > 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.
> >


So Han-lin and I have never tested soraka (with IPU3) on using both cameras
simultaneously. Do you mean that it never works, even if we assign one ImgU
to each camera in PipelineHandlerIPU3?
Is the culprit here [1]?

[1]:
https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/ipu3/ipu3.cpp#n550


>
> > 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 ?
> >


Which web app did you use? I didn't find this feature on Google Meet
though...
I'd like to try how the current libcamera on soraka support this feature :)


>
> > 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.
> >


You're talking about the current situation of IPU3 ImgUs, right?
I thought that libcamera's PipelineHandlerIPU3 only uses the two ImgUs in
video mode? Is the constraint mentioned somewhere in the code or
documentation (in libcamera or the shipped HAL)?


>
> > > Signed-off-by: Harvey Yang <chenghaoyang@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 ?
>
>
I see. Sorry that Han-lin and I didn't fully understand how
libcamera::Camera::acquire() works.

As currently the Android adapter calls libcamera::Camera::start() only when
the first frame request comes, I don't think it's a proper code path that
we tell the user/application about the mutual exclusion constraints (i.e.
fail the start operation).
Therefore, I agree that a new API to let applications query mutual
exclusion constraints makes more sense. In the Android API, we have
|conflicting_devices| [2] and |resource_cost| [3]. Do you think it's
appropriate to simply add the same API in libcamera?

[2]: https://source.android.com/devices/camera/versioning#24
[3]:
https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/main/camera/mojo/camera_common.mojom#30


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

Thanks again!