[libcamera-devel] [PATCH v1 3/3] pipeline: raspberrypi: Add a Unicam dequeue timeout
Naushir Patuck
naush at raspberrypi.com
Wed Mar 23 16:10:00 CET 2022
Hi Laurent,
On Wed, 23 Mar 2022 at 10:51, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> Hi Naush,
>
> 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 :-)
>
> > 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.
Naush
> > > > +}
> > > > +
> > > > void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
> > > > {
> > > > RPi::Stream *stream = nullptr;
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220323/85b5ea0a/attachment.htm>
More information about the libcamera-devel
mailing list