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

[Kieran Bingham]

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?

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


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