[{"id":13559,"web_url":"https://patchwork.libcamera.org/comment/13559/","msgid":"<20201030004159.GM15024@pendragon.ideasonboard.com>","date":"2020-10-30T00:41:59","subject":"Re: [libcamera-devel] [PATCH v3 2/6] libcamera: request: Add\n\ttracepoints","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Thu, Oct 29, 2020 at 07:16:25PM +0900, Paul Elder wrote:\n> Add and use tracepoints in Request. Requests are core to libcamera\n> operation, thus detecting delays in their processing is important, and\n> serves as a good usage example of tracepoints.\n> \n> Signed-off-by: Paul Elder \n> Reviewed-by: Laurent Pinchart \n> \n> ---\n> I wasn't sure what the best way to handle the FrameBuffer enum in the tp\n> file... should it go in a separate file? Will we ever need to use the\n> FrameBuffer status enum in other tracepoints? If we do, it can't be\n> defined multiple times, so it'll have to go in its own tp file, and in\n> the meson tp file list it will have to go first. I guess that's the\n> drawback of concating all the tp files together...\n\nI wonder if the enums shouldn't indeed be placed in separate .tp files\nthat would then be #include'd where needed, with a header guard. That\nway we'll have a guarantee they will be defined on first use in the\ntracepoints.h file.\n\n> Changes in v3:\n> - expand the changelog\n> - add enum tracepoint values so that request status and buffer status\n> are strings instead of ints in the trace\n> - remove cancelled from all request tracepoints, except complete_buffer\n> - expand the complete_buffer tracepoint to include information on the\n> buffer\n> - display the address of the request in all tracepoints\n> - remove tracepoints for add_buffer and find_buffer\n> \n> Changes in v2:\n> - remove tracepoints from uvcvideo\n> - remove comment in changelog that this is only used for demonstration\n> - use Request pointers instead of feeding the fields manually to the\n> tracepoint\n> ---\n> .../internal/tracepoints/meson.build | 1 +\n> .../libcamera/internal/tracepoints/request.tp | 90 +++++++++++++++++++\n> src/libcamera/request.cpp | 11 +++\n> 3 files changed, 102 insertions(+)\n> create mode 100644 include/libcamera/internal/tracepoints/request.tp\n> \n> diff --git a/include/libcamera/internal/tracepoints/meson.build b/include/libcamera/internal/tracepoints/meson.build\n> index 2dd6733e..8410c205 100644\n> --- a/include/libcamera/internal/tracepoints/meson.build\n> +++ b/include/libcamera/internal/tracepoints/meson.build\n> @@ -1,4 +1,5 @@\n> # SPDX-License-Identifier: CC0-1.0\n> \n> tracepoint_files = files([\n> + 'request.tp',\n> ])\n> diff --git a/include/libcamera/internal/tracepoints/request.tp b/include/libcamera/internal/tracepoints/request.tp\n> new file mode 100644\n> index 00000000..92308dcd\n> --- /dev/null\n> +++ b/include/libcamera/internal/tracepoints/request.tp\n> @@ -0,0 +1,90 @@\n> +#include \n> +#include \n> +\n> +TRACEPOINT_ENUM(\n> +\tlibcamera,\n> +\trequest_status,\n> +\tTP_ENUM_VALUES(\n> +\t\tctf_enum_value(\"RequestPending\", 0)\n> +\t\tctf_enum_value(\"RequestComplete\", 1)\n> +\t\tctf_enum_value(\"RequestCancelled\", 2)\n> +\t)\n> +)\n> +\n> +TRACEPOINT_ENUM(\n> +\tlibcamera,\n> +\tbuffer_status,\n> +\tTP_ENUM_VALUES(\n> +\t\tctf_enum_value(\"FrameSuccess\", 0)\n> +\t\tctf_enum_value(\"FrameError\", 1)\n> +\t\tctf_enum_value(\"FrameCancelled\", 2)\n> +\t)\n> +)\n> +\n> +TRACEPOINT_EVENT_CLASS(\n> +\tlibcamera,\n> +\trequest,\n> +\tTP_ARGS(\n> +\t\tlibcamera::Request *, req\n> +\t),\n> +\tTP_FIELDS(\n> +\t\tctf_integer_hex(uintptr_t, request, reinterpret_cast(req))\n> +\t\tctf_integer(uint64_t, cookie, req->cookie())\n> +\t\tctf_enum(libcamera, request_status, uint32_t, status, req->status())\n> +\t)\n> +)\n> +\n> +TRACEPOINT_EVENT_INSTANCE(\n> +\tlibcamera,\n> +\trequest,\n> +\trequest_construct,\n> +\tTP_ARGS(\n> +\t\tlibcamera::Request *, req\n> +\t)\n> +)\n> +\n> +TRACEPOINT_EVENT_INSTANCE(\n> +\tlibcamera,\n> +\trequest,\n> +\trequest_destroy,\n> +\tTP_ARGS(\n> +\t\tlibcamera::Request *, req\n> +\t)\n> +)\n> +\n> +TRACEPOINT_EVENT_INSTANCE(\n> +\tlibcamera,\n> +\trequest,\n> +\trequest_reuse,\n> +\tTP_ARGS(\n> +\t\tlibcamera::Request *, req\n> +\t)\n> +)\n> +\n> +TRACEPOINT_EVENT_INSTANCE(\n> +\tlibcamera,\n> +\trequest,\n> +\trequest_complete,\n> +\tTP_ARGS(\n> +\t\tlibcamera::Request *, req\n> +\t)\n> +)\n> +\n> +\n> +TRACEPOINT_EVENT(\n> +\tlibcamera,\n> +\trequest_complete_buffer,\n> +\tTP_ARGS(\n> +\t\tlibcamera::Request *, req,\n> +\t\tint, cancelled,\n> +\t\tlibcamera::FrameBuffer *, buf\n> +\t),\n> +\tTP_FIELDS(\n> +\t\tctf_integer_hex(uintptr_t, request, reinterpret_cast(req))\n> +\t\tctf_integer(uint64_t, cookie, req->cookie())\n> +\t\tctf_integer(int, status, req->status())\n> +\t\tctf_integer(int, cancelled, cancelled)\n\nI think you could drop cancelled, as it's a result of the buffer status,\nwhich you also capture.\n\n> +\t\tctf_integer_hex(uintptr_t, buffer, reinterpret_cast(buf))\n> +\t\tctf_enum(libcamera, buffer_status, uint32_t, buf_status, buf->metadata().status)\n> +\t)\n> +)\n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index ae8b1660..d1b43697 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -16,6 +16,7 @@\n> \n> #include \"libcamera/internal/camera_controls.h\"\n> #include \"libcamera/internal/log.h\"\n> +#include \"libcamera/internal/tracepoints.h\"\n> \n> /**\n> * \\file request.h\n> @@ -85,10 +86,14 @@ Request::Request(Camera *camera, uint64_t cookie)\n> \t * \\todo: Add a validator for metadata controls.\n> \t */\n> \tmetadata_ = new ControlList(controls::controls);\n> +\n> +\tLIBCAMERA_TRACEPOINT(request_construct, this);\n> }\n> \n> Request::~Request()\n> {\n> +\tLIBCAMERA_TRACEPOINT(request_destroy, this);\n> +\n> \tdelete metadata_;\n> \tdelete controls_;\n> \tdelete validator_;\n> @@ -106,6 +111,8 @@ Request::~Request()\n> */\n> void Request::reuse(ReuseFlag flags)\n> {\n> +\tLIBCAMERA_TRACEPOINT(request_reuse, this);\n> +\n> \tpending_.clear();\n> \tif (flags & ReuseBuffers) {\n> \t\tfor (auto pair : bufferMap_) {\n> @@ -259,6 +266,8 @@ void Request::complete()\n> \tLOG(Request, Debug)\n> \t\t<< \"Request has completed - cookie: \" << cookie_\n> \t\t<< (cancelled_ ? \" [Cancelled]\" : \"\");\n> +\n> +\tLIBCAMERA_TRACEPOINT(request_complete, this);\n> }\n> \n> /**\n> @@ -276,6 +285,8 @@ void Request::complete()\n> */\n> bool Request::completeBuffer(FrameBuffer *buffer)\n> {\n> +\tLIBCAMERA_TRACEPOINT(request_complete_buffer, this, cancelled_, buffer);\n> +\n> \tint ret = pending_.erase(buffer);\n> \tASSERT(ret == 1);\n> \n\nIt looks really nice :-)","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 0630FBDB9B\n\tfor ;\n\tFri, 30 Oct 2020 00:42:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 800A1628A5;\n\tFri, 30 Oct 2020 01:42:50 +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 0DCAD61563\n\tfor ;\n\tFri, 30 Oct 2020 01:42: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 763B79B7;\n\tFri, 30 Oct 2020 01:42: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=\"FPI6lYKA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1604018568;\n\tbh=4zhC9yyvKWi8dT99mnnE5oDEsjaOJtKEcoUXyV/tLsI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FPI6lYKAoEMevqzhPz8m6YndrGO2zyXWJKHHQ50v6F3lfajb85ghmn63aPWKfJrGN\n\tK0HKbbZ/fFgDrRBnXVz5Gh4Iy23pxqq9FniSmbEPt9OD7GAWUpR6AZa0P7Lhd8/QCV\n\tHISXKTBw1EIwCfoVvCKqh9T5pxIqFELe5UvoWrNQ=","Date":"Fri, 30 Oct 2020 02:41:59 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"<20201030004159.GM15024@pendragon.ideasonboard.com>","References":"<20201029101629.61798-1-paul.elder@ideasonboard.com>\n\t<20201029101629.61798-3-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201029101629.61798-3-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 2/6] libcamera: request: Add\n\ttracepoints","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@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]