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

[Laurent Pinchart]

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*() ?

> +		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.

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.

> +	}
> +
>  	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.

> +
>  	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 ?

> +
> +	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