<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>Thank you for your feedback!</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 8 Dec 2021 at 18:40, 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">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 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></blockquote><div><br></div><div>Is this not already handled in the Camera class? If I have a mux with 2 devices</div><div>attached to a single Unicam instance, hence single pipeline handler, I cannot</div><div>run camera in one process and camera 1 in another. Camera::acquire() would</div><div>vail to lock the pipeline handler:</div><div><br></div><div> if (!d->pipe_->lock()) {<br> LOG(Camera, Info)<br> << "Pipeline handler in use by another process";<br> return -EBUSY;<br> }<br></div><div><br></div><div>Perhaps this long point should be turned into Error level? I get the same result</div><div>if I try to run both cameras in a single process (e.g. with cam -c 1 -C -c 2 -C).</div><div> <br></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></blockquote><div><br></div><div>I'm sure I am missing something obvious, but why would this patch in isolation</div><div>cause a regression? The only additional function gained here would be to</div><div>allow the pipeline handler to enumerate multiple cameras attached to a mux,</div><div>but not allow one of the cameras to be run - same as before this change :-)</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></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>