[libcamera-devel] [PATCH v2 1/2] pipeline: raspberrypi: Move sensor entity detection out of registerCamera()
Naushir Patuck
naush at raspberrypi.com
Fri Dec 10 09:38:32 CET 2021
Hi Laurent,
On Thu, 9 Dec 2021 at 23:48, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> Hello,
>
> On Thu, Dec 09, 2021 at 09:27:37AM +0000, Naushir Patuck wrote:
> > On Thu, 9 Dec 2021 at 09:20, Kieran Bingham wrote:
> > > Quoting Naushir Patuck (2021-12-09 08:57:52)
> > > > On Wed, 8 Dec 2021 at 18:40, Laurent Pinchart wrote:
> > > > > On Wed, Dec 08, 2021 at 03:15:26PM +0000, Naushir Patuck wrote:
> > > > > > Enumerate the sensor device entities in
> PipelineHandlerRPi::match() and loop
> > > > > > over PipelineHandlerRPi::registerCamera() for each sensor found.
> This will
> > > > > > allow the pipeline handler to register multiple cameras attached
> to a single
> > > > > > Unicam instance with a Video Mux device.
> > > > > >
> > > > > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > > > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > > > > ---
> > > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 35
> +++++++++++--------
> > > > > > 1 file changed, 20 insertions(+), 15 deletions(-)
> > > > > >
> > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > > index 86851ac467ad..756878c70036 100644
> > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > > @@ -310,7 +310,7 @@ private:
> > > > > > return static_cast<RPiCameraData *>(camera->_d());
> > > > > > }
> > > > > >
> > > > > > - int registerCamera(MediaDevice *unicam, MediaDevice *isp);
> > > > > > + int registerCamera(MediaDevice *unicam, MediaDevice *isp,
> MediaEntity *sensorEntity);
> > > > > > int queueAllBuffers(Camera *camera);
> > > > > > int prepareBuffers(Camera *camera);
> > > > > > void freeBuffers(Camera *camera);
> > > > > > @@ -1029,16 +1029,28 @@ bool
> PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
> > > > > > return false;
> > > > > > }
> > > > > >
> > > > > > - int ret = registerCamera(unicamDevice, ispDevice);
> > > > > > - if (ret) {
> > > > > > - LOG(RPI, Error) << "Failed to register camera: "
> << ret;
> > > > > > - return false;
> > > > > > + /*
> > > > > > + * The loop below is used to register multiple cameras
> behind one or more
> > > > > > + * video mux devices that are attaced to a particular
> Unicam instance.
> > > > >
> > > > > s/attaced/attached/
> > > > >
> > > > > > + * Obviously these cameras cannot be used simultaneously.
> > > > >
> > > > > We need to expose mutual exclusion between cameras to applications
> > > > > through the libcamera public API. It doesn't have to be a blocker
> for
> > > > > the series, but should be done soon after, otherwise trying to use
> both
> > > > > cameras will result in incorrect behaviour (and possibly even
> crashes).
> > > > > Have you given any thought to how this could be done ?
> > > >
> > > > Is this not already handled in the Camera class? If I have a mux
> with 2 devices
> > > > attached to a single Unicam instance, hence single pipeline handler,
> I cannot
> > > > run camera in one process and camera 1 in another.
> Camera::acquire() would
> > > > vail to lock the pipeline handler:
> > > >
> > > > if (!d->pipe_->lock()) {
> > > > LOG(Camera, Info)
> > > > << "Pipeline handler in use by another process";
> > > > return -EBUSY;
> > > > }
> > > >
> > > > Perhaps this long point should be turned into Error level? I get the
> same result
> > > > if I try to run both cameras in a single process (e.g. with cam -c 1
> -C -c 2 -C).
> > >
> > > Hrm ... this seems like a bug somewhere at core level then. A single
> > > pipeline handler should be able to support multiple cameras, (when it
> is
> > > able to)...
> >
> > Laurent did point me to a WIP branch of his that supports multi-camera
> setups.
> > I have not looked at it yet, but perhaps this is fixed there.
>
> That's right. There's work in progress to suppose usage of multiple
> cameras from the same pipeline handler. It will unlock valid use cases,
> but will require pipeline handlers that don't support concurrent usage
> of cameras to implement mutual exclusion.
>
> > > That's why we have per-camera CameraData isn't it ?
> > >
> > > But if we 'fix' that bug, we should indeed make it easy for pipeline
> > > handlers to 'specify' the access controls to their cameras...
> > >
> > > Or perhaps the intention was to have one pipeline handler per camera
> > > that can be accessed independently? After all, the matching does keep
> > > calling until the match() returns false... so it will keep retrying to
> > > keep constructing a new instance of the pipeline handler each time?
> >
> > On my CM4 setup, I was able to run "cam -c 1 -C -c 2 -C" and successfully
> > stream from both sensors. So presumably that was setting up 2x pipeline
> > handlers and working independently. This was on the master branch, not
> > Laurent's WIP branch. I should go back and make sure that still works!
>
> Correct, with one pipeline handler instance per camera, that will work
> nicely out of the box. It's one of the perks of implementing support for
> multiple Unicam instances through multiple pipeline handler instances.
>
> The situation becomes more complex with a single pipeline handler
> instance that registers multiple cameras that can still be used
> concurrently.
>
> > > > > > + */
> > > > > > + unsigned int numCameras = 0;
> > > > > > + for (MediaEntity *entity : unicamDevice->entities()) {
> > > > > > + if (entity->function() != MEDIA_ENT_F_CAM_SENSOR)
> > > > > > + continue;
> > > > > > +
> > > > > > + int ret = registerCamera(unicamDevice, ispDevice,
> entity);
> > > > > > + if (ret)
> > > > > > + LOG(RPI, Error) << "Failed to register
> camera "
> > > > > > + << entity->name() << ": "
> << ret;
> > > > > > + else
> > > > > > + numCameras++;
> > > > >
> > > > > I tend to deal with error first, but that's a personal preference:
> > > > >
> > > > > int ret = registerCamera(unicamDevice, ispDevice,
> entity);
> > > > > if (ret) {
> > > > > LOG(RPI, Error) << "Failed to register
> camera "
> > > > > << entity->name() << ": "
> << ret;
> > > > > continue;
> > > > > }
> > > > >
> > > > > numCameras++;
> > > > >
> > > > > > }
> > > > > >
> > > > > > - return true;
> > > > > > + return !!numCameras;
> > > > >
> > > > > This looks fine, but by itself may cause a regression. Patch 2/2
> will be
> > > > > merged with this one, so the problem would only occur during
> bisection,
> > > > > but still, I think we should squash the two patches together.
> > > >
> > > > I'm sure I am missing something obvious, but why would this patch in
> isolation
> > > > cause a regression? The only additional function gained here would
> be to
> > > > allow the pipeline handler to enumerate multiple cameras attached to
> a mux,
> > > > but not allow one of the cameras to be run - same as before this
> change :-)
> > >
> > > I can't see any regression either. This looks like a refactor,
> > > code-reorganisation to me (for the better, for the purpose of this
> > > series).
>
> The video nodes will be opened once per camera sensor, which shouldn't
> fail. setFormat() is called in configure() only, so as long as the user
> doesn't try to use the second camera, it should be alright.
>
> Maybe you could give it a try ? My concern is that this series may
> introduce some subtle breakages, and someone bisecting the issue may try
> to run this patch alone without 2/2.
>
I give this a quick try and it seemed to be fine:
- Run "cam -c 1 -C" in one terminal. Camera starts streaming as expected.
- Run "cam -c 2 -C" in a second terminal. Camera fails in
Camera::acquire() as the pipeline handler
is locked.
This should be the expected safe behaviour, correct?
Naush
>
> > > > > > }
> > > > > >
> > > > > > -int PipelineHandlerRPi::registerCamera(MediaDevice *unicam,
> MediaDevice *isp)
> > > > > > +int PipelineHandlerRPi::registerCamera(MediaDevice *unicam,
> MediaDevice *isp, MediaEntity *sensorEntity)
> > > > > > {
> > > > > > std::unique_ptr<RPiCameraData> data =
> std::make_unique<RPiCameraData>(this);
> > > > > >
> > > > > > @@ -1079,14 +1091,7 @@ int
> PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp)
> > > > > >
> data->isp_[Isp::Output1].dev()->bufferReady.connect(data.get(),
> &RPiCameraData::ispOutputDequeue);
> > > > > >
> data->isp_[Isp::Stats].dev()->bufferReady.connect(data.get(),
> &RPiCameraData::ispOutputDequeue);
> > > > > >
> > > > > > - /* Identify the sensor. */
> > > > > > - for (MediaEntity *entity : unicam->entities()) {
> > > > > > - if (entity->function() == MEDIA_ENT_F_CAM_SENSOR) {
> > > > > > - data->sensor_ =
> std::make_unique<CameraSensor>(entity);
> > > > > > - break;
> > > > > > - }
> > > > > > - }
> > > > > > -
> > > > > > + data->sensor_ =
> std::make_unique<CameraSensor>(sensorEntity);
> > > > > > if (!data->sensor_)
> > > > > > return -EINVAL;
> > > > > >
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20211210/54b5b020/attachment-0001.htm>
More information about the libcamera-devel
mailing list