[{"id":13381,"web_url":"https://patchwork.libcamera.org/comment/13381/","msgid":"<20201021234423.GT3942@pendragon.ideasonboard.com>","date":"2020-10-21T23:44:23","subject":"Re: [libcamera-devel] [RFC PATCH 1/2] libcamera: tracing: Implement\n\ttracing infrastructure","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:28PM +0900, Paul Elder wrote:\n> Implement tracing infrastructure in libcamera. It takes .tp files, as\n> required by lttng, and generates a tracepoint header and C file, as lttng\n> requires. meson is updated accordingly to get it to compile with the\n> rest of libcamera. Update the documentation accordingly.\n\nMaybe this could be expanded a little bit to explain we're basing the\nimplementation on lttng ? It's implied by \"as required by lttng\", but\nnever explicitly stated (with a bit of extra information about what\nlttng is or why we picked it, as applicable).\n\n> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> ---\n>  Documentation/Doxyfile.in                     |  2 +\n\nI think a tracing.rst will be useful :-)\n\n>  README.rst                                    |  3 ++\n>  include/libcamera/internal/meson.build        |  9 +++++\n>  .../internal/tracepoints/meson.build          |  4 ++\n>  meson.build                                   |  3 ++\n>  meson_options.txt                             |  5 +++\n>  src/libcamera/meson.build                     |  7 ++++\n>  src/libcamera/tracepoints.cpp                 | 10 +++++\n>  utils/meson.build                             |  1 +\n>  utils/tracepoints/generate_header.py          | 40 +++++++++++++++++++\n>  utils/tracepoints/header.tmpl                 | 40 +++++++++++++++++++\n>  utils/tracepoints/meson.build                 |  5 +++\n>  12 files changed, 129 insertions(+)\n>  create mode 100644 include/libcamera/internal/tracepoints/meson.build\n>  create mode 100644 src/libcamera/tracepoints.cpp\n>  create mode 100644 utils/tracepoints/generate_header.py\n>  create mode 100644 utils/tracepoints/header.tmpl\n>  create mode 100644 utils/tracepoints/meson.build\n> \n> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> index 5ccd0d05..9a7a3fb4 100644\n> --- a/Documentation/Doxyfile.in\n> +++ b/Documentation/Doxyfile.in\n> @@ -842,6 +842,8 @@ EXCLUDE                = @TOP_SRCDIR@/include/libcamera/span.h \\\n>  \t\t\t @TOP_SRCDIR@/src/libcamera/ipa_ipc_unixsocket.cpp \\\n>  \t\t\t @TOP_SRCDIR@/src/libcamera/pipeline/ \\\n>  \t\t\t @TOP_SRCDIR@/src/libcamera/proxy/ \\\n> +\t\t\t @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \\\n> +\t\t\t @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \\\n>  \t\t\t @TOP_BUILDDIR@/include/libcamera/ipa/ \\\n>  \t\t\t @TOP_BUILDDIR@/src/libcamera/proxy/\n>  \n> diff --git a/README.rst b/README.rst\n> index fc8f4948..039015ac 100644\n> --- a/README.rst\n> +++ b/README.rst\n> @@ -81,6 +81,9 @@ for gstreamer: [optional]\n>  for qcam: [optional]\n>  \tqtbase5-dev libqt5core5a libqt5gui5 libqt5widgets5 qttools5-dev-tools libtiff-dev\n>  \n> +for tracing with lttng: [optional]\n> +        lttng-ust-dev\n> +\n>  Using GStreamer plugin\n>  ~~~~~~~~~~~~~~~~~~~~~~\n>  \n> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> index 8dd744fd..21805edd 100644\n> --- a/include/libcamera/internal/meson.build\n> +++ b/include/libcamera/internal/meson.build\n> @@ -1,5 +1,14 @@\n>  # SPDX-License-Identifier: CC0-1.0\n>  \n> +subdir('tracepoints')\n> +\n> +libcamera_tracepoint_header = custom_target(\n> +    'tp_header',\n> +    input: [ tracepoints_header_template, tracepoint_files ],\n> +    output: 'tracepoints.h',\n> +    command: [ tracepoints_header_generator, '@OUTPUT@', '@INPUT@' ],\n> +)\n> +\n>  libcamera_internal_headers = files([\n>      'byte_stream_buffer.h',\n>      'camera_controls.h',\n> diff --git a/include/libcamera/internal/tracepoints/meson.build b/include/libcamera/internal/tracepoints/meson.build\n> new file mode 100644\n> index 00000000..2dd6733e\n> --- /dev/null\n> +++ b/include/libcamera/internal/tracepoints/meson.build\n> @@ -0,0 +1,4 @@\n> +# SPDX-License-Identifier: CC0-1.0\n> +\n> +tracepoint_files = files([\n> +])\n> diff --git a/meson.build b/meson.build\n> index de15cc16..b6a8c2d2 100644\n> --- a/meson.build\n> +++ b/meson.build\n> @@ -103,6 +103,9 @@ libcamera_includes = include_directories('include')\n>  # Sub-directories fill py_modules with their dependencies.\n>  py_modules = []\n>  \n> +# We need to check in include/ if we need to generate the tracepoint header\n\nI understand this as we've discussed this topic before, but I'm not sure\nit's very clear. How about\n\n# Libraries used by multiple components.\n\nor something similar ?\n\n> +liblttng = cc.find_library('lttng-ust', required : get_option('tracepoints'))\n> +\n>  # Utilities are parsed first to provide support for other components.\n>  subdir('utils')\n>  \n> diff --git a/meson_options.txt b/meson_options.txt\n> index 7f7b3e59..9e768beb 100644\n> --- a/meson_options.txt\n> +++ b/meson_options.txt\n> @@ -28,6 +28,11 @@ option('test',\n>          type : 'boolean',\n>          description: 'Compile and include the tests')\n>  \n> +option('tracepoints',\n\nMaybe 'tracing' ? Not sure I have a preference.\n\n> +        type : 'feature',\n> +        value : 'auto',\n> +        description: 'Compile libcamera with lttng tracepoints')\n\nAnd 'Enable tracing (based on lttng)' ?\n\n> +\n>  option('v4l2',\n>          type : 'boolean',\n>          value : false,\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 42340eae..7dc77c67 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -58,6 +58,7 @@ libcamera_sources = files([\n>  \n>  libcamera_sources += libcamera_public_headers\n>  libcamera_sources += libcamera_generated_headers\n> +libcamera_sources += libcamera_tracepoint_header\n\nWould it make sense to instead add libcamera_tracepoint_header to\nlibcamera_generated_headers in include/libcamera/internal/meson.build ?\n\n>  \n>  includes = [\n>      libcamera_includes,\n> @@ -75,6 +76,11 @@ if libgnutls.found()\n>      config_h.set('HAVE_GNUTLS', 1)\n>  endif\n>  \n> +if liblttng.found()\n> +    config_h.set('TRACING_ENABLED', 1)\n\nWe usually use HAVE_xxx in config.h, should this be HAVE_TRACING ?\n\n> +    libcamera_sources += files(['tracepoints.cpp'])\n> +endif\n> +\n>  if libudev.found()\n>      config_h.set('HAVE_LIBUDEV', 1)\n>      libcamera_sources += files([\n> @@ -117,6 +123,7 @@ libcamera_deps = [\n>      libatomic,\n>      libdl,\n>      libgnutls,\n> +    liblttng,\n>      libudev,\n>      dependency('threads'),\n>  ]\n> diff --git a/src/libcamera/tracepoints.cpp b/src/libcamera/tracepoints.cpp\n> new file mode 100644\n> index 00000000..0173b75a\n> --- /dev/null\n> +++ b/src/libcamera/tracepoints.cpp\n> @@ -0,0 +1,10 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * tracepoints.cpp - Tracepoints with lttng\n> + */\n> +#define TRACEPOINT_CREATE_PROBES\n> +#define TRACEPOINT_DEFINE\n> +\n> +#include \"libcamera/internal/tracepoints.h\"\n> diff --git a/utils/meson.build b/utils/meson.build\n> index 383d5285..8e28ada7 100644\n> --- a/utils/meson.build\n> +++ b/utils/meson.build\n> @@ -2,6 +2,7 @@\n>  \n>  subdir('ipc')\n>  subdir('ipu3')\n> +subdir('tracepoints')\n>  \n>  ## Code generation\n>  py_modules += ['yaml']\n> diff --git a/utils/tracepoints/generate_header.py b/utils/tracepoints/generate_header.py\n> new file mode 100644\n> index 00000000..52268eba\n> --- /dev/null\n> +++ b/utils/tracepoints/generate_header.py\n\nI'd name this gen-tp-header.py to be consistent with our other scripts.\nYou could also stored it directly in utils/, as (if you agree with my\ncomment below) it would be the only file in utils/tracepoints/, but I\nexpect more scripts to be added there to analyze traces. On the other\nhand, keeping the analyzer scripts separate from the header generation\ncould be good too, so gen-tp-header.py may be best in any case.\n\n> @@ -0,0 +1,40 @@\n> +#!/usr/bin/env python3\n> +# SPDX-License-Identifier: GPL-2.0-or-later\n> +# Copyright (C) 2020, Google Inc.\n> +#\n> +# Author: Paul Elder <paul.elder@ideasonboard.com>\n> +#\n> +# generate_header.py - Generate header file to contain lttng tracepoints\n> +\n> +import datetime\n> +import jinja2\n\nCould you list this as a dependency in README.md, and add it to\npy_modules in meson.build ?\n\n> +import os\n> +import sys\n> +\n> +def main(argv):\n> +    if len(argv) < 3:\n\nShould this be 4, not 3 ?\n\n> +        print(f'Usage: {argv[0]} output template tp_files...')\n> +        return 1\n> +\n> +    output = argv[1]\n> +    template = argv[2]\n> +\n> +    year = datetime.datetime.now().year\n> +    path = output.replace('include/', '', 1)\n> +\n> +    source = ''\n> +    for fname in argv[3:]:\n> +        source += open(fname, 'r').read() + '\\n\\n'\n> +\n> +    template = jinja2.Template(open(template, 'r').read())\n> +    string = template.render(year=year, path=path, source=source)\n> +\n> +    f = open(output, 'w')\n> +    f.write(string)\n> +    f.close()\n\nYou could write this\n\n    open(output, 'w').write(string)\n\nUp to you.\n\n> +\n> +    return 0\n> +\n> +\n> +if __name__ == '__main__':\n> +    sys.exit(main(sys.argv))\n> diff --git a/utils/tracepoints/header.tmpl b/utils/tracepoints/header.tmpl\n> new file mode 100644\n> index 00000000..b1595656\n> --- /dev/null\n> +++ b/utils/tracepoints/header.tmpl\n\nWe already have a few templates in libcamera, and we store them in the\napplicable header directory, with a .in suffix (this would thus be\ntracepoints.h.in, in include/libcamera/internal/). Any reason to depart\nfrom that practice ?\n\n> @@ -0,0 +1,40 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) {{year}}, Google Inc.\n> + *\n> + * tracepoints.h - Tracepoints with lttng\n> + *\n> + * This file is auto-generated. Do not edit.\n> + */\n> +#ifndef __LIBCAMERA_INTERNAL_TRACEPOINTS_H__\n> +#define __LIBCAMERA_INTERNAL_TRACEPOINTS_H__\n> +\n> +#if TRACING_ENABLED\n> +#define LIBCAMERA_TRACEPOINT(...) tracepoint(libcamera, __VA_ARGS__)\n> +#else\n> +#define LIBCAMERA_TRACEPOINT(...) do {} while (0)\n> +#endif\n> +\n> +#endif /* __LIBCAMERA_INTERNAL_TRACEPOINTS_H__ */\n> +\n> +\n> +#if TRACING_ENABLED\n> +\n> +#undef TRACEPOINT_PROVIDER\n> +#define TRACEPOINT_PROVIDER libcamera\n> +\n> +#undef TRACEPOINT_INCLUDE\n> +#define TRACEPOINT_INCLUDE \"{{path}}\"\n> +\n> +#if !defined(INCLUDE_LIBCAMERA_INTERNAL_TRACEPOINTS_TP_H) || defined(TRACEPOINT_HEADER_MULTI_READ)\n> +#define INCLUDE_LIBCAMERA_INTERNAL_TRACEPOINTS_TP_H\n> +\n> +#include <lttng/tracepoint.h>\n> +\n> +{{source}}\n> +\n> +#endif /* INCLUDE_LIBCAMERA_INTERNAL_TRACEPOINTS_TP_H */\n> +\n> +#include <lttng/tracepoint-event.h>\n> +\n> +#endif /* TRACING_ENABLED */\n> diff --git a/utils/tracepoints/meson.build b/utils/tracepoints/meson.build\n> new file mode 100644\n> index 00000000..7294287b\n> --- /dev/null\n> +++ b/utils/tracepoints/meson.build\n> @@ -0,0 +1,5 @@\n> +# SPDX-License-Identifier: CC0-1.0\n> +\n> +tracepoints_header_template = files(['header.tmpl'])\n> +\n> +tracepoints_header_generator = find_program('./generate_header.py')","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 6E985C3D3C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 21 Oct 2020 23:45:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 054BB610FD;\n\tThu, 22 Oct 2020 01:45:11 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C4F33603B0\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 22 Oct 2020 01:45:09 +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 3447D52F;\n\tThu, 22 Oct 2020 01:45:09 +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=\"cJbE5iu3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603323909;\n\tbh=q0qwgjMhV1kiuZpF+GKHsws8flbvfreOljbOsrPNGuc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cJbE5iu3lZK+aJi578vnKdBi8EhjgXpusquW1IS3enPZQEfRvMAopxrsCmL4ZmWU0\n\tdAziVGRbOcZR6glSHBm2YDyftMl0WX2tzirqnQR8ZHW5GKWH5R3chAJMYVvdB4uIT9\n\taL62uuw71CDgGMTP9nvIQz6O6d14z1fx0LrDsZwE=","Date":"Thu, 22 Oct 2020 02:44:23 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Paul Elder <paul.elder@ideasonboard.com>","Message-ID":"<20201021234423.GT3942@pendragon.ideasonboard.com>","References":"<20201019102529.28359-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201019102529.28359-1-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH 1/2] libcamera: tracing: Implement\n\ttracing infrastructure","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","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\" <libcamera-devel-bounces@lists.libcamera.org>"}}]