[libcamera-devel] [PATCH v2 6/6] Documentation: tracing: Add tracing guide

[Laurent Pinchart]

Hi Paul,

Yeeeeaahhhh more documentation \o/ Thank you for this.

On Wed, Oct 28, 2020 at 07:31:51PM +0900, Paul Elder wrote:
> Add guide for tracepoints, including how to define and use them.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> 
> ---
> New in v2
> ---
>  Documentation/guides/tracing.rst | 127 +++++++++++++++++++++++++++++++
>  Documentation/index.rst          |   1 +
>  Documentation/meson.build        |   1 +
>  3 files changed, 129 insertions(+)
>  create mode 100644 Documentation/guides/tracing.rst
> 
> diff --git a/Documentation/guides/tracing.rst b/Documentation/guides/tracing.rst
> new file mode 100644
> index 00000000..31d4ff6e
> --- /dev/null
> +++ b/Documentation/guides/tracing.rst
> @@ -0,0 +1,127 @@
> +.. SPDX-License-Identifier: CC-BY-SA-4.0
> +
> +Tracing Guide
> +=============
> +
> +Guide to tracing in libcamera.
> +
> +Profiling vs Tracing
> +--------------------
> +
> +Profiling is sampling at periodic points in time. This can be done with other
> +tools such as callgrind, perf, gprof, etc., without modification to the
> +application.
> +
> +Tracing is recording timestamps at specific locations. libcamera provides a
> +tracing facility. This guide shows how to use this tracing facility.

I would swap the two paragraphs, and write the second one as

"Tracing should not be confused with profiling, which samples execution
at periodic points in time. This can be done with other tools such as
callgrind, perf, gprof, etc., without modification to the application,
and is out of scope for this guide."

> +
> +Compiling
> +---------
> +
> +To compile libcamera with tracing support, it must be enabled in meson, with
> +``-Dtracepoints=enabled``.

'auto' works too. Maybe this could be phrased "... it must be enabled
through the meson ``tracepoints`` option." as you mention auto below ?

> It depends on ``liblttng-ust-dev`` (for Debian-based
> +distros).

And to be more generic, "It depends on the lttng-ust library (available
in the ``liblttng-ust-dev`` package for Debian-based distributions)." ?

> By default this build configuration option is set to ``auto``, so if

s/this build configuration/the tracepoint/ ?

> +liblttng is detected, it will be enabled by default. Conversely, if the option
> +is set to disabled, then libcamera will be compiled without tracing support.
> +
> +Defining tracepoints
> +--------------------
> +
> +The first of two steps to using tracepoints is to define the tracepoints.
> +
> +Create a file ``include/libcamera/internal/tracepoints/{file}.tp``, where
> +``file`` is a reasonable name related to the category of tracepoints that
> +you wish to define. For example, a tracepoints file for the Request object
> +would be called ``request.tp``. An entry for this file must be added in
> +``include/libcamera/internal/tracepoints/meson.build``.
> +
> +In this tracepoints file, define your tracepoints `as mandated by lttng
> +<https://lttng.org/man/3/lttng-ust>`_. The header boilerplate is not necessary,
> +and you may simply begin with ``TRACEPOINT_EVENT`` (or similar).

