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

[Naushir Patuck]

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


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

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/81e8e47f/attachment.htm>