[libcamera-devel] [PATCH v1 3/3] pipeline: raspberrypi: Add a Unicam dequeue timeout
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Mar 23 21:44:25 CET 2022
Hi Naush,
On Wed, Mar 23, 2022 at 03:10:00PM +0000, Naushir Patuck wrote:
> On Wed, 23 Mar 2022 at 10:51, Laurent Pinchart wrote:
> > On Wed, Mar 23, 2022 at 09:16:51AM +0000, Naushir Patuck wrote:
> > > On Tue, 22 Mar 2022 at 21:51, Laurent Pinchart wrote:
> > > > On Tue, Mar 22, 2022 at 01:16:35PM +0000, Naushir Patuck via
> > > > libcamera-devel wrote:
> > > > > Enable the V4L2VideoDevice dequeue timeout for the Unicam Image node, and
> > > > > connect the timeout signal to a slot in the pipeline handler. This slot will
> > > > > log a fatal error message informing the user of a possible hardware stall.
> > > > >
> > > > > The timeout is calculated as 2x the maximum frame length possible for a given
> > > > > mode, returned by the IPA.
> > > > >
> > > > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > > > ---
> > > > > include/libcamera/ipa/raspberrypi.mojom | 1 +
> > > > > src/ipa/raspberrypi/raspberrypi.cpp | 2 ++
> > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 15 +++++++++++++++
> > > > > 3 files changed, 18 insertions(+)
> > > > >
> > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> > > > > index acd3cafe6c91..5a228b75cd2f 100644
> > > > > --- a/include/libcamera/ipa/raspberrypi.mojom
> > > > > +++ b/include/libcamera/ipa/raspberrypi.mojom
> > > > > @@ -41,6 +41,7 @@ struct IPAConfig {
> > > > > struct StartConfig {
> > > > > libcamera.ControlList controls;
> > > > > int32 dropFrameCount;
> > > > > + uint32 maxSensorFrameLengthMs;
> > > >
> > > > We really need duration types in mojo.
> > >
> > > Yes please!! :-)
> > >
> > > > > };
> > > > >
> > > > > interface IPARPiInterface {
> > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > index fd8fecb07f81..675f9ba1b350 100644
> > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > @@ -280,6 +280,8 @@ void IPARPi::start(const ControlList &controls, ipa::RPi::StartConfig *startConf
> > > > > }
> > > > >
> > > > > startConfig->dropFrameCount = dropFrameCount_;
> > > > > + const Duration maxSensorFrameDuration = mode_.max_frame_length * mode_.line_length;
> > > > > + startConfig->maxSensorFrameLengthMs = maxSensorFrameDuration.get<std::milli>();
> > > > >
> > > > > firstStart_ = false;
> > > > > lastRunTimestamp_ = 0;
> > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > index c2230199fed7..50b39f1adf93 100644
> > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > @@ -45,6 +45,8 @@
> > > > > #include "dma_heaps.h"
> > > > > #include "rpi_stream.h"
> > > > >
> > > > > +using namespace std::literals::chrono_literals;
> > > > > +
> > > > > namespace libcamera {
> > > > >
> > > > > LOG_DEFINE_CATEGORY(RPI)
> > > > > @@ -202,6 +204,7 @@ public:
> > > > > void setIspControls(const ControlList &controls);
> > > > > void setDelayedControls(const ControlList &controls);
> > > > > void setSensorControls(ControlList &controls);
> > > > > + void unicamTimeout(V4L2VideoDevice *dev);
> > > > >
> > > > > /* bufferComplete signal handlers. */
> > > > > void unicamBufferDequeue(FrameBuffer *buffer);
> > > > > @@ -1032,6 +1035,12 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
> > > > > }
> > > > > }
> > > > >
> > > > > + /* Set the dequeue timeout to 2x the maximum possible frame duration. */
> > > > > + utils::Duration duration = startConfig.maxSensorFrameLengthMs * 2 * 1ms;
> > > > > + data->unicam_[Unicam::Image].dev()->setDequeueTimeout(duration);
> > > > > + data->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data, &RPiCameraData::unicamTimeout);
> > > > > + LOG(RPI, Debug) << "Setting sensor timeout to " << duration;
> > > > > +
> > > > > return 0;
> > > > > }
> > > > >
> > > > > @@ -1040,6 +1049,7 @@ void PipelineHandlerRPi::stopDevice(Camera *camera)
> > > > > RPiCameraData *data = cameraData(camera);
> > > > >
> > > > > data->state_ = RPiCameraData::State::Stopped;
> > > > > + data->unicam_[Unicam::Image].dev()->dequeueTimeout.disconnect();
> > > >
> > > > No need to connect and disconnect the signal when starting and stopping,
> > > > you can connect it at init time and keep it connected forever.
> > > >
> > > > > /* Disable SOF event generation. */
> > > > > data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(false);
> > > > > @@ -1757,6 +1767,11 @@ void RPiCameraData::setSensorControls(ControlList &controls)
> > > > > sensor_->setControls(&controls);
> > > > > }
> > > > >
> > > > > +void RPiCameraData::unicamTimeout([[maybe_unused]] V4L2VideoDevice *dev)
> > > > > +{
> > > > > + LOG(RPI, Fatal) << "Unicam has timed out!";
> > > >
> > > > That's harsh, and is likely to trigger if we ever miss a frame due to
> > > > high CPU load. If the goal is to detect a complete stall, I'd increase
> > > > the timeout to at least 10 frames.
> > >
> > > Yes, you are right.
> > >
> > > I actually meant to use Error here - Fatal was a bit leftover from my testing
> > > to see the timeout hit as expected.
> >
> > Maybe the error message in the V4L2VideoDevice class is enough then ?
> >
> > > > How will this work with sensors that use an external trigger ?
> > >
> > > Short answer is it won't and cannot right now. Given this is a purely debug feature,
> > > the only effect it will have is to dump a log message to the console which
> > > I think is fine.
> >
> > Won't the message be repeated at regular intervals though ? That may not
> > be very nice.
>
> Yes they will. However, for now, I feel the advantages of having the log message for
> users with malfunctioning sensors outweigh the (very rare) cases where users will be
> driving the sensor in bulb mode. Also given none of our sensors have this support
> (well, imx477 does, but the kernel driver does not have the swith to do it yet), we may
> not encounter it at all :-)
We can address it when/if needed, that's fine with me, I'll forward bug
reports your way ;-)
> > > In the longer term, perhaps we need to be informed (through the v4l2 driver?) that we
> > > are in "bulb" mode and we must disable timeouts. Or perhaps the application can pass
> > > a config flag?
> >
> > Or perhaps libcamera should handle the internal/external sync
> > configuration itself, I wouldn't be surprised if the IPA module could
> > benefit from more knowledge of how the sensor is triggered. We'll of
> > course need a trigger mode control in that case.
>
> I think this makes sense. This probably wants to have a dedicated v4l2 control behind it
> to control from userland.
>
> > > > > +}
> > > > > +
> > > > > void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
> > > > > {
> > > > > RPi::Stream *stream = nullptr;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list