[libcamera-devel] [RFC PATCH 1/2] libcamera: tracing: Implement tracing infrastructure
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Oct 22 01:44:23 CEST 2020
Hi Paul,
Thank you for the patch.
On Mon, Oct 19, 2020 at 07:25:28PM +0900, Paul Elder wrote:
> Implement tracing infrastructure in libcamera. It takes .tp files, as
> required by lttng, and generates a tracepoint header and C file, as lttng
> requires. meson is updated accordingly to get it to compile with the
> rest of libcamera. Update the documentation accordingly.
Maybe this could be expanded a little bit to explain we're basing the
implementation on lttng ? It's implied by "as required by lttng", but
never explicitly stated (with a bit of extra information about what
lttng is or why we picked it, as applicable).
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
> Documentation/Doxyfile.in | 2 +
I think a tracing.rst will be useful :-)
> README.rst | 3 ++
> include/libcamera/internal/meson.build | 9 +++++
> .../internal/tracepoints/meson.build | 4 ++
> meson.build | 3 ++
> meson_options.txt | 5 +++
> src/libcamera/meson.build | 7 ++++
> src/libcamera/tracepoints.cpp | 10 +++++
> utils/meson.build | 1 +
> utils/tracepoints/generate_header.py | 40 +++++++++++++++++++
> utils/tracepoints/header.tmpl | 40 +++++++++++++++++++
> utils/tracepoints/meson.build | 5 +++
> 12 files changed, 129 insertions(+)
> create mode 100644 include/libcamera/internal/tracepoints/meson.build
> create mode 100644 src/libcamera/tracepoints.cpp
> create mode 100644 utils/tracepoints/generate_header.py
> create mode 100644 utils/tracepoints/header.tmpl
> create mode 100644 utils/tracepoints/meson.build
>
> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> index 5ccd0d05..9a7a3fb4 100644
> --- a/Documentation/Doxyfile.in
> +++ b/Documentation/Doxyfile.in
> @@ -842,6 +842,8 @@ EXCLUDE = @TOP_SRCDIR@/include/libcamera/span.h \
> @TOP_SRCDIR@/src/libcamera/ipa_ipc_unixsocket.cpp \
> @TOP_SRCDIR@/src/libcamera/pipeline/ \
> @TOP_SRCDIR@/src/libcamera/proxy/ \
> + @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \
> + @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \
> @TOP_BUILDDIR@/include/libcamera/ipa/ \
> @TOP_BUILDDIR@/src/libcamera/proxy/
>
> diff --git a/README.rst b/README.rst
> index fc8f4948..039015ac 100644
> --- a/README.rst
> +++ b/README.rst
> @@ -81,6 +81,9 @@ for gstreamer: [optional]
> for qcam: [optional]
> qtbase5-dev libqt5core5a libqt5gui5 libqt5widgets5 qttools5-dev-tools libtiff-dev
>
> +for tracing with lttng: [optional]
> + lttng-ust-dev
> +
> Using GStreamer plugin
> ~~~~~~~~~~~~~~~~~~~~~~
>
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> index 8dd744fd..21805edd 100644
> --- a/include/libcamera/internal/meson.build
> +++ b/include/libcamera/internal/meson.build
> @@ -1,5 +1,14 @@
> # SPDX-License-Identifier: CC0-1.0
>
> +subdir('tracepoints')
> +
> +libcamera_tracepoint_header = custom_target(
> + 'tp_header',
> + input: [ tracepoints_header_template, tracepoint_files ],
> + output: 'tracepoints.h',
> + command: [ tracepoints_header_generator, '@OUTPUT@', '@INPUT@' ],
> +)
> +
> libcamera_internal_headers = files([
> 'byte_stream_buffer.h',
> 'camera_controls.h',
> diff --git a/include/libcamera/internal/tracepoints/meson.build b/include/libcamera/internal/tracepoints/meson.build
> new file mode 100644
> index 00000000..2dd6733e
> --- /dev/null
> +++ b/include/libcamera/internal/tracepoints/meson.build
> @@ -0,0 +1,4 @@
> +# SPDX-License-Identifier: CC0-1.0
> +
> +tracepoint_files = files([
> +])
> diff --git a/meson.build b/meson.build
> index de15cc16..b6a8c2d2 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -103,6 +103,9 @@ libcamera_includes = include_directories('include')
> # Sub-directories fill py_modules with their dependencies.
> py_modules = []
>
> +# We need to check in include/ if we need to generate the tracepoint header
I understand this as we've discussed this topic before, but I'm not sure
it's very clear. How about
# Libraries used by multiple components.
or something similar ?
> +liblttng = cc.find_library('lttng-ust', required : get_option('tracepoints'))
> +
> # Utilities are parsed first to provide support for other components.
> subdir('utils')
>
> diff --git a/meson_options.txt b/meson_options.txt
> index 7f7b3e59..9e768beb 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -28,6 +28,11 @@ option('test',
> type : 'boolean',
> description: 'Compile and include the tests')
>
> +option('tracepoints',
Maybe 'tracing' ? Not sure I have a preference.
> + type : 'feature',
> + value : 'auto',
> + description: 'Compile libcamera with lttng tracepoints')
And 'Enable tracing (based on lttng)' ?
> +
> option('v4l2',
> type : 'boolean',
> value : false,
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 42340eae..7dc77c67 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -58,6 +58,7 @@ libcamera_sources = files([
>
> libcamera_sources += libcamera_public_headers
> libcamera_sources += libcamera_generated_headers
> +libcamera_sources += libcamera_tracepoint_header
Would it make sense to instead add libcamera_tracepoint_header to
libcamera_generated_headers in include/libcamera/internal/meson.build ?
>
> includes = [
> libcamera_includes,
> @@ -75,6 +76,11 @@ if libgnutls.found()
> config_h.set('HAVE_GNUTLS', 1)
> endif
>
> +if liblttng.found()
> + config_h.set('TRACING_ENABLED', 1)
We usually use HAVE_xxx in config.h, should this be HAVE_TRACING ?
> + libcamera_sources += files(['tracepoints.cpp'])
> +endif
> +
> if libudev.found()
> config_h.set('HAVE_LIBUDEV', 1)
> libcamera_sources += files([
> @@ -117,6 +123,7 @@ libcamera_deps = [
> libatomic,
> libdl,
> libgnutls,
> + liblttng,
> libudev,
> dependency('threads'),
> ]
> diff --git a/src/libcamera/tracepoints.cpp b/src/libcamera/tracepoints.cpp
> new file mode 100644
> index 00000000..0173b75a
> --- /dev/null
> +++ b/src/libcamera/tracepoints.cpp
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * tracepoints.cpp - Tracepoints with lttng
> + */
> +#define TRACEPOINT_CREATE_PROBES
> +#define TRACEPOINT_DEFINE
> +
> +#include "libcamera/internal/tracepoints.h"
> diff --git a/utils/meson.build b/utils/meson.build
> index 383d5285..8e28ada7 100644
> --- a/utils/meson.build
> +++ b/utils/meson.build
> @@ -2,6 +2,7 @@
>
> subdir('ipc')
> subdir('ipu3')
> +subdir('tracepoints')
>
> ## Code generation
> py_modules += ['yaml']
> diff --git a/utils/tracepoints/generate_header.py b/utils/tracepoints/generate_header.py
> new file mode 100644
> index 00000000..52268eba
> --- /dev/null
> +++ b/utils/tracepoints/generate_header.py
I'd name this gen-tp-header.py to be consistent with our other scripts.
You could also stored it directly in utils/, as (if you agree with my
comment below) it would be the only file in utils/tracepoints/, but I
expect more scripts to be added there to analyze traces. On the other
hand, keeping the analyzer scripts separate from the header generation
could be good too, so gen-tp-header.py may be best in any case.
> @@ -0,0 +1,40 @@
> +#!/usr/bin/env python3
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# Copyright (C) 2020, Google Inc.
> +#
> +# Author: Paul Elder <paul.elder at ideasonboard.com>
> +#
> +# generate_header.py - Generate header file to contain lttng tracepoints
> +
> +import datetime
> +import jinja2
Could you list this as a dependency in README.md, and add it to
py_modules in meson.build ?
> +import os
> +import sys
> +
> +def main(argv):
> + if len(argv) < 3:
Should this be 4, not 3 ?
> + print(f'Usage: {argv[0]} output template tp_files...')
> + return 1
> +
> + output = argv[1]
> + template = argv[2]
> +
> + year = datetime.datetime.now().year
> + path = output.replace('include/', '', 1)
> +
> + source = ''
> + for fname in argv[3:]:
> + source += open(fname, 'r').read() + '\n\n'
> +
> + template = jinja2.Template(open(template, 'r').read())
> + string = template.render(year=year, path=path, source=source)
> +
> + f = open(output, 'w')
> + f.write(string)
> + f.close()
You could write this
open(output, 'w').write(string)
Up to you.
> +
> + return 0
> +
> +
> +if __name__ == '__main__':
> + sys.exit(main(sys.argv))
> diff --git a/utils/tracepoints/header.tmpl b/utils/tracepoints/header.tmpl
> new file mode 100644
> index 00000000..b1595656
> --- /dev/null
> +++ b/utils/tracepoints/header.tmpl
We already have a few templates in libcamera, and we store them in the
applicable header directory, with a .in suffix (this would thus be
tracepoints.h.in, in include/libcamera/internal/). Any reason to depart
from that practice ?
> @@ -0,0 +1,40 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) {{year}}, Google Inc.
> + *
> + * tracepoints.h - Tracepoints with lttng
> + *
> + * This file is auto-generated. Do not edit.
> + */
> +#ifndef __LIBCAMERA_INTERNAL_TRACEPOINTS_H__
> +#define __LIBCAMERA_INTERNAL_TRACEPOINTS_H__
> +
> +#if TRACING_ENABLED
> +#define LIBCAMERA_TRACEPOINT(...) tracepoint(libcamera, __VA_ARGS__)
> +#else
> +#define LIBCAMERA_TRACEPOINT(...) do {} while (0)
> +#endif
> +
> +#endif /* __LIBCAMERA_INTERNAL_TRACEPOINTS_H__ */
> +
> +
> +#if TRACING_ENABLED
> +
> +#undef TRACEPOINT_PROVIDER
> +#define TRACEPOINT_PROVIDER libcamera
> +
> +#undef TRACEPOINT_INCLUDE
> +#define TRACEPOINT_INCLUDE "{{path}}"
> +
> +#if !defined(INCLUDE_LIBCAMERA_INTERNAL_TRACEPOINTS_TP_H) || defined(TRACEPOINT_HEADER_MULTI_READ)
> +#define INCLUDE_LIBCAMERA_INTERNAL_TRACEPOINTS_TP_H
> +
> +#include <lttng/tracepoint.h>
> +
> +{{source}}
> +
> +#endif /* INCLUDE_LIBCAMERA_INTERNAL_TRACEPOINTS_TP_H */
> +
> +#include <lttng/tracepoint-event.h>
> +
> +#endif /* TRACING_ENABLED */
> diff --git a/utils/tracepoints/meson.build b/utils/tracepoints/meson.build
> new file mode 100644
> index 00000000..7294287b
> --- /dev/null
> +++ b/utils/tracepoints/meson.build
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: CC0-1.0
> +
> +tracepoints_header_template = files(['header.tmpl'])
> +
> +tracepoints_header_generator = find_program('./generate_header.py')
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list