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

24706 diff mbox series

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>
Tested-by: Antoine Bouyer <antoine.bouyer@nxp.com>
---
 src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 46 +++++++++++++++++---
 1 file changed, 40 insertions(+), 6 deletions(-)

Hi Antoine, Andrei,

Quoting Antoine Bouyer (2025-10-20 13:24:00)
> 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.

Overall I think this is a good thing to aim for... We should definitely
not interfere with a running camera just from trying to list them.

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

But I think this 'fixes' patch might be introducing bugs.

Please review below.

> 
> Signed-off-by: Andrei Gansari <andrei.gansari@nxp.com>
> Tested-by: Antoine Bouyer <antoine.bouyer@nxp.com>
> ---
>  src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 46 +++++++++++++++++---
>  1 file changed, 40 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..26f556bbb819 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,36 @@ int PipelineHandlerISI::queueRequestDevice(Camera *camera, Request *request)
>         return 0;
>  }
>  
> +bool PipelineHandlerISI::acquireDevice(Camera *camera)
> +{
> +       ISICameraData *data = cameraData(camera);
> +       int ret;
> +
> +       acquireCount_++;
> +       LOG(ISI, Debug) << "acquireDevice " << data->cameraName()
> +                       << " count " << acquireCount_;
> +
> +       if (acquireCount_ > 1)
> +               return true;
> +
> +       /* Enable routing for all available sensors once */
> +       ret = crossbar_->setRouting(&routing_, V4L2Subdevice::ActiveFormat);
> +       if (ret)
> +               return ret;

This function returns a bool, while setRouting returns an error number,
so I don't think this is doing quite what you intend.

Also - if we have an error on setting the routing, should we fail to
acquire the device, and if so - should we keep acquireCount at 0? Or do
we have to call releaseDevice() even if acquireDevice fails ?


> +
> +       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 +1073,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 +1142,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 +1154,6 @@ bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)
>                 numCameras++;
>         }
>  
> -       ret = crossbar_->setRouting(&routing, V4L2Subdevice::ActiveFormat);
> -       if (ret)
> -               return false;
> -
>         return numCameras > 0;
>  }
>  
> -- 
> 2.34.1
>

Hi Kieran

Thanks for feedback

On 21/10/2025 17:22, Kieran Bingham 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,
> 
> Quoting Antoine Bouyer (2025-10-20 13:24:00)
>> 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.
> 
> Overall I think this is a good thing to aim for... We should definitely
> not interfere with a running camera just from trying to list them.
> 
>> Fixes: 92df79112fb2 ("pipeline: imx8-isi: Add multicamera support")
> 
> But I think this 'fixes' patch might be introducing bugs.
> 
> Please review below.
> 
>>
>> Signed-off-by: Andrei Gansari <andrei.gansari@nxp.com>
>> Tested-by: Antoine Bouyer <antoine.bouyer@nxp.com>
>> ---
>>   src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 46 +++++++++++++++++---
>>   1 file changed, 40 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..26f556bbb819 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,36 @@ int PipelineHandlerISI::queueRequestDevice(Camera *camera, Request *request)
>>          return 0;
>>   }
>>
>> +bool PipelineHandlerISI::acquireDevice(Camera *camera)
>> +{
>> +       ISICameraData *data = cameraData(camera);
>> +       int ret;
>> +
>> +       acquireCount_++;
>> +       LOG(ISI, Debug) << "acquireDevice " << data->cameraName()
>> +                       << " count " << acquireCount_;
>> +
>> +       if (acquireCount_ > 1)
>> +               return true;
>> +
>> +       /* Enable routing for all available sensors once */
>> +       ret = crossbar_->setRouting(&routing_, V4L2Subdevice::ActiveFormat);
>> +       if (ret)
>> +               return ret;
> 
> This function returns a bool, while setRouting returns an error number,
> so I don't think this is doing quite what you intend.

ohh right! Most probably cast-ed to true, which is the opposite of what 
is intended !

> 
> Also - if we have an error on setting the routing, should we fail to
> acquire the device, and if so - should we keep acquireCount at 0? Or do
> we have to call releaseDevice() even if acquireDevice fails ?

Need to rework the logic:
  - test acquireCount_ is non null instead of > 1
  - increase counter before returning, only in case of success: i.e. 
after setRouting without error, or if already non null.

Thanks
Antoine

> 
> 
>> +
>> +       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 +1073,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 +1142,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 +1154,6 @@ bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)
>>                  numCameras++;
>>          }
>>
>> -       ret = crossbar_->setRouting(&routing, V4L2Subdevice::ActiveFormat);
>> -       if (ret)
>> -               return false;
>> -
>>          return numCameras > 0;
>>   }
>>
>> --
>> 2.34.1
>>