<div dir="ltr"><div dir="ltr">Thanks Paul! And sorry for the delay.</div><div dir="ltr"><br></div><div>Please check the v3 patch, which should contain most of the fixes according to your</div><div>comments.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Dec 14, 2022 at 2:49 PM Paul Elder <<a href="mailto:paul.elder@ideasonboard.com">paul.elder@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Harvey,<br>
<br>
Sorry for the delay.<br>
<br>
On Fri, Jul 22, 2022 at 12:00:28PM +0000, Harvey Yang via libcamera-devel wrote:<br>
> As ChromeOS is using perfetto (project named as CrOSetto). When ChromeOS<br>
> uses libcamera, it should use perfetto to collect traces instead of<br>
> lttng.<br>
> <br>
> Signed-off-by: Harvey Yang <<a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br>
> ---<br>
> Documentation/guides/tracing.rst | 114 ++++++++++++++----<br>
> include/libcamera/internal/<a href="http://tracepoints.h.in" rel="noreferrer" target="_blank">tracepoints.h.in</a> | 37 +++++-<br>
> .../internal/tracepoints/meson.build | 25 ++--<br>
> .../internal/tracepoints/pipeline.perfetto | 10 ++<br>
> .../internal/tracepoints/request.perfetto | 30 +++++<br>
> meson.build | 7 +-<br>
> src/android/cros/camera3_hal.cpp | 5 +<br>
> src/android/cros/meson.build | 1 +<br>
> src/libcamera/meson.build | 14 ++-<br>
> src/libcamera/pipeline_perfetto.cpp | 24 ++++<br>
> src/libcamera/request_perfetto.cpp | 73 +++++++++++<br>
> src/libcamera/tracepoints.cpp | 11 ++<br>
> 12 files changed, 312 insertions(+), 39 deletions(-)<br>
> create mode 100644 include/libcamera/internal/tracepoints/pipeline.perfetto<br>
> create mode 100644 include/libcamera/internal/tracepoints/request.perfetto<br>
> create mode 100644 src/libcamera/pipeline_perfetto.cpp<br>
> create mode 100644 src/libcamera/request_perfetto.cpp<br>
> <br>
> diff --git a/Documentation/guides/tracing.rst b/Documentation/guides/tracing.rst<br>
> index ae960d85..53d6232e 100644<br>
> --- a/Documentation/guides/tracing.rst<br>
> +++ b/Documentation/guides/tracing.rst<br>
> @@ -16,33 +16,50 @@ at periodic points in time. This can be done with other tools such as<br>
> callgrind, perf, gprof, etc., without modification to the application,<br>
> and is out of scope for this guide.<br>
> <br>
> +Library: Perfetto vs lttng-ust<br>
> +---------<br>
<br>
The underline needs to be extended. (also in the other instances below)<br>
<br></blockquote><div><br></div><div>Done. Please check if I missed anything.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> +To integrate with CrOS tracing infrastructure, which uses perfetto (or called<br>
> +CrOSetto in CrOS), we implement the tracepoint macros (will be described below)<br>
> +with two different libraries: CrOS with perfetto, and the rest with lttng-ust.<br>
> +<br>
> Compiling<br>
> ---------<br>
> <br>
> -To compile libcamera with tracing support, it must be enabled through the<br>
> -meson ``tracing`` option. It depends on the lttng-ust library (available in the<br>
> -``liblttng-ust-dev`` package for Debian-based distributions).<br>
> -By default the tracing option in meson is set to ``auto``, so if<br>
> -liblttng is detected, it will be enabled by default. Conversely, if the option<br>
> -is set to disabled, then libcamera will be compiled without tracing support.<br>
> +To compile libcamera with perfetto support in CrOS, it must be enabled through<br>
<br>
I'd rather the wording specify "tracing" generically first, then<br>
elaborate between perfetto and lttng.<br>
<br></blockquote><div><br></div><div>Right, updated. Please check.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +the meson ``tracing`` option. In CrOS, it depends on the perfetto library,<br>
> +which is by default available in CrOS; In the rest, it depends on the lttng-ust<br>
> +library (available in the ``liblttng-ust-dev`` package for Debian-based<br>
> +distributions).<br>
> +By default the tracing option in meson is set to ``auto``, so if the library is<br>
> +detected, it will be enabled by default. Conversely, if the option is set to<br>
> +disabled, then libcamera will be compiled without tracing support.<br>
<br>
I'm wondering, does it make sense to allow both to be compiled in at the<br>
same time? I guess we wouldn't use the same binary on CrOS and on<br>
non-CrOS, and we wouldn't use perfetto on non-CrOS. So I guess the only<br>
use case would be if lttng would ever be used on CrOS. Is that ever a<br>
use case?<br>
<br></blockquote><div><br></div><div>I don't think so. CrOS Camera already supports perfetto, and it's the performance</div><div>tracing tool being used across the OS and the VMs. So I believe we don't have</div><div>the use case to compile them both.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> <br>
> Defining tracepoints<br>
> --------------------<br>
> <br>
> libcamera already contains a set of tracepoints. To define additional<br>
> -tracepoints, create a file<br>
> -``include/libcamera/internal/tracepoints/{file}.tp``, where ``file`` is a<br>
> -reasonable name related to the category of tracepoints that you wish to<br>
> -define. For example, the tracepoints file for the Request object is called<br>
> -``request.tp``. An entry for this file must be added in<br>
> -``include/libcamera/internal/tracepoints/meson.build``.<br>
> -<br>
> -In this tracepoints file, define your tracepoints `as mandated by lttng<br>
> +tracepoints, create two files ``{file}.tp`` and ``{file}.perfetto``, for lttng<br>
> +and perfetto respectively, under ``include/libcamera/internal/tracepoints/``,<br>
> +and the perfetto implementation file ``src/libcamera/{file}_perfetto.cpp``,<br>
> +where ``file`` is a reasonable name related to the category of tracepoints that<br>
> +you wish to define. For example, the tracepoints files for the Request object is<br>
> +called ``request.tp`` and ``request.perfetto``. Entries for the files must be<br>
> +added in ``include/libcamera/internal/tracepoints/meson.build``.<br>
> +<br>
> +In the perfetto tracepoints files, declare the tracepoint functions in<br>
> +``{file}.perfetto``, and define them in ``{file}_perfetto.cpp``, `as mandated<br>
> +by perfetto <<a href="https://perfetto.dev/docs/instrumentation/track-events" rel="noreferrer" target="_blank">https://perfetto.dev/docs/instrumentation/track-events</a>>`_.<br>
> +Currently the only enabled perfetto::Category is ``libcamera``. If you intend to<br>
> +use another category, remember to define it in<br>
<br>
I'm wondering if category here in perfetto means something different<br>
than lttng's? lttng only has the top-level category then the trace name<br>
(afaik), so the top-level category is "libcamera" and after that we end<br>
up using an underscore-separated prefix.<br>
<br>
Does perfetto allow another level of catetories? I don't seem to see<br>
any, in which case I think we should just remove this section, as all<br>
the tracepoints that this section refers to are internal to libcamera,<br>
so they'd all use the "libcamera" category.<br>
<br></blockquote><div><br></div><div>So perfetto would allow multiple categories. Currently yes, all tracepoints</div><div>belong to the same category "libcamera". But I just wanted to mention</div><div>here that it's possible for developers to add other categories to filter </div><div>tracepoints in the future.</div><div><br></div><div>Do you think that's worth mentioning here?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +``include/libcamera/internal/<a href="http://tracepoints.h.in" rel="noreferrer" target="_blank">tracepoints.h.in</a>``, and enable it when collecting<br>
> +traces.<br>
<br>
Do you think it's feasible to have just one definition and we can use it<br>
to generate the other? I'd say use lttng's (because it's more specific,<br>
like how it specifies the types of the fields) and then use that to<br>
generate perfetto's.<br>
<br>
Or is it better to just make sure that each tracepoint is defined twice?<br>
I'm not fond of the duplicate work though, so tbh I'd rather one<br>
definition that generates the other, if that's feasible. (Is it time for<br>
me to write yet another code generator?)<br>
<br></blockquote><div><br></div><div>Sorry, I'm not sure what's in your picture, and I don't really understand</div><div>how lttng works: is it to have only one set of macro definitions</div><div>(LIBCAMERA_TRACEPOINT, LIBCAMERA_TRACEPOINT_IPA_BEGIN,</div><div>LIBCAMERA_TRACEPOINT_IPA_END)? Into the macro `tracepoint`?</div><div><br></div><div>Could you give a simple POC that covers one macro? Thanks! </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> +In the lttng tracepoints file, define your tracepoints `as mandated by lttng<br>
> <<a href="https://lttng.org/man/3/lttng-ust" rel="noreferrer" target="_blank">https://lttng.org/man/3/lttng-ust</a>>`_. The header boilerplate must *not* be<br>
> included (as it will conflict with the rest of our infrastructure), and<br>
> only the tracepoint definitions (with the ``TRACEPOINT_*`` macros) should be<br>
> included.<br>
> -<br>
> All tracepoint providers shall be ``libcamera``. According to lttng, the<br>
> tracepoint provider should be per-project; this is the rationale for this<br>
> decision. To group tracepoint events, we recommend using<br>
> @@ -68,9 +85,9 @@ Then to use the tracepoint:<br>
> <br>
> ``LIBCAMERA_TRACEPOINT({tracepoint_event}, args...)``<br>
> <br>
> -This macro must be used, as opposed to lttng's macros directly, because<br>
> -lttng is an optional dependency of libcamera, so the code must compile and run<br>
> -even when lttng is not present or when tracing is disabled.<br>
> +This macro must be used, as opposed to perfetto's/lttng's macros directly,<br>
<br>
s$perfetto's/lttng's$perfetto's or lttng's$<br>
<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +because tracing support is optional of libcamera, so the code must compile and<br>
> +run even when perfetto/lttng is not present or when tracing is disabled.<br>
<br>
s$perfetto/lttng is$perfetto or lttng are/<br>
<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> <br>
> The tracepoint provider name, as declared in the tracepoint definition, is not<br>
> included in the parameters of the tracepoint.<br>
> @@ -86,7 +103,7 @@ and when the pipeline handler receives the corresponding response from the IPA,<br>
> respectively. These are the tracepoints that our sample analysis script<br>
> (see "Analyzing a trace") scans for when computing statistics on IPA call time.<br>
> <br>
> -Using tracepoints (from an application)<br>
> +Using lttng tracepoints (from an application)<br>
<br>
I think this section applies regardless of if we're using lttng or<br>
perfetto.<br>
<br></blockquote><div><br></div><div>Right, updated. I also added a link to perfetto's doc. Please check :)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> ---------------------------------------<br>
> <br>
> As applications are not part of libcamera, but rather users of libcamera,<br>
> @@ -94,13 +111,66 @@ applications should seek their own tracing mechanisms. For ease of tracing<br>
> the application alongside tracing libcamera, it is recommended to also<br>
> `use lttng <<a href="https://lttng.org/docs/#doc-tracing-your-own-user-application" rel="noreferrer" target="_blank">https://lttng.org/docs/#doc-tracing-your-own-user-application</a>>`_.<br>
> <br>
> -Using tracepoints (from closed-source IPA)<br>
> +Using lttng tracepoints (from closed-source IPA)<br>
<br>
Same here.<br>
<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> ------------------------------------------<br>
> <br>
> Similar to applications, closed-source IPAs can simply use lttng on their own,<br>
> or any other tracing mechanism if desired.<br>
> <br>
> -Collecting a trace<br>
> +Collecting a perfetto trace<br>
> +------------------<br>
> +<br>
> +A trace can be collected with the following steps:<br>
> +<br>
> +1. Start `traced` if it hasn't been started.<br>
> +.. code-block:: bash<br>
> +<br>
> + start traced<br>
> +<br>
> +2. Start a consumer that includes “track_event” data source in the trace<br>
> + config, and “libcamera” category.<br>
> +.. code-block:: bash<br>
> +<br>
> + perfetto -c - --txt -o /tmp/perfetto-trace \<br>
> + <<EOF<br>
> +<br>
> + buffers: {<br>
> + size_kb: 63488<br>
> + fill_policy: DISCARD<br>
> + }<br>
> + buffers: {<br>
> + size_kb: 2048<br>
> + fill_policy: DISCARD<br>
> + }<br>
> + data_sources: {<br>
> + config {<br>
> + name: "track_event"<br>
> + track_event_config {<br>
> + enabled_categories: "libcamera"<br>
> + }<br>
> + }<br>
> + }<br>
> + duration_ms: 10000<br>
> +<br>
> + EOF<br>
<br>
Does this have to be fed through stdin or is there an option for<br>
feeding it as a file?<br>
<br></blockquote><div><br></div><div>Both stdin or a file work, `perfetto --help` shows:</div><div>```--config -c : /path/to/trace/config/file or - for stdin```<br></div><div><br></div><div>Do you think I should put this instead of instructing developers to use</div><div>stdin?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> +3. Execute the libcamera behavior you intend to trace during the set duration<br>
> + (10000 ms) in the above example.<br>
> +<br>
> +4. After the consumer (cmd `perfetto`) is done, you can find the trace result<br>
> + in ``/tmp/perfetto-trace``.<br>
> +<br>
> +Analyzing a perfetto trace<br>
> +-----------------<br>
> +<br>
> +Follow the `guide <<a href="https://perfetto.dev/docs/visualization/perfetto-ui" rel="noreferrer" target="_blank">https://perfetto.dev/docs/visualization/perfetto-ui</a>>`_ to<br>
> +visualize the trace.<br>
> +<br>
> +In other words, upload the trace to `Perfetto UI <<a href="https://ui.perfetto.dev/" rel="noreferrer" target="_blank">https://ui.perfetto.dev/</a>>`_,<br>
> +where you can check the timeline of tracepoints on each process/thread. You can<br>
> +also run SQL queries to do analysis.<br>
> +<br>
> +Collecting a lttng trace<br>
<br>
s/a/an/ (yeah, english sucks)<br>
<br></blockquote><div><br></div><div>Ah right. Thanks!</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> ------------------<br>
> <br>
> A trace can be collected fairly simply from lttng:<br>
> @@ -123,7 +193,7 @@ viewed by: ``lttng view -t $PATH_TO_TRACE``, where ``$PATH_TO_TRACE`` is the<br>
> path that was printed when the session was created. This is the same path that<br>
> is used when analyzing traces programatically, as described in the next section.<br>
> <br>
> -Analyzing a trace<br>
> +Analyzing a lttng trace<br>
<br>
Same here.<br>
<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> -----------------<br>
> <br>
> As mentioned above, while an lttng tracing session exists and the trace is not<br>
> diff --git a/include/libcamera/internal/<a href="http://tracepoints.h.in" rel="noreferrer" target="_blank">tracepoints.h.in</a> b/include/libcamera/internal/<a href="http://tracepoints.h.in" rel="noreferrer" target="_blank">tracepoints.h.in</a><br>
> index d0fc1365..d91fadd7 100644<br>
> --- a/include/libcamera/internal/<a href="http://tracepoints.h.in" rel="noreferrer" target="_blank">tracepoints.h.in</a><br>
> +++ b/include/libcamera/internal/<a href="http://tracepoints.h.in" rel="noreferrer" target="_blank">tracepoints.h.in</a><br>
> @@ -9,7 +9,24 @@<br>
> #ifndef __LIBCAMERA_INTERNAL_TRACEPOINTS_H__<br>
> #define __LIBCAMERA_INTERNAL_TRACEPOINTS_H__<br>
> <br>
> -#if HAVE_TRACING<br>
> +#if HAVE_PERFETTO<br>
> +<br>
> +#include <perfetto/perfetto.h><br>
> +<br>
> +PERFETTO_DEFINE_CATEGORIES(<br>
> + perfetto::Category("libcamera")<br>
> + .SetDescription("Events from libcamera"));<br>
> +<br>
> +#define LIBCAMERA_TRACEPOINT(t_name, ...) \<br>
> +LIBCAMERA_TRACE_EVENT_##t_name(__VA_ARGS__)<br>
> +<br>
> +#define LIBCAMERA_TRACEPOINT_IPA_BEGIN(pipe, func) \<br>
> +LIBCAMERA_TRACE_EVENT_ipa_call_begin(#pipe, #func)<br>
> +<br>
> +#define LIBCAMERA_TRACEPOINT_IPA_END(pipe, func) \<br>
> +LIBCAMERA_TRACE_EVENT_ipa_call_end(#pipe, #func)<br>
> +<br>
> +#elif HAVE_TRACING /* !HAVE_PERFETTO */<br>
> #define LIBCAMERA_TRACEPOINT(...) tracepoint(libcamera, __VA_ARGS__)<br>
> <br>
> #define LIBCAMERA_TRACEPOINT_IPA_BEGIN(pipe, func) \<br>
> @@ -18,7 +35,7 @@ tracepoint(libcamera, ipa_call_begin, #pipe, #func)<br>
> #define LIBCAMERA_TRACEPOINT_IPA_END(pipe, func) \<br>
> tracepoint(libcamera, ipa_call_end, #pipe, #func)<br>
> <br>
> -#else<br>
> +#else /* HAVE_PERFETTO */<br>
<br>
Should this be /* !HAVE_PERFETTO && !HAVE_TRACING */ ?<br>
<br></blockquote><div><br></div><div>Right, updated.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Also, probably we should rename TRACING to LTTNG everywhere. perfetto is<br>
tracing too, so it sounds kind of weird to say "!HAVE_TRACING but<br>
HAVE_PERFETTO", at least imo.<br>
<br></blockquote><div><br></div><div>Yes, I've also considered that. Updated.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> <br>
> namespace {<br>
> <br>
> @@ -34,12 +51,15 @@ inline void unused([[maybe_unused]] Args&& ...args)<br>
> #define LIBCAMERA_TRACEPOINT_IPA_BEGIN(pipe, func)<br>
> #define LIBCAMERA_TRACEPOINT_IPA_END(pipe, func)<br>
> <br>
> -#endif /* HAVE_TRACING */<br>
> +#endif /* HAVE_PERFETTO */<br>
> <br>
> #endif /* __LIBCAMERA_INTERNAL_TRACEPOINTS_H__ */<br>
> <br>
> +#if HAVE_PERFETTO<br>
> +<br>
> +#include <perfetto/perfetto.h><br>
> <br>
> -#if HAVE_TRACING<br>
> +#elif HAVE_TRACING<br>
> <br>
> #undef TRACEPOINT_PROVIDER<br>
> #define TRACEPOINT_PROVIDER libcamera<br>
> @@ -52,10 +72,15 @@ inline void unused([[maybe_unused]] Args&& ...args)<br>
> <br>
> #include <lttng/tracepoint.h><br>
> <br>
> -{{source}}<br>
<br>
This won't work, as we need the header guard on the next line.<br>
<br></blockquote><div><br></div><div>Updated. Please check again.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> <br>
> #endif /* INCLUDE_LIBCAMERA_INTERNAL_TRACEPOINTS_TP_H */<br>
> <br>
> #include <lttng/tracepoint-event.h><br>
> <br>
> -#endif /* HAVE_TRACING */<br>
> +#endif /* HAVE_PERFETTO */<br>
> +<br>
> +#if HAVE_PERFETTO || HAVE_TRACING<br>
> +<br>
> +{{source}}<br>
> +<br>
> +#endif /* HAVE_PERFETTO || HAVE_TRACING */<br>
<br>
So this needs to be shuffled around.<br>
<br></blockquote><div><br></div><div>Updated. Please check again.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> diff --git a/include/libcamera/internal/tracepoints/meson.build b/include/libcamera/internal/tracepoints/meson.build<br>
> index d9b2fca5..ff5aece6 100644<br>
> --- a/include/libcamera/internal/tracepoints/meson.build<br>
> +++ b/include/libcamera/internal/tracepoints/meson.build<br>
> @@ -1,12 +1,19 @@<br>
> # SPDX-License-Identifier: CC0-1.0<br>
> <br>
> -# enum files must go first<br>
> -tracepoint_files = files([<br>
> - 'buffer_enums.tp',<br>
> - 'request_enums.tp',<br>
> -])<br>
> +if get_option('android').enabled() and get_option('android_platform') == 'cros'<br>
> + tracepoint_files = files([<br>
> + 'pipeline.perfetto',<br>
> + 'request.perfetto',<br>
> + ])<br>
> +else<br>
> + # enum files must go first<br>
> + tracepoint_files = files([<br>
> + 'buffer_enums.tp',<br>
> + 'request_enums.tp',<br>
> + ])<br>
> <br>
> -tracepoint_files += files([<br>
> - 'pipeline.tp',<br>
> - 'request.tp',<br>
> -])<br>
> + tracepoint_files += files([<br>
> + 'pipeline.tp',<br>
> + 'request.tp',<br>
> + ])<br>
> +endif<br>
> diff --git a/include/libcamera/internal/tracepoints/pipeline.perfetto b/include/libcamera/internal/tracepoints/pipeline.perfetto<br>
> new file mode 100644<br>
> index 00000000..5f45295e<br>
> --- /dev/null<br>
> +++ b/include/libcamera/internal/tracepoints/pipeline.perfetto<br>
> @@ -0,0 +1,10 @@<br>
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> +/*<br>
> + * Copyright (C) 2022, Google Inc.<br>
> + *<br>
> + * pipeline.tp - Tracepoints for pipelines<br>
> + */<br>
> +<br>
> +void LIBCAMERA_TRACE_EVENT_ipa_call_begin(const char *pipe, const char *func);<br>
> +<br>
> +void LIBCAMERA_TRACE_EVENT_ipa_call_end(const char *pipe, const char *func);<br>
> diff --git a/include/libcamera/internal/tracepoints/request.perfetto b/include/libcamera/internal/tracepoints/request.perfetto<br>
> new file mode 100644<br>
> index 00000000..fd6a42a4<br>
> --- /dev/null<br>
> +++ b/include/libcamera/internal/tracepoints/request.perfetto<br>
> @@ -0,0 +1,30 @@<br>
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> +/*<br>
> + * Copyright (C) 2022, Google Inc.<br>
> + *<br>
> + * request.tp - Tracepoints for the request object<br>
> + */<br>
> +<br>
> +#include <libcamera/internal/request.h><br>
> +<br>
> +#include <libcamera/framebuffer.h><br>
> +<br>
> +void LIBCAMERA_TRACE_EVENT_request(libcamera::Request *req);<br>
> +<br>
> +void LIBCAMERA_TRACE_EVENT_request_construct(libcamera::Request *req);<br>
> +<br>
> +void LIBCAMERA_TRACE_EVENT_request_destroy(libcamera::Request *req);<br>
> +<br>
> +void LIBCAMERA_TRACE_EVENT_request_reuse(libcamera::Request *req);<br>
> +<br>
> +void LIBCAMERA_TRACE_EVENT_request_queue(libcamera::Request *req);<br>
> +<br>
> +void LIBCAMERA_TRACE_EVENT_request_device_queue(libcamera::Request *req);<br>
> +<br>
> +void LIBCAMERA_TRACE_EVENT_request_complete(libcamera::Request::Private *req);<br>
> +<br>
> +void LIBCAMERA_TRACE_EVENT_request_cancel(libcamera::Request::Private *req);<br>
> +<br>
> +void LIBCAMERA_TRACE_EVENT_request_complete_buffer(<br>
> + libcamera::Request::Private *req,<br>
> + libcamera::FrameBuffer * buf);<br>
> diff --git a/meson.build b/meson.build<br>
> index 29d8542d..434540e1 100644<br>
> --- a/meson.build<br>
> +++ b/meson.build<br>
> @@ -125,7 +125,11 @@ libcamera_includes = include_directories('include')<br>
> py_modules = []<br>
> <br>
> # Libraries used by multiple components<br>
> -liblttng = cc.find_library('lttng-ust', required : get_option('tracing'))<br>
> +libperfetto = dependency('perfetto', required : get_option('tracing').enabled()<br>
> + and get_option('android').enabled()<br>
> + and get_option('android_platform') == 'cros')<br>
> +liblttng = cc.find_library('lttng-ust', required : get_option('tracing').enabled()<br>
> + and not libperfetto.found())<br>
> <br>
> # Pipeline handlers<br>
> #<br>
> @@ -176,6 +180,7 @@ py_mod.find_installation('python3', modules: py_modules)<br>
> summary({<br>
> 'Enabled pipelines': pipelines,<br>
> 'Enabled IPA modules': ipa_modules,<br>
> + 'Perfetto support': perfetto_enabled,<br>
> 'Tracing support': tracing_enabled,<br>
> 'Android support': android_enabled,<br>
> 'GStreamer support': gst_enabled,<br>
> diff --git a/src/android/cros/camera3_hal.cpp b/src/android/cros/camera3_hal.cpp<br>
> index fb863b5f..2fbd7f14 100644<br>
> --- a/src/android/cros/camera3_hal.cpp<br>
> +++ b/src/android/cros/camera3_hal.cpp<br>
> @@ -7,10 +7,15 @@<br>
> <br>
> #include <cros-camera/cros_camera_hal.h><br>
> <br>
> +#include "libcamera/internal/tracepoints.h"<br>
> #include "../camera_hal_manager.h"<br>
> <br>
> static void set_up([[maybe_unused]] cros::CameraMojoChannelManagerToken *token)<br>
> {<br>
> + perfetto::TracingInitArgs args;<br>
> + args.backends |= perfetto::kSystemBackend;<br>
> + perfetto::Tracing::Initialize(args);<br>
> + perfetto::TrackEvent::Register();<br>
> }<br>
> <br>
> static void tear_down()<br>
> diff --git a/src/android/cros/meson.build b/src/android/cros/meson.build<br>
> index 35995dd8..68f2bd9e 100644<br>
> --- a/src/android/cros/meson.build<br>
> +++ b/src/android/cros/meson.build<br>
> @@ -9,5 +9,6 @@ android_hal_sources += files([<br>
> ])<br>
> <br>
> android_deps += dependency('libcros_camera')<br>
> +android_deps += dependency('perfetto')<br>
> <br>
> android_cpp_args += ['-DOS_CHROMEOS']<br>
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build<br>
> index 26912ca1..39a55a17 100644<br>
> --- a/src/libcamera/meson.build<br>
> +++ b/src/libcamera/meson.build<br>
> @@ -71,11 +71,22 @@ if libgnutls.found()<br>
> config_h.set('HAVE_GNUTLS', 1)<br>
> endif<br>
> <br>
> -if liblttng.found()<br>
> +if libperfetto.found()<br>
> + perfetto_enabled = true<br>
> + tracing_enabled = false<br>
> + config_h.set('HAVE_PERFETTO', 1)<br>
> + libcamera_sources += [<br>
> + 'pipeline_perfetto.cpp',<br>
> + 'request_perfetto.cpp',<br>
> + 'tracepoints.cpp',<br>
> + ]<br>
> +elif liblttng.found()<br>
> + perfetto_enabled = false<br>
> tracing_enabled = true<br>
> config_h.set('HAVE_TRACING', 1)<br>
> libcamera_sources += files(['tracepoints.cpp'])<br>
> else<br>
> + perfetto_enabled = false<br>
> tracing_enabled = false<br>
> endif<br>
> <br>
> @@ -125,6 +136,7 @@ libcamera_deps = [<br>
> libdl,<br>
> libgnutls,<br>
> liblttng,<br>
> + libperfetto,<br>
> libudev,<br>
> ]<br>
> <br>
> diff --git a/src/libcamera/pipeline_perfetto.cpp b/src/libcamera/pipeline_perfetto.cpp<br>
> new file mode 100644<br>
> index 00000000..07b82ffd<br>
> --- /dev/null<br>
> +++ b/src/libcamera/pipeline_perfetto.cpp<br>
<br>
Is it not possible for the definitions to go in the .perfetto file? Or<br>
is there some other reason that makes it strongly discouraged?<br>
<br></blockquote><div><br></div><div>So when I put them into the .perfetto file, which will be put in the</div><div>generated tracepoints.h with `{{source}}`, even with the header</div><div>guard INCLUDE_LIBCAMERA_INTERNAL_TRACEPOINTS_TP_H,</div><div>it still won't compile:</div><div>```</div><div>ld.lld: error: duplicate symbol: LIBCAMERA_TRACE_EVENT_request(libcamera::Request*)<br>>>> defined at tracepoints.h:105 (include/libcamera/internal/tracepoints.h:105) <br>>>> src/libcamera/libcamera.so.0.0.2.p/pipeline_handler.cpp.o:(LIBCAMERA_TRACE_EVENT_request(libcamera::Request*))<br>>>> defined at tracepoints.h:105 (include/libcamera/internal/tracepoints.h:105) <br>>>> src/libcamera/libcamera.so.0.0.2.p/request.cpp.o:(.text._Z29LIBCAMERA_TRACE_EVENT_requestPN9libcamera7RequestE+0x0)<br></div><div>```</div><div><br></div><div>I wonder if it's because of the header guard with another condition:</div><div>`|| defined(TRACEPOINT_HEADER_MULTI_READ)`</div><div><br></div><div>WDYT?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
If it's required to be in a separate .cpp file, I'd at least want it<br>
under src/libcamera/perfetto/ or src/libcamera/tracepoints/ or something<br>
along those lines, and not in the top level src/libcamera/.<br>
<br></blockquote><div><br></div><div>Put them under `src/libcamera/perfetto` now.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
That or a code generator, so that we don't have to duplicate efforts of<br>
tracepoint definitions in the first place, or risk tracepoints not being<br>
defined for one but not the other.<br>
<br></blockquote><div><br></div><div>I'm not sure how to add a code generator that handles some complicated</div><div>macros (like LIBCAMERA_TRACE_EVENT_request_complete_buffer)</div><div>without great effort. Let me know if you have a great idea. Thanks!</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Thanks,<br>
<br>
Paul<br>
<br>
> @@ -0,0 +1,24 @@<br>
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> +/*<br>
> + * Copyright (C) 2022, Google Inc.<br>
> + *<br>
> + * pipeline.tp - Tracepoints for pipelines<br>
> + */<br>
> +<br>
> +#include "libcamera/internal/tracepoints.h"<br>
> +<br>
> +void LIBCAMERA_TRACE_EVENT_ipa_call_begin(const char *pipe, const char *func)<br>
> +{<br>
> + // TODO: Consider TRACE_EVENT_BEGIN<br>
> + TRACE_EVENT("libcamera", "ipa_call_begin",<br>
> + "pipeline_name", pipe,<br>
> + "function_name", func);<br>
> +}<br>
> +<br>
> +void LIBCAMERA_TRACE_EVENT_ipa_call_end(const char *pipe, const char *func)<br>
> +{<br>
> + // TODO: Consider TRACE_EVENT_END<br>
> + TRACE_EVENT("libcamera", "ipa_call_end",<br>
> + "pipeline_name", pipe,<br>
> + "function_name", func);<br>
> +}<br>
> diff --git a/src/libcamera/request_perfetto.cpp b/src/libcamera/request_perfetto.cpp<br>
> new file mode 100644<br>
> index 00000000..2cfff28e<br>
> --- /dev/null<br>
> +++ b/src/libcamera/request_perfetto.cpp<br>
> @@ -0,0 +1,73 @@<br>
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> +/*<br>
> + * Copyright (C) 2022, Google Inc.<br>
> + *<br>
> + * request.tp - Tracepoints for the request object<br>
> + */<br>
> +<br>
> +#include <libcamera/framebuffer.h><br>
> +<br>
> +#include "libcamera/internal/request.h"<br>
> +#include "libcamera/internal/tracepoints.h"<br>
> +<br>
> +void LIBCAMERA_TRACE_EVENT_request(libcamera::Request *req)<br>
> +{<br>
> + TRACE_EVENT("libcamera", "request",<br>
> + "request_ptr", reinterpret_cast<uintptr_t>(req),<br>
> + "cookie", req->cookie(),<br>
> + "status", req->status()); // TODO<br>
> +}<br>
> +<br>
> +void LIBCAMERA_TRACE_EVENT_request_construct(libcamera::Request *req)<br>
> +{<br>
> + TRACE_EVENT("libcamera", "request_construct",<br>
> + "request_ptr", reinterpret_cast<uintptr_t>(req));<br>
> +}<br>
> +<br>
> +void LIBCAMERA_TRACE_EVENT_request_destroy(libcamera::Request *req)<br>
> +{<br>
> + TRACE_EVENT("libcamera", "request_destroy",<br>
> + "request_ptr", reinterpret_cast<uintptr_t>(req));<br>
> +}<br>
> +<br>
> +void LIBCAMERA_TRACE_EVENT_request_reuse(libcamera::Request *req)<br>
> +{<br>
> + TRACE_EVENT("libcamera", "request_reuse",<br>
> + "request_ptr", reinterpret_cast<uintptr_t>(req));<br>
> +}<br>
> +<br>
> +void LIBCAMERA_TRACE_EVENT_request_queue(libcamera::Request *req)<br>
> +{<br>
> + TRACE_EVENT("libcamera", "request_queue",<br>
> + "request_ptr", reinterpret_cast<uintptr_t>(req));<br>
> +}<br>
> +<br>
> +void LIBCAMERA_TRACE_EVENT_request_device_queue(libcamera::Request *req)<br>
> +{<br>
> + TRACE_EVENT("libcamera", "request_device_queue",<br>
> + "request_ptr", reinterpret_cast<uintptr_t>(req));<br>
> +}<br>
> +<br>
> +void LIBCAMERA_TRACE_EVENT_request_complete(libcamera::Request::Private *req)<br>
> +{<br>
> + TRACE_EVENT("libcamera", "request_complete",<br>
> + "request_private_ptr", reinterpret_cast<uintptr_t>(req));<br>
> +}<br>
> +<br>
> +void LIBCAMERA_TRACE_EVENT_request_cancel(libcamera::Request::Private *req)<br>
> +{<br>
> + TRACE_EVENT("libcamera", "request_cancel",<br>
> + "request_private_ptr", reinterpret_cast<uintptr_t>(req));<br>
> +}<br>
> +<br>
> +void LIBCAMERA_TRACE_EVENT_request_complete_buffer(<br>
> + libcamera::Request::Private *req,<br>
> + libcamera::FrameBuffer *buf)<br>
> +{<br>
> + TRACE_EVENT("libcamera", "request_complete_buffer",<br>
> + "request_private_ptr", reinterpret_cast<uintptr_t>(req),<br>
> + "cookie", req->_o<libcamera::Request>()->cookie(),<br>
> + "status", req->_o<libcamera::Request>()->status(), // TODO<br>
> + "buffer_ptr", reinterpret_cast<uintptr_t>(buf),<br>
> + "buffer_status", buf->metadata().status); // TODO<br>
> +}<br>
> diff --git a/src/libcamera/tracepoints.cpp b/src/libcamera/tracepoints.cpp<br>
> index 0173b75a..a07ea531 100644<br>
> --- a/src/libcamera/tracepoints.cpp<br>
> +++ b/src/libcamera/tracepoints.cpp<br>
> @@ -4,7 +4,18 @@<br>
> *<br>
> * tracepoints.cpp - Tracepoints with lttng<br>
> */<br>
> +<br>
> +#if HAVE_PERFETTO<br>
> +<br>
> +#include "libcamera/internal/tracepoints.h"<br>
> +<br>
> +PERFETTO_TRACK_EVENT_STATIC_STORAGE();<br>
> +<br>
> +#else<br>
> +<br>
> #define TRACEPOINT_CREATE_PROBES<br>
> #define TRACEPOINT_DEFINE<br>
> <br>
> #include "libcamera/internal/tracepoints.h"<br>
> +<br>
> +#endif /* HAVE_PERFETTO */<br>
> -- <br>
> 2.37.1.359.gd136c6c3e2-goog<br>
> <br>
</blockquote></div></div>