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