[v2,1/1] pipeline: imx8-isi: Delay ISI routes config to acquire() time

24720 diff mbox

From: Andrei Gansari <andrei.gansari@nxp.com>

Fixes behavior when calling 'cam -l' during a live stream from a camera
in another process.

Issue is that multiple process should be able to list (match procedure)
the camera supported. But only the unique process that lock the media
devices in order to be able to configure then start the pipeline should
setup the routes, graphs etc.

Thus, the setRouting() is to be moved to a PipelineHandlerISI::acquireDevice()
implementation to override the default Pipeline::acquireDevice() function.

Fixes: 92df79112fb2 ("pipeline: imx8-isi: Add multicamera support")

Signed-off-by: Andrei Gansari <andrei.gansari@nxp.com>
Signed-off-by: Antoine Bouyer <antoine.bouyer@nxp.com>
---
 src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 47 +++++++++++++++++---
 1 file changed, 41 insertions(+), 6 deletions(-)

Quoting Antoine Bouyer (2025-10-22 10:38:20)
> From: Andrei Gansari <andrei.gansari@nxp.com>
> 
> Fixes behavior when calling 'cam -l' during a live stream from a camera
> in another process.
> 
> Issue is that multiple process should be able to list (match procedure)
> the camera supported. But only the unique process that lock the media
> devices in order to be able to configure then start the pipeline should
> setup the routes, graphs etc.
> 
> Thus, the setRouting() is to be moved to a PipelineHandlerISI::acquireDevice()
> implementation to override the default Pipeline::acquireDevice() function.
> 
> Fixes: 92df79112fb2 ("pipeline: imx8-isi: Add multicamera support")
> 
> Signed-off-by: Andrei Gansari <andrei.gansari@nxp.com>
> Signed-off-by: Antoine Bouyer <antoine.bouyer@nxp.com>

Thanks for the update - looks good to me.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
>  src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 47 +++++++++++++++++---
>  1 file changed, 41 insertions(+), 6 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> index de09431cb9b9..b8834540688a 100644
> --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> @@ -71,6 +71,8 @@ public:
>  
>         unsigned int xbarSink_ = 0;
>         unsigned int xbarSourceOffset_ = 0;
> +
> +       const std::string &cameraName() const { return sensor_->entity()->name(); }
>  };
>  
>  class ISICameraConfiguration : public CameraConfiguration
> @@ -117,6 +119,9 @@ protected:
>  
>         int queueRequestDevice(Camera *camera, Request *request) override;
>  
> +       bool acquireDevice(Camera *camera) override;
> +       void releaseDevice(Camera *camera) override;
> +
>  private:
>         static constexpr Size kPreviewSize = { 1920, 1080 };
>         static constexpr Size kMinISISize = { 1, 1 };
> @@ -143,6 +148,10 @@ private:
>  
>         std::unique_ptr<V4L2Subdevice> crossbar_;
>         std::vector<Pipe> pipes_;
> +
> +       unsigned int acquireCount_ = 0;
> +
> +       V4L2Subdevice::Routing routing_ = {};
>  };
>  
>  /* -----------------------------------------------------------------------------
> @@ -950,6 +959,37 @@ int PipelineHandlerISI::queueRequestDevice(Camera *camera, Request *request)
>         return 0;
>  }
>  
> +bool PipelineHandlerISI::acquireDevice(Camera *camera)
> +{
> +       ISICameraData *data = cameraData(camera);
> +
> +       LOG(ISI, Debug) << "acquireDevice " << data->cameraName()
> +                       << " count " << acquireCount_;
> +
> +       if (acquireCount_ > 0) {
> +               acquireCount_++;
> +               return true;
> +       }
> +
> +       /* Enable routing for all available sensors once */
> +       int ret = crossbar_->setRouting(&routing_, V4L2Subdevice::ActiveFormat);
> +       if (ret)
> +               return false;
> +
> +       acquireCount_++;
> +       return true;
> +}
> +
> +void PipelineHandlerISI::releaseDevice(Camera *camera)
> +{
> +       ISICameraData *data = cameraData(camera);
> +
> +       ASSERT(acquireCount_);
> +       acquireCount_--;
> +       LOG(ISI, Debug) << "releaseDevice " << data->cameraName()
> +                       << " count " << acquireCount_;
> +}
> +
>  bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)
>  {
>         DeviceMatch dm("mxc-isi");
> @@ -1034,7 +1074,6 @@ bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)
>         unsigned int numSinks = 0;
>         const unsigned int xbarFirstSource = crossbar_->entity()->pads().size() - pipes_.size();
>         const unsigned int maxStreams = pipes_.size() / cameraCount;
> -       V4L2Subdevice::Routing routing = {};
>  
>         for (MediaPad *pad : crossbar_->entity()->pads()) {
>                 unsigned int sink = numSinks;
> @@ -1104,7 +1143,7 @@ bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)
>                 /*  Add routes to the crossbar switch routing table. */
>                 for (unsigned i = 0; i < data->streams_.size(); i++) {
>                         unsigned int sourcePad = xbarFirstSource + data->xbarSourceOffset_ + i;
> -                       routing.emplace_back(V4L2Subdevice::Stream{ data->xbarSink_, 0 },
> +                       routing_.emplace_back(V4L2Subdevice::Stream{ data->xbarSink_, 0 },
>                                              V4L2Subdevice::Stream{ sourcePad, 0 },
>                                              V4L2_SUBDEV_ROUTE_FL_ACTIVE);
>                 }
> @@ -1116,10 +1155,6 @@ bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)
>                 numCameras++;
>         }
>  
> -       ret = crossbar_->setRouting(&routing, V4L2Subdevice::ActiveFormat);
> -       if (ret)
> -               return false;
> -
>         return numCameras > 0;
>  }
>  
> -- 
> 2.34.1
>

