[libcamera-devel] [PATCH v2 1/2] pipeline: raspberrypi: Move sensor entity detection out of registerCamera()
Naushir Patuck
naush at raspberrypi.com
Thu Dec 9 10:27:37 CET 2021
Hi Kieran,
On Thu, 9 Dec 2021 at 09:20, Kieran Bingham <kieran.bingham at ideasonboard.com>
wrote:
> Quoting Naushir Patuck (2021-12-09 08:57:52)
> > Hi Laurent,
> >
> > Thank you for your feedback!
> >
> > On Wed, 8 Dec 2021 at 18:40, Laurent Pinchart <
> > laurent.pinchart at ideasonboard.com> wrote:
> >
> > > Hi Naush,
> > >
> > > Thank you for the patch.
> > >
> > > 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 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!
Naush
>
> > > > + */
> > > > + 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).
>
> > Regards,
> > 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/20211209/629de66d/attachment.htm>
More information about the libcamera-devel
mailing list