| Message ID | 20230426131057.21550-14-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush On Wed, Apr 26, 2023 at 02:10:57PM +0100, Naushir Patuck via libcamera-devel wrote: > We currently rely on a state check to see if any of the IPA and buffer > dequeue signal functions need to run. Replace this check by explicitly > disconnecting the appropriate signals on camera stop. Re-connect the > signals on camera start. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > --- > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 54 +++++++++++--------------- > 1 file changed, 23 insertions(+), 31 deletions(-) > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > index bd7bfb3a7663..4b3f5a7fc9fe 100644 > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > @@ -315,11 +315,8 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr<RPi::CameraData> &camer > > /* An embedded data node will not be present if the sensor does not support it. */ > MediaEntity *unicamEmbedded = unicam->getEntityByName("unicam-embedded"); > - if (unicamEmbedded) { > + if (unicamEmbedded) > data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", unicamEmbedded); > - data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data, > - &Vc4CameraData::unicamBufferDequeue); > - } > > /* Tag the ISP input stream as an import stream. */ > data->isp_[Isp::Input] = RPi::Stream("ISP Input", ispOutput0, Flag::ImportOnly); > @@ -327,18 +324,9 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr<RPi::CameraData> &camer > data->isp_[Isp::Output1] = RPi::Stream("ISP Output1", ispCapture2); > data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", ispCapture3); > > - /* Wire up all the buffer connections. */ > - data->unicam_[Unicam::Image].dev()->bufferReady.connect(data, &Vc4CameraData::unicamBufferDequeue); > - data->isp_[Isp::Input].dev()->bufferReady.connect(data, &Vc4CameraData::ispInputDequeue); > - data->isp_[Isp::Output0].dev()->bufferReady.connect(data, &Vc4CameraData::ispOutputDequeue); > - data->isp_[Isp::Output1].dev()->bufferReady.connect(data, &Vc4CameraData::ispOutputDequeue); > - data->isp_[Isp::Stats].dev()->bufferReady.connect(data, &Vc4CameraData::ispOutputDequeue); > - > if (data->sensorMetadata_ ^ !!data->unicam_[Unicam::Embedded].dev()) { > LOG(RPI, Warning) << "Mismatch between Unicam and CamHelper for embedded data usage!"; > data->sensorMetadata_ = false; > - if (data->unicam_[Unicam::Embedded].dev()) > - data->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect(); > } > > /* > @@ -367,9 +355,7 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr<RPi::CameraData> &camer > return -EINVAL; > } > > - /* Write up all the IPA connections. */ > - data->ipa_->processStatsComplete.connect(data, &Vc4CameraData::processStatsComplete); > - data->ipa_->prepareIspComplete.connect(data, &Vc4CameraData::prepareIspComplete); > + /* Wire up the default IPA connections. The others get connected on start() */ > data->ipa_->setIspControls.connect(data, &Vc4CameraData::setIspControls); > data->ipa_->setCameraTimeout.connect(data, &Vc4CameraData::setCameraTimeout); > > @@ -691,10 +677,31 @@ int Vc4CameraData::platformConfigureIpa(ipa::RPi::ConfigParams ¶ms) > > void Vc4CameraData::platformStart() > { > + unicam_[Unicam::Image].dev()->bufferReady.connect(this, &Vc4CameraData::unicamBufferDequeue); > + isp_[Isp::Input].dev()->bufferReady.connect(this, &Vc4CameraData::ispInputDequeue); > + isp_[Isp::Output0].dev()->bufferReady.connect(this, &Vc4CameraData::ispOutputDequeue); > + isp_[Isp::Output1].dev()->bufferReady.connect(this, &Vc4CameraData::ispOutputDequeue); > + isp_[Isp::Stats].dev()->bufferReady.connect(this, &Vc4CameraData::ispOutputDequeue); > + ipa_->processStatsComplete.connect(this, &Vc4CameraData::processStatsComplete); > + ipa_->prepareIspComplete.connect(this, &Vc4CameraData::prepareIspComplete); > + > + if (sensorMetadata_) > + unicam_[Unicam::Embedded].dev()->bufferReady.connect(this, &Vc4CameraData::unicamBufferDequeue); > } > > void Vc4CameraData::platformStop() > { > + unicam_[Unicam::Image].dev()->bufferReady.disconnect(); > + isp_[Isp::Input].dev()->bufferReady.disconnect(); > + isp_[Isp::Output0].dev()->bufferReady.disconnect(); > + isp_[Isp::Output1].dev()->bufferReady.disconnect(); > + isp_[Isp::Stats].dev()->bufferReady.disconnect(); > + ipa_->processStatsComplete.disconnect(); > + ipa_->prepareIspComplete.disconnect(); > + > + if (sensorMetadata_) > + unicam_[Unicam::Embedded].dev()->bufferReady.disconnect(); > + > bayerQueue_ = {}; > embeddedQueue_ = {}; > } > @@ -704,9 +711,6 @@ void Vc4CameraData::unicamBufferDequeue(FrameBuffer *buffer) > RPi::Stream *stream = nullptr; > unsigned int index; > > - if (!isRunning()) > - return; > - > for (RPi::Stream &s : unicam_) { > index = s.getBufferId(buffer); > if (index) { > @@ -743,9 +747,6 @@ void Vc4CameraData::unicamBufferDequeue(FrameBuffer *buffer) > > void Vc4CameraData::ispInputDequeue(FrameBuffer *buffer) > { > - if (!isRunning()) > - return; > - > LOG(RPI, Debug) << "Stream ISP Input buffer complete" > << ", buffer id " << unicam_[Unicam::Image].getBufferId(buffer) > << ", timestamp: " << buffer->metadata().timestamp; > @@ -760,9 +761,6 @@ void Vc4CameraData::ispOutputDequeue(FrameBuffer *buffer) > RPi::Stream *stream = nullptr; > unsigned int index; > > - if (!isRunning()) > - return; > - > for (RPi::Stream &s : isp_) { > index = s.getBufferId(buffer); > if (index) { > @@ -803,9 +801,6 @@ void Vc4CameraData::ispOutputDequeue(FrameBuffer *buffer) > > void Vc4CameraData::processStatsComplete(const ipa::RPi::BufferIds &buffers) > { > - if (!isRunning()) > - return; > - > FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(buffers.stats & RPi::MaskID); > > handleStreamBuffer(buffer, &isp_[Isp::Stats]); > @@ -846,9 +841,6 @@ void Vc4CameraData::prepareIspComplete(const ipa::RPi::BufferIds &buffers) > unsigned int bayer = buffers.bayer & RPi::MaskID; > FrameBuffer *buffer; > > - if (!isRunning()) > - return; > - > buffer = unicam_[Unicam::Image].getBuffers().at(bayer & RPi::MaskID); > LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << (bayer & RPi::MaskID) > << ", timestamp: " << buffer->metadata().timestamp; > -- > 2.34.1 >
Hi Naush, On Wed, Apr 26, 2023 at 02:10:57PM +0100, Naushir Patuck via libcamera-devel wrote: > We currently rely on a state check to see if any of the IPA and buffer > dequeue signal functions need to run. Replace this check by explicitly > disconnecting the appropriate signals on camera stop. Re-connect the > signals on camera start. I'm a bit concerned about this. I've debugged an issue no later than today where a race condition caused events to be processed after a stop call. I briefly considered disabling the event notifier at stop time (that's equivalent to disconnecting the signal here), but I then realized that a quick stop/start cycle would reenable the notifier before the event loop would get a chance to process and drop the event. Disconnecting signals to ignore events makes state handling implicit, and that in turn makes it more difficult to prove correctness of the code. The resulting race conditions are also likely harder to debug as they will occur less often. > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 54 +++++++++++--------------- > 1 file changed, 23 insertions(+), 31 deletions(-) > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > index bd7bfb3a7663..4b3f5a7fc9fe 100644 > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > @@ -315,11 +315,8 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr<RPi::CameraData> &camer > > /* An embedded data node will not be present if the sensor does not support it. */ > MediaEntity *unicamEmbedded = unicam->getEntityByName("unicam-embedded"); > - if (unicamEmbedded) { > + if (unicamEmbedded) > data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", unicamEmbedded); > - data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data, > - &Vc4CameraData::unicamBufferDequeue); > - } > > /* Tag the ISP input stream as an import stream. */ > data->isp_[Isp::Input] = RPi::Stream("ISP Input", ispOutput0, Flag::ImportOnly); > @@ -327,18 +324,9 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr<RPi::CameraData> &camer > data->isp_[Isp::Output1] = RPi::Stream("ISP Output1", ispCapture2); > data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", ispCapture3); > > - /* Wire up all the buffer connections. */ > - data->unicam_[Unicam::Image].dev()->bufferReady.connect(data, &Vc4CameraData::unicamBufferDequeue); > - data->isp_[Isp::Input].dev()->bufferReady.connect(data, &Vc4CameraData::ispInputDequeue); > - data->isp_[Isp::Output0].dev()->bufferReady.connect(data, &Vc4CameraData::ispOutputDequeue); > - data->isp_[Isp::Output1].dev()->bufferReady.connect(data, &Vc4CameraData::ispOutputDequeue); > - data->isp_[Isp::Stats].dev()->bufferReady.connect(data, &Vc4CameraData::ispOutputDequeue); > - > if (data->sensorMetadata_ ^ !!data->unicam_[Unicam::Embedded].dev()) { > LOG(RPI, Warning) << "Mismatch between Unicam and CamHelper for embedded data usage!"; > data->sensorMetadata_ = false; > - if (data->unicam_[Unicam::Embedded].dev()) > - data->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect(); > } > > /* > @@ -367,9 +355,7 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr<RPi::CameraData> &camer > return -EINVAL; > } > > - /* Write up all the IPA connections. */ > - data->ipa_->processStatsComplete.connect(data, &Vc4CameraData::processStatsComplete); > - data->ipa_->prepareIspComplete.connect(data, &Vc4CameraData::prepareIspComplete); > + /* Wire up the default IPA connections. The others get connected on start() */ > data->ipa_->setIspControls.connect(data, &Vc4CameraData::setIspControls); > data->ipa_->setCameraTimeout.connect(data, &Vc4CameraData::setCameraTimeout); > > @@ -691,10 +677,31 @@ int Vc4CameraData::platformConfigureIpa(ipa::RPi::ConfigParams ¶ms) > > void Vc4CameraData::platformStart() > { > + unicam_[Unicam::Image].dev()->bufferReady.connect(this, &Vc4CameraData::unicamBufferDequeue); > + isp_[Isp::Input].dev()->bufferReady.connect(this, &Vc4CameraData::ispInputDequeue); > + isp_[Isp::Output0].dev()->bufferReady.connect(this, &Vc4CameraData::ispOutputDequeue); > + isp_[Isp::Output1].dev()->bufferReady.connect(this, &Vc4CameraData::ispOutputDequeue); > + isp_[Isp::Stats].dev()->bufferReady.connect(this, &Vc4CameraData::ispOutputDequeue); > + ipa_->processStatsComplete.connect(this, &Vc4CameraData::processStatsComplete); > + ipa_->prepareIspComplete.connect(this, &Vc4CameraData::prepareIspComplete); > + > + if (sensorMetadata_) > + unicam_[Unicam::Embedded].dev()->bufferReady.connect(this, &Vc4CameraData::unicamBufferDequeue); > } > > void Vc4CameraData::platformStop() > { > + unicam_[Unicam::Image].dev()->bufferReady.disconnect(); > + isp_[Isp::Input].dev()->bufferReady.disconnect(); > + isp_[Isp::Output0].dev()->bufferReady.disconnect(); > + isp_[Isp::Output1].dev()->bufferReady.disconnect(); > + isp_[Isp::Stats].dev()->bufferReady.disconnect(); > + ipa_->processStatsComplete.disconnect(); > + ipa_->prepareIspComplete.disconnect(); > + > + if (sensorMetadata_) > + unicam_[Unicam::Embedded].dev()->bufferReady.disconnect(); > + > bayerQueue_ = {}; > embeddedQueue_ = {}; > } > @@ -704,9 +711,6 @@ void Vc4CameraData::unicamBufferDequeue(FrameBuffer *buffer) > RPi::Stream *stream = nullptr; > unsigned int index; > > - if (!isRunning()) > - return; > - > for (RPi::Stream &s : unicam_) { > index = s.getBufferId(buffer); > if (index) { > @@ -743,9 +747,6 @@ void Vc4CameraData::unicamBufferDequeue(FrameBuffer *buffer) > > void Vc4CameraData::ispInputDequeue(FrameBuffer *buffer) > { > - if (!isRunning()) > - return; > - > LOG(RPI, Debug) << "Stream ISP Input buffer complete" > << ", buffer id " << unicam_[Unicam::Image].getBufferId(buffer) > << ", timestamp: " << buffer->metadata().timestamp; > @@ -760,9 +761,6 @@ void Vc4CameraData::ispOutputDequeue(FrameBuffer *buffer) > RPi::Stream *stream = nullptr; > unsigned int index; > > - if (!isRunning()) > - return; > - > for (RPi::Stream &s : isp_) { > index = s.getBufferId(buffer); > if (index) { > @@ -803,9 +801,6 @@ void Vc4CameraData::ispOutputDequeue(FrameBuffer *buffer) > > void Vc4CameraData::processStatsComplete(const ipa::RPi::BufferIds &buffers) > { > - if (!isRunning()) > - return; > - > FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(buffers.stats & RPi::MaskID); > > handleStreamBuffer(buffer, &isp_[Isp::Stats]); > @@ -846,9 +841,6 @@ void Vc4CameraData::prepareIspComplete(const ipa::RPi::BufferIds &buffers) > unsigned int bayer = buffers.bayer & RPi::MaskID; > FrameBuffer *buffer; > > - if (!isRunning()) > - return; > - > buffer = unicam_[Unicam::Image].getBuffers().at(bayer & RPi::MaskID); > LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << (bayer & RPi::MaskID) > << ", timestamp: " << buffer->metadata().timestamp;
Hi Laurent, On Thu, 27 Apr 2023 at 14:55, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Naush, > > On Wed, Apr 26, 2023 at 02:10:57PM +0100, Naushir Patuck via libcamera-devel wrote: > > We currently rely on a state check to see if any of the IPA and buffer > > dequeue signal functions need to run. Replace this check by explicitly > > disconnecting the appropriate signals on camera stop. Re-connect the > > signals on camera start. > > I'm a bit concerned about this. I've debugged an issue no later than > today where a race condition caused events to be processed after a stop > call. I briefly considered disabling the event notifier at stop time > (that's equivalent to disconnecting the signal here), but I then > realized that a quick stop/start cycle would reenable the notifier > before the event loop would get a chance to process and drop the event. > Disconnecting signals to ignore events makes state handling implicit, > and that in turn makes it more difficult to prove correctness of the > code. The resulting race conditions are also likely harder to debug as > they will occur less often. I can remove this patch and revert to using our original state test mechanism to handle this. Naush > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 54 +++++++++++--------------- > > 1 file changed, 23 insertions(+), 31 deletions(-) > > > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > index bd7bfb3a7663..4b3f5a7fc9fe 100644 > > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > @@ -315,11 +315,8 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr<RPi::CameraData> &camer > > > > /* An embedded data node will not be present if the sensor does not support it. */ > > MediaEntity *unicamEmbedded = unicam->getEntityByName("unicam-embedded"); > > - if (unicamEmbedded) { > > + if (unicamEmbedded) > > data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", unicamEmbedded); > > - data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data, > > - &Vc4CameraData::unicamBufferDequeue); > > - } > > > > /* Tag the ISP input stream as an import stream. */ > > data->isp_[Isp::Input] = RPi::Stream("ISP Input", ispOutput0, Flag::ImportOnly); > > @@ -327,18 +324,9 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr<RPi::CameraData> &camer > > data->isp_[Isp::Output1] = RPi::Stream("ISP Output1", ispCapture2); > > data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", ispCapture3); > > > > - /* Wire up all the buffer connections. */ > > - data->unicam_[Unicam::Image].dev()->bufferReady.connect(data, &Vc4CameraData::unicamBufferDequeue); > > - data->isp_[Isp::Input].dev()->bufferReady.connect(data, &Vc4CameraData::ispInputDequeue); > > - data->isp_[Isp::Output0].dev()->bufferReady.connect(data, &Vc4CameraData::ispOutputDequeue); > > - data->isp_[Isp::Output1].dev()->bufferReady.connect(data, &Vc4CameraData::ispOutputDequeue); > > - data->isp_[Isp::Stats].dev()->bufferReady.connect(data, &Vc4CameraData::ispOutputDequeue); > > - > > if (data->sensorMetadata_ ^ !!data->unicam_[Unicam::Embedded].dev()) { > > LOG(RPI, Warning) << "Mismatch between Unicam and CamHelper for embedded data usage!"; > > data->sensorMetadata_ = false; > > - if (data->unicam_[Unicam::Embedded].dev()) > > - data->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect(); > > } > > > > /* > > @@ -367,9 +355,7 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr<RPi::CameraData> &camer > > return -EINVAL; > > } > > > > - /* Write up all the IPA connections. */ > > - data->ipa_->processStatsComplete.connect(data, &Vc4CameraData::processStatsComplete); > > - data->ipa_->prepareIspComplete.connect(data, &Vc4CameraData::prepareIspComplete); > > + /* Wire up the default IPA connections. The others get connected on start() */ > > data->ipa_->setIspControls.connect(data, &Vc4CameraData::setIspControls); > > data->ipa_->setCameraTimeout.connect(data, &Vc4CameraData::setCameraTimeout); > > > > @@ -691,10 +677,31 @@ int Vc4CameraData::platformConfigureIpa(ipa::RPi::ConfigParams ¶ms) > > > > void Vc4CameraData::platformStart() > > { > > + unicam_[Unicam::Image].dev()->bufferReady.connect(this, &Vc4CameraData::unicamBufferDequeue); > > + isp_[Isp::Input].dev()->bufferReady.connect(this, &Vc4CameraData::ispInputDequeue); > > + isp_[Isp::Output0].dev()->bufferReady.connect(this, &Vc4CameraData::ispOutputDequeue); > > + isp_[Isp::Output1].dev()->bufferReady.connect(this, &Vc4CameraData::ispOutputDequeue); > > + isp_[Isp::Stats].dev()->bufferReady.connect(this, &Vc4CameraData::ispOutputDequeue); > > + ipa_->processStatsComplete.connect(this, &Vc4CameraData::processStatsComplete); > > + ipa_->prepareIspComplete.connect(this, &Vc4CameraData::prepareIspComplete); > > + > > + if (sensorMetadata_) > > + unicam_[Unicam::Embedded].dev()->bufferReady.connect(this, &Vc4CameraData::unicamBufferDequeue); > > } > > > > void Vc4CameraData::platformStop() > > { > > + unicam_[Unicam::Image].dev()->bufferReady.disconnect(); > > + isp_[Isp::Input].dev()->bufferReady.disconnect(); > > + isp_[Isp::Output0].dev()->bufferReady.disconnect(); > > + isp_[Isp::Output1].dev()->bufferReady.disconnect(); > > + isp_[Isp::Stats].dev()->bufferReady.disconnect(); > > + ipa_->processStatsComplete.disconnect(); > > + ipa_->prepareIspComplete.disconnect(); > > + > > + if (sensorMetadata_) > > + unicam_[Unicam::Embedded].dev()->bufferReady.disconnect(); > > + > > bayerQueue_ = {}; > > embeddedQueue_ = {}; > > } > > @@ -704,9 +711,6 @@ void Vc4CameraData::unicamBufferDequeue(FrameBuffer *buffer) > > RPi::Stream *stream = nullptr; > > unsigned int index; > > > > - if (!isRunning()) > > - return; > > - > > for (RPi::Stream &s : unicam_) { > > index = s.getBufferId(buffer); > > if (index) { > > @@ -743,9 +747,6 @@ void Vc4CameraData::unicamBufferDequeue(FrameBuffer *buffer) > > > > void Vc4CameraData::ispInputDequeue(FrameBuffer *buffer) > > { > > - if (!isRunning()) > > - return; > > - > > LOG(RPI, Debug) << "Stream ISP Input buffer complete" > > << ", buffer id " << unicam_[Unicam::Image].getBufferId(buffer) > > << ", timestamp: " << buffer->metadata().timestamp; > > @@ -760,9 +761,6 @@ void Vc4CameraData::ispOutputDequeue(FrameBuffer *buffer) > > RPi::Stream *stream = nullptr; > > unsigned int index; > > > > - if (!isRunning()) > > - return; > > - > > for (RPi::Stream &s : isp_) { > > index = s.getBufferId(buffer); > > if (index) { > > @@ -803,9 +801,6 @@ void Vc4CameraData::ispOutputDequeue(FrameBuffer *buffer) > > > > void Vc4CameraData::processStatsComplete(const ipa::RPi::BufferIds &buffers) > > { > > - if (!isRunning()) > > - return; > > - > > FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(buffers.stats & RPi::MaskID); > > > > handleStreamBuffer(buffer, &isp_[Isp::Stats]); > > @@ -846,9 +841,6 @@ void Vc4CameraData::prepareIspComplete(const ipa::RPi::BufferIds &buffers) > > unsigned int bayer = buffers.bayer & RPi::MaskID; > > FrameBuffer *buffer; > > > > - if (!isRunning()) > > - return; > > - > > buffer = unicam_[Unicam::Image].getBuffers().at(bayer & RPi::MaskID); > > LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << (bayer & RPi::MaskID) > > << ", timestamp: " << buffer->metadata().timestamp; > > -- > Regards, > > Laurent Pinchart
Hi Naush, On Thu, Apr 27, 2023 at 03:03:44PM +0100, Naushir Patuck wrote: > On Thu, 27 Apr 2023 at 14:55, Laurent Pinchart wrote: > > On Wed, Apr 26, 2023 at 02:10:57PM +0100, Naushir Patuck via libcamera-devel wrote: > > > We currently rely on a state check to see if any of the IPA and buffer > > > dequeue signal functions need to run. Replace this check by explicitly > > > disconnecting the appropriate signals on camera stop. Re-connect the > > > signals on camera start. > > > > I'm a bit concerned about this. I've debugged an issue no later than > > today where a race condition caused events to be processed after a stop > > call. I briefly considered disabling the event notifier at stop time > > (that's equivalent to disconnecting the signal here), but I then > > realized that a quick stop/start cycle would reenable the notifier > > before the event loop would get a chance to process and drop the event. > > Disconnecting signals to ignore events makes state handling implicit, > > and that in turn makes it more difficult to prove correctness of the > > code. The resulting race conditions are also likely harder to debug as > > they will occur less often. > > I can remove this patch and revert to using our original state test > mechanism to handle this. I think that would be best, but if you're confident that this patch can't cause any race condition or other problem, neither now nor when combined with future changes, I'm not opposed to merging it. It's up to you, whether you consider the issue valid or not. > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 54 +++++++++++--------------- > > > 1 file changed, 23 insertions(+), 31 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > > index bd7bfb3a7663..4b3f5a7fc9fe 100644 > > > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > > @@ -315,11 +315,8 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr<RPi::CameraData> &camer > > > > > > /* An embedded data node will not be present if the sensor does not support it. */ > > > MediaEntity *unicamEmbedded = unicam->getEntityByName("unicam-embedded"); > > > - if (unicamEmbedded) { > > > + if (unicamEmbedded) > > > data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", unicamEmbedded); > > > - data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data, > > > - &Vc4CameraData::unicamBufferDequeue); > > > - } > > > > > > /* Tag the ISP input stream as an import stream. */ > > > data->isp_[Isp::Input] = RPi::Stream("ISP Input", ispOutput0, Flag::ImportOnly); > > > @@ -327,18 +324,9 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr<RPi::CameraData> &camer > > > data->isp_[Isp::Output1] = RPi::Stream("ISP Output1", ispCapture2); > > > data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", ispCapture3); > > > > > > - /* Wire up all the buffer connections. */ > > > - data->unicam_[Unicam::Image].dev()->bufferReady.connect(data, &Vc4CameraData::unicamBufferDequeue); > > > - data->isp_[Isp::Input].dev()->bufferReady.connect(data, &Vc4CameraData::ispInputDequeue); > > > - data->isp_[Isp::Output0].dev()->bufferReady.connect(data, &Vc4CameraData::ispOutputDequeue); > > > - data->isp_[Isp::Output1].dev()->bufferReady.connect(data, &Vc4CameraData::ispOutputDequeue); > > > - data->isp_[Isp::Stats].dev()->bufferReady.connect(data, &Vc4CameraData::ispOutputDequeue); > > > - > > > if (data->sensorMetadata_ ^ !!data->unicam_[Unicam::Embedded].dev()) { > > > LOG(RPI, Warning) << "Mismatch between Unicam and CamHelper for embedded data usage!"; > > > data->sensorMetadata_ = false; > > > - if (data->unicam_[Unicam::Embedded].dev()) > > > - data->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect(); > > > } > > > > > > /* > > > @@ -367,9 +355,7 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr<RPi::CameraData> &camer > > > return -EINVAL; > > > } > > > > > > - /* Write up all the IPA connections. */ > > > - data->ipa_->processStatsComplete.connect(data, &Vc4CameraData::processStatsComplete); > > > - data->ipa_->prepareIspComplete.connect(data, &Vc4CameraData::prepareIspComplete); > > > + /* Wire up the default IPA connections. The others get connected on start() */ > > > data->ipa_->setIspControls.connect(data, &Vc4CameraData::setIspControls); > > > data->ipa_->setCameraTimeout.connect(data, &Vc4CameraData::setCameraTimeout); > > > > > > @@ -691,10 +677,31 @@ int Vc4CameraData::platformConfigureIpa(ipa::RPi::ConfigParams ¶ms) > > > > > > void Vc4CameraData::platformStart() > > > { > > > + unicam_[Unicam::Image].dev()->bufferReady.connect(this, &Vc4CameraData::unicamBufferDequeue); > > > + isp_[Isp::Input].dev()->bufferReady.connect(this, &Vc4CameraData::ispInputDequeue); > > > + isp_[Isp::Output0].dev()->bufferReady.connect(this, &Vc4CameraData::ispOutputDequeue); > > > + isp_[Isp::Output1].dev()->bufferReady.connect(this, &Vc4CameraData::ispOutputDequeue); > > > + isp_[Isp::Stats].dev()->bufferReady.connect(this, &Vc4CameraData::ispOutputDequeue); > > > + ipa_->processStatsComplete.connect(this, &Vc4CameraData::processStatsComplete); > > > + ipa_->prepareIspComplete.connect(this, &Vc4CameraData::prepareIspComplete); > > > + > > > + if (sensorMetadata_) > > > + unicam_[Unicam::Embedded].dev()->bufferReady.connect(this, &Vc4CameraData::unicamBufferDequeue); > > > } > > > > > > void Vc4CameraData::platformStop() > > > { > > > + unicam_[Unicam::Image].dev()->bufferReady.disconnect(); > > > + isp_[Isp::Input].dev()->bufferReady.disconnect(); > > > + isp_[Isp::Output0].dev()->bufferReady.disconnect(); > > > + isp_[Isp::Output1].dev()->bufferReady.disconnect(); > > > + isp_[Isp::Stats].dev()->bufferReady.disconnect(); > > > + ipa_->processStatsComplete.disconnect(); > > > + ipa_->prepareIspComplete.disconnect(); > > > + > > > + if (sensorMetadata_) > > > + unicam_[Unicam::Embedded].dev()->bufferReady.disconnect(); > > > + > > > bayerQueue_ = {}; > > > embeddedQueue_ = {}; > > > } > > > @@ -704,9 +711,6 @@ void Vc4CameraData::unicamBufferDequeue(FrameBuffer *buffer) > > > RPi::Stream *stream = nullptr; > > > unsigned int index; > > > > > > - if (!isRunning()) > > > - return; > > > - > > > for (RPi::Stream &s : unicam_) { > > > index = s.getBufferId(buffer); > > > if (index) { > > > @@ -743,9 +747,6 @@ void Vc4CameraData::unicamBufferDequeue(FrameBuffer *buffer) > > > > > > void Vc4CameraData::ispInputDequeue(FrameBuffer *buffer) > > > { > > > - if (!isRunning()) > > > - return; > > > - > > > LOG(RPI, Debug) << "Stream ISP Input buffer complete" > > > << ", buffer id " << unicam_[Unicam::Image].getBufferId(buffer) > > > << ", timestamp: " << buffer->metadata().timestamp; > > > @@ -760,9 +761,6 @@ void Vc4CameraData::ispOutputDequeue(FrameBuffer *buffer) > > > RPi::Stream *stream = nullptr; > > > unsigned int index; > > > > > > - if (!isRunning()) > > > - return; > > > - > > > for (RPi::Stream &s : isp_) { > > > index = s.getBufferId(buffer); > > > if (index) { > > > @@ -803,9 +801,6 @@ void Vc4CameraData::ispOutputDequeue(FrameBuffer *buffer) > > > > > > void Vc4CameraData::processStatsComplete(const ipa::RPi::BufferIds &buffers) > > > { > > > - if (!isRunning()) > > > - return; > > > - > > > FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(buffers.stats & RPi::MaskID); > > > > > > handleStreamBuffer(buffer, &isp_[Isp::Stats]); > > > @@ -846,9 +841,6 @@ void Vc4CameraData::prepareIspComplete(const ipa::RPi::BufferIds &buffers) > > > unsigned int bayer = buffers.bayer & RPi::MaskID; > > > FrameBuffer *buffer; > > > > > > - if (!isRunning()) > > > - return; > > > - > > > buffer = unicam_[Unicam::Image].getBuffers().at(bayer & RPi::MaskID); > > > LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << (bayer & RPi::MaskID) > > > << ", timestamp: " << buffer->metadata().timestamp;
diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp index bd7bfb3a7663..4b3f5a7fc9fe 100644 --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp @@ -315,11 +315,8 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr<RPi::CameraData> &camer /* An embedded data node will not be present if the sensor does not support it. */ MediaEntity *unicamEmbedded = unicam->getEntityByName("unicam-embedded"); - if (unicamEmbedded) { + if (unicamEmbedded) data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", unicamEmbedded); - data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data, - &Vc4CameraData::unicamBufferDequeue); - } /* Tag the ISP input stream as an import stream. */ data->isp_[Isp::Input] = RPi::Stream("ISP Input", ispOutput0, Flag::ImportOnly); @@ -327,18 +324,9 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr<RPi::CameraData> &camer data->isp_[Isp::Output1] = RPi::Stream("ISP Output1", ispCapture2); data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", ispCapture3); - /* Wire up all the buffer connections. */ - data->unicam_[Unicam::Image].dev()->bufferReady.connect(data, &Vc4CameraData::unicamBufferDequeue); - data->isp_[Isp::Input].dev()->bufferReady.connect(data, &Vc4CameraData::ispInputDequeue); - data->isp_[Isp::Output0].dev()->bufferReady.connect(data, &Vc4CameraData::ispOutputDequeue); - data->isp_[Isp::Output1].dev()->bufferReady.connect(data, &Vc4CameraData::ispOutputDequeue); - data->isp_[Isp::Stats].dev()->bufferReady.connect(data, &Vc4CameraData::ispOutputDequeue); - if (data->sensorMetadata_ ^ !!data->unicam_[Unicam::Embedded].dev()) { LOG(RPI, Warning) << "Mismatch between Unicam and CamHelper for embedded data usage!"; data->sensorMetadata_ = false; - if (data->unicam_[Unicam::Embedded].dev()) - data->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect(); } /* @@ -367,9 +355,7 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr<RPi::CameraData> &camer return -EINVAL; } - /* Write up all the IPA connections. */ - data->ipa_->processStatsComplete.connect(data, &Vc4CameraData::processStatsComplete); - data->ipa_->prepareIspComplete.connect(data, &Vc4CameraData::prepareIspComplete); + /* Wire up the default IPA connections. The others get connected on start() */ data->ipa_->setIspControls.connect(data, &Vc4CameraData::setIspControls); data->ipa_->setCameraTimeout.connect(data, &Vc4CameraData::setCameraTimeout); @@ -691,10 +677,31 @@ int Vc4CameraData::platformConfigureIpa(ipa::RPi::ConfigParams ¶ms) void Vc4CameraData::platformStart() { + unicam_[Unicam::Image].dev()->bufferReady.connect(this, &Vc4CameraData::unicamBufferDequeue); + isp_[Isp::Input].dev()->bufferReady.connect(this, &Vc4CameraData::ispInputDequeue); + isp_[Isp::Output0].dev()->bufferReady.connect(this, &Vc4CameraData::ispOutputDequeue); + isp_[Isp::Output1].dev()->bufferReady.connect(this, &Vc4CameraData::ispOutputDequeue); + isp_[Isp::Stats].dev()->bufferReady.connect(this, &Vc4CameraData::ispOutputDequeue); + ipa_->processStatsComplete.connect(this, &Vc4CameraData::processStatsComplete); + ipa_->prepareIspComplete.connect(this, &Vc4CameraData::prepareIspComplete); + + if (sensorMetadata_) + unicam_[Unicam::Embedded].dev()->bufferReady.connect(this, &Vc4CameraData::unicamBufferDequeue); } void Vc4CameraData::platformStop() { + unicam_[Unicam::Image].dev()->bufferReady.disconnect(); + isp_[Isp::Input].dev()->bufferReady.disconnect(); + isp_[Isp::Output0].dev()->bufferReady.disconnect(); + isp_[Isp::Output1].dev()->bufferReady.disconnect(); + isp_[Isp::Stats].dev()->bufferReady.disconnect(); + ipa_->processStatsComplete.disconnect(); + ipa_->prepareIspComplete.disconnect(); + + if (sensorMetadata_) + unicam_[Unicam::Embedded].dev()->bufferReady.disconnect(); + bayerQueue_ = {}; embeddedQueue_ = {}; } @@ -704,9 +711,6 @@ void Vc4CameraData::unicamBufferDequeue(FrameBuffer *buffer) RPi::Stream *stream = nullptr; unsigned int index; - if (!isRunning()) - return; - for (RPi::Stream &s : unicam_) { index = s.getBufferId(buffer); if (index) { @@ -743,9 +747,6 @@ void Vc4CameraData::unicamBufferDequeue(FrameBuffer *buffer) void Vc4CameraData::ispInputDequeue(FrameBuffer *buffer) { - if (!isRunning()) - return; - LOG(RPI, Debug) << "Stream ISP Input buffer complete" << ", buffer id " << unicam_[Unicam::Image].getBufferId(buffer) << ", timestamp: " << buffer->metadata().timestamp; @@ -760,9 +761,6 @@ void Vc4CameraData::ispOutputDequeue(FrameBuffer *buffer) RPi::Stream *stream = nullptr; unsigned int index; - if (!isRunning()) - return; - for (RPi::Stream &s : isp_) { index = s.getBufferId(buffer); if (index) { @@ -803,9 +801,6 @@ void Vc4CameraData::ispOutputDequeue(FrameBuffer *buffer) void Vc4CameraData::processStatsComplete(const ipa::RPi::BufferIds &buffers) { - if (!isRunning()) - return; - FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(buffers.stats & RPi::MaskID); handleStreamBuffer(buffer, &isp_[Isp::Stats]); @@ -846,9 +841,6 @@ void Vc4CameraData::prepareIspComplete(const ipa::RPi::BufferIds &buffers) unsigned int bayer = buffers.bayer & RPi::MaskID; FrameBuffer *buffer; - if (!isRunning()) - return; - buffer = unicam_[Unicam::Image].getBuffers().at(bayer & RPi::MaskID); LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << (bayer & RPi::MaskID) << ", timestamp: " << buffer->metadata().timestamp;
We currently rely on a state check to see if any of the IPA and buffer dequeue signal functions need to run. Replace this check by explicitly disconnecting the appropriate signals on camera stop. Re-connect the signals on camera start. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/libcamera/pipeline/rpi/vc4/vc4.cpp | 54 +++++++++++--------------- 1 file changed, 23 insertions(+), 31 deletions(-)