[{"id":33393,"web_url":"https://patchwork.libcamera.org/comment/33393/","msgid":"<173987965958.3276240.2139060662969531797@ping.linuxembedded.co.uk>","date":"2025-02-18T11:54:19","subject":"Re: [PATCH v3 2/2] pipeline: simple: Use proper device for frame\n\tstart events","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stanislaw Gruszka (2025-02-18 09:19:51)\n> Currently we use frame start event from video capture device to\n> apply controls. But the capture device might not generate the events.\n> Usually CSI-2 receiver is proper device to subscribe for start\n> frame events.\n> \n> Without DelayedConntrols:applyControls() is possible that we can get\n> call to DelayedControls::get() with frame number that exceed number\n> of saved entries and get below assertion failure:\n> \n> ../src/libcamera/delayed_controls.cpp:227:\n> libcamera::ControlList libcamera::DelayedControls::get(uint32_t):\n> Assertion `info.type() != ControlTypeNone' failed\n> \n> Assertion failure can happen at the beginning of streaming when\n> ControlRingBuffer is not yet filled and there are errors on CSI-2.\n> \n> To fix, loop over devices in the pipeline (starting from the last one),\n> find one that emits start frame events and connect applyControls()\n> to it. Additionally remove direct call to sensor_->setControls() if\n> the emitter device was found.\n> \n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=241\n> Co-developed-by: Hans de Goede \n> Signed-off-by: Hans de Goede \n> Reviewed-by: Hans de Goede # v2\n> Tested-by: Hans de Goede # Lenovo X1 Yoga IPU6 + ov2740 # v2\n> Reviewed-by: Milan Zamazal # v2\n> Signed-off-by: Stanislaw Gruszka \n\nThis still fixes the flicker on X13s\n\nTested-by: Kieran Bingham # Lenovo X13s\n\nBut I'm getting segfaults on stopping the camera. I think that's\nunrelated to this patch - and exists on mainline so :\n\n\nReviewed-by: Kieran Bingham \n\n\n> ---\n> src/libcamera/pipeline/simple/simple.cpp | 40 +++++++++++++++++++++---\n> 1 file changed, 36 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 6e039bf3..ad2167dc 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -277,6 +277,7 @@ public:\n> std::list entities_;\n> std::unique_ptr sensor_;\n> V4L2VideoDevice *video_;\n> + V4L2Subdevice *eventEmitter_;\n> \n> std::vector configs_;\n> std::map> formats_;\n> @@ -579,6 +580,19 @@ int SimpleCameraData::init()\n> \n> properties_ = sensor_->properties();\n> \n> + eventEmitter_ = nullptr;\n> + for (auto it = entities_.rbegin(); it != entities_.rend(); ++it) {\n> + V4L2Subdevice *sd = pipe->subdev(it->entity);\n> + if (!sd || !sd->supportsFrameStartEvent())\n> + continue;\n> +\n> + LOG(SimplePipeline, Debug)\n> + << \"Using \" << it->entity->name() << \" frameStart signal\";\n> +\n> + eventEmitter_ = sd;\n> + break;\n> + }\n> +\n> return 0;\n> }\n> \n> @@ -897,8 +911,11 @@ void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)\n> void SimpleCameraData::setSensorControls(const ControlList &sensorControls)\n> {\n> delayedCtrls_->push(sensorControls);\n> - ControlList ctrls(sensorControls);\n> - sensor_->setControls(&ctrls);\n> + /* Directly apply controls now if there is no frameStart signal */\n> + if (!eventEmitter_ || !eventEmitter_->frameStartEnabled()) {\n> + ControlList ctrls(sensorControls);\n> + sensor_->setControls(&ctrls);\n> + }\n> }\n> \n> /* Retrieve all source pads connected to a sink pad through active routes. */\n> @@ -1285,8 +1302,6 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n> data->delayedCtrls_ =\n> std::make_unique(data->sensor_->device(),\n> params);\n> - data->video_->frameStart.connect(data->delayedCtrls_.get(),\n> - &DelayedControls::applyControls);\n> \n> StreamConfiguration inputCfg;\n> inputCfg.pixelFormat = pipeConfig->captureFormat;\n> @@ -1325,6 +1340,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> {\n> SimpleCameraData *data = cameraData(camera);\n> V4L2VideoDevice *video = data->video_;\n> + V4L2Subdevice *eventEmitter = data->eventEmitter_;\n> int ret;\n> \n> const MediaPad *pad = acquirePipeline(data);\n> @@ -1354,6 +1370,15 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> \n> video->bufferReady.connect(data, &SimpleCameraData::imageBufferReady);\n> \n> + if (eventEmitter) {\n> + ret = eventEmitter->setFrameStartEnabled(true);\n> + if (ret == 0)\n> + eventEmitter->frameStart.connect(data->delayedCtrls_.get(),\n> + &DelayedControls::applyControls);\n> + }\n> +\n> + data->delayedCtrls_->reset();\n> +\n> ret = video->streamOn();\n> if (ret < 0) {\n> stop(camera);\n> @@ -1385,6 +1410,13 @@ void SimplePipelineHandler::stopDevice(Camera *camera)\n> {\n> SimpleCameraData *data = cameraData(camera);\n> V4L2VideoDevice *video = data->video_;\n> + V4L2Subdevice *eventEmitter = data->eventEmitter_;\n> +\n> + if (eventEmitter) {\n> + eventEmitter->setFrameStartEnabled(false);\n> + eventEmitter->frameStart.disconnect(data->delayedCtrls_.get(),\n> + &DelayedControls::applyControls);\n> + }\n> \n> if (data->useConversion_) {\n> if (data->converter_)\n> --\n> 2.43.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7763FBD78E\n\tfor ;\n\tTue, 18 Feb 2025 11:54:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 79A9768685;\n\tTue, 18 Feb 2025 12:54:24 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 37F6A6863B\n\tfor ;\n\tTue, 18 Feb 2025 12:54:22 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DECCD22F;\n\tTue, 18 Feb 2025 12:52:59 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"sx61y2mj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1739879580;\n\tbh=5fpZyK1vltUvb0xc5RLw2AAIiozkb705mm0mypiyB1M=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=sx61y2mjUT+ewSPKmhlIjW4l648aN6zl+563hHIK8vc+n6q70WNP74d4cxlOulADU\n\tnUKuH+uVN13jgCww2Mz4lPsS/becjLpVfBABYxwbPeAG87kK+5j6wxPIrI0IOIdlVA\n\tJNyvDmqDRR3m9DrSlaYDUrSpiY1LaLzGQ+zvxRV0=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250218091951.13017-3-stanislaw.gruszka@linux.intel.com>","References":"<20250218091951.13017-1-stanislaw.gruszka@linux.intel.com>\n\t<20250218091951.13017-3-stanislaw.gruszka@linux.intel.com>","Subject":"Re: [PATCH v3 2/2] pipeline: simple: Use proper device for frame\n\tstart events","From":"Kieran Bingham ","Cc":"Milan Zamazal ,\n\tLaurent Pinchart ,\n\tNaushir Patuck ,\n\tSakari Ailus ,\n\tHans de Goede ","To":"Stanislaw Gruszka ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 18 Feb 2025 11:54:19 +0000","Message-ID":"<173987965958.3276240.2139060662969531797@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33395,"web_url":"https://patchwork.libcamera.org/comment/33395/","msgid":"<173991941560.360601.14685757026957748715@ping.linuxembedded.co.uk>","date":"2025-02-18T22:56:55","subject":"Re: [PATCH v3 2/2] pipeline: simple: Use proper device for frame\n\tstart events","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Kieran Bingham (2025-02-18 11:54:19)\n> Quoting Stanislaw Gruszka (2025-02-18 09:19:51)\n> > Currently we use frame start event from video capture device to\n> > apply controls. But the capture device might not generate the events.\n> > Usually CSI-2 receiver is proper device to subscribe for start\n> > frame events.\n> > \n> > Without DelayedConntrols:applyControls() is possible that we can get\n> > call to DelayedControls::get() with frame number that exceed number\n> > of saved entries and get below assertion failure:\n> > \n> > ../src/libcamera/delayed_controls.cpp:227:\n> > libcamera::ControlList libcamera::DelayedControls::get(uint32_t):\n> > Assertion `info.type() != ControlTypeNone' failed\n> > \n> > Assertion failure can happen at the beginning of streaming when\n> > ControlRingBuffer is not yet filled and there are errors on CSI-2.\n> > \n> > To fix, loop over devices in the pipeline (starting from the last one),\n> > find one that emits start frame events and connect applyControls()\n> > to it. Additionally remove direct call to sensor_->setControls() if\n> > the emitter device was found.\n> > \n> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=241\n> > Co-developed-by: Hans de Goede \n> > Signed-off-by: Hans de Goede \n> > Reviewed-by: Hans de Goede # v2\n> > Tested-by: Hans de Goede # Lenovo X1 Yoga IPU6 + ov2740 # v2\n> > Reviewed-by: Milan Zamazal # v2\n> > Signed-off-by: Stanislaw Gruszka \n> \n> This still fixes the flicker on X13s\n> \n> Tested-by: Kieran Bingham # Lenovo X13s\n> \n> But I'm getting segfaults on stopping the camera. I think that's\n> unrelated to this patch - and exists on mainline so :\n\nConfirmed,\n\ngit revert 9bc8b6a573a63b8c9f5588cdea6da5ae71abd138\n\nand these patches provides stable image and no crashes on\nstart/stop/qcam-exit.\n\nI know Milan is working on a fix for the above so no /need/ to revert,\nwe can wait for the fix - but I think it means these two patches are\ngood to go if I can get another RB tag on patch 1/2.\n\n--\nKieran\n\n\n> \n> \n> Reviewed-by: Kieran Bingham \n> \n> \n> > ---\n> > src/libcamera/pipeline/simple/simple.cpp | 40 +++++++++++++++++++++---\n> > 1 file changed, 36 insertions(+), 4 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > index 6e039bf3..ad2167dc 100644\n> > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > @@ -277,6 +277,7 @@ public:\n> > std::list entities_;\n> > std::unique_ptr sensor_;\n> > V4L2VideoDevice *video_;\n> > + V4L2Subdevice *eventEmitter_;\n> > \n> > std::vector configs_;\n> > std::map> formats_;\n> > @@ -579,6 +580,19 @@ int SimpleCameraData::init()\n> > \n> > properties_ = sensor_->properties();\n> > \n> > + eventEmitter_ = nullptr;\n> > + for (auto it = entities_.rbegin(); it != entities_.rend(); ++it) {\n> > + V4L2Subdevice *sd = pipe->subdev(it->entity);\n> > + if (!sd || !sd->supportsFrameStartEvent())\n> > + continue;\n> > +\n> > + LOG(SimplePipeline, Debug)\n> > + << \"Using \" << it->entity->name() << \" frameStart signal\";\n> > +\n> > + eventEmitter_ = sd;\n> > + break;\n> > + }\n> > +\n> > return 0;\n> > }\n> > \n> > @@ -897,8 +911,11 @@ void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)\n> > void SimpleCameraData::setSensorControls(const ControlList &sensorControls)\n> > {\n> > delayedCtrls_->push(sensorControls);\n> > - ControlList ctrls(sensorControls);\n> > - sensor_->setControls(&ctrls);\n> > + /* Directly apply controls now if there is no frameStart signal */\n> > + if (!eventEmitter_ || !eventEmitter_->frameStartEnabled()) {\n> > + ControlList ctrls(sensorControls);\n> > + sensor_->setControls(&ctrls);\n> > + }\n> > }\n> > \n> > /* Retrieve all source pads connected to a sink pad through active routes. */\n> > @@ -1285,8 +1302,6 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n> > data->delayedCtrls_ =\n> > std::make_unique(data->sensor_->device(),\n> > params);\n> > - data->video_->frameStart.connect(data->delayedCtrls_.get(),\n> > - &DelayedControls::applyControls);\n> > \n> > StreamConfiguration inputCfg;\n> > inputCfg.pixelFormat = pipeConfig->captureFormat;\n> > @@ -1325,6 +1340,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> > {\n> > SimpleCameraData *data = cameraData(camera);\n> > V4L2VideoDevice *video = data->video_;\n> > + V4L2Subdevice *eventEmitter = data->eventEmitter_;\n> > int ret;\n> > \n> > const MediaPad *pad = acquirePipeline(data);\n> > @@ -1354,6 +1370,15 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> > \n> > video->bufferReady.connect(data, &SimpleCameraData::imageBufferReady);\n> > \n> > + if (eventEmitter) {\n> > + ret = eventEmitter->setFrameStartEnabled(true);\n> > + if (ret == 0)\n> > + eventEmitter->frameStart.connect(data->delayedCtrls_.get(),\n> > + &DelayedControls::applyControls);\n> > + }\n> > +\n> > + data->delayedCtrls_->reset();\n> > +\n> > ret = video->streamOn();\n> > if (ret < 0) {\n> > stop(camera);\n> > @@ -1385,6 +1410,13 @@ void SimplePipelineHandler::stopDevice(Camera *camera)\n> > {\n> > SimpleCameraData *data = cameraData(camera);\n> > V4L2VideoDevice *video = data->video_;\n> > + V4L2Subdevice *eventEmitter = data->eventEmitter_;\n> > +\n> > + if (eventEmitter) {\n> > + eventEmitter->setFrameStartEnabled(false);\n> > + eventEmitter->frameStart.disconnect(data->delayedCtrls_.get(),\n> > + &DelayedControls::applyControls);\n> > + }\n> > \n> > if (data->useConversion_) {\n> > if (data->converter_)\n> > --\n> > 2.43.0\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id F183EBDE6B\n\tfor ;\n\tTue, 18 Feb 2025 22:57:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 285EB68686;\n\tTue, 18 Feb 2025 23:57:00 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 20B096863B\n\tfor ;\n\tTue, 18 Feb 2025 23:56:58 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8828763F;\n\tTue, 18 Feb 2025 23:55:35 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ZCXRIiGz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1739919335;\n\tbh=ATzgXGgtVyi6zPitFcVWU/hICD5cSh9rjcxIZM49xdY=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=ZCXRIiGzRR2PJ0+p+U4YQvBs13ntaqMcG+LhLfSOVVBCeb4QLMYXAnTjil3DbOdfD\n\tmycilNc415fkf9LJSlBeUqRCDElu8Fd/IjKHX8Q1ALvDPQJFLK9ZBdZyXh7jbOCLQO\n\tn5sVBQJHNKfio8ZMjDSjyyo5gaaIGl+Hk8Ym1wIU=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<173987965958.3276240.2139060662969531797@ping.linuxembedded.co.uk>","References":"<20250218091951.13017-1-stanislaw.gruszka@linux.intel.com>\n\t<20250218091951.13017-3-stanislaw.gruszka@linux.intel.com>\n\t<173987965958.3276240.2139060662969531797@ping.linuxembedded.co.uk>","Subject":"Re: [PATCH v3 2/2] pipeline: simple: Use proper device for frame\n\tstart events","From":"Kieran Bingham ","Cc":"Milan Zamazal ,\n\tLaurent Pinchart ,\n\tNaushir Patuck ,\n\tSakari Ailus ,\n\tHans de Goede ","To":"Stanislaw Gruszka ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 18 Feb 2025 22:56:55 +0000","Message-ID":"<173991941560.360601.14685757026957748715@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33397,"web_url":"https://patchwork.libcamera.org/comment/33397/","msgid":"<20250219124231.GA26637@pendragon.ideasonboard.com>","date":"2025-02-19T12:42:31","subject":"Re: [PATCH v3 2/2] pipeline: simple: Use proper device for frame\n\tstart events","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Stanislaw,\n\nThank you for the patch.\n\nOn Tue, Feb 18, 2025 at 10:19:51AM +0100, Stanislaw Gruszka wrote:\n> Currently we use frame start event from video capture device to\n> apply controls. But the capture device might not generate the events.\n> Usually CSI-2 receiver is proper device to subscribe for start\n> frame events.\n> \n> Without DelayedConntrols:applyControls() is possible that we can get\n\ns/DelayedConntrols:/DelayedControls::/\n\n> call to DelayedControls::get() with frame number that exceed number\n> of saved entries and get below assertion failure:\n> \n> ../src/libcamera/delayed_controls.cpp:227:\n> libcamera::ControlList libcamera::DelayedControls::get(uint32_t):\n> Assertion `info.type() != ControlTypeNone' failed\n> \n> Assertion failure can happen at the beginning of streaming when\n> ControlRingBuffer is not yet filled and there are errors on CSI-2.\n\nIt seems that this patch fixes two issues. One of them is using the\nproper device to receive frame start events, the other one is related to\nthis assertion failure. I'm not sure to see how this patch fixes the\nsecond issue. Could you split it in two, and maybe provide a bit more\ninformation about the fix for the assertion failure ?\n\n> To fix, loop over devices in the pipeline (starting from the last one),\n> find one that emits start frame events and connect applyControls()\n> to it. Additionally remove direct call to sensor_->setControls() if\n> the emitter device was found.\n> \n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=241\n> Co-developed-by: Hans de Goede \n> Signed-off-by: Hans de Goede \n> Reviewed-by: Hans de Goede # v2\n> Tested-by: Hans de Goede # Lenovo X1 Yoga IPU6 + ov2740 # v2\n> Reviewed-by: Milan Zamazal # v2\n> Signed-off-by: Stanislaw Gruszka \n> ---\n> src/libcamera/pipeline/simple/simple.cpp | 40 +++++++++++++++++++++---\n> 1 file changed, 36 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 6e039bf3..ad2167dc 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -277,6 +277,7 @@ public:\n> \tstd::list entities_;\n> \tstd::unique_ptr sensor_;\n> \tV4L2VideoDevice *video_;\n> +\tV4L2Subdevice *eventEmitter_;\n\nMaybe frameStartEmitter_ to make it move explicit ?\n\n> \n> \tstd::vector configs_;\n> \tstd::map> formats_;\n> @@ -579,6 +580,19 @@ int SimpleCameraData::init()\n> \n> \tproperties_ = sensor_->properties();\n> \n> +\teventEmitter_ = nullptr;\n> +\tfor (auto it = entities_.rbegin(); it != entities_.rend(); ++it) {\n> +\t\tV4L2Subdevice *sd = pipe->subdev(it->entity);\n> +\t\tif (!sd || !sd->supportsFrameStartEvent())\n> +\t\t\tcontinue;\n> +\n> +\t\tLOG(SimplePipeline, Debug)\n> +\t\t\t<< \"Using \" << it->entity->name() << \" frameStart signal\";\n> +\n> +\t\teventEmitter_ = sd;\n> +\t\tbreak;\n> +\t}\n> +\n> \treturn 0;\n> }\n> \n> @@ -897,8 +911,11 @@ void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)\n> void SimpleCameraData::setSensorControls(const ControlList &sensorControls)\n> {\n> \tdelayedCtrls_->push(sensorControls);\n> -\tControlList ctrls(sensorControls);\n> -\tsensor_->setControls(&ctrls);\n> +\t/* Directly apply controls now if there is no frameStart signal */\n> +\tif (!eventEmitter_ || !eventEmitter_->frameStartEnabled()) {\n\nWhat's the use case for the second check, when do we have a non-null\neventEmitter_ that doesn't emit frame start events ?\n\n> +\t\tControlList ctrls(sensorControls);\n> +\t\tsensor_->setControls(&ctrls);\n> +\t}\n> }\n> \n> /* Retrieve all source pads connected to a sink pad through active routes. */\n> @@ -1285,8 +1302,6 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n> \tdata->delayedCtrls_ =\n> \t\tstd::make_unique(data->sensor_->device(),\n> \t\t\t\t\t\t params);\n> -\tdata->video_->frameStart.connect(data->delayedCtrls_.get(),\n> -\t\t\t\t\t &DelayedControls::applyControls);\n> \n> \tStreamConfiguration inputCfg;\n> \tinputCfg.pixelFormat = pipeConfig->captureFormat;\n> @@ -1325,6 +1340,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> {\n> \tSimpleCameraData *data = cameraData(camera);\n> \tV4L2VideoDevice *video = data->video_;\n> +\tV4L2Subdevice *eventEmitter = data->eventEmitter_;\n> \tint ret;\n> \n> \tconst MediaPad *pad = acquirePipeline(data);\n> @@ -1354,6 +1370,15 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> \n> \tvideo->bufferReady.connect(data, &SimpleCameraData::imageBufferReady);\n> \n> +\tif (eventEmitter) {\n> +\t\tret = eventEmitter->setFrameStartEnabled(true);\n> +\t\tif (ret == 0)\n> +\t\t\teventEmitter->frameStart.connect(data->delayedCtrls_.get(),\n> +\t\t\t\t\t\t\t &DelayedControls::applyControls);\n> +\t}\n> +\n> +\tdata->delayedCtrls_->reset();\n> +\n> \tret = video->streamOn();\n> \tif (ret < 0) {\n> \t\tstop(camera);\n> @@ -1385,6 +1410,13 @@ void SimplePipelineHandler::stopDevice(Camera *camera)\n> {\n> \tSimpleCameraData *data = cameraData(camera);\n> \tV4L2VideoDevice *video = data->video_;\n> +\tV4L2Subdevice *eventEmitter = data->eventEmitter_;\n> +\n> +\tif (eventEmitter) {\n> +\t\teventEmitter->setFrameStartEnabled(false);\n> +\t\teventEmitter->frameStart.disconnect(data->delayedCtrls_.get(),\n> +\t\t\t\t\t\t &DelayedControls::applyControls);\n> +\t}\n> \n> \tif (data->useConversion_) {\n> \t\tif (data->converter_)","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CCE60BDE6B\n\tfor ;\n\tWed, 19 Feb 2025 12:42:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3E84A68687;\n\tWed, 19 Feb 2025 13:42:49 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D31F261861\n\tfor ;\n\tWed, 19 Feb 2025 13:42:46 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BEE1318D;\n\tWed, 19 Feb 2025 13:41:23 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"LmLNaKhZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1739968884;\n\tbh=suN4yDt4H3X/cQmKSNt/PL5/67S7TEkf4CLOTjUFugk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LmLNaKhZFbaSGRfdxoOoXdxZazKX3xyAiXl2IXRbJA8I/JAI/yTS/mQzElL0SfV47\n\t32kZkBBFEp6LfW8rOHtB9wbRk/dMwRU+FyZmkKzeaq0YvtEm7kbnC0Lak8uppoVa8a\n\tskoVbVD1ffREjLH4eNeC/pVsgPaSVqBTGJ2RGYsI=","Date":"Wed, 19 Feb 2025 14:42:31 +0200","From":"Laurent Pinchart ","To":"Stanislaw Gruszka ","Cc":"libcamera-devel@lists.libcamera.org, Milan Zamazal ,\n\tKieran Bingham ,\n\tNaushir Patuck ,\n\tSakari Ailus ,\n\tHans de Goede ","Subject":"Re: [PATCH v3 2/2] pipeline: simple: Use proper device for frame\n\tstart events","Message-ID":"<20250219124231.GA26637@pendragon.ideasonboard.com>","References":"<20250218091951.13017-1-stanislaw.gruszka@linux.intel.com>\n\t<20250218091951.13017-3-stanislaw.gruszka@linux.intel.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250218091951.13017-3-stanislaw.gruszka@linux.intel.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33409,"web_url":"https://patchwork.libcamera.org/comment/33409/","msgid":"","date":"2025-02-20T10:10:45","subject":"Re: [PATCH v3 2/2] pipeline: simple: Use proper device for frame\n\tstart events","submitter":{"id":211,"url":"https://patchwork.libcamera.org/api/people/211/","name":"Stanislaw Gruszka","email":"stanislaw.gruszka@linux.intel.com"},"content":"Hi Laurent,\n\nOn Wed, Feb 19, 2025 at 02:42:31PM +0200, Laurent Pinchart wrote:\n> Hi Stanislaw,\n> \n> Thank you for the patch.\n> \n> On Tue, Feb 18, 2025 at 10:19:51AM +0100, Stanislaw Gruszka wrote:\n> > Currently we use frame start event from video capture device to\n> > apply controls. But the capture device might not generate the events.\n> > Usually CSI-2 receiver is proper device to subscribe for start\n> > frame events.\n> > \n> > Without DelayedConntrols:applyControls() is possible that we can get\n> \n> s/DelayedConntrols:/DelayedControls::/\n> > call to DelayedControls::get() with frame number that exceed number\n> > of saved entries and get below assertion failure:\n> > \n> > ../src/libcamera/delayed_controls.cpp:227:\n> > libcamera::ControlList libcamera::DelayedControls::get(uint32_t):\n> > Assertion `info.type() != ControlTypeNone' failed\n> > \n> > Assertion failure can happen at the beginning of streaming when\n> > ControlRingBuffer is not yet filled and there are errors on CSI-2.\n> \n> It seems that this patch fixes two issues. One of them is using the\n> proper device to receive frame start events, the other one is related to\n> this assertion failure. I'm not sure to see how this patch fixes the\n> second issue. Could you split it in two, and maybe provide a bit more\n> information about the fix for the assertion failure ?\n\nNot really sure how to split the patch.\n\nThe assertion failure can happen when delayedCtrls_->get(frame)\ngive us empty control in processStats:\n\nvoid SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)\n{\n\tswIsp_->processStats(frame, bufferId,\n\t\t\t delayedCtrls_->get(frame));\n}\n\nBelow 2 conditions have to be meet to trigger the issue.\n\n1) we process first 16 frames, otherwise DelayedControls::values_\nControlRingBuffer is filled - no empty controls there\n\n2) there must be error on CSI bus and some frames dropped there,\nsuch index calculated by:\n\n ControlList DelayedControls::get(uint32_t sequence)\n {\n\tunsigned int index = std::max(0, sequence - maxDelay_);\n\npoints to entry in ControlRingBuffer not filled previously by:\n\nvoid SimpleCameraData::setSensorControls(const ControlList &sensorControls)\n{\n\tdelayedCtrls_->push(sensorControls);\n\nFor reference, it was discussed here a little:\nhttps://patchwork.libcamera.org/patch/22210/ \n\nAssertion is consequence of not handling frame start event correctly.\nWith the fix, we fill proper entry in ControlRingBuffer, before it's\nread later in processStats(), even if the frame number is not in\nsequence.\n\n> > To fix, loop over devices in the pipeline (starting from the last one),\n> > find one that emits start frame events and connect applyControls()\n> > to it. Additionally remove direct call to sensor_->setControls() if\n> > the emitter device was found.\n> > \n> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=241\n> > Co-developed-by: Hans de Goede \n> > Signed-off-by: Hans de Goede \n> > Reviewed-by: Hans de Goede # v2\n> > Tested-by: Hans de Goede # Lenovo X1 Yoga IPU6 + ov2740 # v2\n> > Reviewed-by: Milan Zamazal # v2\n> > Signed-off-by: Stanislaw Gruszka \n> > ---\n> > src/libcamera/pipeline/simple/simple.cpp | 40 +++++++++++++++++++++---\n> > 1 file changed, 36 insertions(+), 4 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > index 6e039bf3..ad2167dc 100644\n> > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > @@ -277,6 +277,7 @@ public:\n> > \tstd::list entities_;\n> > \tstd::unique_ptr sensor_;\n> > \tV4L2VideoDevice *video_;\n> > +\tV4L2Subdevice *eventEmitter_;\n> \n> Maybe frameStartEmitter_ to make it move explicit ?\n\nOk.\n\n> > \tstd::vector configs_;\n> > \tstd::map> formats_;\n> > @@ -579,6 +580,19 @@ int SimpleCameraData::init()\n> > \n> > \tproperties_ = sensor_->properties();\n> > \n> > +\teventEmitter_ = nullptr;\n> > +\tfor (auto it = entities_.rbegin(); it != entities_.rend(); ++it) {\n> > +\t\tV4L2Subdevice *sd = pipe->subdev(it->entity);\n> > +\t\tif (!sd || !sd->supportsFrameStartEvent())\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tLOG(SimplePipeline, Debug)\n> > +\t\t\t<< \"Using \" << it->entity->name() << \" frameStart signal\";\n> > +\n> > +\t\teventEmitter_ = sd;\n> > +\t\tbreak;\n> > +\t}\n> > +\n> > \treturn 0;\n> > }\n> > \n> > @@ -897,8 +911,11 @@ void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)\n> > void SimpleCameraData::setSensorControls(const ControlList &sensorControls)\n> > {\n> > \tdelayedCtrls_->push(sensorControls);\n> > -\tControlList ctrls(sensorControls);\n> > -\tsensor_->setControls(&ctrls);\n> > +\t/* Directly apply controls now if there is no frameStart signal */\n> > +\tif (!eventEmitter_ || !eventEmitter_->frameStartEnabled()) {\n> \n> What's the use case for the second check, when do we have a non-null\n> eventEmitter_ that doesn't emit frame start events ?\n\nThis is for case (below in this patch) when in SimplePipelineHandler::start()\n\n\t\tret = eventEmitter->setFrameStartEnabled(true);\n\nfails for some reason, even if previous ioctl() call to detect\neventEmitter was successful.\n\nRegards\nStanislaw\n\n> > +\t\tControlList ctrls(sensorControls);\n> > +\t\tsensor_->setControls(&ctrls);\n> > +\t}\n> > }\n> > \n> > /* Retrieve all source pads connected to a sink pad through active routes. */\n> > @@ -1285,8 +1302,6 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n> > \tdata->delayedCtrls_ =\n> > \t\tstd::make_unique(data->sensor_->device(),\n> > \t\t\t\t\t\t params);\n> > -\tdata->video_->frameStart.connect(data->delayedCtrls_.get(),\n> > -\t\t\t\t\t &DelayedControls::applyControls);\n> > \n> > \tStreamConfiguration inputCfg;\n> > \tinputCfg.pixelFormat = pipeConfig->captureFormat;\n> > @@ -1325,6 +1340,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> > {\n> > \tSimpleCameraData *data = cameraData(camera);\n> > \tV4L2VideoDevice *video = data->video_;\n> > +\tV4L2Subdevice *eventEmitter = data->eventEmitter_;\n> > \tint ret;\n> > \n> > \tconst MediaPad *pad = acquirePipeline(data);\n> > @@ -1354,6 +1370,15 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> > \n> > \tvideo->bufferReady.connect(data, &SimpleCameraData::imageBufferReady);\n> > \n> > +\tif (eventEmitter) {\n> > +\t\tret = eventEmitter->setFrameStartEnabled(true);\n> > +\t\tif (ret == 0)\n> > +\t\t\teventEmitter->frameStart.connect(data->delayedCtrls_.get(),\n> > +\t\t\t\t\t\t\t &DelayedControls::applyControls);\n> > +\t}\n> > +\n> > +\tdata->delayedCtrls_->reset();\n> > +\n> > \tret = video->streamOn();\n> > \tif (ret < 0) {\n> > \t\tstop(camera);\n> > @@ -1385,6 +1410,13 @@ void SimplePipelineHandler::stopDevice(Camera *camera)\n> > {\n> > \tSimpleCameraData *data = cameraData(camera);\n> > \tV4L2VideoDevice *video = data->video_;\n> > +\tV4L2Subdevice *eventEmitter = data->eventEmitter_;\n> > +\n> > +\tif (eventEmitter) {\n> > +\t\teventEmitter->setFrameStartEnabled(false);\n> > +\t\teventEmitter->frameStart.disconnect(data->delayedCtrls_.get(),\n> > +\t\t\t\t\t\t &DelayedControls::applyControls);\n> > +\t}\n> > \n> > \tif (data->useConversion_) {\n> > \t\tif (data->converter_)\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 52B57BEFBE\n\tfor ;\n\tThu, 20 Feb 2025 10:11:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 527106869B;\n\tThu, 20 Feb 2025 11:11:14 +0100 (CET)","from mgamail.intel.com (mgamail.intel.com [198.175.65.18])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 11D9F6185D\n\tfor ;\n\tThu, 20 Feb 2025 11:11:11 +0100 (CET)","from fmviesa005.fm.intel.com ([10.60.135.145])\n\tby orvoesa110.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384;\n\t20 Feb 2025 02:10:51 -0800","from sgruszka-mobl.ger.corp.intel.com (HELO localhost)\n\t([10.246.8.237]) by fmviesa005-auth.fm.intel.com with\n\tESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 20 Feb 2025 02:10:48 -0800"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=intel.com header.i=@intel.com\n\theader.b=\"F0QA3dVg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple;\n\td=intel.com; i=@intel.com; q=dns/txt; s=Intel;\n\tt=1740046273; x=1771582273;\n\th=date:from:to:cc:subject:message-id:references:\n\tmime-version:in-reply-to;\n\tbh=RHqcKvd787y7cEf3EOPGjB/hObiE/cTbCMTUtw/HcTw=;\n\tb=F0QA3dVgdLKUjqbduH917739zw8RggI+a7/wikbIlTLRuOy+lMcAQ3Ln\n\tSRYOBlVbeBqERF8gxnT5Y7aaGH5GaZZtohJ8KkENZrUF26mNy1N4iPEnz\n\tYRHid/J5v9ybtv0pt2czLsR3X+ZToBwVFVf35PVOLyRNoRXozzG69ieFc\n\tKW+ljUTNipNWEWHDeJ3nNig0jPcTVQI9RsUklqGxpsnpACuut+TklTX2a\n\trMHy3L1Qt6Uhqb3sK5V8AA3uuaTJHWCw5IkP5T4+9h/mHeqobHohjRZ+N\n\tWmaN2JrpDFrq4HNujlPbXgbaHO73JPBNPkJKR09zhpY9+IPen/+0BRZfH g==;","X-CSE-ConnectionGUID":["w4s46GWLRZCItCijQECZlQ==","3V4j44NcR6Sy1tPgdKCEgg=="],"X-CSE-MsgGUID":["4mUsSzpnSjCbjOwmHtxCAA==","/QtgNve/RUeECJuk7GGnQA=="],"X-IronPort-AV":["E=McAfee;i=\"6700,10204,11314\"; a=\"41021919\"","E=Sophos;i=\"6.12,310,1728975600\"; d=\"scan'208\";a=\"41021919\"","E=Sophos;i=\"6.13,301,1732608000\"; d=\"scan'208\";a=\"119619580\""],"X-ExtLoop1":"1","Date":"Thu, 20 Feb 2025 11:10:45 +0100","From":"Stanislaw Gruszka ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, Milan Zamazal ,\n\tKieran Bingham ,\n\tNaushir Patuck ,\n\tSakari Ailus ,\n\tHans de Goede ","Subject":"Re: [PATCH v3 2/2] pipeline: simple: Use proper device for frame\n\tstart events","Message-ID":"","References":"<20250218091951.13017-1-stanislaw.gruszka@linux.intel.com>\n\t<20250218091951.13017-3-stanislaw.gruszka@linux.intel.com>\n\t<20250219124231.GA26637@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20250219124231.GA26637@pendragon.ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33422,"web_url":"https://patchwork.libcamera.org/comment/33422/","msgid":"<20250224013900.GD22240@pendragon.ideasonboard.com>","date":"2025-02-24T01:39:00","subject":"Re: [PATCH v3 2/2] pipeline: simple: Use proper device for frame\n\tstart events","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Stanislaw,\n\nOn Thu, Feb 20, 2025 at 11:10:45AM +0100, Stanislaw Gruszka wrote:\n> On Wed, Feb 19, 2025 at 02:42:31PM +0200, Laurent Pinchart wrote:\n> > On Tue, Feb 18, 2025 at 10:19:51AM +0100, Stanislaw Gruszka wrote:\n> > > Currently we use frame start event from video capture device to\n> > > apply controls. But the capture device might not generate the events.\n> > > Usually CSI-2 receiver is proper device to subscribe for start\n> > > frame events.\n> > > \n> > > Without DelayedConntrols:applyControls() is possible that we can get\n> > \n> > s/DelayedConntrols:/DelayedControls::/\n> >\n> > > call to DelayedControls::get() with frame number that exceed number\n> > > of saved entries and get below assertion failure:\n> > > \n> > > ../src/libcamera/delayed_controls.cpp:227:\n> > > libcamera::ControlList libcamera::DelayedControls::get(uint32_t):\n> > > Assertion `info.type() != ControlTypeNone' failed\n> > > \n> > > Assertion failure can happen at the beginning of streaming when\n> > > ControlRingBuffer is not yet filled and there are errors on CSI-2.\n> > \n> > It seems that this patch fixes two issues. One of them is using the\n> > proper device to receive frame start events, the other one is related to\n> > this assertion failure. I'm not sure to see how this patch fixes the\n> > second issue. Could you split it in two, and maybe provide a bit more\n> > information about the fix for the assertion failure ?\n> \n> Not really sure how to split the patch.\n\nOne thing that your patch does is to find the right emitter of the frame\nstart signal, instead of hardcoding that to data->video_. I think that\ncould be a patch of its own, it's a worthy fix by itself. Then you have\nthree other changes in this patch:\n\n- Moving the connection of the frameStart signal from configure() to\n start().\n\n- Disconnecting the frameStart() signal in stop().\n\n- Not calling sensor_->setControls() in\n SimpleCameraData::setSensorControls() if there's a frame start\n emitter.\n\nMoving the signal connection to start() happens to fix a use-after-free\nbug (see below), for which I've sent a fix. I think it's still a good\nidea to move the connection to start(), as otherwise the signal would be\nconnected multiple times if you configure the camera multiple times,\nwhich is not a very good idea. I think that, plus moving disconnection\nto stop(), is also a candidate for a standalone patch.\n\nFinally, the change to SimpleCameraData::setSensorControls() should be a\nthird patch.\n\n> The assertion failure can happen when delayedCtrls_->get(frame)\n> give us empty control in processStats:\n> \n> void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)\n> {\n> \tswIsp_->processStats(frame, bufferId,\n> \t\t\t delayedCtrls_->get(frame));\n> }\n> \n> Below 2 conditions have to be meet to trigger the issue.\n> \n> 1) we process first 16 frames, otherwise DelayedControls::values_\n> ControlRingBuffer is filled - no empty controls there\n> \n> 2) there must be error on CSI bus and some frames dropped there,\n> such index calculated by:\n\nI expect CSI-2 errors to be extremely rare. Did you actually get some ?\nOr were frames dropped by the driver due to buffer queue underruns ?\n\n> ControlList DelayedControls::get(uint32_t sequence)\n> {\n> \tunsigned int index = std::max(0, sequence - maxDelay_);\n> \n> points to entry in ControlRingBuffer not filled previously by:\n> \n> void SimpleCameraData::setSensorControls(const ControlList &sensorControls)\n> {\n> \tdelayedCtrls_->push(sensorControls);\n> \n> For reference, it was discussed here a little:\n> https://patchwork.libcamera.org/patch/22210/ \n> \n> Assertion is consequence of not handling frame start event correctly.\n> With the fix, we fill proper entry in ControlRingBuffer, before it's\n> read later in processStats(), even if the frame number is not in\n> sequence.\n> \n> > > To fix, loop over devices in the pipeline (starting from the last one),\n> > > find one that emits start frame events and connect applyControls()\n> > > to it. Additionally remove direct call to sensor_->setControls() if\n> > > the emitter device was found.\n> > > \n> > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=241\n> > > Co-developed-by: Hans de Goede \n> > > Signed-off-by: Hans de Goede \n> > > Reviewed-by: Hans de Goede # v2\n> > > Tested-by: Hans de Goede # Lenovo X1 Yoga IPU6 + ov2740 # v2\n> > > Reviewed-by: Milan Zamazal # v2\n> > > Signed-off-by: Stanislaw Gruszka \n> > > ---\n> > > src/libcamera/pipeline/simple/simple.cpp | 40 +++++++++++++++++++++---\n> > > 1 file changed, 36 insertions(+), 4 deletions(-)\n> > > \n> > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > > index 6e039bf3..ad2167dc 100644\n> > > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > > @@ -277,6 +277,7 @@ public:\n> > > \tstd::list entities_;\n> > > \tstd::unique_ptr sensor_;\n> > > \tV4L2VideoDevice *video_;\n> > > +\tV4L2Subdevice *eventEmitter_;\n> > \n> > Maybe frameStartEmitter_ to make it move explicit ?\n> \n> Ok.\n> \n> > > \tstd::vector configs_;\n> > > \tstd::map> formats_;\n> > > @@ -579,6 +580,19 @@ int SimpleCameraData::init()\n> > > \n> > > \tproperties_ = sensor_->properties();\n> > > \n> > > +\teventEmitter_ = nullptr;\n> > > +\tfor (auto it = entities_.rbegin(); it != entities_.rend(); ++it) {\n\nBy the way, you can write this\n\n\tfor (const Entity &entity : entities_) {\n\t\tV4L2Subdevice *sd = pipe->subdev(entity.entity);\n\t\t...\n\n> > > +\t\tV4L2Subdevice *sd = pipe->subdev(it->entity);\n> > > +\t\tif (!sd || !sd->supportsFrameStartEvent())\n> > > +\t\t\tcontinue;\n> > > +\n> > > +\t\tLOG(SimplePipeline, Debug)\n> > > +\t\t\t<< \"Using \" << it->entity->name() << \" frameStart signal\";\n> > > +\n> > > +\t\teventEmitter_ = sd;\n> > > +\t\tbreak;\n> > > +\t}\n> > > +\n> > > \treturn 0;\n> > > }\n> > > \n> > > @@ -897,8 +911,11 @@ void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)\n> > > void SimpleCameraData::setSensorControls(const ControlList &sensorControls)\n> > > {\n> > > \tdelayedCtrls_->push(sensorControls);\n> > > -\tControlList ctrls(sensorControls);\n> > > -\tsensor_->setControls(&ctrls);\n> > > +\t/* Directly apply controls now if there is no frameStart signal */\n> > > +\tif (!eventEmitter_ || !eventEmitter_->frameStartEnabled()) {\n> > \n> > What's the use case for the second check, when do we have a non-null\n> > eventEmitter_ that doesn't emit frame start events ?\n> \n> This is for case (below in this patch) when in SimplePipelineHandler::start()\n> \n> \t\tret = eventEmitter->setFrameStartEnabled(true);\n> \n> fails for some reason, even if previous ioctl() call to detect\n> eventEmitter was successful.\n\nIs that something you've experienced, or defensive programming ? I don't\nsee why it would fail unless there's a bug somewhere, so the start()\nfunction should probably return an error. It would simplify the logic,\nand we could drop the frameStartEnabled() function from patch 1/2.\n\n> > > +\t\tControlList ctrls(sensorControls);\n> > > +\t\tsensor_->setControls(&ctrls);\n> > > +\t}\n> > > }\n> > > \n> > > /* Retrieve all source pads connected to a sink pad through active routes. */\n> > > @@ -1285,8 +1302,6 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n> > > \tdata->delayedCtrls_ =\n> > > \t\tstd::make_unique(data->sensor_->device(),\n> > > \t\t\t\t\t\t params);\n\nI've just noticed that this block of code can be moved to the\nSimpleCameraData constructor. I've submitted \"[PATCH] pipeline: simple:\nCreate DelayedControls instance once only\" to do so. It can be applied\nbefore or after your series. Would you be able to test it ?\n\nUpdate: I've just realized that this will result in multiple connections\nof the same signal if configure() is called multiple times. We need to\nfirst move connection and disconnection of the frameStart signal to\nstart() and stop(), so my patch should go on top of your series.\n\n> > > -\tdata->video_->frameStart.connect(data->delayedCtrls_.get(),\n> > > -\t\t\t\t\t &DelayedControls::applyControls);\n\nAnd I've also just realized that moving this to start() fixes a\nuse-after-free bug. Kieran found something similar in the rkisp1\npipeline handler, which means we need better infrastructure.\n\nI've submitted \"[PATCH] libcamera: delayed_controls: Inherit from Object\nclass\" to try and address the problem once and for all. This one should\nI think be merged first. Would you be able to test that patch as well,\nwithout your series applied ?\n\nI'm sorry for the mess this caused, it turns out we have quite a few\nissues in a single place.\n\n> > > \n> > > \tStreamConfiguration inputCfg;\n> > > \tinputCfg.pixelFormat = pipeConfig->captureFormat;\n> > > @@ -1325,6 +1340,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> > > {\n> > > \tSimpleCameraData *data = cameraData(camera);\n> > > \tV4L2VideoDevice *video = data->video_;\n> > > +\tV4L2Subdevice *eventEmitter = data->eventEmitter_;\n> > > \tint ret;\n> > > \n> > > \tconst MediaPad *pad = acquirePipeline(data);\n> > > @@ -1354,6 +1370,15 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> > > \n> > > \tvideo->bufferReady.connect(data, &SimpleCameraData::imageBufferReady);\n> > > \n> > > +\tif (eventEmitter) {\n> > > +\t\tret = eventEmitter->setFrameStartEnabled(true);\n> > > +\t\tif (ret == 0)\n> > > +\t\t\teventEmitter->frameStart.connect(data->delayedCtrls_.get(),\n> > > +\t\t\t\t\t\t\t &DelayedControls::applyControls);\n> > > +\t}\n> > > +\n> > > +\tdata->delayedCtrls_->reset();\n> > > +\n> > > \tret = video->streamOn();\n> > > \tif (ret < 0) {\n> > > \t\tstop(camera);\n> > > @@ -1385,6 +1410,13 @@ void SimplePipelineHandler::stopDevice(Camera *camera)\n> > > {\n> > > \tSimpleCameraData *data = cameraData(camera);\n> > > \tV4L2VideoDevice *video = data->video_;\n> > > +\tV4L2Subdevice *eventEmitter = data->eventEmitter_;\n> > > +\n> > > +\tif (eventEmitter) {\n> > > +\t\teventEmitter->setFrameStartEnabled(false);\n> > > +\t\teventEmitter->frameStart.disconnect(data->delayedCtrls_.get(),\n> > > +\t\t\t\t\t\t &DelayedControls::applyControls);\n> > > +\t}\n> > > \n> > > \tif (data->useConversion_) {\n> > > \t\tif (data->converter_)","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 53F3EC32A9\n\tfor ;\n\tMon, 24 Feb 2025 01:39:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 64457686C4;\n\tMon, 24 Feb 2025 02:39:19 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D702861855\n\tfor ;\n\tMon, 24 Feb 2025 02:39:17 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 69B217EA;\n\tMon, 24 Feb 2025 02:37:51 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"GePlmWNU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1740361071;\n\tbh=Vom0aZq6zvpqQ9LJbsxOHhju7bjlRbzByWWPNE3vMwo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GePlmWNUc2fn3mwwpyiDM4Hj6IJy4e4oPvnQ1axJsdXs8e/H2+Kpka6OTBahJ5A4d\n\t6dtT7KvcnwM6/LzM1KPXnnbShn1x8IY8atXuQ57fyVIl4PXp+bjgwbN8C/tkT93/cH\n\tCUQyCGZhp6rB0J8H1PYFDtRLgToP8TiyhfOLeJPU=","Date":"Mon, 24 Feb 2025 03:39:00 +0200","From":"Laurent Pinchart ","To":"Stanislaw Gruszka ","Cc":"libcamera-devel@lists.libcamera.org, Milan Zamazal ,\n\tKieran Bingham ,\n\tNaushir Patuck ,\n\tSakari Ailus ,\n\tHans de Goede ","Subject":"Re: [PATCH v3 2/2] pipeline: simple: Use proper device for frame\n\tstart events","Message-ID":"<20250224013900.GD22240@pendragon.ideasonboard.com>","References":"<20250218091951.13017-1-stanislaw.gruszka@linux.intel.com>\n\t<20250218091951.13017-3-stanislaw.gruszka@linux.intel.com>\n\t<20250219124231.GA26637@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33445,"web_url":"https://patchwork.libcamera.org/comment/33445/","msgid":"","date":"2025-02-24T11:02:30","subject":"Re: [PATCH v3 2/2] pipeline: simple: Use proper device for frame\n\tstart events","submitter":{"id":211,"url":"https://patchwork.libcamera.org/api/people/211/","name":"Stanislaw Gruszka","email":"stanislaw.gruszka@linux.intel.com"},"content":"Hi Laurent,\n\nthanks for tips/clarifications. Have some more questions, please see below.\n\nOn Mon, Feb 24, 2025 at 03:39:00AM +0200, Laurent Pinchart wrote:\n> Hi Stanislaw,\n> \n> On Thu, Feb 20, 2025 at 11:10:45AM +0100, Stanislaw Gruszka wrote:\n> > On Wed, Feb 19, 2025 at 02:42:31PM +0200, Laurent Pinchart wrote:\n> > > On Tue, Feb 18, 2025 at 10:19:51AM +0100, Stanislaw Gruszka wrote:\n> > > > Currently we use frame start event from video capture device to\n> > > > apply controls. But the capture device might not generate the events.\n> > > > Usually CSI-2 receiver is proper device to subscribe for start\n> > > > frame events.\n> > > > \n> > > > Without DelayedConntrols:applyControls() is possible that we can get\n> > > \n> > > s/DelayedConntrols:/DelayedControls::/\n> > >\n> > > > call to DelayedControls::get() with frame number that exceed number\n> > > > of saved entries and get below assertion failure:\n> > > > \n> > > > ../src/libcamera/delayed_controls.cpp:227:\n> > > > libcamera::ControlList libcamera::DelayedControls::get(uint32_t):\n> > > > Assertion `info.type() != ControlTypeNone' failed\n> > > > \n> > > > Assertion failure can happen at the beginning of streaming when\n> > > > ControlRingBuffer is not yet filled and there are errors on CSI-2.\n> > > \n> > > It seems that this patch fixes two issues. One of them is using the\n> > > proper device to receive frame start events, the other one is related to\n> > > this assertion failure. I'm not sure to see how this patch fixes the\n> > > second issue. Could you split it in two, and maybe provide a bit more\n> > > information about the fix for the assertion failure ?\n> > \n> > Not really sure how to split the patch.\n> \n> One thing that your patch does is to find the right emitter of the frame\n> start signal, instead of hardcoding that to data->video_. I think that\n> could be a patch of its own, it's a worthy fix by itself. Then you have\n> three other changes in this patch:\n> \n> - Moving the connection of the frameStart signal from configure() to\n> start().\n> \n> - Disconnecting the frameStart() signal in stop().\n> \n> - Not calling sensor_->setControls() in\n> SimpleCameraData::setSensorControls() if there's a frame start\n> emitter.\n> \n> Moving the signal connection to start() happens to fix a use-after-free\n> bug (see below), for which I've sent a fix. I think it's still a good\n> idea to move the connection to start(), as otherwise the signal would be\n> connected multiple times if you configure the camera multiple times,\n> which is not a very good idea. I think that, plus moving disconnection\n> to stop(), is also a candidate for a standalone patch.\n> \n> Finally, the change to SimpleCameraData::setSensorControls() should be a\n> third patch.\n\nOk, going to split the patches.\n\n> > The assertion failure can happen when delayedCtrls_->get(frame)\n> > give us empty control in processStats:\n> > \n> > void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)\n> > {\n> > \tswIsp_->processStats(frame, bufferId,\n> > \t\t\t delayedCtrls_->get(frame));\n> > }\n> > \n> > Below 2 conditions have to be meet to trigger the issue.\n> > \n> > 1) we process first 16 frames, otherwise DelayedControls::values_\n> > ControlRingBuffer is filled - no empty controls there\n> > \n> > 2) there must be error on CSI bus and some frames dropped there,\n> > such index calculated by:\n> \n> I expect CSI-2 errors to be extremely rare. Did you actually get some ?\n> Or were frames dropped by the driver due to buffer queue underruns ?\n\nIt's quite good reproducible for IPU6 when first start using camera\nafter boot.\n\n[ 82.758542] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Payload checksum (CRC) error\n[ 83.250939] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Single packet header error corrected\n[ 83.250962] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Multiple packet header errors detected\n[ 83.250964] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Payload checksum (CRC) error\n[ 83.250965] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Incomplete long packet detected\n[ 83.250966] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Frame sync error\n[ 83.250967] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Inter-frame long packet discarded\n\nNot really sure why. After initial errors, things get to\nwork and no other cis2 errors happen.\n\n> > ControlList DelayedControls::get(uint32_t sequence)\n> > {\n> > \tunsigned int index = std::max(0, sequence - maxDelay_);\n> > \n> > points to entry in ControlRingBuffer not filled previously by:\n> > \n> > void SimpleCameraData::setSensorControls(const ControlList &sensorControls)\n> > {\n> > \tdelayedCtrls_->push(sensorControls);\n> > \n> > For reference, it was discussed here a little:\n> > https://patchwork.libcamera.org/patch/22210/ \n> > \n> > Assertion is consequence of not handling frame start event correctly.\n> > With the fix, we fill proper entry in ControlRingBuffer, before it's\n> > read later in processStats(), even if the frame number is not in\n> > sequence.\n> > \n> > > > To fix, loop over devices in the pipeline (starting from the last one),\n> > > > find one that emits start frame events and connect applyControls()\n> > > > to it. Additionally remove direct call to sensor_->setControls() if\n> > > > the emitter device was found.\n> > > > \n> > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=241\n> > > > Co-developed-by: Hans de Goede \n> > > > Signed-off-by: Hans de Goede \n> > > > Reviewed-by: Hans de Goede # v2\n> > > > Tested-by: Hans de Goede # Lenovo X1 Yoga IPU6 + ov2740 # v2\n> > > > Reviewed-by: Milan Zamazal # v2\n> > > > Signed-off-by: Stanislaw Gruszka \n> > > > ---\n> > > > src/libcamera/pipeline/simple/simple.cpp | 40 +++++++++++++++++++++---\n> > > > 1 file changed, 36 insertions(+), 4 deletions(-)\n> > > > \n> > > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > > > index 6e039bf3..ad2167dc 100644\n> > > > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > > > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > > > @@ -277,6 +277,7 @@ public:\n> > > > \tstd::list entities_;\n> > > > \tstd::unique_ptr sensor_;\n> > > > \tV4L2VideoDevice *video_;\n> > > > +\tV4L2Subdevice *eventEmitter_;\n> > > \n> > > Maybe frameStartEmitter_ to make it move explicit ?\n> > \n> > Ok.\n> > \n> > > > \tstd::vector configs_;\n> > > > \tstd::map> formats_;\n> > > > @@ -579,6 +580,19 @@ int SimpleCameraData::init()\n> > > > \n> > > > \tproperties_ = sensor_->properties();\n> > > > \n> > > > +\teventEmitter_ = nullptr;\n> > > > +\tfor (auto it = entities_.rbegin(); it != entities_.rend(); ++it) {\n> \n> By the way, you can write this\n> \n> \tfor (const Entity &entity : entities_) {\n> \t\tV4L2Subdevice *sd = pipe->subdev(entity.entity);\n> \t\t...\n\nI wanted to iterate in reverse order. Just in case we have more\ndevices that emits events, so we pick up the last one. \nNot really sure if this is the issue. Should we expect one \nevent device ? Or if there are more than one such device \nin the pipeline, it doesn't matter which one we pick up ?\n\n> > > > +\t\tV4L2Subdevice *sd = pipe->subdev(it->entity);\n> > > > +\t\tif (!sd || !sd->supportsFrameStartEvent())\n> > > > +\t\t\tcontinue;\n> > > > +\n> > > > +\t\tLOG(SimplePipeline, Debug)\n> > > > +\t\t\t<< \"Using \" << it->entity->name() << \" frameStart signal\";\n> > > > +\n> > > > +\t\teventEmitter_ = sd;\n> > > > +\t\tbreak;\n> > > > +\t}\n> > > > +\n> > > > \treturn 0;\n> > > > }\n> > > > \n> > > > @@ -897,8 +911,11 @@ void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)\n> > > > void SimpleCameraData::setSensorControls(const ControlList &sensorControls)\n> > > > {\n> > > > \tdelayedCtrls_->push(sensorControls);\n> > > > -\tControlList ctrls(sensorControls);\n> > > > -\tsensor_->setControls(&ctrls);\n> > > > +\t/* Directly apply controls now if there is no frameStart signal */\n> > > > +\tif (!eventEmitter_ || !eventEmitter_->frameStartEnabled()) {\n> > > \n> > > What's the use case for the second check, when do we have a non-null\n> > > eventEmitter_ that doesn't emit frame start events ?\n> > \n> > This is for case (below in this patch) when in SimplePipelineHandler::start()\n> > \n> > \t\tret = eventEmitter->setFrameStartEnabled(true);\n> > \n> > fails for some reason, even if previous ioctl() call to detect\n> > eventEmitter was successful.\n> \n> Is that something you've experienced, or defensive programming ? I don't\n> see why it would fail unless there's a bug somewhere, so the start()\n> function should probably return an error. It would simplify the logic,\n> and we could drop the frameStartEnabled() function from patch 1/2.\n\nRather defence programming. I considered coding like this:\n\n \t\tret = eventEmitter->setFrameStartEnabled(true);\n\t\tassert(ret == 0);\n\nbut though is better to handle the error properly, v4l2_event_subscribe()\nitself do kmalloc and there are other paths when subscribe ioctl can fail.\n\nReturn an error from start() instead on fallback to non emitter\ncase makes sense. Will change this way. \n\n\n> > > > +\t\tControlList ctrls(sensorControls);\n> > > > +\t\tsensor_->setControls(&ctrls);\n> > > > +\t}\n> > > > }\n> > > > \n> > > > /* Retrieve all source pads connected to a sink pad through active routes. */\n> > > > @@ -1285,8 +1302,6 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n> > > > \tdata->delayedCtrls_ =\n> > > > \t\tstd::make_unique(data->sensor_->device(),\n> > > > \t\t\t\t\t\t params);\n> \n> I've just noticed that this block of code can be moved to the\n> SimpleCameraData constructor. I've submitted \"[PATCH] pipeline: simple:\n> Create DelayedControls instance once only\" to do so. It can be applied\n> before or after your series. Would you be able to test it ?\n> \n> Update: I've just realized that this will result in multiple connections\n> of the same signal if configure() is called multiple times. We need to\n> first move connection and disconnection of the frameStart signal to\n> start() and stop(), so my patch should go on top of your series.\n\nThere is small conflict, so I think I can post it as part of v5\nto make things easier to apply.\n\n> > > > -\tdata->video_->frameStart.connect(data->delayedCtrls_.get(),\n> > > > -\t\t\t\t\t &DelayedControls::applyControls);\n> \n> And I've also just realized that moving this to start() fixes a\n> use-after-free bug. Kieran found something similar in the rkisp1\n> pipeline handler, which means we need better infrastructure.\n> \n> I've submitted \"[PATCH] libcamera: delayed_controls: Inherit from Object\n> class\" to try and address the problem once and for all. This one should\n> I think be merged first. Would you be able to test that patch as well,\n> without your series applied ?\n\nDone the test of this patch. It does not fixes the \n\"state_ == ProxyRunning\" failed in processStatsThread()\"\nassertion issue. Maybe it fixes something else that I'm not \nencountering ? Or maybe also some other class need to\ninherit from Object ? BTW, why not prohibit signals/slots\nfor classes that are not Object ? \n\n> I'm sorry for the mess this caused, it turns out we have quite a few\n> issues in a single place.\n\nNo problem.\n\nRegards\nStanislaw\n\n\n> > > > \n> > > > \tStreamConfiguration inputCfg;\n> > > > \tinputCfg.pixelFormat = pipeConfig->captureFormat;\n> > > > @@ -1325,6 +1340,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> > > > {\n> > > > \tSimpleCameraData *data = cameraData(camera);\n> > > > \tV4L2VideoDevice *video = data->video_;\n> > > > +\tV4L2Subdevice *eventEmitter = data->eventEmitter_;\n> > > > \tint ret;\n> > > > \n> > > > \tconst MediaPad *pad = acquirePipeline(data);\n> > > > @@ -1354,6 +1370,15 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> > > > \n> > > > \tvideo->bufferReady.connect(data, &SimpleCameraData::imageBufferReady);\n> > > > \n> > > > +\tif (eventEmitter) {\n> > > > +\t\tret = eventEmitter->setFrameStartEnabled(true);\n> > > > +\t\tif (ret == 0)\n> > > > +\t\t\teventEmitter->frameStart.connect(data->delayedCtrls_.get(),\n> > > > +\t\t\t\t\t\t\t &DelayedControls::applyControls);\n> > > > +\t}\n> > > > +\n> > > > +\tdata->delayedCtrls_->reset();\n> > > > +\n> > > > \tret = video->streamOn();\n> > > > \tif (ret < 0) {\n> > > > \t\tstop(camera);\n> > > > @@ -1385,6 +1410,13 @@ void SimplePipelineHandler::stopDevice(Camera *camera)\n> > > > {\n> > > > \tSimpleCameraData *data = cameraData(camera);\n> > > > \tV4L2VideoDevice *video = data->video_;\n> > > > +\tV4L2Subdevice *eventEmitter = data->eventEmitter_;\n> > > > +\n> > > > +\tif (eventEmitter) {\n> > > > +\t\teventEmitter->setFrameStartEnabled(false);\n> > > > +\t\teventEmitter->frameStart.disconnect(data->delayedCtrls_.get(),\n> > > > +\t\t\t\t\t\t &DelayedControls::applyControls);\n> > > > +\t}\n> > > > \n> > > > \tif (data->useConversion_) {\n> > > > \t\tif (data->converter_)\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 27FCAC32A9\n\tfor ;\n\tMon, 24 Feb 2025 11:02:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0FA3A686DE;\n\tMon, 24 Feb 2025 12:02:40 +0100 (CET)","from mgamail.intel.com (mgamail.intel.com [198.175.65.11])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 13D2960327\n\tfor ;\n\tMon, 24 Feb 2025 12:02:36 +0100 (CET)","from orviesa009.jf.intel.com ([10.64.159.149])\n\tby orvoesa103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384;\n\t24 Feb 2025 03:02:36 -0800","from sgruszka-mobl.ger.corp.intel.com (HELO localhost)\n\t([10.245.97.15]) by orviesa009-auth.jf.intel.com with\n\tESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 24 Feb 2025 03:02:34 -0800"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=intel.com header.i=@intel.com\n\theader.b=\"mzEHHFIk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple;\n\td=intel.com; i=@intel.com; q=dns/txt; s=Intel;\n\tt=1740394958; x=1771930958;\n\th=date:from:to:cc:subject:message-id:references:\n\tmime-version:in-reply-to;\n\tbh=dp2EeYwzZbXeQQhzlTX1DL68uCFN1MCyazKPXEPnhRQ=;\n\tb=mzEHHFIkawXEAhAT/KmLM78TOsjQJHUkh6kS2WEcAMKomIglKLsZCU5J\n\tWI8JjoGHwmlkNV1Oh12NNHtFKaNYgcktnqyfjzcqhr+pTIt2Oqq4bE52y\n\t+jX+kd6O8spbK7Ubh5JDZ5RrLNZPLpHe+kI9nm8rvOartbQdIyMGzlpjH\n\tloADMA2kTa5ccTPrlp00SewWRGyDEys9lJLxd5JuZCOBlMbfyALkfsn2D\n\trxu+YGV3X49eSNBng6m5cQZcmI8zQhM9lbHHtS61VSHqOVeTlthzpMjVQ\n\tvclud23+J42R/0S81upXv3GwZIXXolQf6+iA2fy8ZAcbILY1acdNfkR3+ A==;","X-CSE-ConnectionGUID":["VS0W4gugS3+JxRtUT42hQw==","DolVJkoySwOnU5rhvdY0Zw=="],"X-CSE-MsgGUID":["FnFiO7lwSpGk3K3/qVlEqw==","lJ6YNfUER0msDZiLgEl7SA=="],"X-IronPort-AV":["E=McAfee;i=\"6700,10204,11354\"; a=\"51359251\"","E=Sophos;i=\"6.13,309,1732608000\"; d=\"scan'208\";a=\"51359251\"","E=Sophos;i=\"6.13,309,1732608000\"; d=\"scan'208\";a=\"115753413\""],"X-ExtLoop1":"1","Date":"Mon, 24 Feb 2025 12:02:30 +0100","From":"Stanislaw Gruszka ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, Milan Zamazal ,\n\tKieran Bingham ,\n\tNaushir Patuck ,\n\tSakari Ailus ,\n\tHans de Goede ","Subject":"Re: [PATCH v3 2/2] pipeline: simple: Use proper device for frame\n\tstart events","Message-ID":"","References":"<20250218091951.13017-1-stanislaw.gruszka@linux.intel.com>\n\t<20250218091951.13017-3-stanislaw.gruszka@linux.intel.com>\n\t<20250219124231.GA26637@pendragon.ideasonboard.com>\n\t\n\t<20250224013900.GD22240@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20250224013900.GD22240@pendragon.ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33447,"web_url":"https://patchwork.libcamera.org/comment/33447/","msgid":"<20250224122412.GA25447@pendragon.ideasonboard.com>","date":"2025-02-24T12:24:12","subject":"Re: [PATCH v3 2/2] pipeline: simple: Use proper device for frame\n\tstart events","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Feb 24, 2025 at 12:02:30PM +0100, Stanislaw Gruszka wrote:\n> Hi Laurent,\n> \n> thanks for tips/clarifications. Have some more questions, please see below.\n> \n> On Mon, Feb 24, 2025 at 03:39:00AM +0200, Laurent Pinchart wrote:\n> > On Thu, Feb 20, 2025 at 11:10:45AM +0100, Stanislaw Gruszka wrote:\n> > > On Wed, Feb 19, 2025 at 02:42:31PM +0200, Laurent Pinchart wrote:\n> > > > On Tue, Feb 18, 2025 at 10:19:51AM +0100, Stanislaw Gruszka wrote:\n> > > > > Currently we use frame start event from video capture device to\n> > > > > apply controls. But the capture device might not generate the events.\n> > > > > Usually CSI-2 receiver is proper device to subscribe for start\n> > > > > frame events.\n> > > > > \n> > > > > Without DelayedConntrols:applyControls() is possible that we can get\n> > > > \n> > > > s/DelayedConntrols:/DelayedControls::/\n> > > >\n> > > > > call to DelayedControls::get() with frame number that exceed number\n> > > > > of saved entries and get below assertion failure:\n> > > > > \n> > > > > ../src/libcamera/delayed_controls.cpp:227:\n> > > > > libcamera::ControlList libcamera::DelayedControls::get(uint32_t):\n> > > > > Assertion `info.type() != ControlTypeNone' failed\n> > > > > \n> > > > > Assertion failure can happen at the beginning of streaming when\n> > > > > ControlRingBuffer is not yet filled and there are errors on CSI-2.\n> > > > \n> > > > It seems that this patch fixes two issues. One of them is using the\n> > > > proper device to receive frame start events, the other one is related to\n> > > > this assertion failure. I'm not sure to see how this patch fixes the\n> > > > second issue. Could you split it in two, and maybe provide a bit more\n> > > > information about the fix for the assertion failure ?\n> > > \n> > > Not really sure how to split the patch.\n> > \n> > One thing that your patch does is to find the right emitter of the frame\n> > start signal, instead of hardcoding that to data->video_. I think that\n> > could be a patch of its own, it's a worthy fix by itself. Then you have\n> > three other changes in this patch:\n> > \n> > - Moving the connection of the frameStart signal from configure() to\n> > start().\n> > \n> > - Disconnecting the frameStart() signal in stop().\n> > \n> > - Not calling sensor_->setControls() in\n> > SimpleCameraData::setSensorControls() if there's a frame start\n> > emitter.\n> > \n> > Moving the signal connection to start() happens to fix a use-after-free\n> > bug (see below), for which I've sent a fix. I think it's still a good\n> > idea to move the connection to start(), as otherwise the signal would be\n> > connected multiple times if you configure the camera multiple times,\n> > which is not a very good idea. I think that, plus moving disconnection\n> > to stop(), is also a candidate for a standalone patch.\n> > \n> > Finally, the change to SimpleCameraData::setSensorControls() should be a\n> > third patch.\n> \n> Ok, going to split the patches.\n> \n> > > The assertion failure can happen when delayedCtrls_->get(frame)\n> > > give us empty control in processStats:\n> > > \n> > > void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)\n> > > {\n> > > \tswIsp_->processStats(frame, bufferId,\n> > > \t\t\t delayedCtrls_->get(frame));\n> > > }\n> > > \n> > > Below 2 conditions have to be meet to trigger the issue.\n> > > \n> > > 1) we process first 16 frames, otherwise DelayedControls::values_\n> > > ControlRingBuffer is filled - no empty controls there\n> > > \n> > > 2) there must be error on CSI bus and some frames dropped there,\n> > > such index calculated by:\n> > \n> > I expect CSI-2 errors to be extremely rare. Did you actually get some ?\n> > Or were frames dropped by the driver due to buffer queue underruns ?\n> \n> It's quite good reproducible for IPU6 when first start using camera\n> after boot.\n> \n> [ 82.758542] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Payload checksum (CRC) error\n> [ 83.250939] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Single packet header error corrected\n> [ 83.250962] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Multiple packet header errors detected\n> [ 83.250964] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Payload checksum (CRC) error\n> [ 83.250965] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Incomplete long packet detected\n> [ 83.250966] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Frame sync error\n> [ 83.250967] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Inter-frame long packet discarded\n> \n> Not really sure why. After initial errors, things get to\n> work and no other cis2 errors happen.\n\nAnd that's only the first time after boot ? That sounds like a driver\nbug. What happens if you unload and reload the driver, does the first\ncapture session after reloading exhibit the same problems ?\n\n> > > ControlList DelayedControls::get(uint32_t sequence)\n> > > {\n> > > \tunsigned int index = std::max(0, sequence - maxDelay_);\n> > > \n> > > points to entry in ControlRingBuffer not filled previously by:\n> > > \n> > > void SimpleCameraData::setSensorControls(const ControlList &sensorControls)\n> > > {\n> > > \tdelayedCtrls_->push(sensorControls);\n> > > \n> > > For reference, it was discussed here a little:\n> > > https://patchwork.libcamera.org/patch/22210/ \n> > > \n> > > Assertion is consequence of not handling frame start event correctly.\n> > > With the fix, we fill proper entry in ControlRingBuffer, before it's\n> > > read later in processStats(), even if the frame number is not in\n> > > sequence.\n> > > \n> > > > > To fix, loop over devices in the pipeline (starting from the last one),\n> > > > > find one that emits start frame events and connect applyControls()\n> > > > > to it. Additionally remove direct call to sensor_->setControls() if\n> > > > > the emitter device was found.\n> > > > > \n> > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=241\n> > > > > Co-developed-by: Hans de Goede \n> > > > > Signed-off-by: Hans de Goede \n> > > > > Reviewed-by: Hans de Goede # v2\n> > > > > Tested-by: Hans de Goede # Lenovo X1 Yoga IPU6 + ov2740 # v2\n> > > > > Reviewed-by: Milan Zamazal # v2\n> > > > > Signed-off-by: Stanislaw Gruszka \n> > > > > ---\n> > > > > src/libcamera/pipeline/simple/simple.cpp | 40 +++++++++++++++++++++---\n> > > > > 1 file changed, 36 insertions(+), 4 deletions(-)\n> > > > > \n> > > > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > > > > index 6e039bf3..ad2167dc 100644\n> > > > > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > > > > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > > > > @@ -277,6 +277,7 @@ public:\n> > > > > \tstd::list entities_;\n> > > > > \tstd::unique_ptr sensor_;\n> > > > > \tV4L2VideoDevice *video_;\n> > > > > +\tV4L2Subdevice *eventEmitter_;\n> > > > \n> > > > Maybe frameStartEmitter_ to make it move explicit ?\n> > > \n> > > Ok.\n> > > \n> > > > > \tstd::vector configs_;\n> > > > > \tstd::map> formats_;\n> > > > > @@ -579,6 +580,19 @@ int SimpleCameraData::init()\n> > > > > \n> > > > > \tproperties_ = sensor_->properties();\n> > > > > \n> > > > > +\teventEmitter_ = nullptr;\n> > > > > +\tfor (auto it = entities_.rbegin(); it != entities_.rend(); ++it) {\n> > \n> > By the way, you can write this\n> > \n> > \tfor (const Entity &entity : entities_) {\n> > \t\tV4L2Subdevice *sd = pipe->subdev(entity.entity);\n> > \t\t...\n> \n> I wanted to iterate in reverse order. Just in case we have more\n> devices that emits events, so we pick up the last one. \n> Not really sure if this is the issue. Should we expect one \n> event device ? Or if there are more than one such device \n> in the pipeline, it doesn't matter which one we pick up ?\n\nOops, I had missed the rbegin(). This being said, if there are more than\none device that can produce a frame start event, I think we should pick\nthe device closest to the camera sensor, so forward iteration is better.\n\n> > > > > +\t\tV4L2Subdevice *sd = pipe->subdev(it->entity);\n> > > > > +\t\tif (!sd || !sd->supportsFrameStartEvent())\n> > > > > +\t\t\tcontinue;\n> > > > > +\n> > > > > +\t\tLOG(SimplePipeline, Debug)\n> > > > > +\t\t\t<< \"Using \" << it->entity->name() << \" frameStart signal\";\n> > > > > +\n> > > > > +\t\teventEmitter_ = sd;\n> > > > > +\t\tbreak;\n> > > > > +\t}\n> > > > > +\n> > > > > \treturn 0;\n> > > > > }\n> > > > > \n> > > > > @@ -897,8 +911,11 @@ void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)\n> > > > > void SimpleCameraData::setSensorControls(const ControlList &sensorControls)\n> > > > > {\n> > > > > \tdelayedCtrls_->push(sensorControls);\n> > > > > -\tControlList ctrls(sensorControls);\n> > > > > -\tsensor_->setControls(&ctrls);\n> > > > > +\t/* Directly apply controls now if there is no frameStart signal */\n> > > > > +\tif (!eventEmitter_ || !eventEmitter_->frameStartEnabled()) {\n> > > > \n> > > > What's the use case for the second check, when do we have a non-null\n> > > > eventEmitter_ that doesn't emit frame start events ?\n> > > \n> > > This is for case (below in this patch) when in SimplePipelineHandler::start()\n> > > \n> > > \t\tret = eventEmitter->setFrameStartEnabled(true);\n> > > \n> > > fails for some reason, even if previous ioctl() call to detect\n> > > eventEmitter was successful.\n> > \n> > Is that something you've experienced, or defensive programming ? I don't\n> > see why it would fail unless there's a bug somewhere, so the start()\n> > function should probably return an error. It would simplify the logic,\n> > and we could drop the frameStartEnabled() function from patch 1/2.\n> \n> Rather defence programming. I considered coding like this:\n> \n> \t\tret = eventEmitter->setFrameStartEnabled(true);\n> \t\tassert(ret == 0);\n> \n> but though is better to handle the error properly, v4l2_event_subscribe()\n> itself do kmalloc and there are other paths when subscribe ioctl can fail.\n> \n> Return an error from start() instead on fallback to non emitter\n> case makes sense. Will change this way. \n\nThanks.\n\n> > > > > +\t\tControlList ctrls(sensorControls);\n> > > > > +\t\tsensor_->setControls(&ctrls);\n> > > > > +\t}\n> > > > > }\n> > > > > \n> > > > > /* Retrieve all source pads connected to a sink pad through active routes. */\n> > > > > @@ -1285,8 +1302,6 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n> > > > > \tdata->delayedCtrls_ =\n> > > > > \t\tstd::make_unique(data->sensor_->device(),\n> > > > > \t\t\t\t\t\t params);\n> > \n> > I've just noticed that this block of code can be moved to the\n> > SimpleCameraData constructor. I've submitted \"[PATCH] pipeline: simple:\n> > Create DelayedControls instance once only\" to do so. It can be applied\n> > before or after your series. Would you be able to test it ?\n> > \n> > Update: I've just realized that this will result in multiple connections\n> > of the same signal if configure() is called multiple times. We need to\n> > first move connection and disconnection of the frameStart signal to\n> > start() and stop(), so my patch should go on top of your series.\n> \n> There is small conflict, so I think I can post it as part of v5\n> to make things easier to apply.\n> \n> > > > > -\tdata->video_->frameStart.connect(data->delayedCtrls_.get(),\n> > > > > -\t\t\t\t\t &DelayedControls::applyControls);\n> > \n> > And I've also just realized that moving this to start() fixes a\n> > use-after-free bug. Kieran found something similar in the rkisp1\n> > pipeline handler, which means we need better infrastructure.\n> > \n> > I've submitted \"[PATCH] libcamera: delayed_controls: Inherit from Object\n> > class\" to try and address the problem once and for all. This one should\n> > I think be merged first. Would you be able to test that patch as well,\n> > without your series applied ?\n> \n> Done the test of this patch. It does not fixes the \n> \"state_ == ProxyRunning\" failed in processStatsThread()\"\n> assertion issue. Maybe it fixes something else that I'm not \n> encountering ? Or maybe also some other class need to\n> inherit from Object ?\n\nI'll reply to this in the context of the other patch.\n\n> BTW, why not prohibit signals/slots for classes that are not Object ? \n\nI'm considering that, not doing so is causing issues. I'll give it a try\nto see how intrusive the change would be.\n\n> > I'm sorry for the mess this caused, it turns out we have quite a few\n> > issues in a single place.\n> \n> No problem.\n> \n> > > > > \n> > > > > \tStreamConfiguration inputCfg;\n> > > > > \tinputCfg.pixelFormat = pipeConfig->captureFormat;\n> > > > > @@ -1325,6 +1340,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> > > > > {\n> > > > > \tSimpleCameraData *data = cameraData(camera);\n> > > > > \tV4L2VideoDevice *video = data->video_;\n> > > > > +\tV4L2Subdevice *eventEmitter = data->eventEmitter_;\n> > > > > \tint ret;\n> > > > > \n> > > > > \tconst MediaPad *pad = acquirePipeline(data);\n> > > > > @@ -1354,6 +1370,15 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> > > > > \n> > > > > \tvideo->bufferReady.connect(data, &SimpleCameraData::imageBufferReady);\n> > > > > \n> > > > > +\tif (eventEmitter) {\n> > > > > +\t\tret = eventEmitter->setFrameStartEnabled(true);\n> > > > > +\t\tif (ret == 0)\n> > > > > +\t\t\teventEmitter->frameStart.connect(data->delayedCtrls_.get(),\n> > > > > +\t\t\t\t\t\t\t &DelayedControls::applyControls);\n> > > > > +\t}\n> > > > > +\n> > > > > +\tdata->delayedCtrls_->reset();\n> > > > > +\n> > > > > \tret = video->streamOn();\n> > > > > \tif (ret < 0) {\n> > > > > \t\tstop(camera);\n> > > > > @@ -1385,6 +1410,13 @@ void SimplePipelineHandler::stopDevice(Camera *camera)\n> > > > > {\n> > > > > \tSimpleCameraData *data = cameraData(camera);\n> > > > > \tV4L2VideoDevice *video = data->video_;\n> > > > > +\tV4L2Subdevice *eventEmitter = data->eventEmitter_;\n> > > > > +\n> > > > > +\tif (eventEmitter) {\n> > > > > +\t\teventEmitter->setFrameStartEnabled(false);\n> > > > > +\t\teventEmitter->frameStart.disconnect(data->delayedCtrls_.get(),\n> > > > > +\t\t\t\t\t\t &DelayedControls::applyControls);\n> > > > > +\t}\n> > > > > \n> > > > > \tif (data->useConversion_) {\n> > > > > \t\tif (data->converter_)","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A3A75C32A9\n\tfor ;\n\tMon, 24 Feb 2025 12:24:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9749F686ED;\n\tMon, 24 Feb 2025 13:24:31 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7D4A961856\n\tfor ;\n\tMon, 24 Feb 2025 13:24:30 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BC4AE220;\n\tMon, 24 Feb 2025 13:23:03 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"NVem40Zb\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1740399783;\n\tbh=6/8StO6R9skUSAd5IbyVPFVuCJ1YFPagxC/RinzcXFc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NVem40ZbH2b/TbkGKUSOxnaYtDFgXLJrGXzJ47E5PUB1KbMhfDkUnpUgkpDINsa9z\n\tDfMUgsJvXbuyFFFgsjvF6aPkX5iCRLtQWCdBWadMZ9lNyWtQjiuTuaitvtUa+/C1ll\n\tqLiXXVJdCOmvEtn9HLLfSqa0ZirVewWWEG8izZGo=","Date":"Mon, 24 Feb 2025 14:24:12 +0200","From":"Laurent Pinchart ","To":"Stanislaw Gruszka ","Cc":"libcamera-devel@lists.libcamera.org, Milan Zamazal ,\n\tKieran Bingham ,\n\tNaushir Patuck ,\n\tSakari Ailus ,\n\tHans de Goede ","Subject":"Re: [PATCH v3 2/2] pipeline: simple: Use proper device for frame\n\tstart events","Message-ID":"<20250224122412.GA25447@pendragon.ideasonboard.com>","References":"<20250218091951.13017-1-stanislaw.gruszka@linux.intel.com>\n\t<20250218091951.13017-3-stanislaw.gruszka@linux.intel.com>\n\t<20250219124231.GA26637@pendragon.ideasonboard.com>\n\t\n\t<20250224013900.GD22240@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33450,"web_url":"https://patchwork.libcamera.org/comment/33450/","msgid":"","date":"2025-02-24T13:29:52","subject":"Re: [PATCH v3 2/2] pipeline: simple: Use proper device for frame\n\tstart events","submitter":{"id":211,"url":"https://patchwork.libcamera.org/api/people/211/","name":"Stanislaw Gruszka","email":"stanislaw.gruszka@linux.intel.com"},"content":"On Mon, Feb 24, 2025 at 02:24:12PM +0200, Laurent Pinchart wrote:\n> On Mon, Feb 24, 2025 at 12:02:30PM +0100, Stanislaw Gruszka wrote:\n > > \n> > > I expect CSI-2 errors to be extremely rare. Did you actually get some ?\n> > > Or were frames dropped by the driver due to buffer queue underruns ?\n> > \n> > It's quite good reproducible for IPU6 when first start using camera\n> > after boot.\n> > \n> > [ 82.758542] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Payload checksum (CRC) error\n> > [ 83.250939] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Single packet header error corrected\n> > [ 83.250962] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Multiple packet header errors detected\n> > [ 83.250964] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Payload checksum (CRC) error\n> > [ 83.250965] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Incomplete long packet detected\n> > [ 83.250966] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Frame sync error\n> > [ 83.250967] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Inter-frame long packet discarded\n> > \n> > Not really sure why. After initial errors, things get to\n> > work and no other cis2 errors happen.\n> \n> And that's only the first time after boot ? That sounds like a driver\n> bug. What happens if you unload and reload the driver, does the first\n> capture session after reloading exhibit the same problems ?\n\nReloading intel-ipu6-isys does not exhibit the problem.\nHowever reloading the sensor driver ov2740 does.\nIs sufficient to reload ov2740 module and start the qcam\nto get those csi2-1 errors.\n\nRegards\nStanislaw","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 905DEC324E\n\tfor ;\n\tMon, 24 Feb 2025 13:30:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B612F686ED;\n\tMon, 24 Feb 2025 14:30:00 +0100 (CET)","from mgamail.intel.com (mgamail.intel.com [192.198.163.12])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9E01561856\n\tfor ;\n\tMon, 24 Feb 2025 14:29:58 +0100 (CET)","from fmviesa004.fm.intel.com ([10.60.135.144])\n\tby fmvoesa106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384;\n\t24 Feb 2025 05:29:57 -0800","from sgruszka-mobl.ger.corp.intel.com (HELO localhost)\n\t([10.245.97.15]) by fmviesa004-auth.fm.intel.com with\n\tESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 24 Feb 2025 05:29:55 -0800"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=intel.com header.i=@intel.com\n\theader.b=\"WhCix63y\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple;\n\td=intel.com; i=@intel.com; q=dns/txt; s=Intel;\n\tt=1740403798; x=1771939798;\n\th=date:from:to:cc:subject:message-id:references:\n\tmime-version:in-reply-to;\n\tbh=ESc3bQ9OetbxQBKuZpACJpmkDVpgGCDkhek72urfMec=;\n\tb=WhCix63yPQrKGMArpuz9rjJDsKkFpJFluSStwG8+r1Xmda9w/R/Ev5ej\n\tGclTw0uQDfrypIAzhSOmkbTkjalqvkkDgx23SBzT3fvNt69jrxD0XywyQ\n\tKnlnR8ZptBDK/88iexNf1A3XpeLf1fymsaFTURjErdBiHE+f677UmN7da\n\tHDga0EyYVOU6cBESHaC9efsuOPHWosP93FRa53XaJKdlLPXXFj/q8JCQc\n\tYwPtZqwzgKw3SKW+zim0jgxu202pRUapM/ELeLRr1EpQUwMO1RNb9o7xo\n\tqmWqeeo9nJn9MNso7Gaw4/tfwgjzJM091qmKBtS2mV9CsR+3Ilp7X/1yI A==;","X-CSE-ConnectionGUID":["YWx6Ng8CRue1aWh9fy0hHQ==","7Y7J4xOJTIaFlia2J7zO6w=="],"X-CSE-MsgGUID":["I0reePhXSdOJrVg1qktHfQ==","hbgXYz0TRB6T1lxFcgZwbw=="],"X-IronPort-AV":["E=McAfee;i=\"6700,10204,11355\"; a=\"45065051\"","E=Sophos;i=\"6.13,309,1732608000\"; d=\"scan'208\";a=\"45065051\"","E=Sophos;i=\"6.13,309,1732608000\"; d=\"scan'208\";a=\"121154755\""],"X-ExtLoop1":"1","Date":"Mon, 24 Feb 2025 14:29:52 +0100","From":"Stanislaw Gruszka ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, Milan Zamazal ,\n\tKieran Bingham ,\n\tNaushir Patuck ,\n\tSakari Ailus ,\n\tHans de Goede ","Subject":"Re: [PATCH v3 2/2] pipeline: simple: Use proper device for frame\n\tstart events","Message-ID":"","References":"<20250218091951.13017-1-stanislaw.gruszka@linux.intel.com>\n\t<20250218091951.13017-3-stanislaw.gruszka@linux.intel.com>\n\t<20250219124231.GA26637@pendragon.ideasonboard.com>\n\t\n\t<20250224013900.GD22240@pendragon.ideasonboard.com>\n\t\n\t<20250224122412.GA25447@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20250224122412.GA25447@pendragon.ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33468,"web_url":"https://patchwork.libcamera.org/comment/33468/","msgid":"<20250224211413.GN6778@pendragon.ideasonboard.com>","date":"2025-02-24T21:14:13","subject":"Re: [PATCH v3 2/2] pipeline: simple: Use proper device for frame\n\tstart events","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"(CC'ing Bingbu)\n\nOn Mon, Feb 24, 2025 at 02:29:52PM +0100, Stanislaw Gruszka wrote:\n> On Mon, Feb 24, 2025 at 02:24:12PM +0200, Laurent Pinchart wrote:\n> > On Mon, Feb 24, 2025 at 12:02:30PM +0100, Stanislaw Gruszka wrote:\n> > > \n> > > > I expect CSI-2 errors to be extremely rare. Did you actually get some ?\n> > > > Or were frames dropped by the driver due to buffer queue underruns ?\n> > > \n> > > It's quite good reproducible for IPU6 when first start using camera\n> > > after boot.\n> > > \n> > > [ 82.758542] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Payload checksum (CRC) error\n> > > [ 83.250939] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Single packet header error corrected\n> > > [ 83.250962] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Multiple packet header errors detected\n> > > [ 83.250964] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Payload checksum (CRC) error\n> > > [ 83.250965] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Incomplete long packet detected\n> > > [ 83.250966] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Frame sync error\n> > > [ 83.250967] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Inter-frame long packet discarded\n> > > \n> > > Not really sure why. After initial errors, things get to\n> > > work and no other cis2 errors happen.\n> > \n> > And that's only the first time after boot ? That sounds like a driver\n> > bug. What happens if you unload and reload the driver, does the first\n> > capture session after reloading exhibit the same problems ?\n> \n> Reloading intel-ipu6-isys does not exhibit the problem.\n> However reloading the sensor driver ov2740 does.\n> Is sufficient to reload ov2740 module and start the qcam\n> to get those csi2-1 errors.\n\nBingbu, Sakari, this sounds like a bug in a driver somewhere.","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 93840C32A9\n\tfor ;\n\tMon, 24 Feb 2025 21:14:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B0BB66870E;\n\tMon, 24 Feb 2025 22:14:32 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C497B686FF\n\tfor ;\n\tMon, 24 Feb 2025 22:14:31 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id ABAEF220;\n\tMon, 24 Feb 2025 22:13:04 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"WP3ynTu0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1740431584;\n\tbh=5Cf0dEWbXOP3RbCg91ZEsy+0AS01Hnizekho+6O6760=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WP3ynTu0CC4VoEs+q4CaCfWtkSltV8MCeklbNeGKkkNKExhZHlQegLl47wjyEfcdE\n\t92OrLMxAoYrHiWsEFLK5A9e/kPxjjCeybq+6cUMlfKBg+0seYMHZpLghUx7CCA6Gl4\n\tkFZyx3nZ79q4nCFYRK7MKk3gTb/TwXwaUp+RYnPg=","Date":"Mon, 24 Feb 2025 23:14:13 +0200","From":"Laurent Pinchart ","To":"Stanislaw Gruszka ","Cc":"libcamera-devel@lists.libcamera.org, Milan Zamazal ,\n\tKieran Bingham ,\n\tNaushir Patuck ,\n\tSakari Ailus ,\n\tHans de Goede , Bingbu Cao ","Subject":"Re: [PATCH v3 2/2] pipeline: simple: Use proper device for frame\n\tstart events","Message-ID":"<20250224211413.GN6778@pendragon.ideasonboard.com>","References":"<20250218091951.13017-1-stanislaw.gruszka@linux.intel.com>\n\t<20250218091951.13017-3-stanislaw.gruszka@linux.intel.com>\n\t<20250219124231.GA26637@pendragon.ideasonboard.com>\n\t\n\t<20250224013900.GD22240@pendragon.ideasonboard.com>\n\t\n\t<20250224122412.GA25447@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33472,"web_url":"https://patchwork.libcamera.org/comment/33472/","msgid":"","date":"2025-02-25T06:05:38","subject":"Re: [PATCH v3 2/2] pipeline: simple: Use proper device for frame\n\tstart events","submitter":{"id":92,"url":"https://patchwork.libcamera.org/api/people/92/","name":"Bingbu Cao","email":"bingbu.cao@linux.intel.com"},"content":"On 2/25/25 5:14 AM, Laurent Pinchart wrote:\n> (CC'ing Bingbu)\n> \n> On Mon, Feb 24, 2025 at 02:29:52PM +0100, Stanislaw Gruszka wrote:\n>> On Mon, Feb 24, 2025 at 02:24:12PM +0200, Laurent Pinchart wrote:\n>>> On Mon, Feb 24, 2025 at 12:02:30PM +0100, Stanislaw Gruszka wrote:\n>>>>\n>>>>> I expect CSI-2 errors to be extremely rare. Did you actually get some ?\n>>>>> Or were frames dropped by the driver due to buffer queue underruns ?\n>>>>\n>>>> It's quite good reproducible for IPU6 when first start using camera\n>>>> after boot.\n>>>>\n>>>> [ 82.758542] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Payload checksum (CRC) error\n>>>> [ 83.250939] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Single packet header error corrected\n>>>> [ 83.250962] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Multiple packet header errors detected\n>>>> [ 83.250964] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Payload checksum (CRC) error\n>>>> [ 83.250965] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Incomplete long packet detected\n>>>> [ 83.250966] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Frame sync error\n>>>> [ 83.250967] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Inter-frame long packet discarded\n>>>>\n>>>> Not really sure why. After initial errors, things get to\n>>>> work and no other cis2 errors happen.\n>>>\n>>> And that's only the first time after boot ? That sounds like a driver\n>>> bug. What happens if you unload and reload the driver, does the first\n>>> capture session after reloading exhibit the same problems ?\n>>\n>> Reloading intel-ipu6-isys does not exhibit the problem.\n>> However reloading the sensor driver ov2740 does.\n>> Is sufficient to reload ov2740 module and start the qcam\n>> to get those csi2-1 errors.\n> \n> Bingbu, Sakari, this sounds like a bug in a driver somewhere.\n>\n\nStanislaw,\n\nWhich device are you using? I can try it if I have such device.\n\nHave you a chance to check whether you can reproduce same problem with\nother camera sensors (or other devices)?\n\nIn my experience, such errors report likely caused by bad frames from\nsensor in corner case.","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 63B54BE173\n\tfor ;\n\tTue, 25 Feb 2025 06:10:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8DDFB68711;\n\tTue, 25 Feb 2025 07:10:50 +0100 (CET)","from mgamail.intel.com (mgamail.intel.com [192.198.163.7])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 839E561853\n\tfor ;\n\tTue, 25 Feb 2025 07:10:48 +0100 (CET)","from fmviesa001.fm.intel.com ([10.60.135.141])\n\tby fmvoesa101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384;\n\t24 Feb 2025 22:10:45 -0800","from ipu5-build.bj.intel.com (HELO [10.238.232.136])\n\t([10.238.232.136])\n\tby smtpauth.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384;\n\t24 Feb 2025 22:10:43 -0800"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=intel.com header.i=@intel.com\n\theader.b=\"fY10+rFa\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple;\n\td=intel.com; i=@intel.com; q=dns/txt; s=Intel;\n\tt=1740463849; x=1771999849;\n\th=subject:to:cc:references:from:message-id:date:\n\tmime-version:in-reply-to:content-transfer-encoding;\n\tbh=teUKebjPbC53eSHDO3JLMtSm7mcEnjRwUpNK0HHi6sc=;\n\tb=fY10+rFaHDlcYDBjPJVr20WBefCwpl+6+IQ9ou1G3oAk5w3iBQ68rPMZ\n\tXRwW6pX8w4xrB+0zjCiNG7MfQewTw1eimfdWFwZsAZowoCssWZKP6tAKt\n\tEBp6tG179UxGhMcr621GFUn/BvcpaKxEylOnbsBG7YMSdkaPkV18fTg1G\n\tgsGL6aaHa881QadUGeoXxHn8V92cR0pb6Ge498DEuwWlQ8Jpo31pKmwb8\n\tU6Mo3ke7uw5fB0a8Kxa7qgu3fTaH379PD5YrtokSxIZchG4gkEnikU82S\n\tJxv/84YqeAk3lal/W9a7TaGustqk6V6860sYu94R6Wzob4Aw4N1B033j5 w==;","X-CSE-ConnectionGUID":["rstTXvLaTrq+J5BYCXQbPA==","8mkhvF7aRNOtAfpzDgPGeA=="],"X-CSE-MsgGUID":["9RrlU08DQH2OAMEnxKWb1w==","NUfA9IMbTfOATJ0Ijufqvg=="],"X-IronPort-AV":["E=McAfee;i=\"6700,10204,11355\"; a=\"66623432\"","E=Sophos;i=\"6.13,313,1732608000\"; d=\"scan'208\";a=\"66623432\"","E=Sophos;i=\"6.12,224,1728975600\"; d=\"scan'208\";a=\"147173250\""],"X-ExtLoop1":"1","Subject":"Re: [PATCH v3 2/2] pipeline: simple: Use proper device for frame\n\tstart events","To":"Laurent Pinchart ,\n\tStanislaw Gruszka ","Cc":"libcamera-devel@lists.libcamera.org, Milan Zamazal ,\n\tKieran Bingham ,\n\tNaushir Patuck ,\n\tSakari Ailus ,\n\tHans de Goede , Bingbu Cao ","References":"<20250218091951.13017-1-stanislaw.gruszka@linux.intel.com>\n\t<20250218091951.13017-3-stanislaw.gruszka@linux.intel.com>\n\t<20250219124231.GA26637@pendragon.ideasonboard.com>\n\t\n\t<20250224013900.GD22240@pendragon.ideasonboard.com>\n\t\n\t<20250224122412.GA25447@pendragon.ideasonboard.com>\n\t\n\t<20250224211413.GN6778@pendragon.ideasonboard.com>","From":"Bingbu Cao ","Message-ID":"","Date":"Tue, 25 Feb 2025 14:05:38 +0800","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20250224211413.GN6778@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=windows-1252","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33495,"web_url":"https://patchwork.libcamera.org/comment/33495/","msgid":"","date":"2025-02-26T08:49:23","subject":"Re: [PATCH v3 2/2] pipeline: simple: Use proper device for frame\n\tstart events","submitter":{"id":211,"url":"https://patchwork.libcamera.org/api/people/211/","name":"Stanislaw Gruszka","email":"stanislaw.gruszka@linux.intel.com"},"content":"Hi Bingbu,\n\nOn Tue, Feb 25, 2025 at 02:05:38PM +0800, Bingbu Cao wrote:\n> \n> On 2/25/25 5:14 AM, Laurent Pinchart wrote:\n> > (CC'ing Bingbu)\n> > \n> > On Mon, Feb 24, 2025 at 02:29:52PM +0100, Stanislaw Gruszka wrote:\n> >> On Mon, Feb 24, 2025 at 02:24:12PM +0200, Laurent Pinchart wrote:\n> >>> On Mon, Feb 24, 2025 at 12:02:30PM +0100, Stanislaw Gruszka wrote:\n> >>>>\n> >>>>> I expect CSI-2 errors to be extremely rare. Did you actually get some ?\n> >>>>> Or were frames dropped by the driver due to buffer queue underruns ?\n> >>>>\n> >>>> It's quite good reproducible for IPU6 when first start using camera\n> >>>> after boot.\n> >>>>\n> >>>> [ 82.758542] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Payload checksum (CRC) error\n> >>>> [ 83.250939] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Single packet header error corrected\n> >>>> [ 83.250962] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Multiple packet header errors detected\n> >>>> [ 83.250964] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Payload checksum (CRC) error\n> >>>> [ 83.250965] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Incomplete long packet detected\n> >>>> [ 83.250966] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Frame sync error\n> >>>> [ 83.250967] intel_ipu6_isys.isys intel_ipu6.isys.40: csi2-1 error: Inter-frame long packet discarded\n> >>>>\n> >>>> Not really sure why. After initial errors, things get to\n> >>>> work and no other cis2 errors happen.\n> >>>\n> >>> And that's only the first time after boot ? That sounds like a driver\n> >>> bug. What happens if you unload and reload the driver, does the first\n> >>> capture session after reloading exhibit the same problems ?\n> >>\n> >> Reloading intel-ipu6-isys does not exhibit the problem.\n> >> However reloading the sensor driver ov2740 does.\n> >> Is sufficient to reload ov2740 module and start the qcam\n> >> to get those csi2-1 errors.\n> > \n> > Bingbu, Sakari, this sounds like a bug in a driver somewhere.\n> >\n> \n> Stanislaw,\n> \n> Which device are you using? I can try it if I have such device.\n\nLenovo X1 ThinkPad Yoga and Nano.\n\n> Have you a chance to check whether you can reproduce same problem with\n> other camera sensors (or other devices)?\n>\n> In my experience, such errors report likely caused by bad frames from\n> sensor in corner case.\n\nFWICT the issue happens only with ov2740 on Lenovo laptops.\nI did not recall having those errors on Dell laptops with\ndifferent sensors and iVSC, which I also used with ipu6-isys.\n\nRegards\nStanislaw","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BAAC0C324C\n\tfor ;\n\tWed, 26 Feb 2025 08:49:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8BDC768742;\n\tWed, 26 Feb 2025 09:49:32 +0100 (CET)","from mgamail.intel.com (mgamail.intel.com [198.175.65.12])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 15A1C60322\n\tfor ;\n\tWed, 26 Feb 2025 09:49:29 +0100 (CET)","from orviesa003.jf.intel.com ([10.64.159.143])\n\tby orvoesa104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384;\n\t26 Feb 2025 00:49:29 -0800","from sgruszka-mobl.ger.corp.intel.com (HELO localhost)\n\t([10.246.21.136]) by ORVIESA003-auth.jf.intel.com with\n\tESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 26 Feb 2025 00:49:25 -0800"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=intel.com header.i=@intel.com\n\theader.b=\"cdAs0mTp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple;\n\td=intel.com; i=@intel.com; q=dns/txt; s=Intel;\n\tt=1740559771; x=1772095771;\n\th=date:from:to:cc:subject:message-id:references:\n\tmime-version:in-reply-to;\n\tbh=fdY4SW+w/nhXUHC/kAJs0noXUWq2ZEU/cNEGDXNU+7c=;\n\tb=cdAs0mTpP6K5XuQHOcBUsHU6hxLSz+h0wPQNFmIv1U6QrIf744JLKIpW\n\tECk8vxovK++K56Q3VxW59odg+kfiGXZGsugi/dFvFuaVOCV2rQNq6wCN7\n\tMjVUT/2u9adD8Kr45rrWpN6dQoU8dXhcWj8p2dAk1nG9iE61XUYgZ8ErU\n\tk6exjYW9h7H/JYCjzB3okGtZBc2/wIiB4TlwGbyiuOXyM5UZSpWm6fJCB\n\tcgoTnlZTAHHMZQHK9wEQ1QxW/Ke3seZAy2yOpVtomYTF8OUxu6xHCjhrR\n\tNHiCdRIO+rTnK11DdSGIMZKJM4YYbE+XomDHVT7HIe4zFOSKDq/Uvi9aR g==;","X-CSE-ConnectionGUID":["lokAwyTZSiy4Kae1TaEC7Q==","yFPdso8DQnaoGtYbi2vdnw=="],"X-CSE-MsgGUID":["6Qda9q0IQimQB4x96swWeQ==","0apLiR1zRnOJ1Ub948/laQ=="],"X-IronPort-AV":["E=McAfee;i=\"6700,10204,11356\"; a=\"52789970\"","E=Sophos;i=\"6.13,316,1732608000\"; d=\"scan'208\";a=\"52789970\"","E=Sophos;i=\"6.13,316,1732608000\"; d=\"scan'208\";a=\"121576564\""],"X-ExtLoop1":"1","Date":"Wed, 26 Feb 2025 09:49:23 +0100","From":"Stanislaw Gruszka ","To":"Bingbu Cao ","Cc":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org,\n\tMilan Zamazal , \n\tKieran Bingham ,\n\tNaushir Patuck ,\n\tSakari Ailus ,\n\tHans de Goede , Bingbu Cao ","Subject":"Re: [PATCH v3 2/2] pipeline: simple: Use proper device for frame\n\tstart events","Message-ID":"","References":"<20250218091951.13017-1-stanislaw.gruszka@linux.intel.com>\n\t<20250218091951.13017-3-stanislaw.gruszka@linux.intel.com>\n\t<20250219124231.GA26637@pendragon.ideasonboard.com>\n\t\n\t<20250224013900.GD22240@pendragon.ideasonboard.com>\n\t\n\t<20250224122412.GA25447@pendragon.ideasonboard.com>\n\t\n\t<20250224211413.GN6778@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]