<div dir="ltr"><div dir="ltr">Hi Kieran,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 9 Dec 2021 at 09:20, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@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">Quoting Naushir Patuck (2021-12-09 08:57:52)<br>
> Hi Laurent,<br>
> <br>
> Thank you for your feedback!<br>
> <br>
> On Wed, 8 Dec 2021 at 18:40, Laurent Pinchart <<br>
> <a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>> wrote:<br>
> <br>
> > Hi Naush,<br>
> ><br>
> > Thank you for the patch.<br>
> ><br>
> > On Wed, Dec 08, 2021 at 03:15:26PM +0000, Naushir Patuck wrote:<br>
> > > Enumerate the sensor device entities in PipelineHandlerRPi::match() and<br>
> > loop<br>
> > > over PipelineHandlerRPi::registerCamera() for each sensor found. This<br>
> > will<br>
> > > allow the pipeline handler to register multiple cameras attached to a<br>
> > 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<br>
> > 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,<br>
> > 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<br>
> > *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<br>
> > or more<br>
> > > + * video mux devices that are attaced to a particular Unicam<br>
> > 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>
> <br>
> Is this not already handled in the Camera class? If I have a mux with 2<br>
> devices<br>
> attached to a single Unicam instance, hence single pipeline handler, I<br>
> 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<br>
> result<br>
> if I try to run both cameras in a single process (e.g. with cam -c 1 -C -c<br>
> 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></blockquote><div><br></div><div>Laurent did point me to a WIP branch of his that supports multi-camera setups.</div><div>I have not looked at it yet, but perhaps this is fixed there.</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>
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>
<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></blockquote><div><br></div><div>On my CM4 setup, I was able to run "cam -c 1 -C -c 2 -C" and successfully</div><div>stream from both sensors. So presumably that was setting up 2x pipeline</div><div>handlers and working independently. This was on the master branch, not</div><div>Laurent's WIP branch. I should go back and make sure that still works!</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>
> > > + 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>
> <br>
> I'm sure I am missing something obvious, but why would this patch in<br>
> 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>
> Regards,<br>
> Naush<br>
> <br>
> <br>
> > > }<br>
> > ><br>
> > > -int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice<br>
> > *isp)<br>
> > > +int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice<br>
> > *isp, MediaEntity *sensorEntity)<br>
> > > {<br>
> > > std::unique_ptr<RPiCameraData> data =<br>
> > std::make_unique<RPiCameraData>(this);<br>
> > ><br>
> > > @@ -1079,14 +1091,7 @@ int<br>
> > PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp)<br>
> > > data->isp_[Isp::Output1].dev()->bufferReady.connect(data.get(),<br>
> > &RPiCameraData::ispOutputDequeue);<br>
> > > data->isp_[Isp::Stats].dev()->bufferReady.connect(data.get(),<br>
> > &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_ =<br>
> > 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>
> ><br>
</blockquote></div></div>