[PATCH v3 2/2] pipeline: simple: Use proper device for frame start events
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Feb 24 13:24:12 CET 2025
On Mon, Feb 24, 2025 at 12:02:30PM +0100, Stanislaw Gruszka wrote:
> Hi Laurent,
>
> thanks for tips/clarifications. Have some more questions, please see below.
>
> On Mon, Feb 24, 2025 at 03:39:00AM +0200, Laurent Pinchart wrote:
> > 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.
>
> Ok, going to split the patches.
>
> > > 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 ?
>
> It's quite good reproducible for IPU6 when first start using camera
> after boot.
>
> [ 82.758542] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Payload checksum (CRC) error
> [ 83.250939] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Single packet header error corrected
> [ 83.250962] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Multiple packet header errors detected
> [ 83.250964] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Payload checksum (CRC) error
> [ 83.250965] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Incomplete long packet detected
> [ 83.250966] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Frame sync error
> [ 83.250967] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Inter-frame long packet discarded
>
> Not really sure why. After initial errors, things get to
> work and no other cis2 errors happen.
And that's only the first time after boot ? That sounds like a driver
bug. What happens if you unload and reload the driver, does the first
capture session after reloading exhibit the same problems ?
> > > 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);
> > ...
>
> I wanted to iterate in reverse order. Just in case we have more
> devices that emits events, so we pick up the last one.
> Not really sure if this is the issue. Should we expect one
> event device ? Or if there are more than one such device
> in the pipeline, it doesn't matter which one we pick up ?
Oops, I had missed the rbegin(). This being said, if there are more than
one device that can produce a frame start event, I think we should pick
the device closest to the camera sensor, so forward iteration is better.
> > > > > + 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.
>
> Rather defence programming. I considered coding like this:
>
> ret = eventEmitter->setFrameStartEnabled(true);
> assert(ret == 0);
>
> but though is better to handle the error properly, v4l2_event_subscribe()
> itself do kmalloc and there are other paths when subscribe ioctl can fail.
>
> Return an error from start() instead on fallback to non emitter
> case makes sense. Will change this way.
Thanks.
> > > > > + 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.
>
> There is small conflict, so I think I can post it as part of v5
> to make things easier to apply.
>
> > > > > - 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 ?
>
> Done the test of this patch. It does not fixes the
> "state_ == ProxyRunning" failed in processStatsThread()"
> assertion issue. Maybe it fixes something else that I'm not
> encountering ? Or maybe also some other class need to
> inherit from Object ?
I'll reply to this in the context of the other patch.
> BTW, why not prohibit signals/slots for classes that are not Object ?
I'm considering that, not doing so is causing issues. I'll give it a try
to see how intrusive the change would be.
> > I'm sorry for the mess this caused, it turns out we have quite a few
> > issues in a single place.
>
> No problem.
>
> > > > >
> > > > > 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