Should we state that the boilerplate part must not be included, as it
could conflict with the rest of our infrastructure ? Maybe "Only the
trace point definitions (with the ``TRACEPOINT_*`` macros should be
included, the boilerplate part of the tracepoint provider must be left
out."  ?

> +
> +All tracepoint providers shall be ``libcamera``. According to lttng, the
> +tracepoint provider shold be per-project; this is the rationale for this

s/shold/should/

> +decision. To group tracepoint events, we recommend using
> +``{class_name}_{tracepoint_name}``, for example, ``request_construct`` for a
> +tracepoint for the constructor of a Request object.

s/a Request object/the Request class/

> +
> +Tracepoint arguments may take C++ objects pointers, in which case the usual
> +C++ namespacing rules apply. The header that contains the necessary class
> +definitions should be ``#include`` ed as well.

"... must be included at the top of the tracepoint provider file." ?

> +
> +Note: the final parameter in ``TP_ARGS`` *must not* have a trailing comma, and
> +the parametsrs to ``TP_FIELDS`` are *space-separated*. Not following these will

s/parametsrs/parameters/

> +cause compiler errors.

s/compiler/compilation/ ?

> +
> +Using tracepoints (in libcamera)
> +--------------------------------
> +
> +To use tracepoints in libcamera, first the header needs to be included:
> +
> +``#include "libcamera/internal/tracepoints.h"``
> +
> +Then to use the tracepoint:
> +
> +``LIBCAMERA_TRACEPOINT({tracepoint_event}, args...)``
> +
> +This macro must be used, as opposed to lttng's macros directly, because
> +lttng is an optional dependency of libcamera, so the code must compile and run
> +even when lttng is not present.

"... not present or when tracing is disabled" ?

> +
> +The tracepoint provider name, as declared in the tracepoint definition, is not
> +included in the parameters of the tracepoint.
> +
> +Using tracepoints (from an application)
> +---------------------------------------
> +
> +As applications are not part of libcamera, but rather users of libcamera,
> +applications should seek their own tracing mechanisms. For ease of tracing
> +the application alongside tracing libcamera, it is recommended to also
> +`use lttng <https://lttng.org/docs/#doc-tracing-your-own-user-application>`_.
> +
> +Using tracepoints (from closed-source IPA)
> +------------------------------------------
> +
> +Similar to applications, closed-source IPAs can simply use lttng on their own,
> +or any other tracing mechanism if desired.
> +
> +Collecting a trace
> +------------------
> +
> +A trace can be collected fairly simply from lttng:
> +
> +.. code-block:: bash
> +
> +   lttng create $SESSION_NAME
> +   lttng enable-event -u libcamera:\*
> +   lttng start
> +   # run libcamera application
> +   lttng stop
> +   lttng view
> +   lttng destroy $SESSION_NAME
> +
> +See the `lttng documentation <https://lttng.org/docs/>`_ for further details.
> +
> +Analyzing a trace
> +-----------------
> +
> +As mentioned above, while an lttng tracing session exists and the trace is not
> +running, the trace output can be obtained by ``lttng view``.
> +
> +babeltrace2 can also be used to view the trace log as text output.  See the

"The trace log can also be viewed as text using babeltrace2." ?

> +`lttng documentation <https://lttng.org/docs/#doc-viewing-and-analyzing-your-traces-bt>`_

I'm getting an error from sphynx:

[9/134] Generating documentation with a custom command
FAILED: Documentation/html
/usr/bin/sphinx-build -D release=v0.0.0+2041-ad88b789 -q -W -b html /[...]/Documentation Documentation/html

Warning, treated as error:
/[...]/Documentation/guides/tracing.rst:4:Duplicate explicit target name: "lttng documentation".

> +for further details.
> +
> +babeltrace2 also has a C API and python bindings that can be used to process
> +traces. See the
> +`lttng python bindings documentation <https://babeltrace.org/docs/v2.0/python/bt2/>`_
> +and the
> +`lttng C API documentation <https://babeltrace.org/docs/v2.0/libbabeltrace2/>`_
> +for more details.

You can move links to the bottom of the document and only reference them
in the text if you wish, to reflow it more easily. There are examples in
the documentation.

> +
> +As an example, there is a script ``utils/tracepoints/analyze.py`` that
> +searches the trace for pairs of events ``libcamera:ipa_call_start`` and
> +``libcamera:ipa_call_finish``, and gathers statistics on the time interval
> +between these pairs of these events, grouped by function name. In other words,

"these pairs of these events" sounds weird.

> +this script gathers statistics for the time taken for an IPA function call
> +(with the tracepoints inserted in the appropriate places).

Maybe this could be reworked to avoid the need to explain it in other
words, by making it clearer the first time ? :-)

Overall this is a good introduction, I'm sure we'll extend it.

> diff --git a/Documentation/index.rst b/Documentation/index.rst
> index 173c326f..8bc8922e 100644
> --- a/Documentation/index.rst
> +++ b/Documentation/index.rst
> @@ -17,3 +17,4 @@
>     Application Writer's Guide <guides/application-developer>
>     Pipeline Handler Writer's Guide <guides/pipeline-handler>
>     IPA Writer's guide <guides/ipa>
> +   Tracing guide <guides/tracing>
> diff --git a/Documentation/meson.build b/Documentation/meson.build
> index f2300dac..17f3b9d7 100644
> --- a/Documentation/meson.build
> +++ b/Documentation/meson.build
> @@ -55,6 +55,7 @@ if sphinx.found()
>          'guides/ipa.rst',
>          'guides/application-developer.rst',
>          'guides/pipeline-handler.rst',
> +        'guides/tracing.rst',
>      ]
>  
>      release = 'release=v' + libcamera_git_version

-- 
Regards,

Laurent Pinchart