<div dir="ltr"><div dir="ltr">Hi Kieran,<div><br></div><div>Thank you for your feedback!</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 8 Dec 2021 at 12:27, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@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">Quoting Naushir Patuck (2021-12-01 11:57:11)<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 enumerateVideoDevices(), called from registerCamera(), is<br>
> used to identify and open all mux and bridge subdevices present in the<br>
> sensor -> Unicam link.<br>
> <br>
> Relevent links are enabled/disabled and pad formats correctly set in configure()<br>
> 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>
> ---<br>
> .../pipeline/raspberrypi/raspberrypi.cpp | 60 +++++++++++++++++++<br>
> 1 file changed, 60 insertions(+)<br>
> <br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 811160490f5c..2512d0746d4a 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -11,6 +11,7 @@<br>
> #include <mutex><br>
> #include <queue><br>
> #include <unordered_set><br>
> +#include <utility><br>
> <br>
> #include <libcamera/camera.h><br>
> #include <libcamera/control_ids.h><br>
> @@ -224,6 +225,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 the sink pad of the entity.<br>
> + */<br>
> + std::vector<std::pair<std::unique_ptr<V4L2Subdevice>, const MediaPad *>> videoDevices;<br>
<br>
Video devices makes me think this is a store of unicams, but really it's<br>
a store of the subdevices and their associated pad/links.<br>
<br>
But ... it feels like that's just a naming conflict, because they are<br>
the "video devices" - it's just a clash with the V4L2VideoDevice which<br>
is ... separate/different.<br>
<br>
I started thinking maybe sensorDevices would make more sense, but that's<br>
not right either - so perhaps bridgeDevices or just subdevices?<br></blockquote><div><br></div><div>I did have the same thought train when deciding on a name here :-)</div><div>It was originally called muxDevices, but that would not capture the</div><div>fact that bridge devices are also accounted for. Perhaps bridgeDevices</div><div>is generic enough to encompass both? Anyone else have suggestions?</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>
Anyway, that's just bikeshedding a name so it shouldn't matter too much.<br>
<br>
<br>
> <br>
> /* DMAHEAP allocation helper. */<br>
> RPi::DmaHeap dmaHeap_;<br>
> @@ -315,6 +321,7 @@ private:<br>
> }<br>
> <br>
> int registerCamera(MediaDevice *unicam, MediaDevice *isp, MediaEntity *sensorEntity);<br>
> + void enumerateVideoDevices(RPiCameraData *data, const MediaPad *sinkPad);<br>
> int queueAllBuffers(Camera *camera);<br>
> int prepareBuffers(Camera *camera);<br>
> void freeBuffers(Camera *camera);<br>
> @@ -848,6 +855,24 @@ 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, pad] : data->videoDevices) {<br>
> + /* Start by disabling all the sink pad links on the devices in the cascade. */<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>
> + l->setEnabled(false);<br>
> + }<br>
> +<br>
> + /* Finally enable the link to this camera, and setup the pad format. */<br>
> + data->sensor_->entity()->pads()[0]->links()[0]->setEnabled(true);<br>
<br>
does this some how need to 'walk' up the graph to the unicam enabling<br>
all links on the way up? (only because you've called it a cascade?)<br></blockquote><div><br></div><div>Yes, you are right. I do need to enable the links on all entity pads up.</div><div>I will rework this loop to do exactly that in the next revision!</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></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>
> + ret = device->setFormat(pad->index(), &sensorFormat);<br>
> + LOG(RPI, Info) << "Configured media link on device " << device->entity()->name()<br>
> + << " at pad " << pad->index();<br>
> + }<br>
> +<br>
> return ret;<br>
> }<br>
> <br>
> @@ -1078,6 +1103,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>
> + MediaPad *sensorSinkPad = sensorEntity->getPadByIndex(0)->links()[0]->sink();<br>
> + enumerateVideoDevices(data.get(), sensorSinkPad);<br>
> +<br>
> data->sensorFormats_ = populateSensorFormats(data->sensor_);<br>
> <br>
> ipa::RPi::SensorConfig sensorConfig;<br>
> @@ -1204,6 +1236,34 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me<br>
> return 0;<br>
> }<br>
> <br>
> +void PipelineHandlerRPi::enumerateVideoDevices(RPiCameraData *data, const MediaPad *sinkPad)<br>
> +{<br>
> + const MediaEntity *entity = sinkPad->entity();<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>
> + LOG(RPI, Info) << "Found video mux device " << entity->name()<br>
> + << " linked to sink pad " << sinkPad->index();<br>
> +<br>
> + data->videoDevices.emplace_back(std::make_unique<V4L2Subdevice>(entity), sinkPad);<br>
> + data->videoDevices.back().first->open();<br>
> +<br>
> + for (const MediaPad *pad : entity->pads()) {<br>
> + /*<br>
> + * Iterate through all the sink pads down the cascade to find any<br>
> + * other Video Mux and Bridge devices.<br>
> + */<br>
> + if (!(pad->flags() & MEDIA_PAD_FL_SOURCE))<br>
> + continue;<br>
> +<br>
> + for (const MediaLink *l : pad->links())<br>
> + enumerateVideoDevices(data, l->sink());<br>
<br>
Nice, so it's turtles all the way down ... ;-)<br>
<br>
Bikeshedding of a name aside, my only query is with the enabling of the<br>
links on the way back up from the sensorEntity now that it supports<br>
cascading.<br>
<br>
I don't mind if that's added later when hardware is available to test,<br>
but if my interpretation is right that it would need to do it there,<br>
please add a todo.<br>
<br>
Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
<br>
--<br>
Kieran<br>
<br>
> + }<br>
> +}<br>
> +<br>
> int PipelineHandlerRPi::queueAllBuffers(Camera *camera)<br>
> {<br>
> RPiCameraData *data = cameraData(camera);<br>
> -- <br>
> 2.25.1<br>
><br>
</blockquote></div></div>