Hi Laurent

Thanks for your feedback

On 22/10/2025 14:43, Laurent Pinchart wrote:
> Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button
> 
> 
> Hi Antoine, Andrei,
> 
> Thank you for the patch.
> 
> On Wed, Oct 22, 2025 at 11:38:20AM +0200, Antoine Bouyer wrote:
>> From: Andrei Gansari <andrei.gansari@nxp.com>
>>
>> Fixes behavior when calling 'cam -l' during a live stream from a camera
>> in another process.
>>
>> Issue is that multiple process should be able to list (match procedure)
>> the camera supported. But only the unique process that lock the media
>> devices in order to be able to configure then start the pipeline should
>> setup the routes, graphs etc.
>>
>> Thus, the setRouting() is to be moved to a PipelineHandlerISI::acquireDevice()
>> implementation to override the default Pipeline::acquireDevice() function.
>>
>> Fixes: 92df79112fb2 ("pipeline: imx8-isi: Add multicamera support")
>>
> 
> Extra blank line.

Ok. Will remove it in V3
> 
>> Signed-off-by: Andrei Gansari <andrei.gansari@nxp.com>
>> Signed-off-by: Antoine Bouyer <antoine.bouyer@nxp.com>
>> ---
>>   src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 47 +++++++++++++++++---
>>   1 file changed, 41 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
>> index de09431cb9b9..b8834540688a 100644
>> --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
>> +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
>> @@ -71,6 +71,8 @@ public:
>>
>>        unsigned int xbarSink_ = 0;
>>        unsigned int xbarSourceOffset_ = 0;
>> +
>> +     const std::string &cameraName() const { return sensor_->entity()->name(); }
>>   };
>>
>>   class ISICameraConfiguration : public CameraConfiguration
>> @@ -117,6 +119,9 @@ protected:
>>
>>        int queueRequestDevice(Camera *camera, Request *request) override;
>>
>> +     bool acquireDevice(Camera *camera) override;
>> +     void releaseDevice(Camera *camera) override;
>> +
>>   private:
>>        static constexpr Size kPreviewSize = { 1920, 1080 };
>>        static constexpr Size kMinISISize = { 1, 1 };
>> @@ -143,6 +148,10 @@ private:
>>
>>        std::unique_ptr<V4L2Subdevice> crossbar_;
>>        std::vector<Pipe> pipes_;
>> +
>> +     unsigned int acquireCount_ = 0;
> 
> This essentially duplicates PipelineHandler::useCount_. Is there a
> reason to do so instead of changing acquireDevice() to only be called
> once ? You could then drop releaseDevice().

