[libcamera-devel] [PATCH v1 2/2] pipeline: raspberrypi: Add support for Video Mux and Bridge devices

[Naushir Patuck]

Hi Kieran,

Thank you for your feedback!

On Wed, 8 Dec 2021 at 12:27, Kieran Bingham <kieran.bingham at ideasonboard.com>
wrote:

> Quoting Naushir Patuck (2021-12-01 11:57:11)
> > This change will allow the pipeline handler to enumerate and control
> Video
> > Mux or Bridge devices that may be attached between sensors and a
> particular
> > Unicam instance. Cascaded mux or bridge devices are also handled.
> >
> > A new member function enumerateVideoDevices(), called from
> registerCamera(), is
> > used to identify and open all mux and bridge subdevices present in the
> > sensor -> Unicam link.
> >
> > Relevent links are enabled/disabled and pad formats correctly set in
> configure()
> > before the camera is started.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 60 +++++++++++++++++++
> >  1 file changed, 60 insertions(+)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 811160490f5c..2512d0746d4a 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -11,6 +11,7 @@
> >  #include <mutex>
> >  #include <queue>
> >  #include <unordered_set>
> > +#include <utility>
> >
> >  #include <libcamera/camera.h>
> >  #include <libcamera/control_ids.h>
> > @@ -224,6 +225,11 @@ public:
> >         std::vector<RPi::Stream *> streams_;
> >         /* Stores the ids of the buffers mapped in the IPA. */
> >         std::unordered_set<unsigned int> ipaBuffers_;
> > +       /*
> > +        * Stores a cascade of Video Mux or Bridge devices between the
> sensor and
> > +        * Unicam together with the sink pad of the entity.
> > +        */
> > +       std::vector<std::pair<std::unique_ptr<V4L2Subdevice>, const
> MediaPad *>> videoDevices;
>
> Video devices makes me think this is a store of unicams, but really it's
> a store of the subdevices and their associated pad/links.
>
> But ... it feels like that's just a naming conflict, because they are
> the "video devices" - it's just a clash with the V4L2VideoDevice which
> is ... separate/different.
>
> I started thinking maybe sensorDevices would make more sense, but that's
> not right either - so perhaps bridgeDevices or just subdevices?
>

I did have the same thought train when deciding on a name here :-)
It was originally called muxDevices, but that would not capture the
fact that bridge devices are also accounted for.  Perhaps bridgeDevices
is generic enough to encompass both?  Anyone else have suggestions?


>
> Anyway, that's just bikeshedding a name so it shouldn't matter too much.
>
>
> >
> >         /* DMAHEAP allocation helper. */
> >         RPi::DmaHeap dmaHeap_;
> > @@ -315,6 +321,7 @@ private:
> >         }
> >
> >         int registerCamera(MediaDevice *unicam, MediaDevice *isp,
> MediaEntity *sensorEntity);
> > +       void enumerateVideoDevices(RPiCameraData *data, const MediaPad
> *sinkPad);
> >         int queueAllBuffers(Camera *camera);
> >         int prepareBuffers(Camera *camera);
> >         void freeBuffers(Camera *camera);
> > @@ -848,6 +855,24 @@ int PipelineHandlerRPi::configure(Camera *camera,
> CameraConfiguration *config)
> >          */
> >         data->properties_.set(properties::ScalerCropMaximum,
> data->sensorInfo_.analogCrop);
> >
> > +       /* Setup the Video Mux/Bridge entities. */
> > +       for (auto &[device, pad] : data->videoDevices) {
> > +               /* Start by disabling all the sink pad links on the
> devices in the cascade. */
> > +               for (const MediaPad *p : device->entity()->pads()) {
> > +                       if (!(p->flags() & MEDIA_PAD_FL_SINK))
> > +                               continue;
> > +
> > +                       for (MediaLink *l : p->links())
> > +                               l->setEnabled(false);
> > +               }
> > +
> > +               /* Finally enable the link to this camera, and setup the
> pad format. */
> > +
>  data->sensor_->entity()->pads()[0]->links()[0]->setEnabled(true);
>
> does this some how need to 'walk' up the graph to the unicam enabling
> all links on the way up? (only because you've called it a cascade?)
>

Yes, you are right.  I do need to enable the links on all entity pads up.
I will rework this loop to do exactly that in the next revision!

Regards,
Naush



>
>
> > +               ret = device->setFormat(pad->index(), &sensorFormat);
> > +               LOG(RPI, Info) << "Configured media link on device " <<
> device->entity()->name()
> > +                              << " at pad " << pad->index();
> > +       }
> > +
> >         return ret;
> >  }
> >
> > @@ -1078,6 +1103,13 @@ int
> PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me
> >         if (data->sensor_->init())
> >                 return -EINVAL;
> >
> > +       /*
> > +        * Enumerate all the Video Mux/Bridge devices across the sensor
> -> unicam
> > +        * link. There may be a cascade of devices in this link!
> > +        */
> > +       MediaPad *sensorSinkPad =
> sensorEntity->getPadByIndex(0)->links()[0]->sink();
> > +       enumerateVideoDevices(data.get(), sensorSinkPad);
> > +
> >         data->sensorFormats_ = populateSensorFormats(data->sensor_);
> >
> >         ipa::RPi::SensorConfig sensorConfig;
> > @@ -1204,6 +1236,34 @@ int
> PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me
> >         return 0;
> >  }
> >
> > +void PipelineHandlerRPi::enumerateVideoDevices(RPiCameraData *data,
> const MediaPad *sinkPad)
> > +{
> > +       const MediaEntity *entity = sinkPad->entity();
> > +
> > +       /* We only deal with Video Mux and Bridge devices in cascade. */
> > +       if (entity->function() != MEDIA_ENT_F_VID_MUX &&
> > +           entity->function() != MEDIA_ENT_F_VID_IF_BRIDGE)
> > +               return;
> > +
> > +       LOG(RPI, Info) << "Found video mux device " << entity->name()
> > +                      << " linked to sink pad " << sinkPad->index();
> > +
> > +
>  data->videoDevices.emplace_back(std::make_unique<V4L2Subdevice>(entity),
> sinkPad);
> > +       data->videoDevices.back().first->open();
> > +
> > +       for (const MediaPad *pad : entity->pads()) {
> > +               /*
> > +                * Iterate through all the sink pads down the cascade to
> find any
> > +                * other Video Mux and Bridge devices.
> > +                */
> > +               if (!(pad->flags() & MEDIA_PAD_FL_SOURCE))
> > +                       continue;
> > +
> > +               for (const MediaLink *l : pad->links())
> > +                       enumerateVideoDevices(data, l->sink());
>
> Nice, so it's turtles all the way down ... ;-)
>
> Bikeshedding of a name aside, my only query is with the enabling of the
> links on the way back up from the sensorEntity now that it supports
> cascading.
>
> I don't mind if that's added later when hardware is available to test,
> but if my interpretation is right that it would need to do it there,
> please add a todo.
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> --
> Kieran
>
> > +       }
> > +}
> > +
> >  int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
> >  {
> >         RPiCameraData *data = cameraData(camera);
> > --
> > 2.25.1
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20211208/869a2d86/attachment-0001.htm>