| Message ID | 20250218091951.13017-3-stanislaw.gruszka@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Stanislaw Gruszka (2025-02-18 09:19:51) > 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 > 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. > > 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@redhat.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > Reviewed-by: Hans de Goede <hdegoede@redhat.com> # v2 > Tested-by: Hans de Goede <hdegoede@redhat.com> # Lenovo X1 Yoga IPU6 + ov2740 # v2 > Reviewed-by: Milan Zamazal <mzamazal@redhat.com> # v2 > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> This still fixes the flicker on X13s Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> # Lenovo X13s But I'm getting segfaults on stopping the camera. I think that's unrelated to this patch - and exists on mainline so : Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.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_; > > 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) { > + 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()) { > + 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); > - data->video_->frameStart.connect(data->delayedCtrls_.get(), > - &DelayedControls::applyControls); > > 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_) > -- > 2.43.0 >
Quoting Kieran Bingham (2025-02-18 11:54:19) > Quoting Stanislaw Gruszka (2025-02-18 09:19:51) > > 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 > > 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. > > > > 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@redhat.com> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> # v2 > > Tested-by: Hans de Goede <hdegoede@redhat.com> # Lenovo X1 Yoga IPU6 + ov2740 # v2 > > Reviewed-by: Milan Zamazal <mzamazal@redhat.com> # v2 > > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> > > This still fixes the flicker on X13s > > Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> # Lenovo X13s > > But I'm getting segfaults on stopping the camera. I think that's > unrelated to this patch - and exists on mainline so : Confirmed, git revert 9bc8b6a573a63b8c9f5588cdea6da5ae71abd138 and these patches provides stable image and no crashes on start/stop/qcam-exit. I know Milan is working on a fix for the above so no /need/ to revert, we can wait for the fix - but I think it means these two patches are good to go if I can get another RB tag on patch 1/2. -- Kieran > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.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_; > > > > 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) { > > + 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()) { > > + 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); > > - data->video_->frameStart.connect(data->delayedCtrls_.get(), > > - &DelayedControls::applyControls); > > > > 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_) > > -- > > 2.43.0 > >
Hi Stanislaw, Thank you for the patch. 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 ? > 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@redhat.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > Reviewed-by: Hans de Goede <hdegoede@redhat.com> # v2 > Tested-by: Hans de Goede <hdegoede@redhat.com> # Lenovo X1 Yoga IPU6 + ov2740 # v2 > Reviewed-by: Milan Zamazal <mzamazal@redhat.com> # v2 > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@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 ? > > 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) { > + 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 ? > + 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); > - data->video_->frameStart.connect(data->delayedCtrls_.get(), > - &DelayedControls::applyControls); > > 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_)
Hi Laurent, On Wed, Feb 19, 2025 at 02:42:31PM +0200, Laurent Pinchart wrote: > Hi Stanislaw, > > Thank you for the patch. > > 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. 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: 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@redhat.com> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> # v2 > > Tested-by: Hans de Goede <hdegoede@redhat.com> # Lenovo X1 Yoga IPU6 + ov2740 # v2 > > Reviewed-by: Milan Zamazal <mzamazal@redhat.com> # v2 > > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@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) { > > + 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. Regards Stanislaw > > + 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); > > - data->video_->frameStart.connect(data->delayedCtrls_.get(), > > - &DelayedControls::applyControls); > > > > 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
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@redhat.com> > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> # v2 > > > Tested-by: Hans de Goede <hdegoede@redhat.com> # Lenovo X1 Yoga IPU6 + ov2740 # v2 > > > Reviewed-by: Milan Zamazal <mzamazal@redhat.com> # v2 > > > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@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_)
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@redhat.com> > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> # v2 > > > > Tested-by: Hans de Goede <hdegoede@redhat.com> # Lenovo X1 Yoga IPU6 + ov2740 # v2 > > > > Reviewed-by: Milan Zamazal <mzamazal@redhat.com> # v2 > > > > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@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
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@redhat.com> > > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> # v2 > > > > > Tested-by: Hans de Goede <hdegoede@redhat.com> # Lenovo X1 Yoga IPU6 + ov2740 # v2 > > > > > Reviewed-by: Milan Zamazal <mzamazal@redhat.com> # v2 > > > > > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@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_)
On Mon, Feb 24, 2025 at 02:24:12PM +0200, Laurent Pinchart wrote: > On Mon, Feb 24, 2025 at 12:02:30PM +0100, Stanislaw Gruszka wrote: > > > > > 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 ? Reloading intel-ipu6-isys does not exhibit the problem. However reloading the sensor driver ov2740 does. Is sufficient to reload ov2740 module and start the qcam to get those csi2-1 errors. Regards Stanislaw
(CC'ing Bingbu) On Mon, Feb 24, 2025 at 02:29:52PM +0100, Stanislaw Gruszka wrote: > On Mon, Feb 24, 2025 at 02:24:12PM +0200, Laurent Pinchart wrote: > > On Mon, Feb 24, 2025 at 12:02:30PM +0100, Stanislaw Gruszka wrote: > > > > > > > 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 ? > > Reloading intel-ipu6-isys does not exhibit the problem. > However reloading the sensor driver ov2740 does. > Is sufficient to reload ov2740 module and start the qcam > to get those csi2-1 errors. Bingbu, Sakari, this sounds like a bug in a driver somewhere.
On 2/25/25 5:14 AM, Laurent Pinchart wrote: > (CC'ing Bingbu) > > On Mon, Feb 24, 2025 at 02:29:52PM +0100, Stanislaw Gruszka wrote: >> On Mon, Feb 24, 2025 at 02:24:12PM +0200, Laurent Pinchart wrote: >>> On Mon, Feb 24, 2025 at 12:02:30PM +0100, Stanislaw Gruszka wrote: >>>> >>>>> 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 ? >> >> Reloading intel-ipu6-isys does not exhibit the problem. >> However reloading the sensor driver ov2740 does. >> Is sufficient to reload ov2740 module and start the qcam >> to get those csi2-1 errors. > > Bingbu, Sakari, this sounds like a bug in a driver somewhere. > Stanislaw, Which device are you using? I can try it if I have such device. Have you a chance to check whether you can reproduce same problem with other camera sensors (or other devices)? In my experience, such errors report likely caused by bad frames from sensor in corner case.
Hi Bingbu, On Tue, Feb 25, 2025 at 02:05:38PM +0800, Bingbu Cao wrote: > > On 2/25/25 5:14 AM, Laurent Pinchart wrote: > > (CC'ing Bingbu) > > > > On Mon, Feb 24, 2025 at 02:29:52PM +0100, Stanislaw Gruszka wrote: > >> On Mon, Feb 24, 2025 at 02:24:12PM +0200, Laurent Pinchart wrote: > >>> On Mon, Feb 24, 2025 at 12:02:30PM +0100, Stanislaw Gruszka wrote: > >>>> > >>>>> 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 ? > >> > >> Reloading intel-ipu6-isys does not exhibit the problem. > >> However reloading the sensor driver ov2740 does. > >> Is sufficient to reload ov2740 module and start the qcam > >> to get those csi2-1 errors. > > > > Bingbu, Sakari, this sounds like a bug in a driver somewhere. > > > > Stanislaw, > > Which device are you using? I can try it if I have such device. Lenovo X1 ThinkPad Yoga and Nano. > Have you a chance to check whether you can reproduce same problem with > other camera sensors (or other devices)? > > In my experience, such errors report likely caused by bad frames from > sensor in corner case. FWICT the issue happens only with ov2740 on Lenovo laptops. I did not recall having those errors on Dell laptops with different sensors and iVSC, which I also used with ipu6-isys. Regards Stanislaw
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_; 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) { + 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()) { + 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); - data->video_->frameStart.connect(data->delayedCtrls_.get(), - &DelayedControls::applyControls); 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_)