[v3,4/4] libcamera: pipeline: rkisp1: Convert to use MediaPipeline
diff mbox series

Message ID 20241008231314.744556-5-kieran.bingham@ideasonboard.com
State New
Headers show
Series
  • MediaPipeline: Complex input device support
Related show

Commit Message

Kieran Bingham Oct. 8, 2024, 11:13 p.m. UTC 
Use the new MediaPipeline to manage and identify all sensors connected
to complex pipelines that can connect to the CSI2 receiver before the
ISP.

This can include chained multiplexers that supply multiple cameras, so
make use of the MediaDevice::locateEntities to search for all cameras
and construct a pipeline for each.

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 86 +++++++++---------------
 1 file changed, 32 insertions(+), 54 deletions(-)

Comments

Dan Scally Oct. 24, 2024, 9:07 a.m. UTC | #1 
Hi Kieran

On 09/10/2024 00:13, Kieran Bingham wrote:
> Use the new MediaPipeline to manage and identify all sensors connected
> to complex pipelines that can connect to the CSI2 receiver before the
> ISP.
>
> This can include chained multiplexers that supply multiple cameras, so
> make use of the MediaDevice::locateEntities to search for all cameras
> and construct a pipeline for each.
>
> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>   src/libcamera/pipeline/rkisp1/rkisp1.cpp | 86 +++++++++---------------
>   1 file changed, 32 insertions(+), 54 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 2fee84e56d4d..e94f047ba70c 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -37,6 +37,7 @@
>   #include "libcamera/internal/framebuffer.h"
>   #include "libcamera/internal/ipa_manager.h"
>   #include "libcamera/internal/media_device.h"
> +#include "libcamera/internal/media_pipeline.h"
>   #include "libcamera/internal/pipeline_handler.h"
>   #include "libcamera/internal/v4l2_subdevice.h"
>   #include "libcamera/internal/v4l2_videodevice.h"
> @@ -108,6 +109,11 @@ public:
>   
>   	std::unique_ptr<ipa::rkisp1::IPAProxyRkISP1> ipa_;
>   
> +	/*
> +	 * All entities in the pipeline, from the camera sensor to the RkISP1.
> +	 */
> +	MediaPipeline pipe_;
> +
>   private:
>   	void paramFilled(unsigned int frame, unsigned int bytesused);
>   	void setSensorControls(unsigned int frame,
> @@ -171,8 +177,7 @@ private:
>   	friend RkISP1CameraData;
>   	friend RkISP1Frames;
>   
> -	int initLinks(Camera *camera, const CameraSensor *sensor,
> -		      const RkISP1CameraConfiguration &config);
> +	int initLinks(Camera *camera, const RkISP1CameraConfiguration &config);
>   	int createCamera(MediaEntity *sensor);
>   	void tryCompleteRequest(RkISP1FrameInfo *info);
>   	void bufferReady(FrameBuffer *buffer);
> @@ -187,7 +192,6 @@ private:
>   	std::unique_ptr<V4L2Subdevice> isp_;
>   	std::unique_ptr<V4L2VideoDevice> param_;
>   	std::unique_ptr<V4L2VideoDevice> stat_;
> -	std::unique_ptr<V4L2Subdevice> csi_;
>   
>   	bool hasSelfPath_;
>   	bool isRaw_;
> @@ -201,8 +205,6 @@ private:
>   	std::queue<FrameBuffer *> availableStatBuffers_;
>   
>   	Camera *activeCamera_;
> -
> -	const MediaPad *ispSink_;
>   };
>   
>   RkISP1Frames::RkISP1Frames(PipelineHandler *pipe)
> @@ -712,7 +714,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>   	CameraSensor *sensor = data->sensor_.get();
>   	int ret;
>   
> -	ret = initLinks(camera, sensor, *config);
> +	ret = initLinks(camera, *config);
>   	if (ret)
>   		return ret;
>   
> @@ -729,12 +731,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;
> -	}
> +	/* Propagate format through the internal media pipeline up to the ISP */
> +	ret = data->pipe_.configure(sensor, &format);
> +	if (ret < 0)
> +		return ret;
>   
> +	LOG(RkISP1, Debug) << "Configuring ISP with : " << format;
>   	ret = isp_->setFormat(0, &format);
>   	if (ret < 0)
>   		return ret;
> @@ -1035,7 +1037,6 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)
>    */
>   
>   int PipelineHandlerRkISP1::initLinks(Camera *camera,
> -				     const CameraSensor *sensor,
>   				     const RkISP1CameraConfiguration &config)
>   {
>   	RkISP1CameraData *data = cameraData(camera);
> @@ -1046,31 +1047,16 @@ int PipelineHandlerRkISP1::initLinks(Camera *camera,
>   		return ret;
>   
>   	/*
> -	 * Configure the sensor links: enable the link corresponding to this
> -	 * camera.
> +	 * Configure the sensor links: enable the links corresponding to this
> +	 * pipeline all the way up to the ISP, through any connected CSI receiver.
>   	 */
> -	for (MediaLink *link : ispSink_->links()) {
> -		if (link->source()->entity() != sensor->entity())
> -			continue;
> -
> -		LOG(RkISP1, Debug)
> -			<< "Enabling link from sensor '"
> -			<< link->source()->entity()->name()
> -			<< "' to ISP";
> -
> -		ret = link->setEnabled(true);
> -		if (ret < 0)
> -			return ret;
> -	}
> -
> -	if (csi_) {
> -		MediaLink *link = isp_->entity()->getPadByIndex(0)->links().at(0);
> -
> -		ret = link->setEnabled(true);
> -		if (ret < 0)
> -			return ret;
> +	ret = data->pipe_.initLinks();
> +	if (ret) {
> +		LOG(RkISP1, Error) << "Failed to set up pipe links";
> +		return ret;
>   	}
>   
> +	/* Configure the paths after the ISP */
>   	for (const StreamConfiguration &cfg : config) {
>   		if (cfg.stream() == &data->mainPathStream_)
>   			ret = data->mainPath_->setEnabled(true);
> @@ -1094,6 +1080,13 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>   		std::make_unique<RkISP1CameraData>(this, &mainPath_,
>   						   hasSelfPath_ ? &selfPath_ : nullptr);
>   
> +	/* Identify the pipeline path between the sensor and the rkisp1_isp */
> +	ret = data->pipe_.init(sensor, "rkisp1_isp");

The "rkisp1_isp" entity is a subdevice right? That makes me a bit more wary of things in the last 
patch, as the impression I got from it was that the design is for sensor subdevice to video 
device...let me go back to that.

> +	if (ret) {
> +		LOG(RkISP1, Error) << "Failed to identify path from sensor to rkisp1_isp";
> +		return ret;
> +	}
> +
>   	data->sensor_ = std::make_unique<CameraSensor>(sensor);
>   	ret = data->sensor_->init();
>   	if (ret)
> @@ -1129,6 +1122,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>   	const std::string &id = data->sensor_->id();
>   	std::shared_ptr<Camera> camera =
>   		Camera::create(std::move(data), id, streams);
> +
>   	registerCamera(std::move(camera));
>   
>   	return 0;
> @@ -1136,8 +1130,6 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>   
>   bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
>   {
> -	const MediaPad *pad;
> -
>   	DeviceMatch dm("rkisp1");
>   	dm.add("rkisp1_isp");
>   	dm.add("rkisp1_resizer_mainpath");
> @@ -1162,22 +1154,6 @@ 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_->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)
> @@ -1205,8 +1181,10 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
>   	 * camera instance for each of them.
>   	 */
>   	bool registered = false;
> -	for (MediaLink *link : ispSink_->links()) {
> -		if (!createCamera(link->source()->entity()))
> +
> +	for (MediaEntity *entity : media_->locateEntities(MEDIA_ENT_F_CAM_SENSOR)) {
> +		LOG(RkISP1, Info) << "Identified " << entity->name();

Does this need to be an Info, or would Debug do?


Otherwise I think this looks ok:

Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

> +		if (!createCamera(entity))
>   			registered = true;
>   	}
>
Kieran Bingham Oct. 24, 2024, 11:42 a.m. UTC | #2 
Quoting Dan Scally (2024-10-24 10:07:32)
> Hi Kieran
> 
> On 09/10/2024 00:13, Kieran Bingham wrote:
> > Use the new MediaPipeline to manage and identify all sensors connected
> > to complex pipelines that can connect to the CSI2 receiver before the
> > ISP.
> >
> > This can include chained multiplexers that supply multiple cameras, so
> > make use of the MediaDevice::locateEntities to search for all cameras
> > and construct a pipeline for each.
> >
> > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >   src/libcamera/pipeline/rkisp1/rkisp1.cpp | 86 +++++++++---------------
> >   1 file changed, 32 insertions(+), 54 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 2fee84e56d4d..e94f047ba70c 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -37,6 +37,7 @@
> >   #include "libcamera/internal/framebuffer.h"
> >   #include "libcamera/internal/ipa_manager.h"
> >   #include "libcamera/internal/media_device.h"
> > +#include "libcamera/internal/media_pipeline.h"
> >   #include "libcamera/internal/pipeline_handler.h"
> >   #include "libcamera/internal/v4l2_subdevice.h"
> >   #include "libcamera/internal/v4l2_videodevice.h"
> > @@ -108,6 +109,11 @@ public:
> >   
> >       std::unique_ptr<ipa::rkisp1::IPAProxyRkISP1> ipa_;
> >   
> > +     /*
> > +      * All entities in the pipeline, from the camera sensor to the RkISP1.
> > +      */
> > +     MediaPipeline pipe_;
> > +
> >   private:
> >       void paramFilled(unsigned int frame, unsigned int bytesused);
> >       void setSensorControls(unsigned int frame,
> > @@ -171,8 +177,7 @@ private:
> >       friend RkISP1CameraData;
> >       friend RkISP1Frames;
> >   
> > -     int initLinks(Camera *camera, const CameraSensor *sensor,
> > -                   const RkISP1CameraConfiguration &config);
> > +     int initLinks(Camera *camera, const RkISP1CameraConfiguration &config);
> >       int createCamera(MediaEntity *sensor);
> >       void tryCompleteRequest(RkISP1FrameInfo *info);
> >       void bufferReady(FrameBuffer *buffer);
> > @@ -187,7 +192,6 @@ private:
> >       std::unique_ptr<V4L2Subdevice> isp_;
> >       std::unique_ptr<V4L2VideoDevice> param_;
> >       std::unique_ptr<V4L2VideoDevice> stat_;
> > -     std::unique_ptr<V4L2Subdevice> csi_;
> >   
> >       bool hasSelfPath_;
> >       bool isRaw_;
> > @@ -201,8 +205,6 @@ private:
> >       std::queue<FrameBuffer *> availableStatBuffers_;
> >   
> >       Camera *activeCamera_;
> > -
> > -     const MediaPad *ispSink_;
> >   };
> >   
> >   RkISP1Frames::RkISP1Frames(PipelineHandler *pipe)
> > @@ -712,7 +714,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> >       CameraSensor *sensor = data->sensor_.get();
> >       int ret;
> >   
> > -     ret = initLinks(camera, sensor, *config);
> > +     ret = initLinks(camera, *config);
> >       if (ret)
> >               return ret;
> >   
> > @@ -729,12 +731,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;
> > -     }
> > +     /* Propagate format through the internal media pipeline up to the ISP */
> > +     ret = data->pipe_.configure(sensor, &format);
> > +     if (ret < 0)
> > +             return ret;
> >   
> > +     LOG(RkISP1, Debug) << "Configuring ISP with : " << format;
> >       ret = isp_->setFormat(0, &format);
> >       if (ret < 0)
> >               return ret;
> > @@ -1035,7 +1037,6 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)
> >    */
> >   
> >   int PipelineHandlerRkISP1::initLinks(Camera *camera,
> > -                                  const CameraSensor *sensor,
> >                                    const RkISP1CameraConfiguration &config)
> >   {
> >       RkISP1CameraData *data = cameraData(camera);
> > @@ -1046,31 +1047,16 @@ int PipelineHandlerRkISP1::initLinks(Camera *camera,
> >               return ret;
> >   
> >       /*
> > -      * Configure the sensor links: enable the link corresponding to this
> > -      * camera.
> > +      * Configure the sensor links: enable the links corresponding to this
> > +      * pipeline all the way up to the ISP, through any connected CSI receiver.
> >        */
> > -     for (MediaLink *link : ispSink_->links()) {
> > -             if (link->source()->entity() != sensor->entity())
> > -                     continue;
> > -
> > -             LOG(RkISP1, Debug)
> > -                     << "Enabling link from sensor '"
> > -                     << link->source()->entity()->name()
> > -                     << "' to ISP";
> > -
> > -             ret = link->setEnabled(true);
> > -             if (ret < 0)
> > -                     return ret;
> > -     }
> > -
> > -     if (csi_) {
> > -             MediaLink *link = isp_->entity()->getPadByIndex(0)->links().at(0);
> > -
> > -             ret = link->setEnabled(true);
> > -             if (ret < 0)
> > -                     return ret;
> > +     ret = data->pipe_.initLinks();
> > +     if (ret) {
> > +             LOG(RkISP1, Error) << "Failed to set up pipe links";
> > +             return ret;
> >       }
> >   
> > +     /* Configure the paths after the ISP */
> >       for (const StreamConfiguration &cfg : config) {
> >               if (cfg.stream() == &data->mainPathStream_)
> >                       ret = data->mainPath_->setEnabled(true);
> > @@ -1094,6 +1080,13 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> >               std::make_unique<RkISP1CameraData>(this, &mainPath_,
> >                                                  hasSelfPath_ ? &selfPath_ : nullptr);
> >   
> > +     /* Identify the pipeline path between the sensor and the rkisp1_isp */
> > +     ret = data->pipe_.init(sensor, "rkisp1_isp");
> 
> The "rkisp1_isp" entity is a subdevice right? That makes me a bit more wary of things in the last 
> patch, as the impression I got from it was that the design is for sensor subdevice to video 
> device...let me go back to that.

Yes, correct - it's a subdevice. The original code base this came from
was the simple pipeline handler which ends at a video device. But the
RKISP1 feeds into the ISP.

What I want from this series is to abstract a path of connected entities
- and make sure that 'segment' is configurable independently regardless
of length. In this instance - the raw data path from the sensor into
the ISP only.



> 
> > +     if (ret) {
> > +             LOG(RkISP1, Error) << "Failed to identify path from sensor to rkisp1_isp";
> > +             return ret;
> > +     }
> > +
> >       data->sensor_ = std::make_unique<CameraSensor>(sensor);
> >       ret = data->sensor_->init();
> >       if (ret)
> > @@ -1129,6 +1122,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> >       const std::string &id = data->sensor_->id();
> >       std::shared_ptr<Camera> camera =
> >               Camera::create(std::move(data), id, streams);
> > +
> >       registerCamera(std::move(camera));
> >   
> >       return 0;
> > @@ -1136,8 +1130,6 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> >   
> >   bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
> >   {
> > -     const MediaPad *pad;
> > -
> >       DeviceMatch dm("rkisp1");
> >       dm.add("rkisp1_isp");
> >       dm.add("rkisp1_resizer_mainpath");
> > @@ -1162,22 +1154,6 @@ 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_->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)
> > @@ -1205,8 +1181,10 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
> >        * camera instance for each of them.
> >        */
> >       bool registered = false;
> > -     for (MediaLink *link : ispSink_->links()) {
> > -             if (!createCamera(link->source()->entity()))
> > +
> > +     for (MediaEntity *entity : media_->locateEntities(MEDIA_ENT_F_CAM_SENSOR)) {
> > +             LOG(RkISP1, Info) << "Identified " << entity->name();
> 
> Does this need to be an Info, or would Debug do?

Yes, likely just a debug level is fine here ... We don't want things to
be too verbose at startup - though it does help me seeing this at
startup at the moment but I think I can manage to set a debug level ;-)

> Otherwise I think this looks ok:
> 
> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

Thanks

> 
> > +             if (!createCamera(entity))
> >                       registered = true;
> >       }
> >
Jacopo Mondi Oct. 24, 2024, 4:29 p.m. UTC | #3 
Hi Kieran

On Wed, Oct 09, 2024 at 12:13:14AM +0100, Kieran Bingham wrote:
> Use the new MediaPipeline to manage and identify all sensors connected
> to complex pipelines that can connect to the CSI2 receiver before the
> ISP.
>
> This can include chained multiplexers that supply multiple cameras, so
> make use of the MediaDevice::locateEntities to search for all cameras
> and construct a pipeline for each.
>
> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 86 +++++++++---------------
>  1 file changed, 32 insertions(+), 54 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 2fee84e56d4d..e94f047ba70c 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -37,6 +37,7 @@
>  #include "libcamera/internal/framebuffer.h"
>  #include "libcamera/internal/ipa_manager.h"
>  #include "libcamera/internal/media_device.h"
> +#include "libcamera/internal/media_pipeline.h"
>  #include "libcamera/internal/pipeline_handler.h"
>  #include "libcamera/internal/v4l2_subdevice.h"
>  #include "libcamera/internal/v4l2_videodevice.h"
> @@ -108,6 +109,11 @@ public:
>
>  	std::unique_ptr<ipa::rkisp1::IPAProxyRkISP1> ipa_;
>
> +	/*
> +	 * All entities in the pipeline, from the camera sensor to the RkISP1.
> +	 */

(Almost) fits on 1 line

> +	MediaPipeline pipe_;
> +
>  private:
>  	void paramFilled(unsigned int frame, unsigned int bytesused);
>  	void setSensorControls(unsigned int frame,
> @@ -171,8 +177,7 @@ private:
>  	friend RkISP1CameraData;
>  	friend RkISP1Frames;
>
> -	int initLinks(Camera *camera, const CameraSensor *sensor,
> -		      const RkISP1CameraConfiguration &config);
> +	int initLinks(Camera *camera, const RkISP1CameraConfiguration &config);
>  	int createCamera(MediaEntity *sensor);
>  	void tryCompleteRequest(RkISP1FrameInfo *info);
>  	void bufferReady(FrameBuffer *buffer);
> @@ -187,7 +192,6 @@ private:
>  	std::unique_ptr<V4L2Subdevice> isp_;
>  	std::unique_ptr<V4L2VideoDevice> param_;
>  	std::unique_ptr<V4L2VideoDevice> stat_;
> -	std::unique_ptr<V4L2Subdevice> csi_;
>
>  	bool hasSelfPath_;
>  	bool isRaw_;
> @@ -201,8 +205,6 @@ private:
>  	std::queue<FrameBuffer *> availableStatBuffers_;
>
>  	Camera *activeCamera_;
> -
> -	const MediaPad *ispSink_;
>  };
>
>  RkISP1Frames::RkISP1Frames(PipelineHandler *pipe)
> @@ -712,7 +714,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>  	CameraSensor *sensor = data->sensor_.get();
>  	int ret;
>
> -	ret = initLinks(camera, sensor, *config);
> +	ret = initLinks(camera, *config);
>  	if (ret)
>  		return ret;
>
> @@ -729,12 +731,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;
> -	}
> +	/* Propagate format through the internal media pipeline up to the ISP */
> +	ret = data->pipe_.configure(sensor, &format);
> +	if (ret < 0)
> +		return ret;

Actually the API is not that bad

>
> +	LOG(RkISP1, Debug) << "Configuring ISP with : " << format;
>  	ret = isp_->setFormat(0, &format);
>  	if (ret < 0)
>  		return ret;
> @@ -1035,7 +1037,6 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)
>   */
>
>  int PipelineHandlerRkISP1::initLinks(Camera *camera,
> -				     const CameraSensor *sensor,
>  				     const RkISP1CameraConfiguration &config)
>  {
>  	RkISP1CameraData *data = cameraData(camera);
> @@ -1046,31 +1047,16 @@ int PipelineHandlerRkISP1::initLinks(Camera *camera,
>  		return ret;
>
>  	/*
> -	 * Configure the sensor links: enable the link corresponding to this
> -	 * camera.
> +	 * Configure the sensor links: enable the links corresponding to this
> +	 * pipeline all the way up to the ISP, through any connected CSI receiver.
>  	 */
> -	for (MediaLink *link : ispSink_->links()) {
> -		if (link->source()->entity() != sensor->entity())
> -			continue;
> -
> -		LOG(RkISP1, Debug)
> -			<< "Enabling link from sensor '"
> -			<< link->source()->entity()->name()
> -			<< "' to ISP";
> -
> -		ret = link->setEnabled(true);
> -		if (ret < 0)
> -			return ret;
> -	}
> -
> -	if (csi_) {
> -		MediaLink *link = isp_->entity()->getPadByIndex(0)->links().at(0);
> -
> -		ret = link->setEnabled(true);
> -		if (ret < 0)
> -			return ret;
> +	ret = data->pipe_.initLinks();
> +	if (ret) {
> +		LOG(RkISP1, Error) << "Failed to set up pipe links";
> +		return ret;
>  	}
>
> +	/* Configure the paths after the ISP */
>  	for (const StreamConfiguration &cfg : config) {
>  		if (cfg.stream() == &data->mainPathStream_)
>  			ret = data->mainPath_->setEnabled(true);
> @@ -1094,6 +1080,13 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>  		std::make_unique<RkISP1CameraData>(this, &mainPath_,
>  						   hasSelfPath_ ? &selfPath_ : nullptr);
>
> +	/* Identify the pipeline path between the sensor and the rkisp1_isp */
> +	ret = data->pipe_.init(sensor, "rkisp1_isp");
> +	if (ret) {
> +		LOG(RkISP1, Error) << "Failed to identify path from sensor to rkisp1_isp";
> +		return ret;
> +	}
> +
>  	data->sensor_ = std::make_unique<CameraSensor>(sensor);
>  	ret = data->sensor_->init();
>  	if (ret)
> @@ -1129,6 +1122,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>  	const std::string &id = data->sensor_->id();
>  	std::shared_ptr<Camera> camera =
>  		Camera::create(std::move(data), id, streams);
> +
>  	registerCamera(std::move(camera));
>
>  	return 0;
> @@ -1136,8 +1130,6 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>
>  bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
>  {
> -	const MediaPad *pad;
> -
>  	DeviceMatch dm("rkisp1");
>  	dm.add("rkisp1_isp");
>  	dm.add("rkisp1_resizer_mainpath");
> @@ -1162,22 +1154,6 @@ 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_->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)
> @@ -1205,8 +1181,10 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
>  	 * camera instance for each of them.
>  	 */
>  	bool registered = false;
> -	for (MediaLink *link : ispSink_->links()) {
> -		if (!createCamera(link->source()->entity()))
> +
> +	for (MediaEntity *entity : media_->locateEntities(MEDIA_ENT_F_CAM_SENSOR)) {
> +		LOG(RkISP1, Info) << "Identified " << entity->name();
> +		if (!createCamera(entity))
>  			registered = true;

Looks good
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

>  	}
>
> --
> 2.34.1
>

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index 2fee84e56d4d..e94f047ba70c 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -37,6 +37,7 @@ 
 #include "libcamera/internal/framebuffer.h"
 #include "libcamera/internal/ipa_manager.h"
 #include "libcamera/internal/media_device.h"
+#include "libcamera/internal/media_pipeline.h"
 #include "libcamera/internal/pipeline_handler.h"
 #include "libcamera/internal/v4l2_subdevice.h"
 #include "libcamera/internal/v4l2_videodevice.h"
@@ -108,6 +109,11 @@  public:
 
 	std::unique_ptr<ipa::rkisp1::IPAProxyRkISP1> ipa_;
 
+	/*
+	 * All entities in the pipeline, from the camera sensor to the RkISP1.
+	 */
+	MediaPipeline pipe_;
+
 private:
 	void paramFilled(unsigned int frame, unsigned int bytesused);
 	void setSensorControls(unsigned int frame,
@@ -171,8 +177,7 @@  private:
 	friend RkISP1CameraData;
 	friend RkISP1Frames;
 
-	int initLinks(Camera *camera, const CameraSensor *sensor,
-		      const RkISP1CameraConfiguration &config);
+	int initLinks(Camera *camera, const RkISP1CameraConfiguration &config);
 	int createCamera(MediaEntity *sensor);
 	void tryCompleteRequest(RkISP1FrameInfo *info);
 	void bufferReady(FrameBuffer *buffer);
@@ -187,7 +192,6 @@  private:
 	std::unique_ptr<V4L2Subdevice> isp_;
 	std::unique_ptr<V4L2VideoDevice> param_;
 	std::unique_ptr<V4L2VideoDevice> stat_;
-	std::unique_ptr<V4L2Subdevice> csi_;
 
 	bool hasSelfPath_;
 	bool isRaw_;
@@ -201,8 +205,6 @@  private:
 	std::queue<FrameBuffer *> availableStatBuffers_;
 
 	Camera *activeCamera_;
-
-	const MediaPad *ispSink_;
 };
 
 RkISP1Frames::RkISP1Frames(PipelineHandler *pipe)
@@ -712,7 +714,7 @@  int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
 	CameraSensor *sensor = data->sensor_.get();
 	int ret;
 
-	ret = initLinks(camera, sensor, *config);
+	ret = initLinks(camera, *config);
 	if (ret)
 		return ret;
 
@@ -729,12 +731,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;
-	}
+	/* Propagate format through the internal media pipeline up to the ISP */
+	ret = data->pipe_.configure(sensor, &format);
+	if (ret < 0)
+		return ret;
 
+	LOG(RkISP1, Debug) << "Configuring ISP with : " << format;
 	ret = isp_->setFormat(0, &format);
 	if (ret < 0)
 		return ret;
@@ -1035,7 +1037,6 @@  int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)
  */
 
 int PipelineHandlerRkISP1::initLinks(Camera *camera,
-				     const CameraSensor *sensor,
 				     const RkISP1CameraConfiguration &config)
 {
 	RkISP1CameraData *data = cameraData(camera);
@@ -1046,31 +1047,16 @@  int PipelineHandlerRkISP1::initLinks(Camera *camera,
 		return ret;
 
 	/*
-	 * Configure the sensor links: enable the link corresponding to this
-	 * camera.
+	 * Configure the sensor links: enable the links corresponding to this
+	 * pipeline all the way up to the ISP, through any connected CSI receiver.
 	 */
-	for (MediaLink *link : ispSink_->links()) {
-		if (link->source()->entity() != sensor->entity())
-			continue;
-
-		LOG(RkISP1, Debug)
-			<< "Enabling link from sensor '"
-			<< link->source()->entity()->name()
-			<< "' to ISP";
-
-		ret = link->setEnabled(true);
-		if (ret < 0)
-			return ret;
-	}
-
-	if (csi_) {
-		MediaLink *link = isp_->entity()->getPadByIndex(0)->links().at(0);
-
-		ret = link->setEnabled(true);
-		if (ret < 0)
-			return ret;
+	ret = data->pipe_.initLinks();
+	if (ret) {
+		LOG(RkISP1, Error) << "Failed to set up pipe links";
+		return ret;
 	}
 
+	/* Configure the paths after the ISP */
 	for (const StreamConfiguration &cfg : config) {
 		if (cfg.stream() == &data->mainPathStream_)
 			ret = data->mainPath_->setEnabled(true);
@@ -1094,6 +1080,13 @@  int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
 		std::make_unique<RkISP1CameraData>(this, &mainPath_,
 						   hasSelfPath_ ? &selfPath_ : nullptr);
 
+	/* Identify the pipeline path between the sensor and the rkisp1_isp */
+	ret = data->pipe_.init(sensor, "rkisp1_isp");
+	if (ret) {
+		LOG(RkISP1, Error) << "Failed to identify path from sensor to rkisp1_isp";
+		return ret;
+	}
+
 	data->sensor_ = std::make_unique<CameraSensor>(sensor);
 	ret = data->sensor_->init();
 	if (ret)
@@ -1129,6 +1122,7 @@  int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
 	const std::string &id = data->sensor_->id();
 	std::shared_ptr<Camera> camera =
 		Camera::create(std::move(data), id, streams);
+
 	registerCamera(std::move(camera));
 
 	return 0;
@@ -1136,8 +1130,6 @@  int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
 
 bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
 {
-	const MediaPad *pad;
-
 	DeviceMatch dm("rkisp1");
 	dm.add("rkisp1_isp");
 	dm.add("rkisp1_resizer_mainpath");
@@ -1162,22 +1154,6 @@  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_->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)
@@ -1205,8 +1181,10 @@  bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
 	 * camera instance for each of them.
 	 */
 	bool registered = false;
-	for (MediaLink *link : ispSink_->links()) {
-		if (!createCamera(link->source()->entity()))
+
+	for (MediaEntity *entity : media_->locateEntities(MEDIA_ENT_F_CAM_SENSOR)) {
+		LOG(RkISP1, Info) << "Identified " << entity->name();
+		if (!createCamera(entity))
 			registered = true;
 	}