[PATCH v3 2/2] pipeline: simple: Use proper device for frame start events
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Feb 24 02:39:00 CET 2025
Hi Stanislaw,
On Thu, Feb 20, 2025 at 11:10:45AM +0100, Stanislaw Gruszka wrote:
> On Wed, Feb 19, 2025 at 02:42:31PM +0200, Laurent Pinchart wrote:
> > On Tue, Feb 18, 2025 at 10:19:51AM +0100, Stanislaw Gruszka wrote:
> > > Currently we use frame start event from video capture device to
> > > apply controls. But the capture device might not generate the events.
> > > Usually CSI-2 receiver is proper device to subscribe for start
> > > frame events.
> > >
> > > Without DelayedConntrols:applyControls() is possible that we can get
> >
> > s/DelayedConntrols:/DelayedControls::/
> >
> > > call to DelayedControls::get() with frame number that exceed number
> > > of saved entries and get below assertion failure:
> > >
> > > ../src/libcamera/delayed_controls.cpp:227:
> > > libcamera::ControlList libcamera::DelayedControls::get(uint32_t):
> > > Assertion `info.type() != ControlTypeNone' failed
> > >
> > > Assertion failure can happen at the beginning of streaming when
> > > ControlRingBuffer is not yet filled and there are errors on CSI-2.
> >
> > It seems that this patch fixes two issues. One of them is using the
> > proper device to receive frame start events, the other one is related to
> > this assertion failure. I'm not sure to see how this patch fixes the
> > second issue. Could you split it in two, and maybe provide a bit more
> > information about the fix for the assertion failure ?
>
> Not really sure how to split the patch.
One thing that your patch does is to find the right emitter of the frame
start signal, instead of hardcoding that to data->video_. I think that
could be a patch of its own, it's a worthy fix by itself. Then you have
three other changes in this patch:
- Moving the connection of the frameStart signal from configure() to
start().
- Disconnecting the frameStart() signal in stop().
- Not calling sensor_->setControls() in
SimpleCameraData::setSensorControls() if there's a frame start
emitter.
Moving the signal connection to start() happens to fix a use-after-free
bug (see below), for which I've sent a fix. I think it's still a good
idea to move the connection to start(), as otherwise the signal would be
connected multiple times if you configure the camera multiple times,
which is not a very good idea. I think that, plus moving disconnection
to stop(), is also a candidate for a standalone patch.
Finally, the change to SimpleCameraData::setSensorControls() should be a
third patch.
> The assertion failure can happen when delayedCtrls_->get(frame)
> give us empty control in processStats:
>
> void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
> {
> swIsp_->processStats(frame, bufferId,
> delayedCtrls_->get(frame));
> }
>
> Below 2 conditions have to be meet to trigger the issue.
>
> 1) we process first 16 frames, otherwise DelayedControls::values_
> ControlRingBuffer is filled - no empty controls there
>
> 2) there must be error on CSI bus and some frames dropped there,
> such index calculated by:
I expect CSI-2 errors to be extremely rare. Did you actually get some ?
Or were frames dropped by the driver due to buffer queue underruns ?
> ControlList DelayedControls::get(uint32_t sequence)
> {
> unsigned int index = std::max<int>(0, sequence - maxDelay_);
>
> points to entry in ControlRingBuffer not filled previously by:
>
> void SimpleCameraData::setSensorControls(const ControlList &sensorControls)
> {
> delayedCtrls_->push(sensorControls);
>
> For reference, it was discussed here a little:
> https://patchwork.libcamera.org/patch/22210/
>
> Assertion is consequence of not handling frame start event correctly.
> With the fix, we fill proper entry in ControlRingBuffer, before it's
> read later in processStats(), even if the frame number is not in
> sequence.
>
> > > To fix, loop over devices in the pipeline (starting from the last one),
> > > find one that emits start frame events and connect applyControls()
> > > to it. Additionally remove direct call to sensor_->setControls() if
> > > the emitter device was found.
> > >
> > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=241
> > > Co-developed-by: Hans de Goede <hdegoede at redhat.com>
> > > Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> > > Reviewed-by: Hans de Goede <hdegoede at redhat.com> # v2
> > > Tested-by: Hans de Goede <hdegoede at redhat.com> # Lenovo X1 Yoga IPU6 + ov2740 # v2
> > > Reviewed-by: Milan Zamazal <mzamazal at redhat.com> # v2
> > > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka at linux.intel.com>
> > > ---
> > > src/libcamera/pipeline/simple/simple.cpp | 40 +++++++++++++++++++++---
> > > 1 file changed, 36 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > > index 6e039bf3..ad2167dc 100644
> > > --- a/src/libcamera/pipeline/simple/simple.cpp
> > > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > > @@ -277,6 +277,7 @@ public:
> > > std::list<Entity> entities_;
> > > std::unique_ptr<CameraSensor> sensor_;
> > > V4L2VideoDevice *video_;
> > > + V4L2Subdevice *eventEmitter_;
> >
> > Maybe frameStartEmitter_ to make it move explicit ?
>
> Ok.
>
> > > std::vector<Configuration> configs_;
> > > std::map<PixelFormat, std::vector<const Configuration *>> formats_;
> > > @@ -579,6 +580,19 @@ int SimpleCameraData::init()
> > >
> > > properties_ = sensor_->properties();
> > >
> > > + eventEmitter_ = nullptr;
> > > + for (auto it = entities_.rbegin(); it != entities_.rend(); ++it) {
By the way, you can write this
for (const Entity &entity : entities_) {
V4L2Subdevice *sd = pipe->subdev(entity.entity);
...
> > > + V4L2Subdevice *sd = pipe->subdev(it->entity);
> > > + if (!sd || !sd->supportsFrameStartEvent())
> > > + continue;
> > > +
> > > + LOG(SimplePipeline, Debug)
> > > + << "Using " << it->entity->name() << " frameStart signal";
> > > +
> > > + eventEmitter_ = sd;
> > > + break;
> > > + }
> > > +
> > > return 0;
> > > }
> > >
> > > @@ -897,8 +911,11 @@ void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
> > > void SimpleCameraData::setSensorControls(const ControlList &sensorControls)
> > > {
> > > delayedCtrls_->push(sensorControls);
> > > - ControlList ctrls(sensorControls);
> > > - sensor_->setControls(&ctrls);
> > > + /* Directly apply controls now if there is no frameStart signal */
> > > + if (!eventEmitter_ || !eventEmitter_->frameStartEnabled()) {
> >
> > What's the use case for the second check, when do we have a non-null
> > eventEmitter_ that doesn't emit frame start events ?
>
> This is for case (below in this patch) when in SimplePipelineHandler::start()
>
> ret = eventEmitter->setFrameStartEnabled(true);
>
> fails for some reason, even if previous ioctl() call to detect
> eventEmitter was successful.
Is that something you've experienced, or defensive programming ? I don't
see why it would fail unless there's a bug somewhere, so the start()
function should probably return an error. It would simplify the logic,
and we could drop the frameStartEnabled() function from patch 1/2.
> > > + ControlList ctrls(sensorControls);
> > > + sensor_->setControls(&ctrls);
> > > + }
> > > }
> > >
> > > /* Retrieve all source pads connected to a sink pad through active routes. */
> > > @@ -1285,8 +1302,6 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
> > > data->delayedCtrls_ =
> > > std::make_unique<DelayedControls>(data->sensor_->device(),
> > > params);
I've just noticed that this block of code can be moved to the
SimpleCameraData constructor. I've submitted "[PATCH] pipeline: simple:
Create DelayedControls instance once only" to do so. It can be applied
before or after your series. Would you be able to test it ?
Update: I've just realized that this will result in multiple connections
of the same signal if configure() is called multiple times. We need to
first move connection and disconnection of the frameStart signal to
start() and stop(), so my patch should go on top of your series.
> > > - data->video_->frameStart.connect(data->delayedCtrls_.get(),
> > > - &DelayedControls::applyControls);
And I've also just realized that moving this to start() fixes a
use-after-free bug. Kieran found something similar in the rkisp1
pipeline handler, which means we need better infrastructure.
I've submitted "[PATCH] libcamera: delayed_controls: Inherit from Object
class" to try and address the problem once and for all. This one should
I think be merged first. Would you be able to test that patch as well,
without your series applied ?
I'm sorry for the mess this caused, it turns out we have quite a few
issues in a single place.
> > >
> > > StreamConfiguration inputCfg;
> > > inputCfg.pixelFormat = pipeConfig->captureFormat;
> > > @@ -1325,6 +1340,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL
> > > {
> > > SimpleCameraData *data = cameraData(camera);
> > > V4L2VideoDevice *video = data->video_;
> > > + V4L2Subdevice *eventEmitter = data->eventEmitter_;
> > > int ret;
> > >
> > > const MediaPad *pad = acquirePipeline(data);
> > > @@ -1354,6 +1370,15 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL
> > >
> > > video->bufferReady.connect(data, &SimpleCameraData::imageBufferReady);
> > >
> > > + if (eventEmitter) {
> > > + ret = eventEmitter->setFrameStartEnabled(true);
> > > + if (ret == 0)
> > > + eventEmitter->frameStart.connect(data->delayedCtrls_.get(),
> > > + &DelayedControls::applyControls);
> > > + }
> > > +
> > > + data->delayedCtrls_->reset();
> > > +
> > > ret = video->streamOn();
> > > if (ret < 0) {
> > > stop(camera);
> > > @@ -1385,6 +1410,13 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
> > > {
> > > SimpleCameraData *data = cameraData(camera);
> > > V4L2VideoDevice *video = data->video_;
> > > + V4L2Subdevice *eventEmitter = data->eventEmitter_;
> > > +
> > > + if (eventEmitter) {
> > > + eventEmitter->setFrameStartEnabled(false);
> > > + eventEmitter->frameStart.disconnect(data->delayedCtrls_.get(),
> > > + &DelayedControls::applyControls);
> > > + }
> > >
> > > if (data->useConversion_) {
> > > if (data->converter_)
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list