I voluntarily did not use useCount_ counter because it is private, so I 
could not monitor it in PipelineHandlerISI to verify whether the routing 
was already configured or not.

Do you suggest to move useCount_ to protected, so I can check (useCount 
== 0) from PipelineHandlerISI::acquireDevice ? Then indeed, 
releaseDevice becomes useless as I don't need acquireCount_ anymore 
inside imx8-isi pipeline.

Or do you suggest to keep useCount_ private, and change 
PipelineHandler::acquire to call acquireDevice only if useCount_ is null ?

I assume 2nd option could have side effect, since acquireDevice has the 
camera as parameter. Some pipeline handlers may need to know which 
camera is being acquired.

Already tested option #1 is functional on my side.

Best regards
Antoine

> 
>> +
>> +     V4L2Subdevice::Routing routing_ = {};
>>   };
>>
>>   /* -----------------------------------------------------------------------------
>> @@ -950,6 +959,37 @@ int PipelineHandlerISI::queueRequestDevice(Camera *camera, Request *request)
>>        return 0;
>>   }
>>
>> +bool PipelineHandlerISI::acquireDevice(Camera *camera)
>> +{
>> +     ISICameraData *data = cameraData(camera);
>> +
>> +     LOG(ISI, Debug) << "acquireDevice " << data->cameraName()
>> +                     << " count " << acquireCount_;
>> +
>> +     if (acquireCount_ > 0) {
>> +             acquireCount_++;
>> +             return true;
>> +     }
>> +
>> +     /* Enable routing for all available sensors once */
>> +     int ret = crossbar_->setRouting(&routing_, V4L2Subdevice::ActiveFormat);
>> +     if (ret)
>> +             return false;
>> +
>> +     acquireCount_++;
>> +     return true;
>> +}
>> +
>> +void PipelineHandlerISI::releaseDevice(Camera *camera)
>> +{
>> +     ISICameraData *data = cameraData(camera);
>> +
>> +     ASSERT(acquireCount_);
>> +     acquireCount_--;
>> +     LOG(ISI, Debug) << "releaseDevice " << data->cameraName()
>> +                     << " count " << acquireCount_;
>> +}
>> +
>>   bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)
>>   {
>>        DeviceMatch dm("mxc-isi");
>> @@ -1034,7 +1074,6 @@ bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)
>>        unsigned int numSinks = 0;
>>        const unsigned int xbarFirstSource = crossbar_->entity()->pads().size() - pipes_.size();
>>        const unsigned int maxStreams = pipes_.size() / cameraCount;
>> -     V4L2Subdevice::Routing routing = {};
>>
>>        for (MediaPad *pad : crossbar_->entity()->pads()) {
>>                unsigned int sink = numSinks;
>> @@ -1104,7 +1143,7 @@ bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)
>>                /*  Add routes to the crossbar switch routing table. */
>>                for (unsigned i = 0; i < data->streams_.size(); i++) {
>>                        unsigned int sourcePad = xbarFirstSource + data->xbarSourceOffset_ + i;
>> -                     routing.emplace_back(V4L2Subdevice::Stream{ data->xbarSink_, 0 },
>> +                     routing_.emplace_back(V4L2Subdevice::Stream{ data->xbarSink_, 0 },
>>                                             V4L2Subdevice::Stream{ sourcePad, 0 },
>>                                             V4L2_SUBDEV_ROUTE_FL_ACTIVE);
>>                }
>> @@ -1116,10 +1155,6 @@ bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)
>>                numCameras++;
>>        }
>>
>> -     ret = crossbar_->setRouting(&routing, V4L2Subdevice::ActiveFormat);
>> -     if (ret)
>> -             return false;
>> -
>>        return numCameras > 0;
>>   }
>>
> 
> --
> Regards,
> 
> Laurent Pinchart