[libcamera-devel] [PATCH 13/13] pipeline: vc4: Connect/disconnect IPA and dequeue signals on start/stop
Naushir Patuck
naush at raspberrypi.com
Thu Apr 27 16:03:44 CEST 2023
Hi Laurent,
On Thu, 27 Apr 2023 at 14:55, Laurent Pinchart
<laurent.pinchart at 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 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