[libcamera-devel] [PATCH v4 2/2] pipeline: raspberrypi: Add support for Video Mux and Bridge devices
Naushir Patuck
naush at raspberrypi.com
Tue Jan 4 12:18:05 CET 2022
Hi Laurent,
Thank you for your feedback.
On Tue, 21 Dec 2021 at 16:59, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> Hi Naush,
>
> Thank you for the patch.
>
> On Tue, Dec 14, 2021 at 02:00:02PM +0000, Naushir Patuck wrote:
> > 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 RPiCameraData::enumerateVideoDevices(), called from
> > PipelineHandlerRPi::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
> > PipelineHandlerRPi::configure() before the camera is started.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > ---
> > .../pipeline/raspberrypi/raspberrypi.cpp | 139 ++++++++++++++++++
> > 1 file changed, 139 insertions(+)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 2a2fb5273eb8..4ec646d3c7a3 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>
> > @@ -191,6 +192,8 @@ public:
> > int loadIPA(ipa::RPi::SensorConfig *sensorConfig);
> > int configureIPA(const CameraConfiguration *config);
> >
> > + void enumerateVideoDevices(MediaLink *link);
> > +
> > void statsMetadataComplete(uint32_t bufferId, const ControlList
> &controls);
> > void runIsp(uint32_t bufferId);
> > void embeddedComplete(uint32_t bufferId);
> > @@ -220,6 +223,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_;
> > @@ -868,6 +876,38 @@ 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, with the exception of the link connecting the
> device.
> > + */
> > + for (const MediaPad *p : device->entity()->pads()) {
> > + if (!(p->flags() & MEDIA_PAD_FL_SINK))
> > + continue;
> > +
> > + for (MediaLink *l : p->links()) {
> > + if (l != link)
> > + l->setEnabled(false);
> > + }
> > + }
>
> Once the subdev routing API lands it would be great if the muxes could
> implement it...
>
> > +
> > + /* Next, enable the entity -> entity links, and setup the
> pad format. */
> > + link->setEnabled(true);
> > + const MediaPad *srcPad = link->sink();
>
> That looks weird, *source* pad = link->*sink*() ?
>
Yes, you are correct, will fix.
>
> > + ret = device->setFormat(srcPad->index(), &sensorFormat);
> > + if (ret) {
> > + LOG(RPI, Error) << "Failed to set format on " <<
> device->entity()->name()
> > + << " pad " << srcPad->index()
> > + << " with format " <<
> format.toString()
> > + << ": " << ret;
> > + return ret;
> > + }
> > +
> > + LOG(RPI, Info) << "Configured media link on device " <<
> device->entity()->name()
> > + << " on pad " << srcPad->index();
>
> Maybe Debug instead of Info ? Let's not be too verbose by default.
>
Ack.
>
> When propagating formats you're supposed to then read the format from
> the source pad and propagate it. Some bridges may change the media bus
> code for instance.
>
> This may be handled later if needed, but it would then be nice to record
> it in a comment in the code.
>
I'll add a \todo in the comment to say this.
>
> > + }
> > +
> > return ret;
> > }
> >
> > @@ -1098,6 +1138,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];
> > + data->enumerateVideoDevices(link);
>
> Wouldn't it be clearer to pass the sensor pointer to this function
> instead of the link ? You mention "sensor -> unicam link" in the
> comment, which doesn't match what MediaLink models which is a
> point-to-point link, not a connection through multiple entities.
>
I need to pass in the link pointer as this is what is needed to recurse
down the chain.
You are right about the comment being wrong, perhaps s/link/chain/?
>
> > +
> > data->sensorFormats_ = populateSensorFormats(data->sensor_);
> >
> > ipa::RPi::SensorConfig sensorConfig;
> > @@ -1447,6 +1494,98 @@ int RPiCameraData::configureIPA(const
> CameraConfiguration *config)
> > return 0;
> > }
> >
> > +/*
> > + * enumerateVideoDevices() iterates over the Media Controller topology,
> starting
> > + * at the sensor and finishing at Unicam. For each sensor,
> RPiCameraData stores
> > + * a unique list of any intermediate video mux or bridge devices
> connected in a
> > + * cascade, together with the entity to entity link.
> > + *
> > + * Entity pad configuration and link enabling happens at the end of
> configure().
> > + * We first disables all pad links on each entity device in the chain,
> and then
> > + * selectively enabling the specific links to link sensor to Unicam
> across all
> > + * intermediate muxes and bridges.
> > + *
> > + * In the cascaded topology below, if Sensor1 is used, the Mux2 -> Mux1
> link
> > + * will be disabled, and Sensor1 -> Mux1 -> Unicam links enabled.
> Alternatively,
> > + * if Sensor3 is used, the Sensor2 -> Mux2 and Sensor1 -> Mux1 links
> are disabled,
> > + * and Sensor3 -> Mux2 -> Mux1 -> Unicam links are enabled. All other
> links will
> > + * remain unchanged.
> > + *
> > + * +----------+
> > + * | Unicam |
> > + * +-----^----+
> > + * |
> > + * +---+---+
> > + * | Mux1 <-------+
> > + * +--^----+ |
> > + * | |
> > + * +-----+---+ +---+---+
> > + * | Sensor1 | | Mux2 |<--+
> > + * +---------+ +-^-----+ |
> > + * | |
> > + * +-------+-+ +---+-----+
> > + * | Sensor2 | | Sensor3 |
> > + * +---------+ +---------+
> > + */
> > +void RPiCameraData::enumerateVideoDevices(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;
> > +
> > + /* Find the source pad for this Video Mux or Bridge device. */
> > + const MediaPad *entitySourcePad = nullptr;
> > + for (const MediaPad *pad : entity->pads()) {
> > + if (pad->flags() & MEDIA_PAD_FL_SOURCE) {
> > + /*
> > + * We can only deal with devices that have a
> single source
> > + * pad. If this device has multple source pads,
> ignore it
> > + * and this branch in the cascade.
> > + */
> > + if (entitySourcePad)
> > + return;
> > +
> > + entitySourcePad = pad;
> > + }
> > + }
> > +
> > + LOG(RPI, Info) << "Found video mux device " << entity->name()
> > + << " linked to sink pad " << sinkPad->index();
>
> Same here, Debug ?
>
Ack.
Regards,
Naush
>
> > +
> > +
> bridgeDevices_.emplace_back(std::make_unique<V4L2Subdevice>(entity), link);
> > + bridgeDevices_.back().first->open();
> > +
> > + /*
> > + * Iterate through all the sink pad links down the cascade to find
> any
> > + * other Video Mux and Bridge devices.
> > + */
> > + for (MediaLink *l : entitySourcePad->links()) {
> > + enumerateVideoDevices(l);
> > + /* Once we reach the Unicam entity, we are done. */
> > + if (l->sink()->entity()->name() == "unicam-image") {
> > + unicamFound = true;
> > + break;
> > + }
> > + }
> > +
> > + /* 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!";
> > + bridgeDevices_.clear();
> > + }
> > + }
> > +}
> > +
> > void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const
> ControlList &controls)
> > {
> > if (state_ == State::Stopped)
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220104/4d99a0c1/attachment-0001.htm>
More information about the libcamera-devel
mailing list