[PATCH 4/4] libcamera: pipeline: rkisp1: Convert to use MediaPipeline

[Kieran Bingham]

Quoting Jacopo Mondi (2024-09-27 21:51:10)
> Hi Kieran
> 
> On Mon, Sep 16, 2024 at 04:02:41PM GMT, 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 multiplexors that supply multiple cameras, so
> > make use of the MediaDevice::locateEntities to search for all cameras
> > and construct a pipeline for each.
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 65 ++++++++++++------------
> >  1 file changed, 32 insertions(+), 33 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 2fee84e56d4d..914f5181b51d 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);
> > @@ -712,7 +717,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 +734,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 +1040,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 +1050,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 +1083,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");
> 
> This part is not clear to me how can work at
> MediaPipeline::configure() time, as the pipeline keeps a V4L2Subevice
> instance open for the CSI-2 receiver, and since the init() call stops
> at "rkisp1_isp" I presume data->pipe_ creates and entity for the
> CSI-2 receiver as well ?

Yes, the CSI2 receiver can now be managed through the MediaPipeline
path.

Lets see if there's more handling I can remove from the rkisp1 pipeline
handler to further simplify.

> 
> > +     if (ret) {
> > +             LOG(RkISP1, Error) << "Failed to identify path from sensor to sink";
> > +             return ret;
> > +     }
> > +
> >       data->sensor_ = std::make_unique<CameraSensor>(sensor);
> >       ret = data->sensor_->init();
> >       if (ret)
> > @@ -1129,6 +1125,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;
> > @@ -1205,8 +1202,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()))
> 
> Maybe we don't need "ispSink_" anymore ?

Oh - I think that's a good spot. I'll see what else I can find too.
--
Kieran


> 
> > +
> > +     for (MediaEntity *entity : media_->locateEntities(MEDIA_ENT_F_CAM_SENSOR)) {
> > +             LOG(RkISP1, Info) << "Identified " << entity->name();
> > +             if (!createCamera(entity))
> >                       registered = true;
> >       }
> >
> > --
> > 2.46.0
> >