[{"id":13382,"web_url":"https://patchwork.libcamera.org/comment/13382/","msgid":"<20201022011613.GU3942@pendragon.ideasonboard.com>","date":"2020-10-22T01:16:13","subject":"Re: [libcamera-devel] [RFC PATCH 2/2] [DO NOT MERGE] libcamera:\n\tuvcvideo, request: Add tracepoints","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 Mon, Oct 19, 2020 at 07:25:29PM +0900, Paul Elder wrote:\n> Add and use tracepoints in the uvcvideo pipeline handler and Request.\n> This is only meant to show how one would create and use tracepoints.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> Setting log level is also available. I haven't tested it yet, but in\n> theory is should Just Work (TM). It looks like you can set log levels on\n> the event definitions, and not when you emit the events.\n> ---\n> .../internal/tracepoints/meson.build | 2 +\n> .../libcamera/internal/tracepoints/request.tp | 91 +++++++++++++++++++\n> .../internal/tracepoints/uvcvideo.tp | 13 +++\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 3 +\n> src/libcamera/request.cpp | 16 ++++\n\nI think you could already split this in two, one patch for the libcamera\ncore and one for the pipeline handler, as I believe the series will\ngraduate to non-RFC status fairly soon.\n\n> 5 files changed, 125 insertions(+)\n> create mode 100644 include/libcamera/internal/tracepoints/request.tp\n> create mode 100644 include/libcamera/internal/tracepoints/uvcvideo.tp\n> \n> diff --git a/include/libcamera/internal/tracepoints/meson.build b/include/libcamera/internal/tracepoints/meson.build\n> index 2dd6733e..3ec53fbc 100644\n> --- a/include/libcamera/internal/tracepoints/meson.build\n> +++ b/include/libcamera/internal/tracepoints/meson.build\n> @@ -1,4 +1,6 @@\n> # SPDX-License-Identifier: CC0-1.0\n> \n> tracepoint_files = files([\n> + 'request.tp',\n> + 'uvcvideo.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..c64bbd43\n> --- /dev/null\n> +++ b/include/libcamera/internal/tracepoints/request.tp\n> @@ -0,0 +1,91 @@\n> +TRACEPOINT_EVENT_CLASS(\n> +\tlibcamera,\n> +\trequest,\n> +\tTP_ARGS(\n> +\t\tuint64_t, cookie,\n> +\t\tint, status,\n> +\t\tint, cancelled\n> +\t),\n> +\tTP_FIELDS(\n> +\t\tctf_integer(uint64_t, cookie, cookie)\n> +\t\tctf_integer(int, status, status)\n> +\t\tctf_integer(int, cancelled, cancelled)\n> +\t)\n> +)\n> +\n> +TRACEPOINT_EVENT_INSTANCE(\n> +\tlibcamera,\n> +\trequest,\n> +\trequest_construct,\n> +\tTP_ARGS(\n> +\t\tuint64_t, cookie,\n> +\t\tint, status,\n> +\t\tint, cancelled\n> +\t)\n> +)\n\nIf I understand this correctly, TRACEPOINT_EVENT_CLASS will generate one\nserialization function, and TRACEPOINT_EVENT_INSTANCE, whose TP_ARGS\nmust match the class', then use it.\n\nI think we need to think a bit about the naming scheme here. This code\ninitially made me believe the event class was a good match for the C++\nclass, but it's really about classes of events that take the same\narguments, not C++ classes. If we needed to add other arguments to some\nof the tracepoints for Request, we would need different event classes,\nfor the C++ class. I thus wonder if the tracepoint class name should\ncontain more information.\n\nAlternatively, is the provider name required to be the same for all our\ntracepoints, or could it include the C++ class name (e.g.\nlibcamera_request) ? What impact would this have ?\n\nI was also wondering if we could use names that would make the class\nname more apparent (for instance Request::Request instead of\nrequest_construct), but all names need to be valid C symbol names. We\ncould encode them using the C++ name encoding scheme, and decode them\nwhen consuming the traces (which would require pre-processing the .tp\nfile with a custom tool). That's a possible improvement for (much) later\n:-)\n\n> +\n> +TRACEPOINT_EVENT_INSTANCE(\n> +\tlibcamera,\n> +\trequest,\n> +\trequest_deconstruct,\n> +\tTP_ARGS(\n> +\t\tuint64_t, cookie,\n> +\t\tint, status,\n> +\t\tint, cancelled\n> +\t)\n> +)\n> +\n> +TRACEPOINT_EVENT_INSTANCE(\n> +\tlibcamera,\n> +\trequest,\n> +\trequest_reuse,\n> +\tTP_ARGS(\n> +\t\tuint64_t, cookie,\n> +\t\tint, status,\n> +\t\tint, cancelled\n> +\t)\n> +)\n> +\n> +TRACEPOINT_EVENT_INSTANCE(\n> +\tlibcamera,\n> +\trequest,\n> +\trequest_add_buffer,\n> +\tTP_ARGS(\n> +\t\tuint64_t, cookie,\n> +\t\tint, status,\n> +\t\tint, cancelled\n> +\t)\n> +)\n> +\n> +TRACEPOINT_EVENT_INSTANCE(\n> +\tlibcamera,\n> +\trequest,\n> +\trequest_find_buffer,\n> +\tTP_ARGS(\n> +\t\tuint64_t, cookie,\n> +\t\tint, status,\n> +\t\tint, cancelle\n> +\t)\n> +)\n> +\n> +TRACEPOINT_EVENT_INSTANCE(\n> +\tlibcamera,\n> +\trequest,\n> +\trequest_complete,\n> +\tTP_ARGS(\n> +\t\tuint64_t, cookie,\n> +\t\tint, status,\n> +\t\tint, cancelled\n> +\t)\n> +)\n> +\n> +TRACEPOINT_EVENT_INSTANCE(\n> +\tlibcamera,\n> +\trequest,\n> +\trequest_complete_buffer,\n> +\tTP_ARGS(\n> +\t\tuint64_t, cookie,\n> +\t\tint, status,\n> +\t\tint, cancelled\n> +\t)\n> +)\n> diff --git a/include/libcamera/internal/tracepoints/uvcvideo.tp b/include/libcamera/internal/tracepoints/uvcvideo.tp\n> new file mode 100644\n> index 00000000..61592ce5\n> --- /dev/null\n> +++ b/include/libcamera/internal/tracepoints/uvcvideo.tp\n> @@ -0,0 +1,13 @@\n> +TRACEPOINT_EVENT_CLASS(\n> +\tlibcamera,\n> +\tuvcvideo,\n> +\tTP_ARGS(),\n> +\tTP_FIELDS()\n> +)\n> +\n> +TRACEPOINT_EVENT_INSTANCE(\n> +\tlibcamera,\n> +\tuvcvideo,\n> +\tuvcvideo_contruct,\n> +\tTP_ARGS()\n> +)\n\nWith a single instance, you can use TRACEPOINT_EVENT().\n\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index 8ec0dac1..bc01a78c 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -27,6 +27,8 @@\n> #include \"libcamera/internal/v4l2_controls.h\"\n> #include \"libcamera/internal/v4l2_videodevice.h\"\n> \n> +#include \"libcamera/internal/tracepoints.h\"\n\nNo need to keep this header separate.\n\n> +\n> namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(UVC)\n> @@ -171,6 +173,7 @@ CameraConfiguration::Status UVCCameraConfiguration::validate()\n> PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)\n> \t: PipelineHandler(manager)\n> {\n> +\tLIBCAMERA_TRACEPOINT(uvcvideo_contruct);\n> }\n> \n> CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera,\n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index ae8b1660..74c1a7b0 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -17,6 +17,8 @@\n> #include \"libcamera/internal/camera_controls.h\"\n> #include \"libcamera/internal/log.h\"\n> \n> +#include \"libcamera/internal/tracepoints.h\"\n\nDoes generating a single header with all tracepoints risk increasing\ncompilation time ? I wonder if one libcamera/tracepoints/.h for\neach .tp would be better.\n\n> +\n> /**\n> * \\file request.h\n> * \\brief Describes a frame capture request to be processed by a camera\n> @@ -85,10 +87,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, cookie_, status_, cancelled_);\n\nThe Request class is, I think, a good candidate for tracepoints. For the\nnext example, I think you could drop some of the tracepoints (I'm about\nfindBuffer for instance), and possibly tailor the argument to the\ntracepoint event if other information would be useful to capture.\n\nWould it also make sense to capture a pointer to the request, to match\nthe different events ? That's probably a question that applies to all,\nor most, of the events we will have.\n\n> }\n> \n> Request::~Request()\n> {\n> +\tLIBCAMERA_TRACEPOINT(request_deconstruct, cookie_, status_, cancelled_);\n> +\n> \tdelete metadata_;\n> \tdelete controls_;\n> \tdelete validator_;\n> @@ -106,6 +112,8 @@ Request::~Request()\n> */\n> void Request::reuse(ReuseFlag flags)\n> {\n> +\tLIBCAMERA_TRACEPOINT(request_reuse, cookie_, status_, cancelled_);\n> +\n> \tpending_.clear();\n> \tif (flags & ReuseBuffers) {\n> \t\tfor (auto pair : bufferMap_) {\n> @@ -167,6 +175,8 @@ void Request::reuse(ReuseFlag flags)\n> */\n> int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)\n> {\n> +\tLIBCAMERA_TRACEPOINT(request_add_buffer, cookie_, status_, cancelled_);\n> +\n> \tif (!stream) {\n> \t\tLOG(Request, Error) << \"Invalid stream reference\";\n> \t\treturn -EINVAL;\n> @@ -202,6 +212,8 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)\n> */\n> FrameBuffer *Request::findBuffer(const Stream *stream) const\n> {\n> +\tLIBCAMERA_TRACEPOINT(request_find_buffer, cookie_, status_, cancelled_);\n> +\n> \tconst auto it = bufferMap_.find(stream);\n> \tif (it == bufferMap_.end())\n> \t\treturn nullptr;\n> @@ -253,6 +265,8 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const\n> */\n> void Request::complete()\n> {\n> +\tLIBCAMERA_TRACEPOINT(request_complete, cookie_, status_, cancelled_);\n> +\n> \tASSERT(!hasPendingBuffers());\n> \tstatus_ = cancelled_ ? RequestCancelled : RequestComplete;\n> \n> @@ -276,6 +290,8 @@ void Request::complete()\n> */\n> bool Request::completeBuffer(FrameBuffer *buffer)\n> {\n> +\tLIBCAMERA_TRACEPOINT(request_complete_buffer, cookie_, status_, cancelled_);\n> +\n> \tint ret = pending_.erase(buffer);\n> \tASSERT(ret == 1);\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 945A6BDB13\n\tfor ;\n\tThu, 22 Oct 2020 01:17:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0340761059;\n\tThu, 22 Oct 2020 03:17:01 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 927996034F\n\tfor ;\n\tThu, 22 Oct 2020 03:16:59 +0200 (CEST)","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 157D63D;\n\tThu, 22 Oct 2020 03:16:59 +0200 (CEST)"],"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=\"REYVTJto\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603329419;\n\tbh=m8wAzClBV1KXUrJsXF31xxUQ5g6YfptFLJANa/fsagw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=REYVTJtoOvsHAw3HuxDAeGKVEjro/arcSx2exN61qVqEdftHoe5q9cv3AKRdWymv4\n\tyaNjVvKzXVNzsWu6uLr1Q1jnDHz64gllveDHkxDsHlUKfRqWppVzZm6dQkvuE5SIF8\n\tl6PoXDCOFvCa9Nt6dKmZwWO12/lBv5RZJDT+gf5o=","Date":"Thu, 22 Oct 2020 04:16:13 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"<20201022011613.GU3942@pendragon.ideasonboard.com>","References":"<20201019102529.28359-1-paul.elder@ideasonboard.com>\n\t<20201019102529.28359-2-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201019102529.28359-2-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH 2/2] [DO NOT MERGE] libcamera:\n\tuvcvideo, request: Add tracepoints","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\" "}}]