[libcamera-devel] [PATCH] pipeline: rkisp1: Support media graph with separate CSI RX

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Wed, Jun 22, 2022 at 04:46:38PM +0900, Paul Elder via libcamera-devel wrote:
> The rkisp1 hardware supports a parallel interface, where the sensor
> would be connected directly, as well as a CSI receiver. Although at the
> moment the rkisp1 driver doesn't expose the CSI receiver as a separate
> subdev, this patch allows the rkisp1 pipeline handler to continue
> working even in the event that it eventually does.

The rkisp1 hardware supports both a CSI-2 input and a parallel input,
where the sensor is connected directly to the ISP. On RK3399, the CSI-2
receiver is internal, but on the i.MX8MP, the CSI-2 receiver is a
separate IP core, connected to the parallel input of the ISP, and gets
exposed to userspace as a V4L2 subdev. To prepare for this, handle an
optional CSI-2 receiver subdev in the pipeline.

> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 35 ++++++++++++++++++++++--
>  1 file changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 43b76e14..a233c961 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -178,6 +178,7 @@ private:
>  	std::unique_ptr<V4L2Subdevice> isp_;
>  	std::unique_ptr<V4L2VideoDevice> param_;
>  	std::unique_ptr<V4L2VideoDevice> stat_;
> +	std::unique_ptr<V4L2Subdevice> csi_;
>  
>  	RkISP1MainPath mainPath_;
>  	RkISP1SelfPath selfPath_;
> @@ -591,6 +592,12 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>  
>  	LOG(RkISP1, Debug) << "Sensor configured with " << format;
>  
> +	if (csi_) {
> +		ret = csi_->setFormat(0, &format);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	ret = isp_->setFormat(0, &format);
>  	if (ret < 0)
>  		return ret;
> @@ -892,7 +899,7 @@ int PipelineHandlerRkISP1::initLinks(Camera *camera,
>  	 * Configure the sensor links: enable the link corresponding to this
>  	 * camera.
>  	 */
> -	const MediaPad *pad = isp_->entity()->getPadByIndex(0);
> +	const MediaPad *pad = (csi_ ? csi_ : isp_)->entity()->getPadByIndex(0);
>  	for (MediaLink *link : pad->links()) {
>  		if (link->source()->entity() != sensor->entity())
>  			continue;
> @@ -907,6 +914,18 @@ int PipelineHandlerRkISP1::initLinks(Camera *camera,
>  			return ret;
>  	}
>  
> +	if (csi_) {
> +		const MediaPad *ispPad = isp_->entity()->getPadByIndex(0);
> +		for (MediaLink *link : ispPad->links()) {
> +			if (link->source()->entity() != csi_->entity())
> +				continue;
> +
> +			ret = link->setEnabled(true);
> +			if (ret < 0)
> +				return ret;

I think you can break here.

> +		}
> +	}
> +
>  	for (const StreamConfiguration &cfg : config) {
>  		if (cfg.stream() == &data->mainPathStream_)
>  			ret = data->mainPath_->setEnabled(true);
> @@ -1005,6 +1024,18 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
>  	if (isp_->open() < 0)
>  		return false;
>  
> +	pad = isp_->entity()->getPadByIndex(0);
> +	if (!pad || pad->links().size() != 1)

The second check will break Scarlet, as it has two sensors attached
directly to the ISP sink pad. Let's replace it with a
pad->links().empty() check.

> +		return false;
> +
> +	csi_ = std::make_unique<V4L2Subdevice>(pad->links().at(0)->source()->entity());
> +	if (!csi_ || csi_->open() < 0)
> +		return false;
> +
> +	/* If the media device has no 1th pad, it's the sensor */
> +	if (!csi_->entity()->getPadByIndex(1))

We could have a sensor with multiple subdevs, so this isn't very
future-proof. How about testing the entity function ? The CSI-2 receiver
subdev uses MEDIA_ENT_F_VID_IF_BRIDGE.

> +		csi_ = nullptr;

Could you avoid creating the csi_ to destroy it right after in that case
? Something like

	/* Locate and open the optional CSI-2 receiver. */
	MediaPad *ispSink = isp_->entity()->getPadByIndex(0);
	if (!ispSink || ispSink->links().empty())
		return false;

	pad = ispSink->links().at(0)->source();
	if (pad->entity()->function() == MEDIA_ENT_F_VID_IF_BRIDGE) {
		csi_ = std::make_unique<V4L2Subdevice>(pad->entity());
		if (!csi_ || csi_->open() < 0)
			return false;

		ispSink = csi_->entity()->getPadByIndex(0);
		if (!ispSink)
			return false;
	}

> +
>  	/* Locate and open the stats and params video nodes. */
>  	stat_ = V4L2VideoDevice::fromEntityName(media_, "rkisp1_stats");
>  	if (stat_->open() < 0)
> @@ -1030,7 +1061,7 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
>  	 * Enumerate all sensors connected to the ISP and create one
>  	 * camera instance for each of them.
>  	 */
> -	pad = isp_->entity()->getPadByIndex(0);
> +	pad = (csi_ ? csi_ : isp_)->entity()->getPadByIndex(0);
>  	if (!pad)
>  		return false;

And you can then drop this, and use ispSink instead of pad in the loop
below.

I'm actually tempted to make ispSink a member variable, so you could
then use it in PipelineHandlerRkISP1::initLinks() instead of duplicating
logic.

>  

-- 
Regards,

Laurent Pinchart