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

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

Commit Message

Paul Elder July 13, 2022, 11:34 a.m. UTC 
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>

---
Changes in v2:
- clean up logic of determining pads/prescence of the CSI-2 receiver
  subdev
- upgrade the changelog
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 42 ++++++++++++++++++++----
 1 file changed, 35 insertions(+), 7 deletions(-)

Comments

Laurent Pinchart July 13, 2022, 3:50 p.m. UTC | #1 
Hi Paul,

Thank you for the patch.

On Wed, Jul 13, 2022 at 08:34:43PM +0900, Paul Elder via libcamera-devel wrote:
> 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>
> 
> ---
> Changes in v2:
> - clean up logic of determining pads/prescence of the CSI-2 receiver
>   subdev
> - upgrade the changelog
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 42 ++++++++++++++++++++----
>  1 file changed, 35 insertions(+), 7 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 3dc0850c..2b0ead5d 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_;
> @@ -188,6 +189,8 @@ private:
>  	std::queue<FrameBuffer *> availableStatBuffers_;
>  
>  	Camera *activeCamera_;
> +
> +	const MediaPad *ispSink_;
>  };
>  
>  RkISP1Frames::RkISP1Frames(PipelineHandler *pipe)
> @@ -599,6 +602,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;
> @@ -900,8 +909,7 @@ int PipelineHandlerRkISP1::initLinks(Camera *camera,
>  	 * Configure the sensor links: enable the link corresponding to this
>  	 * camera.
>  	 */
> -	const MediaPad *pad = isp_->entity()->getPadByIndex(0);
> -	for (MediaLink *link : pad->links()) {
> +	for (MediaLink *link : ispSink_->links()) {
>  		if (link->source()->entity() != sensor->entity())
>  			continue;
>  
> @@ -915,6 +923,14 @@ int PipelineHandlerRkISP1::initLinks(Camera *camera,
>  			return ret;
>  	}
>  
> +	if (csi_) {
> +		MediaLink *link = isp_->entity()->getPadByIndex(0)->links().at(0);
> +
> +		ret = link->setEnabled(true);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	for (const StreamConfiguration &cfg : config) {
>  		if (cfg.stream() == &data->mainPathStream_)
>  			ret = data->mainPath_->setEnabled(true);
> @@ -1013,6 +1029,22 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
>  	if (isp_->open() < 0)
>  		return false;
>  
> +	/* Locate and open the optional CSI-2 receiver. */
> +	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)

You can drop the !csi_ check as std::make_unique<> never returns a null
pointer. With this fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +			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)
> @@ -1038,12 +1070,8 @@ 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);
> -	if (!pad)
> -		return false;
> -
>  	bool registered = false;
> -	for (MediaLink *link : pad->links()) {
> +	for (MediaLink *link : ispSink_->links()) {
>  		if (!createCamera(link->source()->entity()))
>  			registered = true;
>  	}
Jacopo Mondi July 14, 2022, 7:28 a.m. UTC | #2 
Hi Paul,

On Wed, Jul 13, 2022 at 06:50:49PM +0300, Laurent Pinchart via libcamera-devel wrote:
> Hi Paul,
>
> Thank you for the patch.
>
> On Wed, Jul 13, 2022 at 08:34:43PM +0900, Paul Elder via libcamera-devel wrote:
> > 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>
> >
> > ---
> > Changes in v2:
> > - clean up logic of determining pads/prescence of the CSI-2 receiver
> >   subdev
> > - upgrade the changelog
> > ---
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 42 ++++++++++++++++++++----
> >  1 file changed, 35 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 3dc0850c..2b0ead5d 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_;
> > @@ -188,6 +189,8 @@ private:
> >  	std::queue<FrameBuffer *> availableStatBuffers_;
> >
> >  	Camera *activeCamera_;
> > +
> > +	const MediaPad *ispSink_;
> >  };
> >
> >  RkISP1Frames::RkISP1Frames(PipelineHandler *pipe)
> > @@ -599,6 +602,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;
> > @@ -900,8 +909,7 @@ int PipelineHandlerRkISP1::initLinks(Camera *camera,
> >  	 * Configure the sensor links: enable the link corresponding to this
> >  	 * camera.
> >  	 */
> > -	const MediaPad *pad = isp_->entity()->getPadByIndex(0);
> > -	for (MediaLink *link : pad->links()) {
> > +	for (MediaLink *link : ispSink_->links()) {
> >  		if (link->source()->entity() != sensor->entity())
> >  			continue;
> >
> > @@ -915,6 +923,14 @@ int PipelineHandlerRkISP1::initLinks(Camera *camera,
> >  			return ret;
> >  	}
> >
> > +	if (csi_) {
> > +		MediaLink *link = isp_->entity()->getPadByIndex(0)->links().at(0);
> > +
> > +		ret = link->setEnabled(true);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +
> >  	for (const StreamConfiguration &cfg : config) {
> >  		if (cfg.stream() == &data->mainPathStream_)
> >  			ret = data->mainPath_->setEnabled(true);
> > @@ -1013,6 +1029,22 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
> >  	if (isp_->open() < 0)
> >  		return false;
> >
> > +	/* Locate and open the optional CSI-2 receiver. */
> > +	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)
>
> You can drop the !csi_ check as std::make_unique<> never returns a null
> pointer. With this fixed,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>

Indeed fixes the pipeline initialization with a separate CSI-2 receiver.

Tested-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

> > +			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)
> > @@ -1038,12 +1070,8 @@ 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);
> > -	if (!pad)
> > -		return false;
> > -
> >  	bool registered = false;
> > -	for (MediaLink *link : pad->links()) {
> > +	for (MediaLink *link : ispSink_->links()) {
> >  		if (!createCamera(link->source()->entity()))
> >  			registered = true;
> >  	}
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index 3dc0850c..2b0ead5d 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_;
@@ -188,6 +189,8 @@  private:
 	std::queue<FrameBuffer *> availableStatBuffers_;
 
 	Camera *activeCamera_;
+
+	const MediaPad *ispSink_;
 };
 
 RkISP1Frames::RkISP1Frames(PipelineHandler *pipe)
@@ -599,6 +602,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;
@@ -900,8 +909,7 @@  int PipelineHandlerRkISP1::initLinks(Camera *camera,
 	 * Configure the sensor links: enable the link corresponding to this
 	 * camera.
 	 */
-	const MediaPad *pad = isp_->entity()->getPadByIndex(0);
-	for (MediaLink *link : pad->links()) {
+	for (MediaLink *link : ispSink_->links()) {
 		if (link->source()->entity() != sensor->entity())
 			continue;
 
@@ -915,6 +923,14 @@  int PipelineHandlerRkISP1::initLinks(Camera *camera,
 			return ret;
 	}
 
+	if (csi_) {
+		MediaLink *link = isp_->entity()->getPadByIndex(0)->links().at(0);
+
+		ret = link->setEnabled(true);
+		if (ret < 0)
+			return ret;
+	}
+
 	for (const StreamConfiguration &cfg : config) {
 		if (cfg.stream() == &data->mainPathStream_)
 			ret = data->mainPath_->setEnabled(true);
@@ -1013,6 +1029,22 @@  bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
 	if (isp_->open() < 0)
 		return false;
 
+	/* Locate and open the optional CSI-2 receiver. */
+	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)
@@ -1038,12 +1070,8 @@  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);
-	if (!pad)
-		return false;
-
 	bool registered = false;
-	for (MediaLink *link : pad->links()) {
+	for (MediaLink *link : ispSink_->links()) {
 		if (!createCamera(link->source()->entity()))
 			registered = true;
 	}