[{"id":15461,"web_url":"https://patchwork.libcamera.org/comment/15461/","msgid":"","date":"2021-03-03T18:45:22","subject":"Re: [libcamera-devel] [PATCH 0/3] IPU3 Stability","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Wed, Mar 03, 2021 at 05:04:23PM +0000, Kieran Bingham wrote:\n> While working on the IPU3 IPA, some crashes have been occuring on\n> shutdown races.\n> \n> Patches 2/3 and 3/3 at least presently work around those issues, and\n> while they may be suitable for integration now - both suggest that more\n> work is needed to ensure that the IPU3 pipeline handler is stable and\n> implemented correctly.\n> \n> Patch 2/3 highlights that we may need to take more concern over the\n> lifetime management of a Request and the associated FrameInfo that is\n> supported with that.\n\nI think 2/3 is suitable for integration, even if API improvements are\npossible. This isn't a completely unexpected situation, now that we have\nan implementation we find the related issues, that's a way to improve\nAPIs.\n\n> Patch 3/3 shows that the IPU3 Pipeline handler is queueing buffers after\n> it has released the buffers on the video nodes, and needs further\n> consideration as well (though I beleive that patch is still worthy of\n> integration on it's own).\n\nI have more doubts about this one, as there's really something that\nshould be fixed on the pipeline handler side. I was considering\nproposing the Fatal log level, but I suppose it will not make a big\ndifference compared to a segfault :-) Or maybe it would, the log message\ncan then clearly state that the problem is in the caller.\n\n> The issue leading to what would be a crash without patch 3/3 is when\n> shutting down, the IPA running in parallel can emit an event from:\n> void IPU3CameraData::queueFrameAction()\n> as \n> \n> case ipa::ipu3::ActionParamFilled:\n> \n> which then goes on to queue a buffer to the three relevant V4L2 devices:\n> \n> \timgu_->param_->queueBuffer(info->paramBuffer);\n> \timgu_->stat_->queueBuffer(info->statBuffer);\n> \timgu_->input_->queueBuffer(info->rawBuffer);\n> \n> \n> This is occuring after those devices have released their buffers, and\n> thus the v4l2 buffer cache (cache_) is deleted and set to nullptr,\n> resulting in a segmentation fault within the V4L2VideoDevice otherwise.\n\nLooking at PipelineHandlerIPU3::stop(), we have\n\n\tdata->ipa_->stop();\n\n\tfreeBuffers(camera);\n\nThe IPA stop() call should be synchronous, which means that any pending\nasynchronous call from the IPA module to the pipeline handler should be\nprocessed before stop() returns.\n\nOn the IPA side, the stop() function should stop any internal thread,\nwhich will make sure that the API won't emit any event on its own after\nstop() returns. The stop() implementation is currently empty, but as\nwe're not creating any custom thread in the IPU3 IPA at this time, it\nshouldn't be a problem.\n\nIt could also be that the pipeline handler sends more frame events to\nthe IPA module after stop(), but given the cross-thread nature of the\ncalls, I wouldn't expect that to go through if the IPA thread created by\nthe proxy is stopped. Still, it may be worth double-checking this.\n\nTracing the IPA calls could be useful here. Please let me know if I can\nhelp.\n\n> Kieran Bingham (3):\n> libcamera: pipeline: ipu3: Fix spelling error\n> libcamera: pipeline: ipu3: Ensure that IPU3Frames::info is not used\n> after delete\n> libcamera: v4l2_videodevice: Prevent queueing buffers without a cache\n> \n> src/libcamera/pipeline/ipu3/frames.cpp | 2 +-\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 20 ++++++++++++++++++--\n> src/libcamera/v4l2_videodevice.cpp | 10 ++++++++++\n> 3 files changed, 29 insertions(+), 3 deletions(-)\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 8B7CCBD1F1\n\tfor ;\n\tWed, 3 Mar 2021 18:45:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 11E9168A98;\n\tWed, 3 Mar 2021 19:45:53 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 15B4B68A7E\n\tfor ;\n\tWed, 3 Mar 2021 19:45:52 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 81DC08CA;\n\tWed, 3 Mar 2021 19:45:51 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"oXm+9evw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1614797151;\n\tbh=Bwr0xtEPxea/weMZSU/TZUTlOi+aG9o1al6YBzmxaoQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=oXm+9evwD2Q47lgJFkIqBuohDZdOJkVLwTyEFJv8DecfrmTIhnzOwTkFwLyBV03k4\n\tfIFcyK2wzYhzjk79jpFJlU8DOLYgzcoFLqGiLW6Zv18DS+oQztEC271w1V/tjCPORA\n\tTUnTfLJp7Ariou7gIG7Q6ZUNCwOQ60slYd9T/afg=","Date":"Wed, 3 Mar 2021 20:45:22 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210303170426.189648-1-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210303170426.189648-1-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 0/3] IPU3 Stability","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","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15479,"web_url":"https://patchwork.libcamera.org/comment/15479/","msgid":"<92b2159b-2a2a-b3bb-e01b-500448d40001@ideasonboard.com>","date":"2021-03-04T10:14:40","subject":"Re: [libcamera-devel] [PATCH 0/3] IPU3 Stability","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 03/03/2021 18:45, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> On Wed, Mar 03, 2021 at 05:04:23PM +0000, Kieran Bingham wrote:\n>> While working on the IPU3 IPA, some crashes have been occuring on\n>> shutdown races.\n>>\n>> Patches 2/3 and 3/3 at least presently work around those issues, and\n>> while they may be suitable for integration now - both suggest that more\n>> work is needed to ensure that the IPU3 pipeline handler is stable and\n>> implemented correctly.\n>>\n>> Patch 2/3 highlights that we may need to take more concern over the\n>> lifetime management of a Request and the associated FrameInfo that is\n>> supported with that.\n> \n> I think 2/3 is suitable for integration, even if API improvements are\n> possible. This isn't a completely unexpected situation, now that we have\n> an implementation we find the related issues, that's a way to improve\n> APIs.\n\nYes, well we certainly need to fix it to unblock other developments.\n\n>> Patch 3/3 shows that the IPU3 Pipeline handler is queueing buffers after\n>> it has released the buffers on the video nodes, and needs further\n>> consideration as well (though I beleive that patch is still worthy of\n>> integration on it's own).\n> \n> I have more doubts about this one, as there's really something that\n> should be fixed on the pipeline handler side. I was considering\n> proposing the Fatal log level, but I suppose it will not make a big\n> difference compared to a segfault :-) Or maybe it would, the log message\n> can then clearly state that the problem is in the caller.\n\n\nYou know, locally it's a fatal on my tree right now, so it's harder to\ndisagree, but in this patch I set it to Error because I believe it's\nbetter not to crash right now.\n\nIt's not a Fatally unrecoverable error. It's an issue in the the\npipeline handler, doing something it shouldn't, and it is recoverable\n(Just don't queue).\n\nBut it should still be fixed by the pipeline handler, hence loud error\nmessage.\n\nI would love to be able to make this an Error with a backtrace ... to be\n/really/ loud, but I'm not sure yet about having an extra log level\nbetween Error and Fatal.\n\n\n\n\n>> The issue leading to what would be a crash without patch 3/3 is when\n>> shutting down, the IPA running in parallel can emit an event from:\n>> void IPU3CameraData::queueFrameAction()\n>> as \n>>\n>> case ipa::ipu3::ActionParamFilled:\n>>\n>> which then goes on to queue a buffer to the three relevant V4L2 devices:\n>>\n>> \timgu_->param_->queueBuffer(info->paramBuffer);\n>> \timgu_->stat_->queueBuffer(info->statBuffer);\n>> \timgu_->input_->queueBuffer(info->rawBuffer);\n>>\n>>\n>> This is occuring after those devices have released their buffers, and\n>> thus the v4l2 buffer cache (cache_) is deleted and set to nullptr,\n>> resulting in a segmentation fault within the V4L2VideoDevice otherwise.\n> \n> Looking at PipelineHandlerIPU3::stop(), we have\n> \n> \tdata->ipa_->stop();\n> \n> \tfreeBuffers(camera);\n> \n> The IPA stop() call should be synchronous, which means that any pending\n> asynchronous call from the IPA module to the pipeline handler should be\n> processed before stop() returns.\n> \n> On the IPA side, the stop() function should stop any internal thread,\n> which will make sure that the API won't emit any event on its own after\n> stop() returns. The stop() implementation is currently empty, but as\n> we're not creating any custom thread in the IPU3 IPA at this time, it\n> shouldn't be a problem.\n> \n> It could also be that the pipeline handler sends more frame events to\n> the IPA module after stop(), but given the cross-thread nature of the\n> calls, I wouldn't expect that to go through if the IPA thread created by\n> the proxy is stopped. Still, it may be worth double-checking this.\n> \n> Tracing the IPA calls could be useful here. Please let me know if I can\n> help.\n\nI enabled tracing with:\n lttng enable-event -u libcamera:\\*\n\nbut got not IPA traces.\n\nOk - so I've discussed this with Paul, and it's because IPA traces\naren't enabled ;-)\n\nAdding a patch he has adds more context there, and it seems like we\nmight be able to add more tracepoints automatically to see when messages\nare sent back from the IPA - that will make it clear if the IPA is\nsending a fillParams message back /after/ having already stopped.\n\nI'll defer further investigation on this until after I have that\nvisibility for now.\n\n\n\n>> Kieran Bingham (3):\n>> libcamera: pipeline: ipu3: Fix spelling error\n>> libcamera: pipeline: ipu3: Ensure that IPU3Frames::info is not used\n>> after delete\n>> libcamera: v4l2_videodevice: Prevent queueing buffers without a cache\n>>\n>> src/libcamera/pipeline/ipu3/frames.cpp | 2 +-\n>> src/libcamera/pipeline/ipu3/ipu3.cpp | 20 ++++++++++++++++++--\n>> src/libcamera/v4l2_videodevice.cpp | 10 ++++++++++\n>> 3 files changed, 29 insertions(+), 3 deletions(-)\n>>\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 61FB0BD80C\n\tfor ;\n\tThu, 4 Mar 2021 10:14:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C167568A92;\n\tThu, 4 Mar 2021 11:14:45 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8283E602EC\n\tfor ;\n\tThu, 4 Mar 2021 11:14:44 +0100 (CET)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4E52527A;\n\tThu, 4 Mar 2021 11:14:43 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"r5PRBXAm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1614852883;\n\tbh=so1QpRSKrx95bCTtD3XtH1UYi+sIe9bF/3HFSETBnQk=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=r5PRBXAmZ2fARDjFUBs0IMt80VzoDud26AETbUBlu4vwdrstEM+ttnZsWtXFi0f2A\n\tTHH4x/BOE17iMWJe2wUUITMQB7GKRwsYSfmaeKFtqe/4iEehbC7lLEtwgQ9kccE8a8\n\tF9Qen+uIzMDfybiLYRdAfsfOmeI8QYDBkK61uM7g=","To":"Laurent Pinchart ,\n\tPaul Elder ","References":"<20210303170426.189648-1-kieran.bingham@ideasonboard.com>\n\t","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<92b2159b-2a2a-b3bb-e01b-500448d40001@ideasonboard.com>","Date":"Thu, 4 Mar 2021 10:14:40 +0000","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":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH 0/3] IPU3 Stability","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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15480,"web_url":"https://patchwork.libcamera.org/comment/15480/","msgid":"","date":"2021-03-04T14:12:18","subject":"Re: [libcamera-devel] [PATCH 0/3] IPU3 Stability","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Thu, Mar 04, 2021 at 10:14:40AM +0000, Kieran Bingham wrote:\n> On 03/03/2021 18:45, Laurent Pinchart wrote:\n> > On Wed, Mar 03, 2021 at 05:04:23PM +0000, Kieran Bingham wrote:\n> >> While working on the IPU3 IPA, some crashes have been occuring on\n> >> shutdown races.\n> >>\n> >> Patches 2/3 and 3/3 at least presently work around those issues, and\n> >> while they may be suitable for integration now - both suggest that more\n> >> work is needed to ensure that the IPU3 pipeline handler is stable and\n> >> implemented correctly.\n> >>\n> >> Patch 2/3 highlights that we may need to take more concern over the\n> >> lifetime management of a Request and the associated FrameInfo that is\n> >> supported with that.\n> > \n> > I think 2/3 is suitable for integration, even if API improvements are\n> > possible. This isn't a completely unexpected situation, now that we have\n> > an implementation we find the related issues, that's a way to improve\n> > APIs.\n> \n> Yes, well we certainly need to fix it to unblock other developments.\n> \n> >> Patch 3/3 shows that the IPU3 Pipeline handler is queueing buffers after\n> >> it has released the buffers on the video nodes, and needs further\n> >> consideration as well (though I beleive that patch is still worthy of\n> >> integration on it's own).\n> > \n> > I have more doubts about this one, as there's really something that\n> > should be fixed on the pipeline handler side. I was considering\n> > proposing the Fatal log level, but I suppose it will not make a big\n> > difference compared to a segfault :-) Or maybe it would, the log message\n> > can then clearly state that the problem is in the caller.\n> \n> You know, locally it's a fatal on my tree right now, so it's harder to\n> disagree, but in this patch I set it to Error because I believe it's\n> better not to crash right now.\n\n:-)\n\n> It's not a Fatally unrecoverable error. It's an issue in the the\n> pipeline handler, doing something it shouldn't, and it is recoverable\n> (Just don't queue).\n\nThe part on the V4L2VideoDevice side may be, but it's a symptom of\nsomething really wrong happening, which will likely cause other\nundesired behaviour in the pipeline handler. What those behaviours will\nbe depend on the pipeline handler, but the worst case would be some\nsilent memory corruption that will cause a crash later. A fatal error\nhere would help pin-pointing the root cause, making debugging of the\npipeline handler easier.\n\nGranted, the error message will help there too (returning an error\nsilently would be the worst option). I suppose our ways of screaming\nloudly differ, mine goes through an assertion failure :-)\n\n> But it should still be fixed by the pipeline handler, hence loud error\n> message.\n> \n> I would love to be able to make this an Error with a backtrace ... to be\n> /really/ loud, but I'm not sure yet about having an extra log level\n> between Error and Fatal.\n\nIt's an interesting thought, I'll sleep on it.\n\n> >> The issue leading to what would be a crash without patch 3/3 is when\n> >> shutting down, the IPA running in parallel can emit an event from:\n> >> void IPU3CameraData::queueFrameAction()\n> >> as \n> >>\n> >> case ipa::ipu3::ActionParamFilled:\n> >>\n> >> which then goes on to queue a buffer to the three relevant V4L2 devices:\n> >>\n> >> \timgu_->param_->queueBuffer(info->paramBuffer);\n> >> \timgu_->stat_->queueBuffer(info->statBuffer);\n> >> \timgu_->input_->queueBuffer(info->rawBuffer);\n> >>\n> >>\n> >> This is occuring after those devices have released their buffers, and\n> >> thus the v4l2 buffer cache (cache_) is deleted and set to nullptr,\n> >> resulting in a segmentation fault within the V4L2VideoDevice otherwise.\n> > \n> > Looking at PipelineHandlerIPU3::stop(), we have\n> > \n> > \tdata->ipa_->stop();\n> > \n> > \tfreeBuffers(camera);\n> > \n> > The IPA stop() call should be synchronous, which means that any pending\n> > asynchronous call from the IPA module to the pipeline handler should be\n> > processed before stop() returns.\n> > \n> > On the IPA side, the stop() function should stop any internal thread,\n> > which will make sure that the API won't emit any event on its own after\n> > stop() returns. The stop() implementation is currently empty, but as\n> > we're not creating any custom thread in the IPU3 IPA at this time, it\n> > shouldn't be a problem.\n> > \n> > It could also be that the pipeline handler sends more frame events to\n> > the IPA module after stop(), but given the cross-thread nature of the\n> > calls, I wouldn't expect that to go through if the IPA thread created by\n> > the proxy is stopped. Still, it may be worth double-checking this.\n> > \n> > Tracing the IPA calls could be useful here. Please let me know if I can\n> > help.\n> \n> I enabled tracing with:\n> lttng enable-event -u libcamera:\\*\n> \n> but got not IPA traces.\n> \n> Ok - so I've discussed this with Paul, and it's because IPA traces\n> aren't enabled ;-)\n> \n> Adding a patch he has adds more context there, and it seems like we\n> might be able to add more tracepoints automatically to see when messages\n> are sent back from the IPA - that will make it clear if the IPA is\n> sending a fillParams message back /after/ having already stopped.\n> \n> I'll defer further investigation on this until after I have that\n> visibility for now.\n> \n> >> Kieran Bingham (3):\n> >> libcamera: pipeline: ipu3: Fix spelling error\n> >> libcamera: pipeline: ipu3: Ensure that IPU3Frames::info is not used\n> >> after delete\n> >> libcamera: v4l2_videodevice: Prevent queueing buffers without a cache\n> >>\n> >> src/libcamera/pipeline/ipu3/frames.cpp | 2 +-\n> >> src/libcamera/pipeline/ipu3/ipu3.cpp | 20 ++++++++++++++++++--\n> >> src/libcamera/v4l2_videodevice.cpp | 10 ++++++++++\n> >> 3 files changed, 29 insertions(+), 3 deletions(-)\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 9AA5DBD80C\n\tfor ;\n\tThu, 4 Mar 2021 14:12:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EABF668A93;\n\tThu, 4 Mar 2021 15:12:49 +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 4E979602ED\n\tfor ;\n\tThu, 4 Mar 2021 15:12:49 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A3DCD27A;\n\tThu, 4 Mar 2021 15:12:48 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"qOrwtr02\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1614867168;\n\tbh=dsRBUtByl++B5y6vmAaopFek/Ro3PeE+UcUIEaamZfA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qOrwtr02dOyDpvsg+OgRcRYhcPDjlAxfscvvtjEyOnd7wPTGvZdmj/2TRob4ruLEK\n\tBOjqQkOLgmaVzTyFJ5QJDxX9HxMjxUCo1bF/XuzLHtuvyuUMngFQUhhH0DaOXHOPzB\n\ti7mI63QcqdibBAi4ooX0ctV4eEce1TIoAUD30HEI=","Date":"Thu, 4 Mar 2021 16:12:18 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210303170426.189648-1-kieran.bingham@ideasonboard.com>\n\t\n\t<92b2159b-2a2a-b3bb-e01b-500448d40001@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<92b2159b-2a2a-b3bb-e01b-500448d40001@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 0/3] IPU3 Stability","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","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15490,"web_url":"https://patchwork.libcamera.org/comment/15490/","msgid":"<20210305194509.qx6nqcbqlgm5accm@uno.localdomain>","date":"2021-03-05T19:45:09","subject":"Re: [libcamera-devel] [PATCH 0/3] IPU3 Stability","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran\n\nOn Wed, Mar 03, 2021 at 05:04:23PM +0000, Kieran Bingham wrote:\n> While working on the IPU3 IPA, some crashes have been occuring on\n> shutdown races.\n>\n> Patches 2/3 and 3/3 at least presently work around those issues, and\n> while they may be suitable for integration now - both suggest that more\n> work is needed to ensure that the IPU3 pipeline handler is stable and\n> implemented correctly.\n>\n> Patch 2/3 highlights that we may need to take more concern over the\n> lifetime management of a Request and the associated FrameInfo that is\n> supported with that.\n>\n> Patch 3/3 shows that the IPU3 Pipeline handler is queueing buffers after\n> it has released the buffers on the video nodes, and needs further\n> consideration as well (though I beleive that patch is still worthy of\n> integration on it's own).\n>\n> The issue leading to what would be a crash without patch 3/3 is when\n> shutting down, the IPA running in parallel can emit an event from:\n> void IPU3CameraData::queueFrameAction()\n> as\n>\n> case ipa::ipu3::ActionParamFilled:\n>\n> which then goes on to queue a buffer to the three relevant V4L2 devices:\n>\n> \timgu_->param_->queueBuffer(info->paramBuffer);\n> \timgu_->stat_->queueBuffer(info->statBuffer);\n> \timgu_->input_->queueBuffer(info->rawBuffer);\n>\n>\n> This is occuring after those devices have released their buffers, and\n> thus the v4l2 buffer cache (cache_) is deleted and set to nullptr,\n> resulting in a segmentation fault within the V4L2VideoDevice otherwise.\n\nI've been running these patches in my tree for a few days and indeed I\nhaven't noticed the nasty segfaults I've been having before when\nrunning CTS on IPU3.\n\nFor the series\nTested-by: Jacopo Mondi \n\nI'll review patches singularly next\n\nThanks\n j\n\n>\n>\n>\n> Kieran Bingham (3):\n> libcamera: pipeline: ipu3: Fix spelling error\n> libcamera: pipeline: ipu3: Ensure that IPU3Frames::info is not used\n> after delete\n> libcamera: v4l2_videodevice: Prevent queueing buffers without a cache\n>\n> src/libcamera/pipeline/ipu3/frames.cpp | 2 +-\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 20 ++++++++++++++++++--\n> src/libcamera/v4l2_videodevice.cpp | 10 ++++++++++\n> 3 files changed, 29 insertions(+), 3 deletions(-)\n>\n> --\n> 2.25.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 9F294BD1F1\n\tfor ;\n\tFri, 5 Mar 2021 19:44:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0BBC568A9F;\n\tFri, 5 Mar 2021 20:44:42 +0100 (CET)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3387868A69\n\tfor ;\n\tFri, 5 Mar 2021 20:44:41 +0100 (CET)","from uno.localdomain (host-79-22-58-175.retail.telecomitalia.it\n\t[79.22.58.175]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 70D391C0005;\n\tFri, 5 Mar 2021 19:44:40 +0000 (UTC)"],"X-Originating-IP":"79.22.58.175","Date":"Fri, 5 Mar 2021 20:45:09 +0100","From":"Jacopo Mondi ","To":"Kieran Bingham ","Message-ID":"<20210305194509.qx6nqcbqlgm5accm@uno.localdomain>","References":"<20210303170426.189648-1-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210303170426.189648-1-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 0/3] IPU3 Stability","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","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15509,"web_url":"https://patchwork.libcamera.org/comment/15509/","msgid":"<9b6f19f9-9615-223f-ff12-86e44db33ea8@ideasonboard.com>","date":"2021-03-08T12:06:20","subject":"Re: [libcamera-devel] [PATCH 0/3] IPU3 Stability","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 04/03/2021 14:12, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> On Thu, Mar 04, 2021 at 10:14:40AM +0000, Kieran Bingham wrote:\n>> On 03/03/2021 18:45, Laurent Pinchart wrote:\n>>> On Wed, Mar 03, 2021 at 05:04:23PM +0000, Kieran Bingham wrote:\n>>>> While working on the IPU3 IPA, some crashes have been occuring on\n>>>> shutdown races.\n>>>>\n>>>> Patches 2/3 and 3/3 at least presently work around those issues, and\n>>>> while they may be suitable for integration now - both suggest that more\n>>>> work is needed to ensure that the IPU3 pipeline handler is stable and\n>>>> implemented correctly.\n>>>>\n>>>> Patch 2/3 highlights that we may need to take more concern over the\n>>>> lifetime management of a Request and the associated FrameInfo that is\n>>>> supported with that.\n>>>\n>>> I think 2/3 is suitable for integration, even if API improvements are\n>>> possible. This isn't a completely unexpected situation, now that we have\n>>> an implementation we find the related issues, that's a way to improve\n>>> APIs.\n>>\n>> Yes, well we certainly need to fix it to unblock other developments.\n>>\n>>>> Patch 3/3 shows that the IPU3 Pipeline handler is queueing buffers after\n>>>> it has released the buffers on the video nodes, and needs further\n>>>> consideration as well (though I beleive that patch is still worthy of\n>>>> integration on it's own).\n>>>\n>>> I have more doubts about this one, as there's really something that\n>>> should be fixed on the pipeline handler side. I was considering\n>>> proposing the Fatal log level, but I suppose it will not make a big\n>>> difference compared to a segfault :-) Or maybe it would, the log message\n>>> can then clearly state that the problem is in the caller.\n>>\n>> You know, locally it's a fatal on my tree right now, so it's harder to\n>> disagree, but in this patch I set it to Error because I believe it's\n>> better not to crash right now.\n> \n> :-)\n> \n>> It's not a Fatally unrecoverable error. It's an issue in the the\n>> pipeline handler, doing something it shouldn't, and it is recoverable\n>> (Just don't queue).\n> \n> The part on the V4L2VideoDevice side may be, but it's a symptom of\n> something really wrong happening, which will likely cause other\n> undesired behaviour in the pipeline handler. What those behaviours will\n> be depend on the pipeline handler, but the worst case would be some\n> silent memory corruption that will cause a crash later. A fatal error\n> here would help pin-pointing the root cause, making debugging of the\n> pipeline handler easier.\n> \n> Granted, the error message will help there too (returning an error\n> silently would be the worst option). I suppose our ways of screaming\n> loudly differ, mine goes through an assertion failure :-)\n> \n>> But it should still be fixed by the pipeline handler, hence loud error\n>> message.\n>>\n>> I would love to be able to make this an Error with a backtrace ... to be\n>> /really/ loud, but I'm not sure yet about having an extra log level\n>> between Error and Fatal.\n> \n> It's an interesting thought, I'll sleep on it.\n\nDid you get any sleep?\n\n\nI'd like to get these patches in at least so that we can ensure IPA\ndevelopment can continue.\n\nWe can always make queuing a buffer to V4L2VideoDevice after it's been\nstopped a fatal error (Or at least a backtracable error) after the\npipeline handler has been fixed...\n\n\n\n>>>> The issue leading to what would be a crash without patch 3/3 is when\n>>>> shutting down, the IPA running in parallel can emit an event from:\n>>>> void IPU3CameraData::queueFrameAction()\n>>>> as \n>>>>\n>>>> case ipa::ipu3::ActionParamFilled:\n>>>>\n>>>> which then goes on to queue a buffer to the three relevant V4L2 devices:\n>>>>\n>>>> \timgu_->param_->queueBuffer(info->paramBuffer);\n>>>> \timgu_->stat_->queueBuffer(info->statBuffer);\n>>>> \timgu_->input_->queueBuffer(info->rawBuffer);\n>>>>\n>>>>\n>>>> This is occuring after those devices have released their buffers, and\n>>>> thus the v4l2 buffer cache (cache_) is deleted and set to nullptr,\n>>>> resulting in a segmentation fault within the V4L2VideoDevice otherwise.\n>>>\n>>> Looking at PipelineHandlerIPU3::stop(), we have\n>>>\n>>> \tdata->ipa_->stop();\n>>>\n>>> \tfreeBuffers(camera);\n>>>\n>>> The IPA stop() call should be synchronous, which means that any pending\n>>> asynchronous call from the IPA module to the pipeline handler should be\n>>> processed before stop() returns.\n>>>\n>>> On the IPA side, the stop() function should stop any internal thread,\n>>> which will make sure that the API won't emit any event on its own after\n>>> stop() returns. The stop() implementation is currently empty, but as\n>>> we're not creating any custom thread in the IPU3 IPA at this time, it\n>>> shouldn't be a problem.\n>>>\n>>> It could also be that the pipeline handler sends more frame events to\n>>> the IPA module after stop(), but given the cross-thread nature of the\n>>> calls, I wouldn't expect that to go through if the IPA thread created by\n>>> the proxy is stopped. Still, it may be worth double-checking this.\n>>>\n>>> Tracing the IPA calls could be useful here. Please let me know if I can\n>>> help.\n>>\n>> I enabled tracing with:\n>> lttng enable-event -u libcamera:\\*\n>>\n>> but got not IPA traces.\n>>\n>> Ok - so I've discussed this with Paul, and it's because IPA traces\n>> aren't enabled ;-)\n>>\n>> Adding a patch he has adds more context there, and it seems like we\n>> might be able to add more tracepoints automatically to see when messages\n>> are sent back from the IPA - that will make it clear if the IPA is\n>> sending a fillParams message back /after/ having already stopped.\n>>\n>> I'll defer further investigation on this until after I have that\n>> visibility for now.\n>>\n>>>> Kieran Bingham (3):\n>>>> libcamera: pipeline: ipu3: Fix spelling error\n>>>> libcamera: pipeline: ipu3: Ensure that IPU3Frames::info is not used\n>>>> after delete\n>>>> libcamera: v4l2_videodevice: Prevent queueing buffers without a cache\n>>>>\n>>>> src/libcamera/pipeline/ipu3/frames.cpp | 2 +-\n>>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 20 ++++++++++++++++++--\n>>>> src/libcamera/v4l2_videodevice.cpp | 10 ++++++++++\n>>>> 3 files changed, 29 insertions(+), 3 deletions(-)\n>>>>\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 976B8BD80C\n\tfor ;\n\tMon, 8 Mar 2021 12:06:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 141FD68A92;\n\tMon, 8 Mar 2021 13:06: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 6C569602E6\n\tfor ;\n\tMon, 8 Mar 2021 13:06:23 +0100 (CET)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C861DAC1;\n\tMon, 8 Mar 2021 13:06:22 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"qp9eUOPz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1615205183;\n\tbh=okDEswRcbeSX32ox/njvo1cz8jGYXsIngtFZUXPBlNU=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=qp9eUOPzDvamwbCiB0/Kg6bicfSbfKIus8XBWE+bWAeXNywnK8g16IUDx7MunYGcw\n\t92mJ+LMRjUPdSdqERGRopmO1L3tpqOYduP9uWNx2vI3CqdAQMsIC/L5uIWalse91LB\n\tv2HY3M/nPTfBiOaVAewsWRrK4KKX9CgyYGAJdv8c=","To":"Laurent Pinchart ","References":"<20210303170426.189648-1-kieran.bingham@ideasonboard.com>\n\t\n\t<92b2159b-2a2a-b3bb-e01b-500448d40001@ideasonboard.com>\n\t","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<9b6f19f9-9615-223f-ff12-86e44db33ea8@ideasonboard.com>","Date":"Mon, 8 Mar 2021 12:06:20 +0000","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":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH 0/3] IPU3 Stability","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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15522,"web_url":"https://patchwork.libcamera.org/comment/15522/","msgid":"","date":"2021-03-08T15:00:06","subject":"Re: [libcamera-devel] [PATCH 0/3] IPU3 Stability","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Mon, Mar 08, 2021 at 12:06:20PM +0000, Kieran Bingham wrote:\n> On 04/03/2021 14:12, Laurent Pinchart wrote:\n> > On Thu, Mar 04, 2021 at 10:14:40AM +0000, Kieran Bingham wrote:\n> >> On 03/03/2021 18:45, Laurent Pinchart wrote:\n> >>> On Wed, Mar 03, 2021 at 05:04:23PM +0000, Kieran Bingham wrote:\n> >>>> While working on the IPU3 IPA, some crashes have been occuring on\n> >>>> shutdown races.\n> >>>>\n> >>>> Patches 2/3 and 3/3 at least presently work around those issues, and\n> >>>> while they may be suitable for integration now - both suggest that more\n> >>>> work is needed to ensure that the IPU3 pipeline handler is stable and\n> >>>> implemented correctly.\n> >>>>\n> >>>> Patch 2/3 highlights that we may need to take more concern over the\n> >>>> lifetime management of a Request and the associated FrameInfo that is\n> >>>> supported with that.\n> >>>\n> >>> I think 2/3 is suitable for integration, even if API improvements are\n> >>> possible. This isn't a completely unexpected situation, now that we have\n> >>> an implementation we find the related issues, that's a way to improve\n> >>> APIs.\n> >>\n> >> Yes, well we certainly need to fix it to unblock other developments.\n> >>\n> >>>> Patch 3/3 shows that the IPU3 Pipeline handler is queueing buffers after\n> >>>> it has released the buffers on the video nodes, and needs further\n> >>>> consideration as well (though I beleive that patch is still worthy of\n> >>>> integration on it's own).\n> >>>\n> >>> I have more doubts about this one, as there's really something that\n> >>> should be fixed on the pipeline handler side. I was considering\n> >>> proposing the Fatal log level, but I suppose it will not make a big\n> >>> difference compared to a segfault :-) Or maybe it would, the log message\n> >>> can then clearly state that the problem is in the caller.\n> >>\n> >> You know, locally it's a fatal on my tree right now, so it's harder to\n> >> disagree, but in this patch I set it to Error because I believe it's\n> >> better not to crash right now.\n> > \n> > :-)\n> > \n> >> It's not a Fatally unrecoverable error. It's an issue in the the\n> >> pipeline handler, doing something it shouldn't, and it is recoverable\n> >> (Just don't queue).\n> > \n> > The part on the V4L2VideoDevice side may be, but it's a symptom of\n> > something really wrong happening, which will likely cause other\n> > undesired behaviour in the pipeline handler. What those behaviours will\n> > be depend on the pipeline handler, but the worst case would be some\n> > silent memory corruption that will cause a crash later. A fatal error\n> > here would help pin-pointing the root cause, making debugging of the\n> > pipeline handler easier.\n> > \n> > Granted, the error message will help there too (returning an error\n> > silently would be the worst option). I suppose our ways of screaming\n> > loudly differ, mine goes through an assertion failure :-)\n> > \n> >> But it should still be fixed by the pipeline handler, hence loud error\n> >> message.\n> >>\n> >> I would love to be able to make this an Error with a backtrace ... to be\n> >> /really/ loud, but I'm not sure yet about having an extra log level\n> >> between Error and Fatal.\n> > \n> > It's an interesting thought, I'll sleep on it.\n> \n> Did you get any sleep?\n\nYes :-)\n\nWe already have an error level with a backtrace, that's Fatal. Currently\nit terminates program execution with std::abort(), and I'd be fine only\ndoing so for debug builds. It doesn't seem we need an additional level.\n\n> I'd like to get these patches in at least so that we can ensure IPA\n> development can continue.\n\n1/3 and 2/3 can be merged right away.\n\nFor 3/3, I'll reply to the patch now.\n\n> We can always make queuing a buffer to V4L2VideoDevice after it's been\n> stopped a fatal error (Or at least a backtracable error) after the\n> pipeline handler has been fixed...\n> \n> >>>> The issue leading to what would be a crash without patch 3/3 is when\n> >>>> shutting down, the IPA running in parallel can emit an event from:\n> >>>> void IPU3CameraData::queueFrameAction()\n> >>>> as \n> >>>>\n> >>>> case ipa::ipu3::ActionParamFilled:\n> >>>>\n> >>>> which then goes on to queue a buffer to the three relevant V4L2 devices:\n> >>>>\n> >>>> \timgu_->param_->queueBuffer(info->paramBuffer);\n> >>>> \timgu_->stat_->queueBuffer(info->statBuffer);\n> >>>> \timgu_->input_->queueBuffer(info->rawBuffer);\n> >>>>\n> >>>>\n> >>>> This is occuring after those devices have released their buffers, and\n> >>>> thus the v4l2 buffer cache (cache_) is deleted and set to nullptr,\n> >>>> resulting in a segmentation fault within the V4L2VideoDevice otherwise.\n> >>>\n> >>> Looking at PipelineHandlerIPU3::stop(), we have\n> >>>\n> >>> \tdata->ipa_->stop();\n> >>>\n> >>> \tfreeBuffers(camera);\n> >>>\n> >>> The IPA stop() call should be synchronous, which means that any pending\n> >>> asynchronous call from the IPA module to the pipeline handler should be\n> >>> processed before stop() returns.\n> >>>\n> >>> On the IPA side, the stop() function should stop any internal thread,\n> >>> which will make sure that the API won't emit any event on its own after\n> >>> stop() returns. The stop() implementation is currently empty, but as\n> >>> we're not creating any custom thread in the IPU3 IPA at this time, it\n> >>> shouldn't be a problem.\n> >>>\n> >>> It could also be that the pipeline handler sends more frame events to\n> >>> the IPA module after stop(), but given the cross-thread nature of the\n> >>> calls, I wouldn't expect that to go through if the IPA thread created by\n> >>> the proxy is stopped. Still, it may be worth double-checking this.\n> >>>\n> >>> Tracing the IPA calls could be useful here. Please let me know if I can\n> >>> help.\n> >>\n> >> I enabled tracing with:\n> >> lttng enable-event -u libcamera:\\*\n> >>\n> >> but got not IPA traces.\n> >>\n> >> Ok - so I've discussed this with Paul, and it's because IPA traces\n> >> aren't enabled ;-)\n> >>\n> >> Adding a patch he has adds more context there, and it seems like we\n> >> might be able to add more tracepoints automatically to see when messages\n> >> are sent back from the IPA - that will make it clear if the IPA is\n> >> sending a fillParams message back /after/ having already stopped.\n> >>\n> >> I'll defer further investigation on this until after I have that\n> >> visibility for now.\n> >>\n> >>>> Kieran Bingham (3):\n> >>>> libcamera: pipeline: ipu3: Fix spelling error\n> >>>> libcamera: pipeline: ipu3: Ensure that IPU3Frames::info is not used\n> >>>> after delete\n> >>>> libcamera: v4l2_videodevice: Prevent queueing buffers without a cache\n> >>>>\n> >>>> src/libcamera/pipeline/ipu3/frames.cpp | 2 +-\n> >>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 20 ++++++++++++++++++--\n> >>>> src/libcamera/v4l2_videodevice.cpp | 10 ++++++++++\n> >>>> 3 files changed, 29 insertions(+), 3 deletions(-)\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 9375BBD80C\n\tfor ;\n\tMon, 8 Mar 2021 15:00:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D516D68A9A;\n\tMon, 8 Mar 2021 16:00:39 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C074D68A92\n\tfor ;\n\tMon, 8 Mar 2021 16:00:37 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 21CC7AC1;\n\tMon, 8 Mar 2021 16:00:37 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ryNTvsNT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1615215637;\n\tbh=pGa+1hkM0m8yTDHM/6FvvadXbXJrous1mDlBs94kJD0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ryNTvsNTyFx6GOYhJxjIN5eGLPEJuj5vHtmAY3z0/kIJQuK3kC7xqtbwDhAIIab4N\n\tx419W/yf2WvF2oXJV8ADx6CgKaHr57DpmKZf3/HB1D3Z79IZjC5Izd5u0+8w69gICq\n\tt9yaP1Zpheo9Ydx15m4BWxFPmCCO4IDNn7Cd5sbU=","Date":"Mon, 8 Mar 2021 17:00:06 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210303170426.189648-1-kieran.bingham@ideasonboard.com>\n\t\n\t<92b2159b-2a2a-b3bb-e01b-500448d40001@ideasonboard.com>\n\t\n\t<9b6f19f9-9615-223f-ff12-86e44db33ea8@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<9b6f19f9-9615-223f-ff12-86e44db33ea8@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 0/3] IPU3 Stability","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","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]