[libcamera-devel] [RFC PATCH 1/2] libcamera: tracing: Implement tracing infrastructure

[Laurent Pinchart]

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