[libcamera-devel] [PATCH 13/13] pipeline: vc4: Connect/disconnect IPA and dequeue signals on start/stop
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Apr 27 16:57:08 CEST 2023
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 at 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
More information about the libcamera-devel
mailing list