[{"id":23203,"web_url":"https://patchwork.libcamera.org/comment/23203/","msgid":"","date":"2022-05-27T19:09:13","subject":"Re: [libcamera-devel] [PATCH v3 18/30] py: unittests: Fix\n\ttest_select()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Tomi,\n\nThank you for the patch.\n\nOn Fri, May 27, 2022 at 05:44:35PM +0300, Tomi Valkeinen wrote:\n> The test_select() current uses self.assertTrue(len(ready_reqs) > 0) to\n\ns/current/currently/\n\n> see that cm.get_ready_requests() returns something. This is not always\n> the case, as there may be two eventfd events queued, and the first call\n> to cm.get_ready_requests() returns all the requests, and thus the second\n> call returns none.\n> \n> Remove the self.assertTrue(len(ready_reqs) > 0) assert.\n\nThis convinces me even more that there's room for improvement :-) It\nseems a bit confusing for applications.\n\n> Signed-off-by: Tomi Valkeinen \n\nReviewed-by: Laurent Pinchart \n\n> ---\n> test/py/unittests.py | 2 --\n> 1 file changed, 2 deletions(-)\n> \n> diff --git a/test/py/unittests.py b/test/py/unittests.py\n> index 45b35223..33b35a0a 100755\n> --- a/test/py/unittests.py\n> +++ b/test/py/unittests.py\n> @@ -287,8 +287,6 @@ class SimpleCaptureMethods(CameraTesterBase):\n> \n> ready_reqs = cm.get_ready_requests()\n> \n> - self.assertTrue(len(ready_reqs) > 0)\n> -\n> reqs += ready_reqs\n> \n> if len(reqs) == num_bufs:","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 50334BD161\n\tfor ;\n\tFri, 27 May 2022 19:09:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B5DC5633A3;\n\tFri, 27 May 2022 21:09:21 +0200 (CEST)","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 66997633A2\n\tfor ;\n\tFri, 27 May 2022 21:09:19 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [46.183.103.8])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BA26A32A;\n\tFri, 27 May 2022 21:09:18 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1653678561;\n\tbh=ecpglwFiW2GpX8c3CC1Mi9e/YpqIjmA0TjBu7Ok0zuM=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=aeddoFNb6ZX8wY+I8PJ+xoLv8w/vjCgGxJdKoP/fEdUL1dpl+iGXWZky7uT09iCdf\n\t4NZ2t8O85NsumS76DJat7cJ+M9EuV+TsQiPHR/TC6r6tkpObWIIHkmHWmIBQPOfphQ\n\tnz1cshonZaR6x9szh9/oM77SwPXqb02ZxaTyJZaAv/mtZWl6lIB5pK++BvXBBkyLYR\n\ty89I84sf9DABrSPWUfvK19umGCct2myb4+Pe7fS96BgHo8Z6aN1wwUa6pWjnhGuDm3\n\txH1lySt6wJdBry4RA9lpJ4SkTOudZdPXYEkUdvC2x+X0FtYRvvWc1ZT88faZ0DY3pI\n\tTbuxfQAq2taEg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1653678559;\n\tbh=ecpglwFiW2GpX8c3CC1Mi9e/YpqIjmA0TjBu7Ok0zuM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rZyI2A/zcKmAhiiIq8/7XXH4RF3FanDLSmrTI1b8/XquLGBQ7rVukIcAA5ExWs7WO\n\t/vsPfP75qjXdhifkoc9zbofaT0Q8kA9zB+mjbdFX3CqF7tST9YpzzYmQq77Zk5E1Fl\n\tNQASdISn53m/4671x8IqzO6j6493Uhjm/Yj8n9es="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"rZyI2A/z\"; dkim-atps=neutral","Date":"Fri, 27 May 2022 22:09:13 +0300","To":"Tomi Valkeinen ","Message-ID":"","References":"<20220527144447.94891-1-tomi.valkeinen@ideasonboard.com>\n\t<20220527144447.94891-19-tomi.valkeinen@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220527144447.94891-19-tomi.valkeinen@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 18/30] py: unittests: Fix\n\ttest_select()","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23221,"web_url":"https://patchwork.libcamera.org/comment/23221/","msgid":"<27efe443-0ee7-5e07-dfc2-1b6c381305d7@ideasonboard.com>","date":"2022-05-30T07:01:11","subject":"Re: [libcamera-devel] [PATCH v3 18/30] py: unittests: Fix\n\ttest_select()","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 27/05/2022 22:09, Laurent Pinchart wrote:\n> Hi Tomi,\n> \n> Thank you for the patch.\n> \n> On Fri, May 27, 2022 at 05:44:35PM +0300, Tomi Valkeinen wrote:\n>> The test_select() current uses self.assertTrue(len(ready_reqs) > 0) to\n> \n> s/current/currently/\n> \n>> see that cm.get_ready_requests() returns something. This is not always\n>> the case, as there may be two eventfd events queued, and the first call\n>> to cm.get_ready_requests() returns all the requests, and thus the second\n>> call returns none.\n\nThe above is actually not quite true. There are no \"eventfd events\", \njust a value, and if that value is > 0, the eventfd is readable.\n\nSo what happens here is that the user calls read_event(), which clear \nthat value. Then, before the user calls get_ready_requests(), there's a \nrequest-complete event, and the bindings increase the value. And now, \nwhen the user calls get_ready_requests(), it clears all the requests, \nbut the eventfd value is 1 and thus the select on the eventfd will \ntrigger immediately again, but there are no requests in the list.\n\n>> Remove the self.assertTrue(len(ready_reqs) > 0) assert.\n> \n> This convinces me even more that there's room for improvement :-) It\n> seems a bit confusing for applications.\n\nI'm open to suggestions =).\n\nI like the current method as it's very straightforward, just one mutex \nwhich is only kept when adding a Request to the gReqList, or swap()'ing \nthe gReqList to a local var. And the read_event() and \nget_ready_requests() should allow the caller to handle the event loop as \nhe wishes, both blocking and non-blocking.\n\nNow, we do always call read_event() and get_ready_requests() together in \nour .py scripts. So perhaps we should merge them, and add new separate \nfunctions if someone ever needs them. However, it won't change the \nproblem solved in this patch in any way.\n\nWe could also change the merged function so that the mutex is kept while \nread() is called and the gReqList is swap()'d. This would solve the \nproblem fixed in this patch. However, it would mean that we'd possibly \nbe blocked in read() while keeping the mutex and that would cause a \ndeadlock.\n\nAnyway, my opinion is that it's not an issue if there's an event from \nlibcamera but get_ready_requests() doesn't return anything.\n\n Tomi","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 6BD03BD161\n\tfor ;\n\tMon, 30 May 2022 07:01:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4A16E65634;\n\tMon, 30 May 2022 09:01:18 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4714261FB7\n\tfor ;\n\tMon, 30 May 2022 09:01:16 +0200 (CEST)","from [192.168.1.111] (91-156-85-209.elisa-laajakaista.fi\n\t[91.156.85.209])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 677626DF;\n\tMon, 30 May 2022 09:01:15 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1653894078;\n\tbh=WPA+A6ApSNrPDPnhHU+GKTWGQ1jF1iWqanCYIUgYxxg=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=reknKAw0gvWAXfmM8dmaQr4F5jHBySUf7BGK5OusaRjHrvKs/TRwA/JCC1PabKse4\n\tODDqokjOzmRe/51NmxG5CgtFPIkrVGgTAk58fHJGnkblxFi6f+xkmTakTGPrFoVmsx\n\t/LeoDhaEhb1ZMAROCz1pLI7W1IAEawM43utYjF9s3LoJN57ER1U+1c0ahwpl/rhFmA\n\tPkWL4pn8NL7FibrvNC+je24Nd3LpHSskJr/YYxAWXPXBi4QZW0sUGzv51XZsWGuxPl\n\tH7HE9Hw7Mysu23/BG8r+c8Nlk3EPeenNDKlFuOO+Btw83ZpMvYKMUEmXxAtT+RNCRX\n\tBzU1b2OJad2IA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1653894075;\n\tbh=WPA+A6ApSNrPDPnhHU+GKTWGQ1jF1iWqanCYIUgYxxg=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=K5987K1A3LXnPfaIPUqyC7frrlqVGP8BSDKwoL+7Za0n0f9jCakELbmGDOoGyJjsT\n\tIMIOmvs+CDPO60dISsphrYvwBj3A8C/QlmqCEAQ5WNbu8hADLNT7obhP0ag1s4E4UI\n\txX3Juwz1vK20ABnzV8+SDIzQbEsyYCeFRE6fDOcI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"K5987K1A\"; dkim-atps=neutral","Message-ID":"<27efe443-0ee7-5e07-dfc2-1b6c381305d7@ideasonboard.com>","Date":"Mon, 30 May 2022 10:01:11 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.9.1","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20220527144447.94891-1-tomi.valkeinen@ideasonboard.com>\n\t<20220527144447.94891-19-tomi.valkeinen@ideasonboard.com>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v3 18/30] py: unittests: Fix\n\ttest_select()","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","From":"Tomi Valkeinen via libcamera-devel\n\t","Reply-To":"Tomi Valkeinen ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23226,"web_url":"https://patchwork.libcamera.org/comment/23226/","msgid":"","date":"2022-05-30T08:25:34","subject":"Re: [libcamera-devel] [PATCH v3 18/30] py: unittests: Fix\n\ttest_select()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Tomi,\n\nOn Mon, May 30, 2022 at 10:01:11AM +0300, Tomi Valkeinen wrote:\n> On 27/05/2022 22:09, Laurent Pinchart wrote:\n> > On Fri, May 27, 2022 at 05:44:35PM +0300, Tomi Valkeinen wrote:\n> >> The test_select() current uses self.assertTrue(len(ready_reqs) > 0) to\n> > \n> > s/current/currently/\n> > \n> >> see that cm.get_ready_requests() returns something. This is not always\n> >> the case, as there may be two eventfd events queued, and the first call\n> >> to cm.get_ready_requests() returns all the requests, and thus the second\n> >> call returns none.\n> \n> The above is actually not quite true. There are no \"eventfd events\", \n> just a value, and if that value is > 0, the eventfd is readable.\n> \n> So what happens here is that the user calls read_event(), which clear \n> that value. Then, before the user calls get_ready_requests(), there's a \n> request-complete event, and the bindings increase the value. And now, \n> when the user calls get_ready_requests(), it clears all the requests, \n> but the eventfd value is 1 and thus the select on the eventfd will \n> trigger immediately again, but there are no requests in the list.\n> \n> >> Remove the self.assertTrue(len(ready_reqs) > 0) assert.\n> > \n> > This convinces me even more that there's room for improvement :-) It\n> > seems a bit confusing for applications.\n> \n> I'm open to suggestions =).\n> \n> I like the current method as it's very straightforward, just one mutex \n> which is only kept when adding a Request to the gReqList, or swap()'ing \n> the gReqList to a local var. And the read_event() and \n> get_ready_requests() should allow the caller to handle the event loop as \n> he wishes, both blocking and non-blocking.\n> \n> Now, we do always call read_event() and get_ready_requests() together in \n> our .py scripts. So perhaps we should merge them, and add new separate \n> functions if someone ever needs them. However, it won't change the \n> problem solved in this patch in any way.\n> \n> We could also change the merged function so that the mutex is kept while \n> read() is called and the gReqList is swap()'d. This would solve the \n> problem fixed in this patch. However, it would mean that we'd possibly \n> be blocked in read() while keeping the mutex and that would cause a \n> deadlock.\n> \n> Anyway, my opinion is that it's not an issue if there's an event from \n> libcamera but get_ready_requests() doesn't return anything.\n\nI've expressed myself incorrectly. This is totally fine, what I'd like\nto see removed is the read_event() function. What happens if you call\nread_event) in get_ready_requests() ?","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 803FEBD161\n\tfor ;\n\tMon, 30 May 2022 08:25:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D84C965633;\n\tMon, 30 May 2022 10:25:39 +0200 (CEST)","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 DF3A26040B\n\tfor ;\n\tMon, 30 May 2022 10:25:37 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(lmontsouris-659-1-41-236.w92-154.abo.wanadoo.fr [92.154.76.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1E7436BD;\n\tMon, 30 May 2022 10:25:37 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1653899139;\n\tbh=o4VCd/Q1Eg8pKNbtCv34Wg5dlkeRoWxLyXmEdzkT+H8=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=dkaiP0EOr46VevsEeCFWTPCVkaY3OQcqRu7ibA76r0crhPC9NQJagoU1a5FyolD9Z\n\tvqdCIRrLUJjMet90W0StFJAoea/w92zf8JbH2tc2GqBujch+j0hGoSiCvxGaRuR6iG\n\tftusOqOD6vkYo2GGHPzNNW/YlyGaK2h1LJXBvOAEZBoPPvM+Q27e5wJZDQEgDSNB7r\n\tI5MnbeLepL1sgSCnk8GkZhkwWJOt+e4c3A3QDgcvrA0hAR8bQ4hfvG3euUJJCK1J+3\n\t+q5eM78nCLWoh4eMUdlD7xhlwM/TYoij/mCO4DN5NAY9/64u9cbhRpKyfBBhGNGd4m\n\tLq+QZaR+mvhIA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1653899137;\n\tbh=o4VCd/Q1Eg8pKNbtCv34Wg5dlkeRoWxLyXmEdzkT+H8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=JKnYAp4fkEycx1JhfQyE8WUaKbmzhr4JwEVc3GBet72l1j1PgL9M5AZ/Y0WgFOTt2\n\t5rZDPwtqUwpknHaKPg+bXyV+CA+x/KIVukSOn9YcbthrdKj0S9i5mNfn8eEGO5l6BX\n\tSyAh91EpHBsfXP3K/Szgt87NlrMAxuf/XFAUIHOg="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"JKnYAp4f\"; dkim-atps=neutral","Date":"Mon, 30 May 2022 11:25:34 +0300","To":"Tomi Valkeinen ","Message-ID":"","References":"<20220527144447.94891-1-tomi.valkeinen@ideasonboard.com>\n\t<20220527144447.94891-19-tomi.valkeinen@ideasonboard.com>\n\t\n\t<27efe443-0ee7-5e07-dfc2-1b6c381305d7@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<27efe443-0ee7-5e07-dfc2-1b6c381305d7@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 18/30] py: unittests: Fix\n\ttest_select()","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23227,"web_url":"https://patchwork.libcamera.org/comment/23227/","msgid":"<6a774054-7952-3053-3bbe-39b88031bab9@ideasonboard.com>","date":"2022-05-30T08:36:52","subject":"Re: [libcamera-devel] [PATCH v3 18/30] py: unittests: Fix\n\ttest_select()","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 30/05/2022 11:25, Laurent Pinchart wrote:\n> Hi Tomi,\n> \n> On Mon, May 30, 2022 at 10:01:11AM +0300, Tomi Valkeinen wrote:\n>> On 27/05/2022 22:09, Laurent Pinchart wrote:\n>>> On Fri, May 27, 2022 at 05:44:35PM +0300, Tomi Valkeinen wrote:\n>>>> The test_select() current uses self.assertTrue(len(ready_reqs) > 0) to\n>>>\n>>> s/current/currently/\n>>>\n>>>> see that cm.get_ready_requests() returns something. This is not always\n>>>> the case, as there may be two eventfd events queued, and the first call\n>>>> to cm.get_ready_requests() returns all the requests, and thus the second\n>>>> call returns none.\n>>\n>> The above is actually not quite true. There are no \"eventfd events\",\n>> just a value, and if that value is > 0, the eventfd is readable.\n>>\n>> So what happens here is that the user calls read_event(), which clear\n>> that value. Then, before the user calls get_ready_requests(), there's a\n>> request-complete event, and the bindings increase the value. And now,\n>> when the user calls get_ready_requests(), it clears all the requests,\n>> but the eventfd value is 1 and thus the select on the eventfd will\n>> trigger immediately again, but there are no requests in the list.\n>>\n>>>> Remove the self.assertTrue(len(ready_reqs) > 0) assert.\n>>>\n>>> This convinces me even more that there's room for improvement :-) It\n>>> seems a bit confusing for applications.\n>>\n>> I'm open to suggestions =).\n>>\n>> I like the current method as it's very straightforward, just one mutex\n>> which is only kept when adding a Request to the gReqList, or swap()'ing\n>> the gReqList to a local var. And the read_event() and\n>> get_ready_requests() should allow the caller to handle the event loop as\n>> he wishes, both blocking and non-blocking.\n>>\n>> Now, we do always call read_event() and get_ready_requests() together in\n>> our .py scripts. So perhaps we should merge them, and add new separate\n>> functions if someone ever needs them. However, it won't change the\n>> problem solved in this patch in any way.\n>>\n>> We could also change the merged function so that the mutex is kept while\n>> read() is called and the gReqList is swap()'d. This would solve the\n>> problem fixed in this patch. However, it would mean that we'd possibly\n>> be blocked in read() while keeping the mutex and that would cause a\n>> deadlock.\n>>\n>> Anyway, my opinion is that it's not an issue if there's an event from\n>> libcamera but get_ready_requests() doesn't return anything.\n> \n> I've expressed myself incorrectly. This is totally fine, what I'd like\n> to see removed is the read_event() function. What happens if you call\n> read_event) in get_ready_requests() ?\n\nread_event() blocks until there is an \"event\". In our current .py files \nthis is fine, because in all the cases we either 1) call read_event() & \nget_ready_requests() because select indicates that there is an event, or \n2) we want the read_event() to block if there's no event.\n\nI think the only case where merging read_event() and \nget_ready_requests() would cause a problem is if you want to do polling. \nAt the moment you can just call get_ready_requests() and it never blocks \nand returns any requests that have been completed.\n\nThen again, even with a merged function the above could be accomplished \nif the user changes the eventfd to non-blocking mode in the application \ncode. With that change, read_event() never blocks, although at the \nmoment it throws an exception if read() fails (because at the moment \nread() should never fail). We should probably then change it to return a \nret val instead.\n\nBut I don't really know if polling is a use case we're interested in \nsupporting. Even blocking read feels a bit pointless. I think in all but \nthe most simple examples you want to properly wait on the fd, instead of \npolling or blocking. Especially as it's trivial to do with Python.\n\n Tomi","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 E8834BD160\n\tfor ;\n\tMon, 30 May 2022 08:36:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4402865633;\n\tMon, 30 May 2022 10:36:57 +0200 (CEST)","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 94B7B6040B\n\tfor ;\n\tMon, 30 May 2022 10:36:55 +0200 (CEST)","from [192.168.1.111] (91-156-85-209.elisa-laajakaista.fi\n\t[91.156.85.209])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C04346BD;\n\tMon, 30 May 2022 10:36:54 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1653899817;\n\tbh=7fcNls+6iAjFa2Xsmlijh/DeCocC2SuDvVUtqt+5A8U=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=lLclmPVdBnNk2HnlPpPwS1C0Dq8D8XZ+Blh1kzGbtbDWbWWDeHtUvbImZTu/gZJnr\n\tKWss2YjYPa5JTA1fyQTOlu6UdOzmYGAIJYtGieLaMVPCU2GMVEfbBHV4YtRxe1acfz\n\tKFmyXgA8R3gkZeuWpwii7GSRgyMOsqYC4htdX1aoN4UuZTCB3LP70OxKWCkGw/Ynns\n\tlmlbqn+DCj1gHwOHWZOrCPMZX1871JwaMgFcHiNYDEG6O+EJAanDknR8wVeDY6kVrT\n\tXbH3hR5moxyc86DhDHb1PFNN/8jkLAr5ngoTjMTikongazHZHPP+YJy+KYY1vumJw4\n\tAZPmcjMmjsL8A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1653899815;\n\tbh=7fcNls+6iAjFa2Xsmlijh/DeCocC2SuDvVUtqt+5A8U=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=E08/SiKMLr5GL+6vOqhHHX+DN+XZz3EIobOQAGW99WNDIug3stKKo5QY5wdHYUFMZ\n\to++EpnkNY6Pmh0TdKP44p3vCMPVVXIZ2t01Y7QzcV1tCNliDsvMyGxG2HXBOv+nRYD\n\tw/e3nZ4TruNtj+qFLke5Py6wawMODIvGSLtc0Wik="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"E08/SiKM\"; dkim-atps=neutral","Message-ID":"<6a774054-7952-3053-3bbe-39b88031bab9@ideasonboard.com>","Date":"Mon, 30 May 2022 11:36:52 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.9.1","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20220527144447.94891-1-tomi.valkeinen@ideasonboard.com>\n\t<20220527144447.94891-19-tomi.valkeinen@ideasonboard.com>\n\t\n\t<27efe443-0ee7-5e07-dfc2-1b6c381305d7@ideasonboard.com>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v3 18/30] py: unittests: Fix\n\ttest_select()","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","From":"Tomi Valkeinen via libcamera-devel\n\t","Reply-To":"Tomi Valkeinen ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23228,"web_url":"https://patchwork.libcamera.org/comment/23228/","msgid":"","date":"2022-05-30T08:44:02","subject":"Re: [libcamera-devel] [PATCH v3 18/30] py: unittests: Fix\n\ttest_select()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Tomi,\n\nOn Mon, May 30, 2022 at 11:36:52AM +0300, Tomi Valkeinen wrote:\n> On 30/05/2022 11:25, Laurent Pinchart wrote:\n> > On Mon, May 30, 2022 at 10:01:11AM +0300, Tomi Valkeinen wrote:\n> >> On 27/05/2022 22:09, Laurent Pinchart wrote:\n> >>> On Fri, May 27, 2022 at 05:44:35PM +0300, Tomi Valkeinen wrote:\n> >>>> The test_select() current uses self.assertTrue(len(ready_reqs) > 0) to\n> >>>\n> >>> s/current/currently/\n> >>>\n> >>>> see that cm.get_ready_requests() returns something. This is not always\n> >>>> the case, as there may be two eventfd events queued, and the first call\n> >>>> to cm.get_ready_requests() returns all the requests, and thus the second\n> >>>> call returns none.\n> >>\n> >> The above is actually not quite true. There are no \"eventfd events\",\n> >> just a value, and if that value is > 0, the eventfd is readable.\n> >>\n> >> So what happens here is that the user calls read_event(), which clear\n> >> that value. Then, before the user calls get_ready_requests(), there's a\n> >> request-complete event, and the bindings increase the value. And now,\n> >> when the user calls get_ready_requests(), it clears all the requests,\n> >> but the eventfd value is 1 and thus the select on the eventfd will\n> >> trigger immediately again, but there are no requests in the list.\n> >>\n> >>>> Remove the self.assertTrue(len(ready_reqs) > 0) assert.\n> >>>\n> >>> This convinces me even more that there's room for improvement :-) It\n> >>> seems a bit confusing for applications.\n> >>\n> >> I'm open to suggestions =).\n> >>\n> >> I like the current method as it's very straightforward, just one mutex\n> >> which is only kept when adding a Request to the gReqList, or swap()'ing\n> >> the gReqList to a local var. And the read_event() and\n> >> get_ready_requests() should allow the caller to handle the event loop as\n> >> he wishes, both blocking and non-blocking.\n> >>\n> >> Now, we do always call read_event() and get_ready_requests() together in\n> >> our .py scripts. So perhaps we should merge them, and add new separate\n> >> functions if someone ever needs them. However, it won't change the\n> >> problem solved in this patch in any way.\n> >>\n> >> We could also change the merged function so that the mutex is kept while\n> >> read() is called and the gReqList is swap()'d. This would solve the\n> >> problem fixed in this patch. However, it would mean that we'd possibly\n> >> be blocked in read() while keeping the mutex and that would cause a\n> >> deadlock.\n> >>\n> >> Anyway, my opinion is that it's not an issue if there's an event from\n> >> libcamera but get_ready_requests() doesn't return anything.\n> > \n> > I've expressed myself incorrectly. This is totally fine, what I'd like\n> > to see removed is the read_event() function. What happens if you call\n> > read_event) in get_ready_requests() ?\n> \n> read_event() blocks until there is an \"event\". In our current .py files \n> this is fine, because in all the cases we either 1) call read_event() & \n> get_ready_requests() because select indicates that there is an event, or \n> 2) we want the read_event() to block if there's no event.\n> \n> I think the only case where merging read_event() and \n> get_ready_requests() would cause a problem is if you want to do polling. \n> At the moment you can just call get_ready_requests() and it never blocks \n> and returns any requests that have been completed.\n> \n> Then again, even with a merged function the above could be accomplished \n> if the user changes the eventfd to non-blocking mode in the application \n> code. With that change, read_event() never blocks, although at the \n> moment it throws an exception if read() fails (because at the moment \n> read() should never fail). We should probably then change it to return a \n> ret val instead.\n> \n> But I don't really know if polling is a use case we're interested in \n> supporting. Even blocking read feels a bit pointless. I think in all but \n> the most simple examples you want to properly wait on the fd, instead of \n> polling or blocking. Especially as it's trivial to do with Python.\n\nAgreed :-)","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 2F3C1BD161\n\tfor ;\n\tMon, 30 May 2022 08:44:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7BD5565633;\n\tMon, 30 May 2022 10:44:06 +0200 (CEST)","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 4B7376040B\n\tfor ;\n\tMon, 30 May 2022 10:44:05 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(lmontsouris-659-1-41-236.w92-154.abo.wanadoo.fr [92.154.76.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 960656BD;\n\tMon, 30 May 2022 10:44:04 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1653900246;\n\tbh=aKz/RWsqI2ErrBOJO78YDbm2x+muM1rUvxCn9e5G4Ow=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=OEylO6J39FAAbdS1tFLasBVyGyRMHnm/q5rSou6/fVSki7PH9HGdG+x/HdFpc9YZP\n\t9Nki9cHWOnJqA8d5mm9t3N13kmYXtdTQU17vRvHwJosa2GwDFUCNT2gpykb5RYt8dL\n\t8Pho5fz+xL5bMnHbULovYRW/PGX0sQfbddf/Le9ahj1MApTdepyc012ong6ly8eoKi\n\tgGKXFj+kn31RBKZcc5inOqzWAdFF3uGA2HeBM9ZOrRd4G5truzVo8lJPrBNljGTZEk\n\tQWn1aXEXApmL8mwNz+KZhWIaHU/iN8+EopEOEbDwF6qeK8/SGfReWIXnrIoE5LIWqr\n\thjvHmFD3JS6lw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1653900244;\n\tbh=aKz/RWsqI2ErrBOJO78YDbm2x+muM1rUvxCn9e5G4Ow=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=d+KPQfQLS21TmQVuqg0rDrWBTiE4B6GBSZWdm7uDmloEKq64ciSvsLfWmJPYpt3is\n\tRle2QEM6noJB2It54HgCUG0LuB/GWVev9jnsp/na0v/ITqCGP4KR0dAFkP1vMbz5Al\n\trQu2UKuFXwVJ4QmqGVaj8q7z6C/RFKhdnIs8N19Q="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"d+KPQfQL\"; dkim-atps=neutral","Date":"Mon, 30 May 2022 11:44:02 +0300","To":"Tomi Valkeinen ","Message-ID":"","References":"<20220527144447.94891-1-tomi.valkeinen@ideasonboard.com>\n\t<20220527144447.94891-19-tomi.valkeinen@ideasonboard.com>\n\t\n\t<27efe443-0ee7-5e07-dfc2-1b6c381305d7@ideasonboard.com>\n\t\n\t<6a774054-7952-3053-3bbe-39b88031bab9@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<6a774054-7952-3053-3bbe-39b88031bab9@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 18/30] py: unittests: Fix\n\ttest_select()","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]