[PATCH v3 4/4] libcamera: pipeline: rkisp1: Convert to use MediaPipeline
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Oct 24 13:42:05 CEST 2024
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 at ideasonboard.com>
> > Signed-off-by: Kieran Bingham <kieran.bingham at 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 at ideasonboard.com>
Thanks
>
> > + if (!createCamera(entity))
> > registered = true;
> > }
> >
More information about the libcamera-devel
mailing list