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

[Kieran Bingham]

Quoting Naushir Patuck (2021-12-08 15:15:27)
> 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      | 78 +++++++++++++++++++
>  1 file changed, 78 insertions(+)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 756878c70036..ca176ecb40ec 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -12,6 +12,7 @@
>  #include <mutex>
>  #include <queue>
>  #include <unordered_set>
> +#include <utility>
>  
>  #include <libcamera/base/shared_fd.h>
>  #include <libcamera/base/utils.h>
> @@ -220,6 +221,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 media link across the entities.
> +        */
> +       std::vector<std::pair<std::unique_ptr<V4L2Subdevice>, MediaLink *>> bridgeDevices_;
>  
>         /* DMAHEAP allocation helper. */
>         RPi::DmaHeap dmaHeap_;
> @@ -311,6 +317,7 @@ private:
>         }
>  
>         int registerCamera(MediaDevice *unicam, MediaDevice *isp, MediaEntity *sensorEntity);
> +       void enumerateVideoDevices(RPiCameraData *data, MediaLink *link);
>         int queueAllBuffers(Camera *camera);
>         int prepareBuffers(Camera *camera);
>         void freeBuffers(Camera *camera);
> @@ -868,6 +875,25 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>          */
>         data->properties_.set(properties::ScalerCropMaximum, data->sensorInfo_.analogCrop);
>  
> +       /* Setup the Video Mux/Bridge entities. */
> +       for (auto &[device, link] : data->bridgeDevices_) {
> +               /* 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, and setup the pad format. */
> +               link->setEnabled(true);

Isn't this going to enable /all/ bridge links in the cascade
incorrectly?


   ┌──────────┐
   │  Unicam  │
   └─────▲────┘
         │
     ┌───┴───┐
     │  Mux  ◄───────┐
     └──▲────┘       │
        │            │
  ┌─────┴───┐    ┌───┴───┐
  │ Sensor1 │    │  Mux  │◄──┐
  └─────────┘    └─▲─────┘   │
                   │         │
           ┌───────┴─┐   ┌───┴─────┐
           │ Sensor2 │   │ Sensor3 │
           └─────────┘   └─────────┘

In that 'use case' we're now iterating over all bridges and enabling
their link, I think which means we've just enabled both muxes,
immediately after disabling them - even for Sensor1?

Maybe I've mis-understood it, but I thought there would be something
that would start at the desired sensor, and walk up the links to the
unicam enabling those links (and only those) as it went up?

The walk only has to be done once too, so perhaps a per-camera(sensor)
vector of links to iterate and enable?

Or maybe I'm missing the separation between the bridgeDevices_ and the
per-camera instances. But if that's already happening, I can't then see
how each camera data clears all the bridges used by other cameras...


> +               const MediaPad *srcPad = link->sink();
> +               ret = device->setFormat(srcPad->index(), &sensorFormat);

This assignment of ret is in a loop, so earlier failures are going to
get ignored, unchecked.

> +               LOG(RPI, Info) << "Configured media link on device " << device->entity()->name()
> +                              << " at pad " << srcPad->index();
> +       }
> +
>         return ret;
>  }
>  
> @@ -1098,6 +1124,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!
> +        */
> +       MediaLink *link = sensorEntity->getPadByIndex(0)->links()[0];
> +       enumerateVideoDevices(data.get(), link);
> +
>         data->sensorFormats_ = populateSensorFormats(data->sensor_);
>  
>         ipa::RPi::SensorConfig sensorConfig;
> @@ -1224,6 +1257,51 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me
>         return 0;
>  }
>  
> +void PipelineHandlerRPi::enumerateVideoDevices(RPiCameraData *data, MediaLink *link)
> +{
> +       const MediaPad *sinkPad = link->sink();
> +       const MediaEntity *entity = sinkPad->entity();
> +       bool unicamFound = false;
> +
> +       /* 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->bridgeDevices_.emplace_back(std::make_unique<V4L2Subdevice>(entity), link);
> +       data->bridgeDevices_.back().first->open();

Going through the above, I can't help but wonder if the bridgeDevices_
should be stored as a single instance in the Pipeline handler (not each
CameraData, we have one CameraData per camera right? if so, how does
camera3 disable all the links? does it know about every path?)


> +
> +       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 (MediaLink *l : pad->links()) {
> +                       enumerateVideoDevices(data, l);
> +                       if (l->sink()->entity()->name() == "unicam-image")
> +                               unicamFound = true;
> +               }
> +       }
> +
> +       /* This identifies the end of our entity enumeration recursion. */
> +       if (link->source()->entity()->function() == MEDIA_ENT_F_CAM_SENSOR) {
> +               /*
> +               * If Unicam is not at the end of this cascade, we cannot configure
> +               * this topology automatically, so remove all entity references.
> +               */
> +               if (!unicamFound) {
> +                       LOG(RPI, Warning) << "Cannot automatically configure this MC topology!";
> +                       data->bridgeDevices_.clear();
> +               }
> +       }
> +}
> +
>  int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
>  {
>         RPiCameraData *data = cameraData(camera);
> -- 
> 2.25.1
>