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

14958 diff mbox series

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@raspberrypi.com>
---
 .../pipeline/raspberrypi/raspberrypi.cpp      | 60 +++++++++++++++++++
 1 file changed, 60 insertions(+)

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@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@ideasonboard.com>

--
Kieran

> +       }
> +}
> +
>  int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
>  {
>         RPiCameraData *data = cameraData(camera);
> -- 
> 2.25.1
>

Hi Kieran,

Thank you for your feedback!

On Wed, 8 Dec 2021 at 12:27, Kieran Bingham <kieran.bingham@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@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@ideasonboard.com>
>
> --
> Kieran
>
> > +       }
> > +}
> > +
> >  int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
> >  {
> >         RPiCameraData *data = cameraData(camera);
> > --
> > 2.25.1
> >
>