[libcamera-devel] [PATCH] pipeline: rkisp1: Support media graph with separate CSI RX

[Kieran Bingham]

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 at 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
>