[{"id":32664,"web_url":"https://patchwork.libcamera.org/comment/32664/","msgid":"<173387039086.1401494.2029006348016958056@ping.linuxembedded.co.uk>","date":"2024-12-10T22:39:50","subject":"Re: [RFC] delayed_controls: avoid reading undefined control value","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stanislaw Gruszka (2024-12-06 11:27:43)\n> Limit ControlRingBuffer index by number of queued entries in order do\n> avoid reading undefined control value with type ControlTypeNone .\n> \n> It can happen at the begging of streaming when ControlRingBuffer is\n\ns/begging/beginning/\n\n> not yet filled and there are errors on CSI-2 bus resulting dropping\n> frames. Then we can call to DelayedControls::get() with frame number\n> that exceed number 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> Bug: https://bugs.libcamera.org/show_bug.cgi?id=241\n> Signed-off-by: Stanislaw Gruszka \n> ---\n> Notes: When debugging this I notice that the push() and get() are\n> done in different threads. Maybe mutex should be\n> used? Not sure how synchronization works for mojom signals\n> handlers.\n\nI thought they would be in the same thread as the CameraManager event\nloop... Perhaps we could/should have an assertion that the thread is\nthe same - otherwise - yes it would probably need a lock if there's a\nreader and a writer on both ends of the queue.\n\nI'd double check the threading first.\n\n> Also I'm not sure if similar change like in this patch\n> is not needed for rpi DelayedControls\n\nProbably. There's very little different between the two implementations,\nso we should probably keep them aligned with bugfixes at least until\nsomeone tries to realign them both again.\n\nIt looks like the cookie feature was easier to fork than to add, I can't\nremember all the details ;_(\n\n\n> Alternative fix would be to pre-fill ControlRingBuffer with\n> valid control values. Should we care about possibility\n> of queueCount_ overflowing ? \n\nI'm not sure what 'valid' control values would be ? I think just\nclamping to the most recent data is probably the best we can do ? And 'I\nthink' would reflect the most up to date information we have about\nwhat's configured on the sensor - so I think it's still even the 'most\ncorrect' (and probably even 'is' correct) data we can return in the\nget() call in this event.\n\n\nI think I can already throw this on here, it seems like a sane input\nparameter validation:\n\n\nReviewed-by: Kieran Bingham \n\n> \n> src/libcamera/delayed_controls.cpp | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\n> index 94d0a575..78b0b8f7 100644\n> --- a/src/libcamera/delayed_controls.cpp\n> +++ b/src/libcamera/delayed_controls.cpp\n> @@ -202,7 +202,7 @@ bool DelayedControls::push(const ControlList &controls)\n> */\n> ControlList DelayedControls::get(uint32_t sequence)\n> {\n> - unsigned int index = std::max(0, sequence - maxDelay_);\n> + unsigned int index = std::clamp(sequence - maxDelay_, 0, queueCount_ - 1);\n> \n> ControlList out(device_->controls());\n> for (const auto &ctrl : values_) {\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 C0234C32DE\n\tfor ;\n\tTue, 10 Dec 2024 22:39:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C9F9667E95;\n\tTue, 10 Dec 2024 23:39:54 +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 D6248618AF\n\tfor ;\n\tTue, 10 Dec 2024 23:39:53 +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 3358B496;\n\tTue, 10 Dec 2024 23:39:21 +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=\"Hiycf8eh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733870361;\n\tbh=c5evBsag9k1WfCVBM2nnGbKNo1iR9YK6DKCHmi58wQU=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=Hiycf8ehvOV3cJmZA2PbEUmkkBX3nuLoiZfrvsKke9g82aWP/uxm89IG9WsMWNWr/\n\tmtOs2LTSY5zI0c63J/xxels/HFKgR8hM7hLRCYiuehZ5DeIuGS2Yqj6azU/fKQPU0e\n\tM2AZJA8Ww3PhRXVL0cqqWCjiMMYZlZU0chIb0kMI=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20241206112743.95435-1-stanislaw.gruszka@linux.intel.com>","References":"<20241206112743.95435-1-stanislaw.gruszka@linux.intel.com>","Subject":"Re: [RFC] delayed_controls: avoid reading undefined control value","From":"Kieran Bingham ","Cc":"Milan Zamazal ","To":"Stanislaw Gruszka ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 10 Dec 2024 22:39:50 +0000","Message-ID":"<173387039086.1401494.2029006348016958056@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":32666,"web_url":"https://patchwork.libcamera.org/comment/32666/","msgid":"<20241211074329.GA24546@pendragon.ideasonboard.com>","date":"2024-12-11T07:43:29","subject":"Re: [RFC] delayed_controls: avoid reading undefined control value","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"(CC'ing Naush for the question related to the Raspberry Pi\nimplementation)\n\nOn Tue, Dec 10, 2024 at 10:39:50PM +0000, Kieran Bingham wrote:\n> Quoting Stanislaw Gruszka (2024-12-06 11:27:43)\n> > Limit ControlRingBuffer index by number of queued entries in order do\n> > avoid reading undefined control value with type ControlTypeNone .\n> > \n> > It can happen at the begging of streaming when ControlRingBuffer is\n> \n> s/begging/beginning/\n> \n> > not yet filled and there are errors on CSI-2 bus resulting dropping\n> > frames. Then we can call to DelayedControls::get() with frame number\n> > that exceed number 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> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=241\n> > Signed-off-by: Stanislaw Gruszka \n> > ---\n> > Notes: When debugging this I notice that the push() and get() are\n> > done in different threads. Maybe mutex should be\n> > used? Not sure how synchronization works for mojom signals\n> > handlers.\n\nThe comment related to the SoftwareISP::inputBufferReady signal, in the\nSimpleCameraData::init() function, possibly applies to other signals of\nthe SoftwareISP class, although I wouldn't expect it would affect the\nsetSensorControls signal. Stanislaw, could you provide more information\nabout which threads the push and get functions are called from ?\nEverything should be called from the camera manager thread.\n\nLooking at this more deeply, I think the outputBufferReady and\nstatsReady signals may be mishandled, as they seem to be emitted from\nthe soft ISP worker thread but not treated the same way as the\ninputBufferReady signal. We shouldn't have to deal with it when\nconnecting the signals in the user, the SoftwareISP class should handle\nthread crossing, and emit all its signals from the camera manager\nthread. Milan, is this something you could address ?\n\n> I thought they would be in the same thread as the CameraManager event\n> loop... Perhaps we could/should have an assertion that the thread is\n> the same - otherwise - yes it would probably need a lock if there's a\n> reader and a writer on both ends of the queue.\n> \n> I'd double check the threading first.\n> \n> > Also I'm not sure if similar change like in this patch\n> > is not needed for rpi DelayedControls\n> \n> Probably. There's very little different between the two implementations,\n> so we should probably keep them aligned with bugfixes at least until\n> someone tries to realign them both again.\n\nNaush, could you check this ?\n\n> It looks like the cookie feature was easier to fork than to add, I can't\n> remember all the details ;_(\n> \n> \n> > Alternative fix would be to pre-fill ControlRingBuffer with\n> > valid control values. Should we care about possibility\n> > of queueCount_ overflowing ? \n> \n> I'm not sure what 'valid' control values would be ? I think just\n> clamping to the most recent data is probably the best we can do ? And 'I\n> think' would reflect the most up to date information we have about\n> what's configured on the sensor - so I think it's still even the 'most\n> correct' (and probably even 'is' correct) data we can return in the\n> get() call in this event.\n> \n> \n> I think I can already throw this on here, it seems like a sane input\n> parameter validation:\n> \n> \n> Reviewed-by: Kieran Bingham \n> \n> > src/libcamera/delayed_controls.cpp | 2 +-\n> > 1 file changed, 1 insertion(+), 1 deletion(-)\n> > \n> > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\n> > index 94d0a575..78b0b8f7 100644\n> > --- a/src/libcamera/delayed_controls.cpp\n> > +++ b/src/libcamera/delayed_controls.cpp\n> > @@ -202,7 +202,7 @@ bool DelayedControls::push(const ControlList &controls)\n> > */\n> > ControlList DelayedControls::get(uint32_t sequence)\n> > {\n> > - unsigned int index = std::max(0, sequence - maxDelay_);\n> > + unsigned int index = std::clamp(sequence - maxDelay_, 0, queueCount_ - 1);\n> > \n> > ControlList out(device_->controls());\n> > for (const auto &ctrl : values_) {","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 F1A48C32DD\n\tfor ;\n\tWed, 11 Dec 2024 07:43:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3323B67E99;\n\tWed, 11 Dec 2024 08:43:58 +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 9B258618AA\n\tfor ;\n\tWed, 11 Dec 2024 08:43: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 77C2D352;\n\tWed, 11 Dec 2024 08:43:13 +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=\"V46SGz+w\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733902993;\n\tbh=AeWZuDc1u2H8cEgMdZj8CxSbOaIVrU2jQabpCc+WFgI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=V46SGz+we33RZcihKMI3apzuTUItV01NkqAAMoDgR7oABJRmGrkU7JM+XV/8PUERB\n\t+8Lu4G7VbhUOtdXrNhxYorO3Vfb3Bl0AueBwM5rzlhxngh2ayr4cbIhsfxeXgbC1tZ\n\tMsZiAH9MS54v+H8QtTVg6hrqpisuD1ws35+PTpbw=","Date":"Wed, 11 Dec 2024 09:43:29 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Stanislaw Gruszka ,\n\tlibcamera-devel@lists.libcamera.org,\n\tMilan Zamazal , \n\tNaushir Patuck ","Subject":"Re: [RFC] delayed_controls: avoid reading undefined control value","Message-ID":"<20241211074329.GA24546@pendragon.ideasonboard.com>","References":"<20241206112743.95435-1-stanislaw.gruszka@linux.intel.com>\n\t<173387039086.1401494.2029006348016958056@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<173387039086.1401494.2029006348016958056@ping.linuxembedded.co.uk>","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":32669,"web_url":"https://patchwork.libcamera.org/comment/32669/","msgid":"","date":"2024-12-11T09:03:39","subject":"Re: [RFC] delayed_controls: avoid reading undefined control value","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"On Wed, 11 Dec 2024 at 07:43, Laurent Pinchart\n wrote:\n>\n> (CC'ing Naush for the question related to the Raspberry Pi\n> implementation)\n>\n> On Tue, Dec 10, 2024 at 10:39:50PM +0000, Kieran Bingham wrote:\n> > Quoting Stanislaw Gruszka (2024-12-06 11:27:43)\n> > > Limit ControlRingBuffer index by number of queued entries in order do\n> > > avoid reading undefined control value with type ControlTypeNone .\n> > >\n> > > It can happen at the begging of streaming when ControlRingBuffer is\n> >\n> > s/begging/beginning/\n> >\n> > > not yet filled and there are errors on CSI-2 bus resulting dropping\n> > > frames. Then we can call to DelayedControls::get() with frame number\n> > > that exceed number 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> > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=241\n> > > Signed-off-by: Stanislaw Gruszka \n> > > ---\n> > > Notes: When debugging this I notice that the push() and get() are\n> > > done in different threads. Maybe mutex should be\n> > > used? Not sure how synchronization works for mojom signals\n> > > handlers.\n>\n> The comment related to the SoftwareISP::inputBufferReady signal, in the\n> SimpleCameraData::init() function, possibly applies to other signals of\n> the SoftwareISP class, although I wouldn't expect it would affect the\n> setSensorControls signal. Stanislaw, could you provide more information\n> about which threads the push and get functions are called from ?\n> Everything should be called from the camera manager thread.\n>\n> Looking at this more deeply, I think the outputBufferReady and\n> statsReady signals may be mishandled, as they seem to be emitted from\n> the soft ISP worker thread but not treated the same way as the\n> inputBufferReady signal. We shouldn't have to deal with it when\n> connecting the signals in the user, the SoftwareISP class should handle\n> thread crossing, and emit all its signals from the camera manager\n> thread. Milan, is this something you could address ?\n>\n> > I thought they would be in the same thread as the CameraManager event\n> > loop... Perhaps we could/should have an assertion that the thread is\n> > the same - otherwise - yes it would probably need a lock if there's a\n> > reader and a writer on both ends of the queue.\n> >\n> > I'd double check the threading first.\n> >\n> > > Also I'm not sure if similar change like in this patch\n> > > is not needed for rpi DelayedControls\n> >\n> > Probably. There's very little different between the two implementations,\n> > so we should probably keep them aligned with bugfixes at least until\n> > someone tries to realign them both again.\n>\n> Naush, could you check this ?\n\nTheoretically if our kernel would somehow dequeue buffers without\ntriggering the frame start event, this could hit us. However, in that\nsituation something much worse has gone wrong before the invalid\nindexing into the ring buffer. I don't recall any user raising an\nissue like this so I doubt it's a problem for us in practice. I'm ok\nto add a fix in our DelayedControls implementation, but I would have a\nslight preference on changing the fix to seed the entire ring buffer\nwith initial values rather than clamp the index.\n\nNaush\n\n\n>\n> > It looks like the cookie feature was easier to fork than to add, I can't\n> > remember all the details ;_(\n> >\n> >\n> > > Alternative fix would be to pre-fill ControlRingBuffer with\n> > > valid control values. Should we care about possibility\n> > > of queueCount_ overflowing ?\n> >\n> > I'm not sure what 'valid' control values would be ? I think just\n> > clamping to the most recent data is probably the best we can do ? And 'I\n> > think' would reflect the most up to date information we have about\n> > what's configured on the sensor - so I think it's still even the 'most\n> > correct' (and probably even 'is' correct) data we can return in the\n> > get() call in this event.\n> >\n> >\n> > I think I can already throw this on here, it seems like a sane input\n> > parameter validation:\n> >\n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> > > src/libcamera/delayed_controls.cpp | 2 +-\n> > > 1 file changed, 1 insertion(+), 1 deletion(-)\n> > >\n> > > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\n> > > index 94d0a575..78b0b8f7 100644\n> > > --- a/src/libcamera/delayed_controls.cpp\n> > > +++ b/src/libcamera/delayed_controls.cpp\n> > > @@ -202,7 +202,7 @@ bool DelayedControls::push(const ControlList &controls)\n> > > */\n> > > ControlList DelayedControls::get(uint32_t sequence)\n> > > {\n> > > - unsigned int index = std::max(0, sequence - maxDelay_);\n> > > + unsigned int index = std::clamp(sequence - maxDelay_, 0, queueCount_ - 1);\n> > >\n> > > ControlList out(device_->controls());\n> > > for (const auto &ctrl : values_) {\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 5050FC32DD\n\tfor ;\n\tWed, 11 Dec 2024 09:04:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 721ED67EA1;\n\tWed, 11 Dec 2024 10:04:14 +0100 (CET)","from mail-yb1-xb35.google.com (mail-yb1-xb35.google.com\n\t[IPv6:2607:f8b0:4864:20::b35])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 902A266136\n\tfor ;\n\tWed, 11 Dec 2024 10:04:11 +0100 (CET)","by mail-yb1-xb35.google.com with SMTP id\n\t3f1490d57ef6-e3983bcc0dcso574906276.0\n\tfor ;\n\tWed, 11 Dec 2024 01:04:11 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"LWQr4kdy\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1733907850; x=1734512650;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=M0syemPovxGBYkhygo6Qlt4SVEjTbG+0PEPqbeJypHU=;\n\tb=LWQr4kdygOYGmS64tDE62uyL0tcXp+yrLwOl91MdDWJxz5XqDg3fN0X0/ds+4ZyK41\n\tZg0PAPpKG3dtkCzAEyoK61yw4V5D/1aZ8C5ctuuxewgEDMRRLkVgmbH6qc0InArwEV5/\n\t39wMgMH+FSfyS+SxkkP7BPSI4qs/B42JqX0Pzpc8Ij3XkyhE/NFtTQkasBncBbvUyH8s\n\tgqbh4cdNRLX0n2ur6EStqAjAvMgI2FYy/u3JajUNyBhhniyiWgt2HhdqwATM0Oh845CK\n\t8pLc5ORPi86eGekBGeNjSm8au+fCF+bxrCXcphRZqC16nM2rquvQwSiIQRoWhHi/QF3V\n\tpIMA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1733907850; x=1734512650;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=M0syemPovxGBYkhygo6Qlt4SVEjTbG+0PEPqbeJypHU=;\n\tb=Y6RrgACva8GldPxWbVoYoEjMh6M1rFRBrhyRE5n/WMA/jrLXY3dck30+kIEz5TS9GB\n\tBCbwDqfA/QIxjmxwGHTeygKcz4Yxv6UN3HyIt1y3HyZmgi2xJjJe+ZIMpSKdgl/UVCWl\n\toCPjN7fSFmd2ZQ6cDOcP0FeivQfdnlPzslPLA13R1pcX3xs7kMmdZnCHcEGUQByULZgK\n\tnFPqv7UdtTu5uIkLzAzlnOFbgVhBpCeNIR9IlglIGgHh9u/IL8S350a15+Mbvmu14cZy\n\tQR8Jj/nQev+yJ2sn0G6EDC9KOP9yetX15xE3pKTm+Gv15pfHfcYlyn+/g7tZ/x1aW1Nn\n\t0hKQ==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCWTNeXqspQ7xHtb4O6SuMNrGkpRN1jxsk631Q7Lfv7VRBujOt/k4AMpMaoG9qG50EuLvQFR0IzyAth4DdV3lXY=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YzuGv4wSrHW7QOB+rpTTdW50Vjh+yjRjKMUUWALS03HhScMJY4T\n\tY/1ONv8Jssyr5Ho07IX8oHvV/GBAj5RUtou+AIAXZ9yxZybNhLrcKJTZlJbLx7b/vfBeIYfGC45\n\tENAvC8OxTA0ixP+hTlHb5/8nWXgelskAG31U6gw==","X-Gm-Gg":"ASbGncvXJiTUda5Saoz9ZvQtWnfLy4V1dBshIFZHxvKzpPWqhErIwNH80bLyBJ2CEVf\n\tSXciGVTMBDF8oO9qYlNjewMKECjABfCYOkPVLYdXRcroYuLV43Ze7W31knQq07qcBeBk=","X-Google-Smtp-Source":"AGHT+IHS5AHn4cCX0xZ3wX5nbxiYjlYSj4RLrf9JYtU5i25H7nH1gFiH8IX94bwEGP3hr0Bt3F5yBNZ2fNOBfqBCfFw=","X-Received":"by 2002:a05:6902:1b0a:b0:e38:2041:e9c4 with SMTP id\n\t3f1490d57ef6-e3c8e4534ffmr1113555276.4.1733907850324; Wed, 11 Dec 2024\n\t01:04:10 -0800 (PST)","MIME-Version":"1.0","References":"<20241206112743.95435-1-stanislaw.gruszka@linux.intel.com>\n\t<173387039086.1401494.2029006348016958056@ping.linuxembedded.co.uk>\n\t<20241211074329.GA24546@pendragon.ideasonboard.com>","In-Reply-To":"<20241211074329.GA24546@pendragon.ideasonboard.com>","From":"Naushir Patuck ","Date":"Wed, 11 Dec 2024 09:03:39 +0000","Message-ID":"","Subject":"Re: [RFC] delayed_controls: avoid reading undefined control value","To":"Laurent Pinchart ","Cc":"Kieran Bingham , \n\tStanislaw Gruszka ,\n\tlibcamera-devel@lists.libcamera.org, Milan Zamazal ","Content-Type":"text/plain; charset=\"UTF-8\"","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":32670,"web_url":"https://patchwork.libcamera.org/comment/32670/","msgid":"","date":"2024-12-11T09:26:26","subject":"Re: [RFC] delayed_controls: avoid reading undefined control value","submitter":{"id":211,"url":"https://patchwork.libcamera.org/api/people/211/","name":"Stanislaw Gruszka","email":"stanislaw.gruszka@linux.intel.com"},"content":"On Wed, Dec 11, 2024 at 09:03:39AM +0000, Naushir Patuck wrote:\n> On Wed, 11 Dec 2024 at 07:43, Laurent Pinchart\n> wrote:\n> >\n> > (CC'ing Naush for the question related to the Raspberry Pi\n> > implementation)\n> >\n> > On Tue, Dec 10, 2024 at 10:39:50PM +0000, Kieran Bingham wrote:\n> > > Quoting Stanislaw Gruszka (2024-12-06 11:27:43)\n> > > > Limit ControlRingBuffer index by number of queued entries in order do\n> > > > avoid reading undefined control value with type ControlTypeNone .\n> > > >\n> > > > It can happen at the begging of streaming when ControlRingBuffer is\n> > >\n> > > s/begging/beginning/\n> > >\n> > > > not yet filled and there are errors on CSI-2 bus resulting dropping\n> > > > frames. Then we can call to DelayedControls::get() with frame number\n> > > > that exceed number 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> > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=241\n> > > > Signed-off-by: Stanislaw Gruszka \n> > > > ---\n> > > > Notes: When debugging this I notice that the push() and get() are\n> > > > done in different threads. Maybe mutex should be\n> > > > used? Not sure how synchronization works for mojom signals\n> > > > handlers.\n> >\n> > The comment related to the SoftwareISP::inputBufferReady signal, in the\n> > SimpleCameraData::init() function, possibly applies to other signals of\n> > the SoftwareISP class, although I wouldn't expect it would affect the\n> > setSensorControls signal. Stanislaw, could you provide more information\n> > about which threads the push and get functions are called from ?\n> > Everything should be called from the camera manager thread.\n> >\n> > Looking at this more deeply, I think the outputBufferReady and\n> > statsReady signals may be mishandled, as they seem to be emitted from\n> > the soft ISP worker thread but not treated the same way as the\n> > inputBufferReady signal. We shouldn't have to deal with it when\n> > connecting the signals in the user, the SoftwareISP class should handle\n> > thread crossing, and emit all its signals from the camera manager\n> > thread. Milan, is this something you could address ?\n> >\n> > > I thought they would be in the same thread as the CameraManager event\n> > > loop... Perhaps we could/should have an assertion that the thread is\n> > > the same - otherwise - yes it would probably need a lock if there's a\n> > > reader and a writer on both ends of the queue.\n> > >\n> > > I'd double check the threading first.\n> > >\n> > > > Also I'm not sure if similar change like in this patch\n> > > > is not needed for rpi DelayedControls\n> > >\n> > > Probably. There's very little different between the two implementations,\n> > > so we should probably keep them aligned with bugfixes at least until\n> > > someone tries to realign them both again.\n> >\n> > Naush, could you check this ?\n> \n> Theoretically if our kernel would somehow dequeue buffers without\n> triggering the frame start event, this could hit us. However, in that\n\nAfter debug this further on my setup, this is actually wrong for simple\npipeline. We do not correctly subscribe for frameStart events and\nnever call DelayedControls::applyControls(uint32_t sequence).\n\nI'll debug this a bit more and provide more details and answers\nto questions.\n\nRegards\nStanislaw\n\n> indexing into the ring buffer. I don't recall any user raising an\n> issue like this so I doubt it's a problem for us in practice. I'm ok\n> to add a fix in our DelayedControls implementation, but I would have a\n> slight preference on changing the fix to seed the entire ring buffer\n> with initial values rather than clamp the index.\n> \n> Naush\n> \n> \n> >\n> > > It looks like the cookie feature was easier to fork than to add, I can't\n> > > remember all the details ;_(\n> > >\n> > >\n> > > > Alternative fix would be to pre-fill ControlRingBuffer with\n> > > > valid control values. Should we care about possibility\n> > > > of queueCount_ overflowing ?\n> > >\n> > > I'm not sure what 'valid' control values would be ? I think just\n> > > clamping to the most recent data is probably the best we can do ? And 'I\n> > > think' would reflect the most up to date information we have about\n> > > what's configured on the sensor - so I think it's still even the 'most\n> > > correct' (and probably even 'is' correct) data we can return in the\n> > > get() call in this event.\n> > >\n> > >\n> > > I think I can already throw this on here, it seems like a sane input\n> > > parameter validation:\n> > >\n> > >\n> > > Reviewed-by: Kieran Bingham \n> > >\n> > > > src/libcamera/delayed_controls.cpp | 2 +-\n> > > > 1 file changed, 1 insertion(+), 1 deletion(-)\n> > > >\n> > > > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\n> > > > index 94d0a575..78b0b8f7 100644\n> > > > --- a/src/libcamera/delayed_controls.cpp\n> > > > +++ b/src/libcamera/delayed_controls.cpp\n> > > > @@ -202,7 +202,7 @@ bool DelayedControls::push(const ControlList &controls)\n> > > > */\n> > > > ControlList DelayedControls::get(uint32_t sequence)\n> > > > {\n> > > > - unsigned int index = std::max(0, sequence - maxDelay_);\n> > > > + unsigned int index = std::clamp(sequence - maxDelay_, 0, queueCount_ - 1);\n> > > >\n> > > > ControlList out(device_->controls());\n> > > > for (const auto &ctrl : values_) {\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 9D2B7BD80A\n\tfor ;\n\tWed, 11 Dec 2024 09:26:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A633467EA6;\n\tWed, 11 Dec 2024 10:26:34 +0100 (CET)","from mgamail.intel.com (mgamail.intel.com [192.198.163.17])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9A08266136\n\tfor ;\n\tWed, 11 Dec 2024 10:26:31 +0100 (CET)","from fmviesa008.fm.intel.com ([10.60.135.148])\n\tby fmvoesa111.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384;\n\t11 Dec 2024 01:26:30 -0800","from sgruszka-mobl.ger.corp.intel.com (HELO localhost)\n\t([10.245.100.183]) by fmviesa008-auth.fm.intel.com with\n\tESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 Dec 2024 01:26:28 -0800"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=intel.com header.i=@intel.com\n\theader.b=\"YPB38tvb\"; 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=1733909192; x=1765445192;\n\th=date:from:to:cc:subject:message-id:references:\n\tmime-version:in-reply-to;\n\tbh=p01vOA9OYA8P3UKMPv7b/EpXa6E9VQE0mEhk/zlFWjU=;\n\tb=YPB38tvbN3tQTdJg3xY9zE/3P7TGfQur6XgfCvcEV2uIRaGeqEn4QHNR\n\tGu8KJ0mCrCzfYM1cnzMZBkKheyC3PCVZAH3SOHP7JVDvzDFwPTUAK84xg\n\tCUqCTYNBjy9JvqsXDbeYI6XS1QpkV5rzFedQgtOnFRvEA0/FqCxHGjjo2\n\tsStAVKd4NspYissB7bhX606KZdW9zs5vE/PPlHQflAPgB+kqFbzNTHHFJ\n\twrm1nMpaM4jvTeUUesiA+y45S8e6+ud3dqQTXN8n9bksy+Ixp9Hi6jCFT\n\tUIBNzShAfPS8t6sMi1uA0NCUxHaAh42j96B+l/TZReBnBxTckCYwuKMO6 w==;","X-CSE-ConnectionGUID":["sZzUpELvSTu36IQeTHAvhg==","tn4z5SHqQCGQOt4JDUTS8w=="],"X-CSE-MsgGUID":["J3LbIcdmSsac6RP66w2kig==","4inB7qFcTTeoB5D/T1zw2w=="],"X-IronPort-AV":["E=McAfee;i=\"6700,10204,11282\"; a=\"34181859\"","E=Sophos;i=\"6.12,225,1728975600\"; d=\"scan'208\";a=\"34181859\"","E=Sophos;i=\"6.12,225,1728975600\"; d=\"scan'208\";a=\"95930158\""],"X-ExtLoop1":"1","Date":"Wed, 11 Dec 2024 10:26:26 +0100","From":"Stanislaw Gruszka ","To":"Naushir Patuck ","Cc":"Laurent Pinchart ,\n\tKieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org, Milan Zamazal ","Subject":"Re: [RFC] delayed_controls: avoid reading undefined control value","Message-ID":"","References":"<20241206112743.95435-1-stanislaw.gruszka@linux.intel.com>\n\t<173387039086.1401494.2029006348016958056@ping.linuxembedded.co.uk>\n\t<20241211074329.GA24546@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\" "}},{"id":32674,"web_url":"https://patchwork.libcamera.org/comment/32674/","msgid":"","date":"2024-12-11T11:08:35","subject":"Re: [RFC] delayed_controls: avoid reading undefined control value","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Stanislaw,\n\nOn Wed, Dec 11, 2024 at 10:26:26AM +0100, Stanislaw Gruszka wrote:\n> On Wed, Dec 11, 2024 at 09:03:39AM +0000, Naushir Patuck wrote:\n> > On Wed, 11 Dec 2024 at 07:43, Laurent Pinchart\n> > wrote:\n> > >\n> > > (CC'ing Naush for the question related to the Raspberry Pi\n> > > implementation)\n> > >\n> > > On Tue, Dec 10, 2024 at 10:39:50PM +0000, Kieran Bingham wrote:\n> > > > Quoting Stanislaw Gruszka (2024-12-06 11:27:43)\n> > > > > Limit ControlRingBuffer index by number of queued entries in order do\n> > > > > avoid reading undefined control value with type ControlTypeNone .\n> > > > >\n> > > > > It can happen at the begging of streaming when ControlRingBuffer is\n> > > >\n> > > > s/begging/beginning/\n> > > >\n> > > > > not yet filled and there are errors on CSI-2 bus resulting dropping\n> > > > > frames. Then we can call to DelayedControls::get() with frame number\n> > > > > that exceed number 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> > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=241\n> > > > > Signed-off-by: Stanislaw Gruszka \n> > > > > ---\n> > > > > Notes: When debugging this I notice that the push() and get() are\n> > > > > done in different threads. Maybe mutex should be\n> > > > > used? Not sure how synchronization works for mojom signals\n> > > > > handlers.\n> > >\n> > > The comment related to the SoftwareISP::inputBufferReady signal, in the\n> > > SimpleCameraData::init() function, possibly applies to other signals of\n> > > the SoftwareISP class, although I wouldn't expect it would affect the\n> > > setSensorControls signal. Stanislaw, could you provide more information\n> > > about which threads the push and get functions are called from ?\n> > > Everything should be called from the camera manager thread.\n> > >\n> > > Looking at this more deeply, I think the outputBufferReady and\n> > > statsReady signals may be mishandled, as they seem to be emitted from\n> > > the soft ISP worker thread but not treated the same way as the\n> > > inputBufferReady signal. We shouldn't have to deal with it when\n> > > connecting the signals in the user, the SoftwareISP class should handle\n> > > thread crossing, and emit all its signals from the camera manager\n> > > thread. Milan, is this something you could address ?\n> > >\n> > > > I thought they would be in the same thread as the CameraManager event\n> > > > loop... Perhaps we could/should have an assertion that the thread is\n> > > > the same - otherwise - yes it would probably need a lock if there's a\n> > > > reader and a writer on both ends of the queue.\n> > > >\n> > > > I'd double check the threading first.\n> > > >\n> > > > > Also I'm not sure if similar change like in this patch\n> > > > > is not needed for rpi DelayedControls\n> > > >\n> > > > Probably. There's very little different between the two implementations,\n> > > > so we should probably keep them aligned with bugfixes at least until\n> > > > someone tries to realign them both again.\n> > >\n> > > Naush, could you check this ?\n> > \n> > Theoretically if our kernel would somehow dequeue buffers without\n> > triggering the frame start event, this could hit us. However, in that\n> \n> After debug this further on my setup, this is actually wrong for simple\n> pipeline. We do not correctly subscribe for frameStart events and\n> never call DelayedControls::applyControls(uint32_t sequence).\n> \n> I'll debug this a bit more and provide more details and answers\n> to questions.\n> \n> Regards\n> Stanislaw\n> \n> > indexing into the ring buffer. I don't recall any user raising an\n> > issue like this so I doubt it's a problem for us in practice. I'm ok\n> > to add a fix in our DelayedControls implementation, but I would have a\n> > slight preference on changing the fix to seed the entire ring buffer\n> > with initial values rather than clamp the index.\n\nI would even go a step further and log an error and return an empty\nControlsList. If we hit that case something in the pipeline went wrong\nand that needs fixing (as you also found out).\n\nRegards,\nStefan\n\n> > \n> > Naush\n> > \n> > \n> > >\n> > > > It looks like the cookie feature was easier to fork than to add, I can't\n> > > > remember all the details ;_(\n> > > >\n> > > >\n> > > > > Alternative fix would be to pre-fill ControlRingBuffer with\n> > > > > valid control values. Should we care about possibility\n> > > > > of queueCount_ overflowing ?\n> > > >\n> > > > I'm not sure what 'valid' control values would be ? I think just\n> > > > clamping to the most recent data is probably the best we can do ? And 'I\n> > > > think' would reflect the most up to date information we have about\n> > > > what's configured on the sensor - so I think it's still even the 'most\n> > > > correct' (and probably even 'is' correct) data we can return in the\n> > > > get() call in this event.\n> > > >\n> > > >\n> > > > I think I can already throw this on here, it seems like a sane input\n> > > > parameter validation:\n> > > >\n> > > >\n> > > > Reviewed-by: Kieran Bingham \n> > > >\n> > > > > src/libcamera/delayed_controls.cpp | 2 +-\n> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)\n> > > > >\n> > > > > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\n> > > > > index 94d0a575..78b0b8f7 100644\n> > > > > --- a/src/libcamera/delayed_controls.cpp\n> > > > > +++ b/src/libcamera/delayed_controls.cpp\n> > > > > @@ -202,7 +202,7 @@ bool DelayedControls::push(const ControlList &controls)\n> > > > > */\n> > > > > ControlList DelayedControls::get(uint32_t sequence)\n> > > > > {\n> > > > > - unsigned int index = std::max(0, sequence - maxDelay_);\n> > > > > + unsigned int index = std::clamp(sequence - maxDelay_, 0, queueCount_ - 1);\n> > > > >\n> > > > > ControlList out(device_->controls());\n> > > > > for (const auto &ctrl : values_) {\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 C7149C32DD\n\tfor ;\n\tWed, 11 Dec 2024 11:08:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1D0D167EAB;\n\tWed, 11 Dec 2024 12:08:41 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1259F618AE\n\tfor ;\n\tWed, 11 Dec 2024 12:08:39 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:af9:9d8e:d17e:723])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BA9A8352;\n\tWed, 11 Dec 2024 12:08:05 +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=\"tvcUUfF/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733915285;\n\tbh=Qh4wNIbu9NsoPpd8IBYVIbssnOdh6rn1UTpPirVEAsM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tvcUUfF/pLwZujpkBinL3eDPMXR7OXg+W+txWFoDOkB2H61JJLK/IpsG98YgGdTC6\n\t/zjmMjpLwHqUz4UhFRtCiIqHXzwRKxGkYdkcxLPJlcqTDP/rhi9Kbks2lJ7GJOpxf4\n\tbrmfqpfUUH/6zvlrrv4lxeoyTyjCLcc5IyMgG7D0=","Date":"Wed, 11 Dec 2024 12:08:35 +0100","From":"Stefan Klug ","To":"Stanislaw Gruszka ","Cc":"Naushir Patuck , \n\tLaurent Pinchart ,\n\tKieran Bingham , \n\tlibcamera-devel@lists.libcamera.org, Milan Zamazal ","Subject":"Re: [RFC] delayed_controls: avoid reading undefined control value","Message-ID":"","References":"<20241206112743.95435-1-stanislaw.gruszka@linux.intel.com>\n\t<173387039086.1401494.2029006348016958056@ping.linuxembedded.co.uk>\n\t<20241211074329.GA24546@pendragon.ideasonboard.com>\n\t\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":32676,"web_url":"https://patchwork.libcamera.org/comment/32676/","msgid":"<87o71icnoi.fsf@redhat.com>","date":"2024-12-11T14:13:49","subject":"Re: [RFC] delayed_controls: avoid reading undefined control value","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Laurent Pinchart writes:\n\n> (CC'ing Naush for the question related to the Raspberry Pi\n> implementation)\n>\n> On Tue, Dec 10, 2024 at 10:39:50PM +0000, Kieran Bingham wrote:\n>> Quoting Stanislaw Gruszka (2024-12-06 11:27:43)\n>> > Limit ControlRingBuffer index by number of queued entries in order do\n>> > avoid reading undefined control value with type ControlTypeNone .\n>> > \n>> > It can happen at the begging of streaming when ControlRingBuffer is\n>> \n>> s/begging/beginning/\n>> \n>> > not yet filled and there are errors on CSI-2 bus resulting dropping\n>> > frames. Then we can call to DelayedControls::get() with frame number\n>> > that exceed number 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>> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=241\n>> > Signed-off-by: Stanislaw Gruszka \n>> > ---\n>> > Notes: When debugging this I notice that the push() and get() are\n>> > done in different threads. Maybe mutex should be\n>> > used? Not sure how synchronization works for mojom signals\n>> > handlers.\n>\n> The comment related to the SoftwareISP::inputBufferReady signal, in the\n> SimpleCameraData::init() function, possibly applies to other signals of\n> the SoftwareISP class, although I wouldn't expect it would affect the\n> setSensorControls signal. Stanislaw, could you provide more information\n> about which threads the push and get functions are called from ?\n> Everything should be called from the camera manager thread.\n>\n> Looking at this more deeply, I think the outputBufferReady and\n> statsReady signals may be mishandled, as they seem to be emitted from\n> the soft ISP worker thread but not treated the same way as the\n> inputBufferReady signal. We shouldn't have to deal with it when\n> connecting the signals in the user, the SoftwareISP class should handle\n> thread crossing, and emit all its signals from the camera manager\n> thread. Milan, is this something you could address ?\n\nHonestly, I usually get confused easily when trying to follow the\nrelated flows and understanding what's what. If I understand it\ncorrectly, the pipeline handler instance is created in the camera\nmanager thread. Thus inputBufferReady is run there and the other\nsignals could do the same the same way (there is apparently no reason\nwhy not). Then yes, it could be fixed easily.\n\nBTW there is software ISP TODO #3 where you suggest that statsReady\ncould be possibly removed completely. When I worked on buffer sharing,\nI didn't see an obvious way/reason to do it, so let's fix just what's\nimmediately wrong about it for now.\n\n>> I thought they would be in the same thread as the CameraManager event\n>> loop... Perhaps we could/should have an assertion that the thread is\n>> the same - otherwise - yes it would probably need a lock if there's a\n>> reader and a writer on both ends of the queue.\n>> \n>> I'd double check the threading first.\n>> \n>> > Also I'm not sure if similar change like in this patch\n>> > is not needed for rpi DelayedControls\n>> \n>> Probably. There's very little different between the two implementations,\n>> so we should probably keep them aligned with bugfixes at least until\n>> someone tries to realign them both again.\n>\n> Naush, could you check this ?\n>\n>> It looks like the cookie feature was easier to fork than to add, I can't\n>> remember all the details ;_(\n>> \n>> \n>> > Alternative fix would be to pre-fill ControlRingBuffer with\n>> > valid control values. Should we care about possibility\n>> > of queueCount_ overflowing ? \n>> \n>> I'm not sure what 'valid' control values would be ? I think just\n>> clamping to the most recent data is probably the best we can do ? And 'I\n>> think' would reflect the most up to date information we have about\n>> what's configured on the sensor - so I think it's still even the 'most\n>> correct' (and probably even 'is' correct) data we can return in the\n>> get() call in this event.\n>> \n>> \n>> I think I can already throw this on here, it seems like a sane input\n>> parameter validation:\n>> \n>> \n>> Reviewed-by: Kieran Bingham \n>> \n>> > src/libcamera/delayed_controls.cpp | 2 +-\n>> > 1 file changed, 1 insertion(+), 1 deletion(-)\n>> > \n>> > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\n>> > index 94d0a575..78b0b8f7 100644\n>> > --- a/src/libcamera/delayed_controls.cpp\n>> > +++ b/src/libcamera/delayed_controls.cpp\n>> > @@ -202,7 +202,7 @@ bool DelayedControls::push(const ControlList &controls)\n>> > */\n>> > ControlList DelayedControls::get(uint32_t sequence)\n>> > {\n>> > - unsigned int index = std::max(0, sequence - maxDelay_);\n>> > + unsigned int index = std::clamp(sequence - maxDelay_, 0, queueCount_ - 1);\n>> > \n>> > ControlList out(device_->controls());\n>> > for (const auto &ctrl : values_) {","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 053C0C32DD\n\tfor ;\n\tWed, 11 Dec 2024 14:13:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1847B67EAC;\n\tWed, 11 Dec 2024 15:13:56 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EBACD618AE\n\tfor ;\n\tWed, 11 Dec 2024 15:13:54 +0100 (CET)","from mail-wm1-f72.google.com (mail-wm1-f72.google.com\n\t[209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-610-sfY62FrhOyS01pkLZ22gtg-1; Wed, 11 Dec 2024 09:13:52 -0500","by mail-wm1-f72.google.com with SMTP id\n\t5b1f17b1804b1-434f3a758dbso35008365e9.0\n\tfor ;\n\tWed, 11 Dec 2024 06:13:52 -0800 (PST)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\tffacd0b85a97d-3878248f521sm1371232f8f.16.2024.12.11.06.13.49\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 11 Dec 2024 06:13:50 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"MTxm2Hvk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1733926433;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=fHVfoaG+w4zDMLvGSJhITFiB0prnFwurmo0Q/KtCx9k=;\n\tb=MTxm2Hvkyb0CnM1DNSxHOzIpL3bZpa88vAsHb+FdDFK0Vc0HC/doVXoWVJ2XYPYTtsX1jw\n\tfVIpx4f+ICmBkr7Kxp7No28W8sQpqickS/98NxJZUuYrXHw2VsHniNsjimUOXXNNAVTMCO\n\tJOZ4ZqKoktCPt0IivWVXbG2jHOg4Z/U=","X-MC-Unique":"sfY62FrhOyS01pkLZ22gtg-1","X-Mimecast-MFC-AGG-ID":"sfY62FrhOyS01pkLZ22gtg","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1733926431; x=1734531231;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=fHVfoaG+w4zDMLvGSJhITFiB0prnFwurmo0Q/KtCx9k=;\n\tb=s0ybIPnV72S7hDb4ZXuFrkl7ynSwZAiWqExp4GMdEJFu9rQibhQzjVVt+88h+quT7G\n\tTk0QtZemTMCOJOo/KAtOidPtxlfH+/AYIUHpGxO9CkhJJNFPSqyeJLaMl4eU4JPr/RCC\n\tgfa0uWuXH3S44HrocSsYYGMFoxr9Z+Z/xl0b7E2rM32/LRs+ovHbjggfLhRnxab0/0pT\n\t0Q0nqS8KFMaWU24LeHONbGNheQniGuVnSCADl4YqmX19oDn4DM/Qq8iUP/z1TjtT9z/l\n\tinZg6RuRx/f0/Kr8+SZLRwxfKnlEDf0umQNnxDt6SCzaAzej8ydya+RYh43UaZD7eRxF\n\tcnGw==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCU6Pg2dsyF+c4uRoPUuLxGcFkzgoJmUtgopJBG0KUi69eFWpEt25dZnNlj9TKMgMCK82Jvas8KESKwZH9p70+s=@lists.libcamera.org","X-Gm-Message-State":"AOJu0Yy5sUR5f5VAy3QKCpG3Wp4IILVxCXi/w7bZTvJ8T0fgeYwwZyv9\n\tpkQQ6zQ6kekzfn9M+YvtPPC2qQaoD+02KDhmMV9FEAzW9awu5uWYu3p12/y7ccUDJ3OQJrA184y\n\t+gL+EZmTowx0iG+4gMzAz2Hjv7nGOlMj+UDtkANfYhLfrC1lUVxd3EHI90xeu78i7vrl0Koo=","X-Gm-Gg":"ASbGncuD1bPKH/1MIsWyH4sFXRlV22OmMw8Tkj+f94LkoWzbpNHabYI2/shVX792CRB\n\tk2edyjcN5z410lIO/oxS7wzYmVzY0sbFNUID+Z/HWBFYh9k8vF5yplQUYsjhBxIEIjfQ/6lRqsX\n\t+W9r6iv1ctVQgLqXKDs3/OKP8Gi4U0XkA2jSOZv84i07q5SKiccJygO9AAoK1EwjEgZBTbEWhgR\n\t0GsdaHVdtwr9CwjYZ2oPJ+fzf+HFvL2vJDFArv43LbGs2w4k8zZRnTJUVryEK2gSjvFnzodEWU=","X-Received":["by 2002:adf:e182:0:b0:385:fd31:ca23 with SMTP id\n\tffacd0b85a97d-3864cec7d60mr2866660f8f.40.1733926431342; \n\tWed, 11 Dec 2024 06:13:51 -0800 (PST)","by 2002:adf:e182:0:b0:385:fd31:ca23 with SMTP id\n\tffacd0b85a97d-3864cec7d60mr2866629f8f.40.1733926430952; \n\tWed, 11 Dec 2024 06:13:50 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IH4miBKiVE+JG/UPuKLHvgtjhE1UnOStkA7K9Tq5l71/0LXIpdrT/WedjQC8BzAR2F5YTC4ww==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"Kieran Bingham , Stanislaw Gruszka\n\t,\n\tlibcamera-devel@lists.libcamera.org, \n\tNaushir Patuck ","Subject":"Re: [RFC] delayed_controls: avoid reading undefined control value","In-Reply-To":"<20241211074329.GA24546@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Wed, 11 Dec 2024 09:43:29 +0200\")","References":"<20241206112743.95435-1-stanislaw.gruszka@linux.intel.com>\n\t<173387039086.1401494.2029006348016958056@ping.linuxembedded.co.uk>\n\t<20241211074329.GA24546@pendragon.ideasonboard.com>","Date":"Wed, 11 Dec 2024 15:13:49 +0100","Message-ID":"<87o71icnoi.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"dqGv29ipyHYMcrYQJvoVvVGztwp604IYK937Z3Y4qYw_1733926431","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","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":32696,"web_url":"https://patchwork.libcamera.org/comment/32696/","msgid":"","date":"2024-12-12T10:45:15","subject":"Re: [RFC] delayed_controls: avoid reading undefined control value","submitter":{"id":211,"url":"https://patchwork.libcamera.org/api/people/211/","name":"Stanislaw Gruszka","email":"stanislaw.gruszka@linux.intel.com"},"content":"Hi all, thanks for review and comments.\n\nOn Wed, Dec 11, 2024 at 09:43:29AM +0200, Laurent Pinchart wrote:\n> (CC'ing Naush for the question related to the Raspberry Pi\n> implementation)\n> \n> On Tue, Dec 10, 2024 at 10:39:50PM +0000, Kieran Bingham wrote:\n> > Quoting Stanislaw Gruszka (2024-12-06 11:27:43)\n> > > Limit ControlRingBuffer index by number of queued entries in order do\n> > > avoid reading undefined control value with type ControlTypeNone .\n> > > \n> > > It can happen at the begging of streaming when ControlRingBuffer is\n> > \n> > s/begging/beginning/\n> > \n> > > not yet filled and there are errors on CSI-2 bus resulting dropping\n> > > frames. Then we can call to DelayedControls::get() with frame number\n> > > that exceed number 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> > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=241\n> > > Signed-off-by: Stanislaw Gruszka \n> > > ---\n> > > Notes: When debugging this I notice that the push() and get() are\n> > > done in different threads. Maybe mutex should be\n> > > used? Not sure how synchronization works for mojom signals\n> > > handlers.\n> \n> The comment related to the SoftwareISP::inputBufferReady signal, in the\n> SimpleCameraData::init() function, possibly applies to other signals of\n> the SoftwareISP class, although I wouldn't expect it would affect the\n> setSensorControls signal. Stanislaw, could you provide more information\n> about which threads the push and get functions are called from ?\n> Everything should be called from the camera manager thread.\n\nI added debug prints with backtraces to delayedControls push and get.\nHere is the output:\nhttps://gist.github.com/sgruszka/cfe3832a907887512a4953a3353c8806\n\nThreads are different - have different tid, but they seems to be\nserialized. I added extra:\n\nstd::this_thread::sleep_for(std::chrono::milliseconds(500)); \n\nto push and it did not change the ordering - get() was followed by push().\n\n> Looking at this more deeply, I think the outputBufferReady and\n> statsReady signals may be mishandled, as they seem to be emitted from\n> the soft ISP worker thread but not treated the same way as the\n> inputBufferReady signal. We shouldn't have to deal with it when\n> connecting the signals in the user, the SoftwareISP class should handle\n> thread crossing, and emit all its signals from the camera manager\n> thread. Milan, is this something you could address ?\n\n> > I'm not sure what 'valid' control values would be ? I think just\n> > clamping to the most recent data is probably the best we can do ? And 'I\n> > think' would reflect the most up to date information we have about\n> > what's configured on the sensor - so I think it's still even the 'most\n> > correct' (and probably even 'is' correct) data we can return in the\n> > get() call in this event.\n> > \n> > Reviewed-by: Kieran Bingham \n> > \n> > > src/libcamera/delayed_controls.cpp | 2 +-\n> > > 1 file changed, 1 insertion(+), 1 deletion(-)\n> > > \n> > > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\n> > > index 94d0a575..78b0b8f7 100644\n> > > --- a/src/libcamera/delayed_controls.cpp\n> > > +++ b/src/libcamera/delayed_controls.cpp\n> > > @@ -202,7 +202,7 @@ bool DelayedControls::push(const ControlList &controls)\n> > > */\n> > > ControlList DelayedControls::get(uint32_t sequence)\n> > > {\n> > > - unsigned int index = std::max(0, sequence - maxDelay_);\n> > > + unsigned int index = std::clamp(sequence - maxDelay_, 0, queueCount_ - 1);\n\nThere is some drawback except additional cycles pointed by Naush.\nI can see possibility of queueCount_ overlap (for 30 FPS this would be\napproximately after 2 years of using camera non-stop :-) )\nThen we can have clamp call with high value smaller than low value,\nwhat is undefined behaviour. To prevent overlap issue this need to\nbe coded differently, what will add additional complexity (since\nsequence variable can overlap as well).\n\nAfter further debug, the actual problem is not handling startFrame\nsignal and not calling DelayedControls::applyControls(). Having\nDeleyadControls::applyControls() will prevent the assertion as\nDeleyadControls::get() will have proper control values on requested\nframe number, even when some frames are missing.\n\nOne issue in current simple pipeline code is that we lack enabling\nevents by setFrameStartEnabled(true).\nSecond, we have to enable events for proper device. On my setup this is\nCSI-2 receiver. Using video_ results in inappropriate ioctl error.\n\nBelow is a draft patch that try to address those problems.\n\nRegards\nStanislaw\n\nFrom 08d4caa36702da7efe7611365df8d417fedf38ee Mon Sep 17 00:00:00 2001\nFrom: Stanislaw Gruszka \nDate: Thu, 12 Dec 2024 10:55:44 +0100\nSubject: [PATCH] pipeline: simple: Use subdevice for frame start events\n\nSigned-off-by: Stanislaw Gruszka \n---\n src/libcamera/pipeline/simple/simple.cpp | 17 ++++++++++++++++-\n 1 file changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\nindex 8ac24e6e..64525b81 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 *subdev_;\n \n \tstd::vector configs_;\n \tstd::map> formats_;\n@@ -561,6 +562,13 @@ int SimpleCameraData::init()\n \tvideo_ = pipe->video(entities_.back().entity);\n \tASSERT(video_);\n \n+\tfor (auto it = entities_.rbegin(); it != entities_.rend(); ++it) {\n+\t\tif (it->entity->type() == MediaEntity::Type::V4L2Subdevice) {\n+\t\t\tsubdev_ = pipe->subdev(it->entity);\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n \t/*\n \t * Setup links first as some subdev drivers take active links into\n \t * account to propagate TRY formats. Such is life :-(\n@@ -1299,7 +1307,7 @@ 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+\tdata->subdev_->frameStart.connect(data->delayedCtrls_.get(),\n \t\t\t\t\t &DelayedControls::applyControls);\n \n \tStreamConfiguration inputCfg;\n@@ -1339,6 +1347,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n {\n \tSimpleCameraData *data = cameraData(camera);\n \tV4L2VideoDevice *video = data->video_;\n+\tV4L2Subdevice *subdev = data->subdev_;\n \tint ret;\n \n \tconst MediaPad *pad = acquirePipeline(data);\n@@ -1367,6 +1376,12 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n \t}\n \n \tvideo->bufferReady.connect(data, &SimpleCameraData::imageBufferReady);\n+\tif (subdev) {\n+\t\tLOG(SimplePipeline, Debug)\n+\t\t\t<< \"Enable frame start event on \"\n+\t\t\t<< subdev->entity()->name();\n+\t\tsubdev->setFrameStartEnabled(true);\n+\t}\n \n \tret = video->streamOn();\n \tif (ret < 0) {","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 500E6BD80A\n\tfor ;\n\tThu, 12 Dec 2024 10:45:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 579B367ECA;\n\tThu, 12 Dec 2024 11:45:25 +0100 (CET)","from mgamail.intel.com (mgamail.intel.com [192.198.163.9])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6BC1A67E6D\n\tfor ;\n\tThu, 12 Dec 2024 11:45:21 +0100 (CET)","from orviesa004.jf.intel.com ([10.64.159.144])\n\tby fmvoesa103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384;\n\t12 Dec 2024 02:45:20 -0800","from sgruszka-mobl.ger.corp.intel.com (HELO localhost)\n\t([10.245.118.67]) by orviesa004-auth.jf.intel.com with\n\tESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Dec 2024 02:45:18 -0800"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=intel.com header.i=@intel.com\n\theader.b=\"KWLAWRKO\"; 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=1734000322; x=1765536322;\n\th=date:from:to:cc:subject:message-id:references:\n\tmime-version:in-reply-to;\n\tbh=CHvX/se9qfVR47LOAU9gMFS/zLJp/GnKJHI1P9pOyyk=;\n\tb=KWLAWRKOH6UTPDpFIlx/BdQi9A49VKSVps/XwqNZpxQEGdKD5bo5jLps\n\t4+jrtqZUGiMmLxyXK6QOC5Efw8exTpv9OvXAdi/Nz2aPfBC8yf9L7YJ00\n\tgWYnIeNXj470KN3wVaDhJMK3cnfBSk3gLkoF6Z/OANcZn3V12wda1Ge51\n\tNQcloVG8peShR8+Z+wPZ5rmCZahiI6FRFPRQWdIc6hPZg7gGxvAlp2wjz\n\ty3HVv62VudIeX/cAFy+BWAzseWtry+W4EgH+q1CAbGPFkPTW7Ghss7oLZ\n\t3erQwNtUA0wINPRx3xRMGdvdLUxYh5U4vS29J8QpL75J5LD/bQVfBeZvZ Q==;","X-CSE-ConnectionGUID":["829UJrFrTJmUb+0JxLY55Q==","2ql2wF7eR8KG1Vv13S9nIQ=="],"X-CSE-MsgGUID":["SZcF5h8kRJeV6TS3SzmKwg==","3wLcUq/yQAG0L+4MU0cajg=="],"X-IronPort-AV":["E=McAfee;i=\"6700,10204,11283\"; a=\"45083132\"","E=Sophos;i=\"6.12,228,1728975600\"; d=\"scan'208\";a=\"45083132\"","E=Sophos;i=\"6.12,228,1728975600\"; d=\"scan'208\";a=\"101218827\""],"X-ExtLoop1":"1","Date":"Thu, 12 Dec 2024 11:45:15 +0100","From":"Stanislaw Gruszka ","To":"Laurent Pinchart ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org,\n\tMilan Zamazal , \n\tNaushir Patuck ","Subject":"Re: [RFC] delayed_controls: avoid reading undefined control value","Message-ID":"","References":"<20241206112743.95435-1-stanislaw.gruszka@linux.intel.com>\n\t<173387039086.1401494.2029006348016958056@ping.linuxembedded.co.uk>\n\t<20241211074329.GA24546@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20241211074329.GA24546@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":32876,"web_url":"https://patchwork.libcamera.org/comment/32876/","msgid":"<87ldwc3ec5.fsf@redhat.com>","date":"2024-12-18T20:50:50","subject":"Re: [RFC] delayed_controls: avoid reading undefined control value","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Stanislaw,\n\nStanislaw Gruszka writes:\n\n> Hi all, thanks for review and comments.\n>\n> On Wed, Dec 11, 2024 at 09:43:29AM +0200, Laurent Pinchart wrote:\n>> (CC'ing Naush for the question related to the Raspberry Pi\n>> implementation)\n>> \n>> On Tue, Dec 10, 2024 at 10:39:50PM +0000, Kieran Bingham wrote:\n>> > Quoting Stanislaw Gruszka (2024-12-06 11:27:43)\n>> > > Limit ControlRingBuffer index by number of queued entries in order do\n>> > > avoid reading undefined control value with type ControlTypeNone .\n>> > > \n>> > > It can happen at the begging of streaming when ControlRingBuffer is\n>> > \n>> > s/begging/beginning/\n>> > \n>> > > not yet filled and there are errors on CSI-2 bus resulting dropping\n>> > > frames. Then we can call to DelayedControls::get() with frame number\n>> > > that exceed number 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>> > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=241\n>> > > Signed-off-by: Stanislaw Gruszka \n>> > > ---\n>> > > Notes: When debugging this I notice that the push() and get() are\n>> > > done in different threads. Maybe mutex should be\n>> > > used? Not sure how synchronization works for mojom signals\n>> > > handlers.\n>> \n>> The comment related to the SoftwareISP::inputBufferReady signal, in the\n>> SimpleCameraData::init() function, possibly applies to other signals of\n>> the SoftwareISP class, although I wouldn't expect it would affect the\n>> setSensorControls signal. Stanislaw, could you provide more information\n>> about which threads the push and get functions are called from ?\n>> Everything should be called from the camera manager thread.\n>\n> I added debug prints with backtraces to delayedControls push and get.\n> Here is the output:\n> https://gist.github.com/sgruszka/cfe3832a907887512a4953a3353c8806\n>\n> Threads are different - have different tid, but they seems to be\n> serialized. I added extra:\n>\n> std::this_thread::sleep_for(std::chrono::milliseconds(500)); \n>\n> to push and it did not change the ordering - get() was followed by push().\n\nThank you for testing. As I understand the source code comment:\n \n The inputBufferReady signal is emitted from the soft ISP thread,\n and needs to be handled in the pipeline handler thread. Signals\n implement queued delivery, but this works transparently only if\n the receiver is bound to the target thread. As the\n SimpleCameraData class doesn't inherit from the Object class, it\n is not bound to any thread, and the signal would be delivered\n synchronously. Instead, connect the signal to a lambda function\n bound explicitly to the pipe, which is bound to the pipeline\n handler thread. The function then simply forwards the call to\n conversionInputDone().\n\nand since ispStatsReady and outputBufferReady handlers are initiated in\nthe same place:\n\n stats_->finishFrame(frame, 0);\n outputBufferReady.emit(output);\n\nthey are indeed run in the same thread and thus serialized. But they\nblock the caller. I'm not sure how much it matters here; in any case if\nI redirect them to the pipeline handler thread similarly to\ninputBufferReady, it works and is probably more correct. I post a patch\ntomorrow and we can discuss it there further.\n\n>> Looking at this more deeply, I think the outputBufferReady and\n>> statsReady signals may be mishandled, as they seem to be emitted from\n>> the soft ISP worker thread but not treated the same way as the\n>> inputBufferReady signal. We shouldn't have to deal with it when\n>> connecting the signals in the user, the SoftwareISP class should handle\n>> thread crossing, and emit all its signals from the camera manager\n>> thread. Milan, is this something you could address ?\n> \n>> > I'm not sure what 'valid' control values would be ? I think just\n>> > clamping to the most recent data is probably the best we can do ? And 'I\n>> > think' would reflect the most up to date information we have about\n>> > what's configured on the sensor - so I think it's still even the 'most\n>> > correct' (and probably even 'is' correct) data we can return in the\n>> > get() call in this event.\n>> > \n>> > Reviewed-by: Kieran Bingham \n>> > \n>> > > src/libcamera/delayed_controls.cpp | 2 +-\n>> > > 1 file changed, 1 insertion(+), 1 deletion(-)\n>> > > \n>> > > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\n>> > > index 94d0a575..78b0b8f7 100644\n>> > > --- a/src/libcamera/delayed_controls.cpp\n>> > > +++ b/src/libcamera/delayed_controls.cpp\n>> > > @@ -202,7 +202,7 @@ bool DelayedControls::push(const ControlList &controls)\n>> > > */\n>> > > ControlList DelayedControls::get(uint32_t sequence)\n>> > > {\n>> > > - unsigned int index = std::max(0, sequence - maxDelay_);\n>> > > + unsigned int index = std::clamp(sequence - maxDelay_, 0, queueCount_ - 1);\n>\n> There is some drawback except additional cycles pointed by Naush.\n> I can see possibility of queueCount_ overlap (for 30 FPS this would be\n> approximately after 2 years of using camera non-stop :-) )\n> Then we can have clamp call with high value smaller than low value,\n> what is undefined behaviour. To prevent overlap issue this need to\n> be coded differently, what will add additional complexity (since\n> sequence variable can overlap as well).\n>\n> After further debug, the actual problem is not handling startFrame\n> signal and not calling DelayedControls::applyControls(). Having\n> DeleyadControls::applyControls() will prevent the assertion as\n> DeleyadControls::get() will have proper control values on requested\n> frame number, even when some frames are missing.\n>\n> One issue in current simple pipeline code is that we lack enabling\n> events by setFrameStartEnabled(true).\n> Second, we have to enable events for proper device. On my setup this is\n> CSI-2 receiver. Using video_ results in inappropriate ioctl error.\n>\n> Below is a draft patch that try to address those problems.\n>\n> Regards\n> Stanislaw\n>\n> From 08d4caa36702da7efe7611365df8d417fedf38ee Mon Sep 17 00:00:00 2001\n> From: Stanislaw Gruszka \n> Date: Thu, 12 Dec 2024 10:55:44 +0100\n> Subject: [PATCH] pipeline: simple: Use subdevice for frame start events\n>\n> Signed-off-by: Stanislaw Gruszka \n> ---\n> src/libcamera/pipeline/simple/simple.cpp | 17 ++++++++++++++++-\n> 1 file changed, 16 insertions(+), 1 deletion(-)\n>\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 8ac24e6e..64525b81 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 *subdev_;\n> \n> \tstd::vector configs_;\n> \tstd::map> formats_;\n> @@ -561,6 +562,13 @@ int SimpleCameraData::init()\n> \tvideo_ = pipe->video(entities_.back().entity);\n> \tASSERT(video_);\n> \n> +\tfor (auto it = entities_.rbegin(); it != entities_.rend(); ++it) {\n> +\t\tif (it->entity->type() == MediaEntity::Type::V4L2Subdevice) {\n> +\t\t\tsubdev_ = pipe->subdev(it->entity);\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> \t/*\n> \t * Setup links first as some subdev drivers take active links into\n> \t * account to propagate TRY formats. Such is life :-(\n> @@ -1299,7 +1307,7 @@ 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> +\tdata->subdev_->frameStart.connect(data->delayedCtrls_.get(),\n> \t\t\t\t\t &DelayedControls::applyControls);\n> \n> \tStreamConfiguration inputCfg;\n> @@ -1339,6 +1347,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> {\n> \tSimpleCameraData *data = cameraData(camera);\n> \tV4L2VideoDevice *video = data->video_;\n> +\tV4L2Subdevice *subdev = data->subdev_;\n> \tint ret;\n> \n> \tconst MediaPad *pad = acquirePipeline(data);\n> @@ -1367,6 +1376,12 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> \t}\n> \n> \tvideo->bufferReady.connect(data, &SimpleCameraData::imageBufferReady);\n> +\tif (subdev) {\n> +\t\tLOG(SimplePipeline, Debug)\n> +\t\t\t<< \"Enable frame start event on \"\n> +\t\t\t<< subdev->entity()->name();\n> +\t\tsubdev->setFrameStartEnabled(true);\n> +\t}\n> \n> \tret = video->streamOn();\n> \tif (ret < 0) {","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 45999C3301\n\tfor ;\n\tWed, 18 Dec 2024 20:50:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 603A1680B6;\n\tWed, 18 Dec 2024 21:50:57 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 82001680AE\n\tfor ;\n\tWed, 18 Dec 2024 21:50:56 +0100 (CET)","from mail-ed1-f72.google.com (mail-ed1-f72.google.com\n\t[209.85.208.72]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-149--r_spYVUPXex9e8FZFmsxw-1; Wed, 18 Dec 2024 15:50:54 -0500","by mail-ed1-f72.google.com with SMTP id\n\t4fb4d7f45d1cf-5d3f55f8f3aso61793a12.2\n\tfor ;\n\tWed, 18 Dec 2024 12:50:53 -0800 (PST)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\ta640c23a62f3a-aab9606b540sm591558566b.80.2024.12.18.12.50.51\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 18 Dec 2024 12:50:51 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"VRIBNdhU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1734555055;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=MPIo2PNVPyDQhkbtBj+t/z77sjPeMTCIvbQOJ+F0pFc=;\n\tb=VRIBNdhUROd8Cwp7HAF1L4upJ+9p2ATwvcUQ+cBnJ0IWdVfWZ4TZK3gWJA5daYaLs/DZ+2\n\tk9rvLgLigms2DMT38BQKKQv86DL+F7hJkTvQHNmBgPpHWhkstUFkdBv2U909W1MN/LAEaY\n\t0zMPVs9hSKUKmvIRpraLHlcpZZgfQrE=","X-MC-Unique":"-r_spYVUPXex9e8FZFmsxw-1","X-Mimecast-MFC-AGG-ID":"-r_spYVUPXex9e8FZFmsxw","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1734555053; x=1735159853;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=MPIo2PNVPyDQhkbtBj+t/z77sjPeMTCIvbQOJ+F0pFc=;\n\tb=Ho5ByIH6fsB75AFfomamE6rkIATcfIL3kd7n7of1LEgEn3eexfIQ2+3er89/VsAYHv\n\tnIFhvYgvddayRNAzq7VUf16CuANaYgjP2a/5Gv/EKXNzUt4SocsGdRRihQtTPyGH4WUQ\n\tNo0p82Rw3jeGb05WNUWTJj+ASmkTOjsylbE/1sjrFRZe4qJZKiOES/DP3YE5/Lqniyyk\n\tEf7upg9UpzJABcEJQ06ZxelIgcrKEElFT/jYjDVzwvDC++q6ig0ELpP0h+rzCNG3z46W\n\tm/nzA9gQswrS+BUUbV+tkvvw42Qhu87+AMWnnJzQ207GUiHU58kTX8RHDnMtL4dMUH1n\n\tAkGw==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCUw4duQ0sNrMSsLp3CQ9FJEjeIkzLobv2rCfThCYgKPOhNbC0SP4KViOaLFEf1n1qURc0oidSjd7rxS8yz8cn0=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YwRKHu6xmormBm07C1wEPkY4zABdItSIHdiLL/AxqjY9AWYUXCG\n\tjTFnka/5fXaFvHff2JWaNlHrz2ljrgpCuMRQbAmlNDDUsX6xYSuH5vAIYvXIiwKFwCwjfBO0wIE\n\tIW+DqnYJiB8k4bIV0qc4uGmgA0BHcEJVX76AgVOVXgs+YlOjPRFaH950rWe2IsRngi2q9urY=","X-Gm-Gg":"ASbGncs1tt7tuiMOoClpfrqpdorYiAnqUX2Ex4hKDMUxCen/cGsdydNtfzq5ZcNS1DT\n\tkJI0WaRLa8NWPqQTApywjVd4OVU8qT2Qc4yyrSKAPsNUT0kl/O/bd2UERF0aFVvFgk+bMLPF7mE\n\tz+TEL7wzLbszLuH7U5WiIJM6gtUXfRmChsa+9+s48u8kuXacn6RIrWybHzD0uyj68/XDCaS10nk\n\tsrxYqcC/CkcwFOi6IzcRQLgVCM9+UCGjFumHZWC4Z3HYwWsNRiIW+AsQyoq0rSjGRLgFlOxN7DB\n\tbA==","X-Received":["by 2002:a17:907:9615:b0:aab:8ca7:43e5 with SMTP id\n\ta640c23a62f3a-aabf490cf11mr391743766b.38.1734555052574; \n\tWed, 18 Dec 2024 12:50:52 -0800 (PST)","by 2002:a17:907:9615:b0:aab:8ca7:43e5 with SMTP id\n\ta640c23a62f3a-aabf490cf11mr391741666b.38.1734555052111; \n\tWed, 18 Dec 2024 12:50:52 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IGwETKQREAGD2wgd0kAk23LrD5xKSCHdmv+0UgKcB4jiAaRfqN5zFyI8LDRkE+1vIe8yYdu2A==","From":"Milan Zamazal ","To":"Stanislaw Gruszka ","Cc":"Laurent Pinchart , Kieran Bingham\n\t, libcamera-devel@lists.libcamera.org,\n\tNaushir Patuck ","Subject":"Re: [RFC] delayed_controls: avoid reading undefined control value","In-Reply-To":" (Stanislaw Gruszka's message\n\tof \"Thu, 12 Dec 2024 11:45:15 +0100\")","References":"<20241206112743.95435-1-stanislaw.gruszka@linux.intel.com>\n\t<173387039086.1401494.2029006348016958056@ping.linuxembedded.co.uk>\n\t<20241211074329.GA24546@pendragon.ideasonboard.com>\n\t","Date":"Wed, 18 Dec 2024 21:50:50 +0100","Message-ID":"<87ldwc3ec5.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"_RG-KZ93ZOt-gCjv0ogmup7tgBV3sjqV4_Kj0i6ST20_1734555053","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","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\" "}}]