<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>Thank you for your feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 21 Dec 2021 at 16:59, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush,<br>
<br>
Thank you for the patch.<br>
<br>
On Tue, Dec 14, 2021 at 02:00:02PM +0000, Naushir Patuck wrote:<br>
> This change will allow the pipeline handler to enumerate and control Video<br>
> Mux or Bridge devices that may be attached between sensors and a particular<br>
> Unicam instance. Cascaded mux or bridge devices are also handled.<br>
> <br>
> A new member function RPiCameraData::enumerateVideoDevices(), called from<br>
> PipelineHandlerRPi::registerCamera(), is used to identify and open all mux and<br>
> bridge subdevices present in the sensor -> Unicam link.<br>
> <br>
> Relevent links are enabled/disabled and pad formats correctly set in<br>
> PipelineHandlerRPi::configure() before the camera is started.<br>
> <br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
> ---<br>
> .../pipeline/raspberrypi/raspberrypi.cpp | 139 ++++++++++++++++++<br>
> 1 file changed, 139 insertions(+)<br>
> <br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 2a2fb5273eb8..4ec646d3c7a3 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -12,6 +12,7 @@<br>
> #include <mutex><br>
> #include <queue><br>
> #include <unordered_set><br>
> +#include <utility><br>
> <br>
> #include <libcamera/base/shared_fd.h><br>
> #include <libcamera/base/utils.h><br>
> @@ -191,6 +192,8 @@ public:<br>
> int loadIPA(ipa::RPi::SensorConfig *sensorConfig);<br>
> int configureIPA(const CameraConfiguration *config);<br>
> <br>
> + void enumerateVideoDevices(MediaLink *link);<br>
> +<br>
> void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);<br>
> void runIsp(uint32_t bufferId);<br>
> void embeddedComplete(uint32_t bufferId);<br>
> @@ -220,6 +223,11 @@ public:<br>
> std::vector<RPi::Stream *> streams_;<br>
> /* Stores the ids of the buffers mapped in the IPA. */<br>
> std::unordered_set<unsigned int> ipaBuffers_;<br>
> + /*<br>
> + * Stores a cascade of Video Mux or Bridge devices between the sensor and<br>
> + * Unicam together with media link across the entities.<br>
> + */<br>
> + std::vector<std::pair<std::unique_ptr<V4L2Subdevice>, MediaLink *>> bridgeDevices_;<br>
> <br>
> /* DMAHEAP allocation helper. */<br>
> RPi::DmaHeap dmaHeap_;<br>
> @@ -868,6 +876,38 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)<br>
> */<br>
> data->properties_.set(properties::ScalerCropMaximum, data->sensorInfo_.analogCrop);<br>
> <br>
> + /* Setup the Video Mux/Bridge entities. */<br>
> + for (auto &[device, link] : data->bridgeDevices_) {<br>
> + /*<br>
> + * Start by disabling all the sink pad links on the devices in the<br>
> + * cascade, with the exception of the link connecting the device.<br>
> + */<br>
> + for (const MediaPad *p : device->entity()->pads()) {<br>
> + if (!(p->flags() & MEDIA_PAD_FL_SINK))<br>
> + continue;<br>
> +<br>
> + for (MediaLink *l : p->links()) {<br>
> + if (l != link)<br>
> + l->setEnabled(false);<br>
> + }<br>
> + }<br>
<br>
Once the subdev routing API lands it would be great if the muxes could<br>
implement it...<br>
<br>
> +<br>
> + /* Next, enable the entity -> entity links, and setup the pad format. */<br>
> + link->setEnabled(true);<br>
> + const MediaPad *srcPad = link->sink();<br>
<br>
That looks weird, *source* pad = link->*sink*() ?<br></blockquote><div><br></div><div>Yes, you are correct, will fix.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + ret = device->setFormat(srcPad->index(), &sensorFormat);<br>
> + if (ret) {<br>
> + LOG(RPI, Error) << "Failed to set format on " << device->entity()->name()<br>
> + << " pad " << srcPad->index()<br>
> + << " with format " << format.toString()<br>
> + << ": " << ret;<br>
> + return ret;<br>
> + }<br>
> +<br>
> + LOG(RPI, Info) << "Configured media link on device " << device->entity()->name()<br>
> + << " on pad " << srcPad->index();<br>
<br>
Maybe Debug instead of Info ? Let's not be too verbose by default.<br></blockquote><div><br></div><div>Ack.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
When propagating formats you're supposed to then read the format from<br>
the source pad and propagate it. Some bridges may change the media bus<br>
code for instance.<br>
<br>
This may be handled later if needed, but it would then be nice to record<br>
it in a comment in the code.<br></blockquote><div><br></div><div>I'll add a \todo in the comment to say this.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + }<br>
> +<br>
> return ret;<br>
> }<br>
> <br>
> @@ -1098,6 +1138,13 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me<br>
> if (data->sensor_->init())<br>
> return -EINVAL;<br>
> <br>
> + /*<br>
> + * Enumerate all the Video Mux/Bridge devices across the sensor -> unicam<br>
> + * link. There may be a cascade of devices in this link!<br>
> + */<br>
> + MediaLink *link = sensorEntity->getPadByIndex(0)->links()[0];<br>
> + data->enumerateVideoDevices(link);<br>
<br>
Wouldn't it be clearer to pass the sensor pointer to this function<br>
instead of the link ? You mention "sensor -> unicam link" in the<br>
comment, which doesn't match what MediaLink models which is a<br>
point-to-point link, not a connection through multiple entities.<br></blockquote><div><br></div><div>I need to pass in the link pointer as this is what is needed to recurse</div><div>down the chain.</div><div><br></div><div>You are right about the comment being wrong, perhaps s/link/chain/?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +<br>
> data->sensorFormats_ = populateSensorFormats(data->sensor_);<br>
> <br>
> ipa::RPi::SensorConfig sensorConfig;<br>
> @@ -1447,6 +1494,98 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)<br>
> return 0;<br>
> }<br>
> <br>
> +/*<br>
> + * enumerateVideoDevices() iterates over the Media Controller topology, starting<br>
> + * at the sensor and finishing at Unicam. For each sensor, RPiCameraData stores<br>
> + * a unique list of any intermediate video mux or bridge devices connected in a<br>
> + * cascade, together with the entity to entity link.<br>
> + *<br>
> + * Entity pad configuration and link enabling happens at the end of configure().<br>
> + * We first disables all pad links on each entity device in the chain, and then<br>
> + * selectively enabling the specific links to link sensor to Unicam across all<br>
> + * intermediate muxes and bridges.<br>
> + *<br>
> + * In the cascaded topology below, if Sensor1 is used, the Mux2 -> Mux1 link<br>
> + * will be disabled, and Sensor1 -> Mux1 -> Unicam links enabled. Alternatively,<br>
> + * if Sensor3 is used, the Sensor2 -> Mux2 and Sensor1 -> Mux1 links are disabled,<br>
> + * and Sensor3 -> Mux2 -> Mux1 -> Unicam links are enabled. All other links will<br>
> + * remain unchanged.<br>
> + *<br>
> + * +----------+<br>
> + * | Unicam |<br>
> + * +-----^----+<br>
> + * |<br>
> + * +---+---+<br>
> + * | Mux1 <-------+<br>
> + * +--^----+ |<br>
> + * | |<br>
> + * +-----+---+ +---+---+<br>
> + * | Sensor1 | | Mux2 |<--+<br>
> + * +---------+ +-^-----+ |<br>
> + * | |<br>
> + * +-------+-+ +---+-----+<br>
> + * | Sensor2 | | Sensor3 |<br>
> + * +---------+ +---------+<br>
> + */<br>
> +void RPiCameraData::enumerateVideoDevices(MediaLink *link)<br>
> +{<br>
> + const MediaPad *sinkPad = link->sink();<br>
> + const MediaEntity *entity = sinkPad->entity();<br>
> + bool unicamFound = false;<br>
> +<br>
> + /* We only deal with Video Mux and Bridge devices in cascade. */<br>
> + if (entity->function() != MEDIA_ENT_F_VID_MUX &&<br>
> + entity->function() != MEDIA_ENT_F_VID_IF_BRIDGE)<br>
> + return;<br>
> +<br>
> + /* Find the source pad for this Video Mux or Bridge device. */<br>
> + const MediaPad *entitySourcePad = nullptr;<br>
> + for (const MediaPad *pad : entity->pads()) {<br>
> + if (pad->flags() & MEDIA_PAD_FL_SOURCE) {<br>
> + /*<br>
> + * We can only deal with devices that have a single source<br>
> + * pad. If this device has multple source pads, ignore it<br>
> + * and this branch in the cascade.<br>
> + */<br>
> + if (entitySourcePad)<br>
> + return;<br>
> +<br>
> + entitySourcePad = pad;<br>
> + }<br>
> + }<br>
> +<br>
> + LOG(RPI, Info) << "Found video mux device " << entity->name()<br>
> + << " linked to sink pad " << sinkPad->index();<br>
<br>
Same here, Debug ?<br></blockquote><div><br></div><div>Ack.</div><div><br></div><div>Regards,</div><div>Naush</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +<br>
> + bridgeDevices_.emplace_back(std::make_unique<V4L2Subdevice>(entity), link);<br>
> + bridgeDevices_.back().first->open();<br>
> +<br>
> + /*<br>
> + * Iterate through all the sink pad links down the cascade to find any<br>
> + * other Video Mux and Bridge devices.<br>
> + */<br>
> + for (MediaLink *l : entitySourcePad->links()) {<br>
> + enumerateVideoDevices(l);<br>
> + /* Once we reach the Unicam entity, we are done. */<br>
> + if (l->sink()->entity()->name() == "unicam-image") {<br>
> + unicamFound = true;<br>
> + break;<br>
> + }<br>
> + }<br>
> +<br>
> + /* This identifies the end of our entity enumeration recursion. */<br>
> + if (link->source()->entity()->function() == MEDIA_ENT_F_CAM_SENSOR) {<br>
> + /*<br>
> + * If Unicam is not at the end of this cascade, we cannot configure<br>
> + * this topology automatically, so remove all entity references.<br>
> + */<br>
> + if (!unicamFound) {<br>
> + LOG(RPI, Warning) << "Cannot automatically configure this MC topology!";<br>
> + bridgeDevices_.clear();<br>
> + }<br>
> + }<br>
> +}<br>
> +<br>
> void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &controls)<br>
> {<br>
> if (state_ == State::Stopped)<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>