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

[Kieran Bingham]

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)...

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?

> > > +      */
> > > +     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
> >