[PATCH v3 2/2] pipeline: simple: Use proper device for frame start events
Stanislaw Gruszka
stanislaw.gruszka at linux.intel.com
Mon Feb 24 12:02:30 CET 2025
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:
> 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.
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.
> > 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 ?
> > > > + 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.
> > > > + 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 ? BTW, why not prohibit signals/slots
for classes that are not Object ?
> I'm sorry for the mess this caused, it turns out we have quite a few
> issues in a single place.
No problem.
Regards
Stanislaw
> > > >
> > > > 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