[{"id":15664,"web_url":"https://patchwork.libcamera.org/comment/15664/","msgid":"","date":"2021-03-14T00:51:25","subject":"Re: [libcamera-devel] [PATCH 1/8] libcamera: request: Provide a\n\tsequence number","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Fri, Mar 12, 2021 at 06:11:24AM +0000, Kieran Bingham wrote:\n> Provide a sequence number on Requests which are added by the pipeline\n\nDid you mean \"which are queued to the pipeline handler\" ?\n\n> handler.\n> \n> Each pipeline handler keeps a requestSequence and increments everytime a\n> request is queued.\n\nThe sequence number is actually stored in camera data, so it's a\nper-camera sequence. That's better than having it at the pipeline\nhandler level I think, you can just update the commit message.\n\n> The sequence number is associated with the Request and can be utilised\n> for assisting with debugging, and printing the queueing sequence of in\n> flight requests.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> include/libcamera/internal/pipeline_handler.h | 4 +++-\n> include/libcamera/request.h | 2 ++\n> src/libcamera/pipeline_handler.cpp | 2 ++\n> 3 files changed, 7 insertions(+), 1 deletion(-)\n> \n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index 6aca0b46f43d..0655a665a85f 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -38,7 +38,7 @@ class CameraData\n> {\n> public:\n> \texplicit CameraData(PipelineHandler *pipe)\n> -\t\t: pipe_(pipe)\n> +\t\t: pipe_(pipe), requestSequence_(0)\n> \t{\n> \t}\n> \tvirtual ~CameraData() = default;\n> @@ -48,6 +48,8 @@ public:\n> \tControlInfoMap controlInfo_;\n> \tControlList properties_;\n> \n> +\tuint64_t requestSequence_;\n\nLikely not a big deal, but 32-bit would very likely be sufficient (and\nI'd argue that wrap-around would actually be a good to have feature, as\na 20 characters number isn't much more readable than a pointer).\n\n> +\n> private:\n> \tLIBCAMERA_DISABLE_COPY(CameraData)\n> };\n> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> index 6e5aad5f6b75..6f2f881e840a 100644\n> --- a/include/libcamera/request.h\n> +++ b/include/libcamera/request.h\n> @@ -50,6 +50,7 @@ public:\n> \tint addBuffer(const Stream *stream, FrameBuffer *buffer);\n> \tFrameBuffer *findBuffer(const Stream *stream) const;\n> \n> +\tuint64_t sequence() const { return sequence_; }\n\nThis is of course missing documentation :-)\n\n> \tuint64_t cookie() const { return cookie_; }\n> \tStatus status() const { return status_; }\n> \n> @@ -71,6 +72,7 @@ private:\n> \tBufferMap bufferMap_;\n> \tstd::unordered_set pending_;\n> \n> +\tuint64_t sequence_;\n\nShould this be initialized to 0, and reset to 0 in Request::reuse() ?\n\n> \tconst uint64_t cookie_;\n> \tStatus status_;\n> \tbool cancelled_;\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index d22991d318c9..e3d4975d9ffd 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -382,6 +382,8 @@ int PipelineHandler::queueRequest(Request *request)\n> \tCameraData *data = cameraData(camera);\n> \tdata->queuedRequests_.push_back(request);\n> \n> +\trequest->sequence_ = data->requestSequence_++;\n> +\n> \tint ret = queueRequestDevice(camera, request);\n> \tif (ret)\n> \t\tdata->queuedRequests_.remove(request);","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 C1715BD80C\n\tfor ;\n\tSun, 14 Mar 2021 00:52:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 16E706084F;\n\tSun, 14 Mar 2021 01:52:03 +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 7578C602E1\n\tfor ;\n\tSun, 14 Mar 2021 01:52:01 +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 E0F3D55C;\n\tSun, 14 Mar 2021 01:52:00 +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=\"dT3cK8V/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1615683121;\n\tbh=P+VRA78Oy7AZUAZEvLs3/4m8p+MmwfI+rncYeZ0ZvWI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dT3cK8V/sGq448XyFwctQqSH94HIu3Ai/J5TiZcWkUkVckJIyxHo/rVgwmz8Z4B9h\n\tMVVbawSuCisBKHlzb0D2g6Q0/LLZUfe2FX2nTcsIg5sMgXPGZOLmfRS/rAcHy8881t\n\ttfxX0I72yaA5cTvkWQ8TUohQw7EAs3TJA154J5EQ=","Date":"Sun, 14 Mar 2021 02:51:25 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210312061131.854849-1-kieran.bingham@ideasonboard.com>\n\t<20210312061131.854849-2-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210312061131.854849-2-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/8] libcamera: request: Provide a\n\tsequence number","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":15697,"web_url":"https://patchwork.libcamera.org/comment/15697/","msgid":"<571ff112-d9d9-3473-313e-b3cbcb2087b8@ideasonboard.com>","date":"2021-03-15T11:59:53","subject":"Re: [libcamera-devel] [PATCH 1/8] libcamera: request: Provide a\n\tsequence number","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 14/03/2021 00:51, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Fri, Mar 12, 2021 at 06:11:24AM +0000, Kieran Bingham wrote:\n>> Provide a sequence number on Requests which are added by the pipeline\n> \n> Did you mean \"which are queued to the pipeline handler\" ?\n\nYes\n\n> \n>> handler.\n>>\n>> Each pipeline handler keeps a requestSequence and increments everytime a\n>> request is queued.\n> \n> The sequence number is actually stored in camera data, so it's a\n> per-camera sequence. That's better than having it at the pipeline\n> handler level I think, you can just update the commit message.\n\nIndeed, this is certainly a per camera sequence number.\n\nI'll fix the commit message.\n\n\n> \n>> The sequence number is associated with the Request and can be utilised\n>> for assisting with debugging, and printing the queueing sequence of in\n>> flight requests.\n>>\n>> Signed-off-by: Kieran Bingham \n>> ---\n>> include/libcamera/internal/pipeline_handler.h | 4 +++-\n>> include/libcamera/request.h | 2 ++\n>> src/libcamera/pipeline_handler.cpp | 2 ++\n>> 3 files changed, 7 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n>> index 6aca0b46f43d..0655a665a85f 100644\n>> --- a/include/libcamera/internal/pipeline_handler.h\n>> +++ b/include/libcamera/internal/pipeline_handler.h\n>> @@ -38,7 +38,7 @@ class CameraData\n>> {\n>> public:\n>> \texplicit CameraData(PipelineHandler *pipe)\n>> -\t\t: pipe_(pipe)\n>> +\t\t: pipe_(pipe), requestSequence_(0)\n>> \t{\n>> \t}\n>> \tvirtual ~CameraData() = default;\n>> @@ -48,6 +48,8 @@ public:\n>> \tControlInfoMap controlInfo_;\n>> \tControlList properties_;\n>> \n>> +\tuint64_t requestSequence_;\n> \n> Likely not a big deal, but 32-bit would very likely be sufficient (and\n\nBut ... at 120 frames per second ... that would only give us something\nlike 414 days before wrap around! ... how are we going to ...\n\n:-D\n\n\n> I'd argue that wrap-around would actually be a good to have feature, as\n\nIf you /desire/ wrap around we could go to uint16_t ... 65536 frames\nought to be enough for anybody right...\n\n> a 20 characters number isn't much more readable than a pointer).\n\nWell ... I disagree slightly there - a number is sequential ;-)\nA pointer is not.... but getting too long indeed probably defies the point.\n\nAnd I agree with you on 3/8 actually - I like the idea that we don't\nreset the sequence to zero on stop to provide a continuous counter.\nThat may help spot other bugs / flow issues.\n\n\n>> +\n>> private:\n>> \tLIBCAMERA_DISABLE_COPY(CameraData)\n>> };\n>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n>> index 6e5aad5f6b75..6f2f881e840a 100644\n>> --- a/include/libcamera/request.h\n>> +++ b/include/libcamera/request.h\n>> @@ -50,6 +50,7 @@ public:\n>> \tint addBuffer(const Stream *stream, FrameBuffer *buffer);\n>> \tFrameBuffer *findBuffer(const Stream *stream) const;\n>> \n>> +\tuint64_t sequence() const { return sequence_; }\n> \n> This is of course missing documentation :-)\n\nAck.\n\n\n>> \tuint64_t cookie() const { return cookie_; }\n>> \tStatus status() const { return status_; }\n>> \n>> @@ -71,6 +72,7 @@ private:\n>> \tBufferMap bufferMap_;\n>> \tstd::unordered_set pending_;\n>> \n>> +\tuint64_t sequence_;\n> \n> Should this be initialized to 0, and reset to 0 in Request::reuse() ?\n\nMaybe ...\n\nThat makes me wonder if we should initialise the requestSequence_ to\nalways start at 1 to leave 0 as invalid - but that wouldn't help with\nthe wrap-around. Or maybe that's my only weak argument to keep it as\nuint64_t...\n\n\n> \n>> \tconst uint64_t cookie_;\n>> \tStatus status_;\n>> \tbool cancelled_;\n>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n>> index d22991d318c9..e3d4975d9ffd 100644\n>> --- a/src/libcamera/pipeline_handler.cpp\n>> +++ b/src/libcamera/pipeline_handler.cpp\n>> @@ -382,6 +382,8 @@ int PipelineHandler::queueRequest(Request *request)\n>> \tCameraData *data = cameraData(camera);\n>> \tdata->queuedRequests_.push_back(request);\n>> \n>> +\trequest->sequence_ = data->requestSequence_++;\n>> +\n>> \tint ret = queueRequestDevice(camera, request);\n>> \tif (ret)\n>> \t\tdata->queuedRequests_.remove(request);\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 33F41BD80E\n\tfor ;\n\tMon, 15 Mar 2021 11:59:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 93CCE68D3E;\n\tMon, 15 Mar 2021 12:59:57 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 702F560106\n\tfor ;\n\tMon, 15 Mar 2021 12:59:56 +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 E8072556;\n\tMon, 15 Mar 2021 12:59:55 +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=\"NRIQbsiM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1615809596;\n\tbh=ZyRivwZV51cuCiGu4Zu6bBdu/iMdXm0fnBkMXPSM04c=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=NRIQbsiMAewkCkZor06rVOZnZ/1G3bPdvVrrYoxvqXcLEL1w58+0ha70m0n9fI9Tx\n\tO5lqDf10AqK8uJgX3amMKrRvUyAbCXU5d7g+Tx/afaBBxvGmXgfBTK+MZVZwVxsfem\n\t7R07VG+SQB/pUin3UaqgaEqhdLOSw2DWVGT67EWg=","To":"Laurent Pinchart ","References":"<20210312061131.854849-1-kieran.bingham@ideasonboard.com>\n\t<20210312061131.854849-2-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":"<571ff112-d9d9-3473-313e-b3cbcb2087b8@ideasonboard.com>","Date":"Mon, 15 Mar 2021 11:59:53 +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 1/8] libcamera: request: Provide a\n\tsequence number","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":15705,"web_url":"https://patchwork.libcamera.org/comment/15705/","msgid":"","date":"2021-03-15T16:37:00","subject":"Re: [libcamera-devel] [PATCH 1/8] libcamera: request: Provide a\n\tsequence number","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 15, 2021 at 11:59:53AM +0000, Kieran Bingham wrote:\n> On 14/03/2021 00:51, Laurent Pinchart wrote:\n> > On Fri, Mar 12, 2021 at 06:11:24AM +0000, Kieran Bingham wrote:\n> >> Provide a sequence number on Requests which are added by the pipeline\n> > \n> > Did you mean \"which are queued to the pipeline handler\" ?\n> \n> Yes\n> \n> >> handler.\n> >>\n> >> Each pipeline handler keeps a requestSequence and increments everytime a\n> >> request is queued.\n> > \n> > The sequence number is actually stored in camera data, so it's a\n> > per-camera sequence. That's better than having it at the pipeline\n> > handler level I think, you can just update the commit message.\n> \n> Indeed, this is certainly a per camera sequence number.\n> \n> I'll fix the commit message.\n> \n> >> The sequence number is associated with the Request and can be utilised\n> >> for assisting with debugging, and printing the queueing sequence of in\n> >> flight requests.\n> >>\n> >> Signed-off-by: Kieran Bingham \n> >> ---\n> >> include/libcamera/internal/pipeline_handler.h | 4 +++-\n> >> include/libcamera/request.h | 2 ++\n> >> src/libcamera/pipeline_handler.cpp | 2 ++\n> >> 3 files changed, 7 insertions(+), 1 deletion(-)\n> >>\n> >> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> >> index 6aca0b46f43d..0655a665a85f 100644\n> >> --- a/include/libcamera/internal/pipeline_handler.h\n> >> +++ b/include/libcamera/internal/pipeline_handler.h\n> >> @@ -38,7 +38,7 @@ class CameraData\n> >> {\n> >> public:\n> >> \texplicit CameraData(PipelineHandler *pipe)\n> >> -\t\t: pipe_(pipe)\n> >> +\t\t: pipe_(pipe), requestSequence_(0)\n> >> \t{\n> >> \t}\n> >> \tvirtual ~CameraData() = default;\n> >> @@ -48,6 +48,8 @@ public:\n> >> \tControlInfoMap controlInfo_;\n> >> \tControlList properties_;\n> >> \n> >> +\tuint64_t requestSequence_;\n> > \n> > Likely not a big deal, but 32-bit would very likely be sufficient (and\n> \n> But ... at 120 frames per second ... that would only give us something\n> like 414 days before wrap around! ... how are we going to ...\n> \n> :-D\n> \n> \n> > I'd argue that wrap-around would actually be a good to have feature, as\n> \n> If you /desire/ wrap around we could go to uint16_t ... 65536 frames\n> ought to be enough for anybody right...\n\nI've actually considered that. I'm not sure if I like it :-)\n\n> > a 20 characters number isn't much more readable than a pointer).\n> \n> Well ... I disagree slightly there - a number is sequential ;-)\n> A pointer is not.... but getting too long indeed probably defies the point.\n> \n> And I agree with you on 3/8 actually - I like the idea that we don't\n> reset the sequence to zero on stop to provide a continuous counter.\n> That may help spot other bugs / flow issues.\n> \n> >> +\n> >> private:\n> >> \tLIBCAMERA_DISABLE_COPY(CameraData)\n> >> };\n> >> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> >> index 6e5aad5f6b75..6f2f881e840a 100644\n> >> --- a/include/libcamera/request.h\n> >> +++ b/include/libcamera/request.h\n> >> @@ -50,6 +50,7 @@ public:\n> >> \tint addBuffer(const Stream *stream, FrameBuffer *buffer);\n> >> \tFrameBuffer *findBuffer(const Stream *stream) const;\n> >> \n> >> +\tuint64_t sequence() const { return sequence_; }\n> > \n> > This is of course missing documentation :-)\n> \n> Ack.\n> \n> >> \tuint64_t cookie() const { return cookie_; }\n> >> \tStatus status() const { return status_; }\n> >> \n> >> @@ -71,6 +72,7 @@ private:\n> >> \tBufferMap bufferMap_;\n> >> \tstd::unordered_set pending_;\n> >> \n> >> +\tuint64_t sequence_;\n> > \n> > Should this be initialized to 0, and reset to 0 in Request::reuse() ?\n> \n> Maybe ...\n> \n> That makes me wonder if we should initialise the requestSequence_ to\n> always start at 1 to leave 0 as invalid - but that wouldn't help with\n> the wrap-around. Or maybe that's my only weak argument to keep it as\n> uint64_t...\n\nI've also thought about this. I don't think it's a big deal, but we\ncould address it indeed.\n\n> >> \tconst uint64_t cookie_;\n> >> \tStatus status_;\n> >> \tbool cancelled_;\n> >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> >> index d22991d318c9..e3d4975d9ffd 100644\n> >> --- a/src/libcamera/pipeline_handler.cpp\n> >> +++ b/src/libcamera/pipeline_handler.cpp\n> >> @@ -382,6 +382,8 @@ int PipelineHandler::queueRequest(Request *request)\n> >> \tCameraData *data = cameraData(camera);\n> >> \tdata->queuedRequests_.push_back(request);\n> >> \n> >> +\trequest->sequence_ = data->requestSequence_++;\n> >> +\n> >> \tint ret = queueRequestDevice(camera, request);\n> >> \tif (ret)\n> >> \t\tdata->queuedRequests_.remove(request);","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 949BFBD80E\n\tfor ;\n\tMon, 15 Mar 2021 16:37:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DF35968D46;\n\tMon, 15 Mar 2021 17:37:37 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C56CD68D3E\n\tfor ;\n\tMon, 15 Mar 2021 17:37:36 +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 36521316;\n\tMon, 15 Mar 2021 17:37:36 +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=\"sjXoTu5l\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1615826256;\n\tbh=ijVmA8xo+twZAYhtC0d500WVYPyKzGaUk5w42gYV4mo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sjXoTu5l5NVjDsQIgP+zqu/GsbZSoTEJ0mgoGpHruADl8aVtBRkfEKtaJLt7oEGvo\n\tFLLLLdt5AYhQpQyyqTJbLnUcz5oyfJiKT28U8TrwYsFZ9qffKzq8/W3+B57oWA6D0g\n\td8i5mYKgIMZCpR5vX3W8BfSBqew6KyRyJQSr7JD8=","Date":"Mon, 15 Mar 2021 18:37:00 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210312061131.854849-1-kieran.bingham@ideasonboard.com>\n\t<20210312061131.854849-2-kieran.bingham@ideasonboard.com>\n\t\n\t<571ff112-d9d9-3473-313e-b3cbcb2087b8@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<571ff112-d9d9-3473-313e-b3cbcb2087b8@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/8] libcamera: request: Provide a\n\tsequence number","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\" "}}]