[libcamera-devel] pipeline: rkisp1: Support media graph with separate CSI RX
diff mbox series

Message ID 20220622074638.144636-1-paul.elder@ideasonboard.com
State Superseded
Headers show
Series
  • [libcamera-devel] pipeline: rkisp1: Support media graph with separate CSI RX
Related show

Commit Message

Paul Elder June 22, 2022, 7:46 a.m. UTC 
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.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 35 ++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

Comments

Kieran Bingham June 22, 2022, 8:40 a.m. UTC | #1 
Quoting Paul Elder via libcamera-devel (2022-06-22 08:46:38)
> 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.
> 
> Signed-off-by: Paul Elder <paul.elder@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;
> +               }
> +       }
> +
>         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)
> +               return false;
> +
> +       csi_ = std::make_unique<V4L2Subdevice>(pad->links().at(0)->source()->entity());
> +       if (!csi_ || csi_->open() < 0)
> +               return false;

Your commit message talks about how currently there is no CSI subdev, and this
patch allows it to use it if there is one.

But doesn't this statement mean it will fail to fully match if there is
no CSI device? 

> +
> +       /* If the media device has no 1th pad, it's the sensor */
> +       if (!csi_->entity()->getPadByIndex(1))
> +               csi_ = nullptr;
> +
>         /* Locate and open the stats and params video nodes. */
>         stat_ = V4L2VideoDevice::fromEntityName(media_, "rkisp1_stats");

Like for instance, this code would not be executed anymore if !(csi_)

--
Kieran

>         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;
>  
> -- 
> 2.30.2
>
Laurent Pinchart June 22, 2022, 8:46 a.m. UTC | #2 
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@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.

>

Patch
diff mbox series 

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;
+		}
+	}
+
 	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)
+		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))
+		csi_ = nullptr;
+
 	/* 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;