[libcamera-devel] [PATCH v2 1/2] pipeline: raspberrypi: Move sensor entity detection out of registerCamera()

[Laurent Pinchart]

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.

> > > > >  }
> > > > >
> > > > > -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