<div dir="ltr"><div dir="ltr">Hi Kieran,<div><br></div><div>Thanks for your feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 9 Dec 2021 at 09:39, 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-08 15:15:27)<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  Â  Â  | 78 +++++++++++++++++++<br>
>  1 file changed, 78 insertions(+)<br>
> <br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 756878c70036..ca176ecb40ec 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>
> @@ -220,6 +221,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>
> @@ -311,6 +317,7 @@ private:<br>
>  Â  Â  Â  Â }<br>
>  <br>
>  Â  Â  Â  Â int registerCamera(MediaDevice *unicam, MediaDevice *isp, MediaEntity *sensorEntity);<br>
> +  Â  Â  Â void enumerateVideoDevices(RPiCameraData *data, MediaLink *link);<br>
>  Â  Â  Â  Â int queueAllBuffers(Camera *camera);<br>
>  Â  Â  Â  Â int prepareBuffers(Camera *camera);<br>
>  Â  Â  Â  Â void freeBuffers(Camera *camera);<br>
> @@ -868,6 +875,25 @@ 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>
> +  Â  Â  Â  Â  Â  Â  Â /* 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, and setup the pad format. */<br>
> +  Â  Â  Â  Â  Â  Â  Â link->setEnabled(true);<br>
<br>
Isn't this going to enable /all/ bridge links in the cascade<br>
incorrectly?<br>
<br>
<br>
  Â â”Œâ”€â”€â”€â”€â”€â”€â”€â”€â”€â”€â”<br>
  Â â”‚  Unicam  â”‚<br>
  Â â””─────▲────┘<br>
  Â  Â  Â  Â â”‚<br>
  Â  Â â”Œâ”€â”€â”€â”´â”€â”€â”€â”<br>
  Â  Â â”‚  Mux  â—„───────┐<br>
  Â  Â â””──▲────┘  Â  Â  Â â”‚<br>
  Â  Â  Â  â”‚  Â  Â  Â  Â  Â  â”‚<br>
  â”Œâ”€â”€â”€â”€â”€â”´â”€â”€â”€â”Â  Â  â”Œâ”€â”€â”€â”´â”€â”€â”€â”<br>
  â”‚ Sensor1 â”‚  Â  â”‚  Mux  â”‚◄──┐<br>
  â””─────────┘  Â  â””─▲─────┘  Â â”‚<br>
  Â  Â  Â  Â  Â  Â  Â  Â  Â â”‚  Â  Â  Â  Â â”‚<br>
  Â  Â  Â  Â  Â â”Œâ”€â”€â”€â”€â”€â”€â”€â”´â”€â”Â  Â â”Œâ”€â”€â”€â”´â”€â”€â”€â”€â”€â”<br>
  Â  Â  Â  Â  Â â”‚ Sensor2 â”‚  Â â”‚ Sensor3 â”‚<br>
  Â  Â  Â  Â  Â â””─────────┘  Â â””─────────┘<br>
<br>
In that 'use case' we're now iterating over all bridges and enabling<br>
their link, I think which means we've just enabled both muxes,<br>
immediately after disabling them - even for Sensor1?<br>
<br>
Maybe I've mis-understood it, but I thought there would be something<br>
that would start at the desired sensor, and walk up the links to the<br>
unicam enabling those links (and only those) as it went up?<br>
<br>
The walk only has to be done once too, so perhaps a per-camera(sensor)<br>
vector of links to iterate and enable?<br>
<br>
Or maybe I'm missing the separation between the bridgeDevices_ and the<br>
per-camera instances. But if that's already happening, I can't then see<br>
how each camera data clears all the bridges used by other cameras...<br></blockquote><div><br></div><div>Let me explain my intention, and you can then tell me if the code does</div><div>what I think it does :-)</div><div><br></div><div>Each sensor (Sensor{1,2,3}) will register its own Camera object, and</div><div>in that object bridgeDevices_ will store the cascade of muxes between</div><div>the sensor and the Unicam port. So, for Sensor1, we store only 1 mux,</div><div>and Sensor{2,3} will store both muxes. Together with the mux device,</div><div>we also store the entity to entity links.</div><div><br></div><div>The above code goes through those stored entities, first disabling *all*</div><div>links on each device in the chain, and then selectively enabling</div><div>the specific links that are stored in bridgeDevices_ to link sensor</div><div>to Unicam across all intermedia muxes.  If Sensor1 is used, this</div><div>does mean that the Sensor{2,3} -> Mux links might not change</div><div>state but the Mux to Mux link will be disabled.  Similarly, if we are driving</div><div>Sensor3, the  Sensor{1,2} -> Mux link will be disabled.</div><div><br></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>
> +  Â  Â  Â  Â  Â  Â  Â const MediaPad *srcPad = link->sink();<br>
> +  Â  Â  Â  Â  Â  Â  Â ret = device->setFormat(srcPad->index(), &sensorFormat);<br>
<br>
This assignment of ret is in a loop, so earlier failures are going to<br>
get ignored, unchecked.<br></blockquote><div><br></div><div>This should not matter, as ret is only used in the loop to count successful</div><div>camera registrations.  The return value from match() will be false only if</div><div>0 cameras were registered.</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>
> +  Â  Â  Â  Â  Â  Â  Â LOG(RPI, Info) << "Configured media link on device " << device->entity()->name()<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  << " at pad " << srcPad->index();<br>
> +  Â  Â  Â }<br>
> +<br>
>  Â  Â  Â  Â return ret;<br>
>  }<br>
>  <br>
> @@ -1098,6 +1124,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>
> +  Â  Â  Â enumerateVideoDevices(data.get(), link);<br>
> +<br>
>  Â  Â  Â  Â data->sensorFormats_ = populateSensorFormats(data->sensor_);<br>
>  <br>
>  Â  Â  Â  Â ipa::RPi::SensorConfig sensorConfig;<br>
> @@ -1224,6 +1257,51 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me<br>
>  Â  Â  Â  Â return 0;<br>
>  }<br>
>  <br>
> +void PipelineHandlerRPi::enumerateVideoDevices(RPiCameraData *data, 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>
> +  Â  Â  Â LOG(RPI, Info) << "Found video mux device " << entity->name()<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  << " linked to sink pad " << sinkPad->index();<br>
> +<br>
> +  Â  Â  Â data->bridgeDevices_.emplace_back(std::make_unique<V4L2Subdevice>(entity), link);<br>
> +  Â  Â  Â data->bridgeDevices_.back().first->open();<br>
<br>
Going through the above, I can't help but wonder if the bridgeDevices_<br>
should be stored as a single instance in the Pipeline handler (not each<br>
CameraData, we have one CameraData per camera right? if so, how does<br>
camera3 disable all the links? does it know about every path?)<br></blockquote><div><br></div><div>From my description earlier, bridgeDevices_ must be stored in the CameraData, as</div><div>it lists the devices in the path between the sensor and Unicam.  And this is unique</div><div>per-sensor.  Again, this does mean that if we are using Sensor1, links for Sensor{2,3}</div><div>-> Mux are not changed, but the Mux->Mux link will be disabled.  Do you think</div><div>that may not be appropriate leaving them enabled?</div><div><br></div><div>Please shout if this doesn't make sense, and something simpler might equally work :-)</div><div><br></div><div>Cheers,</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>
> +<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 (MediaLink *l : pad->links()) {<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â enumerateVideoDevices(data, l);<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â if (l->sink()->entity()->name() == "unicam-image")<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â unicamFound = true;<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>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â data->bridgeDevices_.clear();<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>