Message ID | 20251022093820.2757759-2-antoine.bouyer@nxp.com |
---|---|
State | New |
Headers | show |
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
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; }