<div dir="ltr"><div dir="ltr">Hi Paul,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Sep 10, 2024 at 12:30 AM 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>
<snip> <br>
<br>
> So in lttng we have a TRACEPOINT_EVENT_CLASS, which defines arguments<br>
> and fields and how to obtain the fields from the args. We can<br>
> instantiate tracepoints with TRACEPOINT_EVENT_INSTANCE, where we only<br>
> have to specify the args, and the fields are already defined and come<br>
> from the class. This reduces duplication in the tracepoint definitions.<br>
> <br>
> I'm wondering from what you've implemented here that perfetto doesn't<br>
> support this? I don't see anything in the perfetto docs that suggest<br>
> that this is possible either. Although we can mitigate this issue by<br>
> generating the perfetto tracepoints :) (this is another reason why I<br>
> think we should use the lttng tracepoints definition as the main one and<br>
> generate the perfetto ones from those)<br>
> <br>
> <br>
> <br>
> I've discussed with ChromeOS Perfetto TL chinglinyu@, and we found that<br>
> there are limited features in lttng [1], and if we use the lttng tracepoints<br>
> definition as the source of truth, it's hard/weird to implement some Perfetto<br>
> concepts, like nested slices (lifetime of trace events) and flows [2].<br>
<br>
Ah ok, I see the problem.<br>
<br>
> <br>
> It's not impossible, as you mentioned above, that we could detect the prefix<br>
> and suffix of each libcamera/lttng macro. However, it's a weird code parser<br>
> and generator design.<br>
> <br>
> [1]: <a href="https://lttng.org/man/3/lttng-ust/v2.8/" rel="noreferrer" target="_blank">https://lttng.org/man/3/lttng-ust/v2.8/</a><br>
> [2]: <a href="https://perfetto.dev/docs/instrumentation/track-events" rel="noreferrer" target="_blank">https://perfetto.dev/docs/instrumentation/track-events</a><br>
> <br>
> <br>
<br>
<snip> <br>
<br>
> <br>
> <br>
> I agree that in the current functionalities, it's not that hard to implement<br>
> a homemade C/python parser, and generate Perfetto code with jinja2.<br>
> However, we not only "limit" the functionalities of Perfetto with lttng's<br>
> definition, but also add another layer to be maintained.<br>
> It's true that having duplication of libcamera tracepoint definitions<br>
> requires more maintenance work when updating the libcamera<br>
> tracepoints, while the effort to maintain the parser and code generation<br>
> is also non-negligible.<br>
> <br>
> I also think that creating a new language from which we generate<br>
> tracepoints for both is an overkill though. It seems that we don't have<br>
> a perfect way to support both lttng and Perfetto...<br>
> <br>
> BTW, I tried lex and yacc to parse lttng macros, and tbh it's very painful<br>
> that it's very hard to use lex to recognize C/C++ type as a token... So<br>
> yeah if we end up deciding to take this route, a homemade parser from<br>
> scratch makes more sense to me.<br>
> <br>
> So let me list the approaches with pros and cons:<br>
> 1. Generate Perfetto code from lttng macros.<br>
> Pros: Easy to update libcamera tracepoint macros.<br>
> Cons: Add maintenance work with the additional parser and code <br>
> generator; hard to implement Perfetto features that lttng doesn't have.<br>
<br>
This makes me wonder, if Perfetto is a superset of the features of<br>
lttng, why not go the other way; use Perfetto tracepoints as the main<br>
authority and then generate lttng tracepoints from those.<br>
<br>
What are your thoughts?<br></blockquote><div><br></div><div>I don't think Perfetto MACROs is a superset of lttng MACROs. If we go</div><div>with this approach, there are some features dropped (like integer_hex</div><div>and enum, and the concept of TP_ARGS), and the code would be messy.</div><div>Take request_complete_buffer as the example:</div><div><br></div><div><font face="arial, sans-serif" color="#000000">```</font></div><div><font face="arial, sans-serif" color="#000000"># Perfetto</font></div><div><font face="arial, sans-serif" color="#000000">TRACE_EVENT("libcamera", "request_complete_buffer",</font></div><div><span style="color:rgb(0,0,0);font-family:arial,sans-serif"> "request_private_ptr", reinterpret_cast<uintptr_t>(req),</span></div><div><span style="color:rgb(0,0,0);font-family:arial,sans-serif"> "cookie", req->_o<libcamera::Request>()->cookie(),</span></div><div><span style="color:rgb(0,0,0);font-family:arial,sans-serif"> "status", req->_o<libcamera::Request>()->status(),</span></div><div><span style="color:rgb(0,0,0);font-family:arial,sans-serif"> "buffer_ptr", reinterpret_cast<uintptr_t>(buf),</span></div><div><span style="color:rgb(0,0,0);font-family:arial,sans-serif"> "buffer_status", buf->metadata().status);</span></div><div><font face="arial, sans-serif" color="#000000">```</font></div><div><font face="arial, sans-serif" color="#000000"><br></font></div><div><font face="arial, sans-serif" color="#000000">It'd be how perfetto implements the macro. Originally lttng has only two</font></div><div><font face="arial, sans-serif" color="#000000">inputs: `libcamera::Request::internal *req` and</font></div><div><font face="arial, sans-serif" color="#000000">`libcamera::FrameBuffer *buf`. Unless we're going to do complicated</font></div><div><font face="arial, sans-serif" color="#000000">type diagnosis and simplification, we might end up with having five</font></div><div><font face="arial, sans-serif" color="#000000">inputs as well. Also, I don't know how to indicate the usage of </font></div><div><font color="#000000" face="arial, sans-serif">ctf_integer_hex (for `req` and `buf`) and ctf_enum (for status).</font></div><div><br></div><div>I'd still prefer the 3rd or 4th approach.</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>
> 2. Creating a new language from which we generate both Perfetto<br>
> and lttng macros.<br>
> Pros: Easy to update libcamera tracepoint macros.<br>
> Cons: Probably an overkill; hard to cover all features from both <br>
> Perfetto and lttng.<br>
> <br>
> 3. As this patch implements: have duplicated libcamera tracepoint <br>
> definitions.<br>
> Pros: Easy to add libcamera tracepoint macros to cover all features<br>
> from Perfetto & lttng.<br>
> Cons: Developer needs to update both sides when the definition of<br>
> libcamera tracepoints changes; Unused macro definitions might be<br>
> adopted in both sides, if a feature is only available in one trace library.<br>
> <br>
> 4. Add another set of libcamera tracepoints for Perfetto, or just use <br>
> Perfetto macros directly in libcamera's source code.<br>
> Pros: Two isolated trace stacks that don't influence each other. All<br>
> features can be implemented easily.<br>
> Cons: Duplicated trace code in source code that might lead to <br>
> confusion; developers also need to update both macros when a<br>
> new trace is needed.<br>
> <br>
> I think approach 3rd and 4th are easier to maintain in the long<br>
> term. WDYT?<br>
> <br>
> Also, I'd like to know if you think Perfetto will be used in other linux<br>
> distributions than ChromeOS or Android.<br>
<br>
If Perfetto gets added to the package repositories in linux<br>
distributions then I think it would be used :)<br>
<br>
Not being able to sudo apt install perfetto is a pretty big blocker.<br>
<br></blockquote><div><br></div><div>Yeah I understand, while Perfetto is self-contained and simple to build</div><div>in linux distros. May we consider testing it on the gitlab CI, after we</div><div>somehow add the support?</div><div><br></div><div>BR,</div><div>Harvey</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>
<br>
Paul<br>
<br>
> <br>
> Chinglin, please help amend anything I missed as well :)<br>
> <br>
> BR,<br>
> Harvey<br>
> <br>
> <br>
> [2] utils/ipc/meson.build<br>
> [3] include/libcamera/ipa/meson.build<br>
> [4] src/libcamera/ipa/meson.build<br>
> <br>
> Would you be able to do this?<br>
> <br>
> <br>
> Paul<br>
> <br>
> > diff --git a/src/libcamera/tracepoints.cpp b/src/libcamera/<br>
> 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.39.0.314.g84b9a713c41-goog<br>
> ><br>
> <br>
</blockquote></div></div>