| Message ID | 20201029101629.61798-3-paul.elder@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, Thank you for the patch. On Thu, Oct 29, 2020 at 07:16:25PM +0900, Paul Elder wrote: > Add and use tracepoints in Request. Requests are core to libcamera > operation, thus detecting delays in their processing is important, and > serves as a good usage example of tracepoints. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > I wasn't sure what the best way to handle the FrameBuffer enum in the tp > file... should it go in a separate file? Will we ever need to use the > FrameBuffer status enum in other tracepoints? If we do, it can't be > defined multiple times, so it'll have to go in its own tp file, and in > the meson tp file list it will have to go first. I guess that's the > drawback of concating all the tp files together... I wonder if the enums shouldn't indeed be placed in separate .tp files that would then be #include'd where needed, with a header guard. That way we'll have a guarantee they will be defined on first use in the tracepoints.h file. > Changes in v3: > - expand the changelog > - add enum tracepoint values so that request status and buffer status > are strings instead of ints in the trace > - remove cancelled from all request tracepoints, except complete_buffer > - expand the complete_buffer tracepoint to include information on the > buffer > - display the address of the request in all tracepoints > - remove tracepoints for add_buffer and find_buffer > > Changes in v2: > - remove tracepoints from uvcvideo > - remove comment in changelog that this is only used for demonstration > - use Request pointers instead of feeding the fields manually to the > tracepoint > --- > .../internal/tracepoints/meson.build | 1 + > .../libcamera/internal/tracepoints/request.tp | 90 +++++++++++++++++++ > src/libcamera/request.cpp | 11 +++ > 3 files changed, 102 insertions(+) > create mode 100644 include/libcamera/internal/tracepoints/request.tp > > diff --git a/include/libcamera/internal/tracepoints/meson.build b/include/libcamera/internal/tracepoints/meson.build > index 2dd6733e..8410c205 100644 > --- a/include/libcamera/internal/tracepoints/meson.build > +++ b/include/libcamera/internal/tracepoints/meson.build > @@ -1,4 +1,5 @@ > # SPDX-License-Identifier: CC0-1.0 > > tracepoint_files = files([ > + 'request.tp', > ]) > diff --git a/include/libcamera/internal/tracepoints/request.tp b/include/libcamera/internal/tracepoints/request.tp > new file mode 100644 > index 00000000..92308dcd > --- /dev/null > +++ b/include/libcamera/internal/tracepoints/request.tp > @@ -0,0 +1,90 @@ > +#include <libcamera/buffer.h> > +#include <libcamera/request.h> > + > +TRACEPOINT_ENUM( > + libcamera, > + request_status, > + TP_ENUM_VALUES( > + ctf_enum_value("RequestPending", 0) > + ctf_enum_value("RequestComplete", 1) > + ctf_enum_value("RequestCancelled", 2) > + ) > +) > + > +TRACEPOINT_ENUM( > + libcamera, > + buffer_status, > + TP_ENUM_VALUES( > + ctf_enum_value("FrameSuccess", 0) > + ctf_enum_value("FrameError", 1) > + ctf_enum_value("FrameCancelled", 2) > + ) > +) > + > +TRACEPOINT_EVENT_CLASS( > + libcamera, > + request, > + TP_ARGS( > + libcamera::Request *, req > + ), > + TP_FIELDS( > + ctf_integer_hex(uintptr_t, request, reinterpret_cast<uintptr_t>(req)) > + ctf_integer(uint64_t, cookie, req->cookie()) > + ctf_enum(libcamera, request_status, uint32_t, status, req->status()) > + ) > +) > + > +TRACEPOINT_EVENT_INSTANCE( > + libcamera, > + request, > + request_construct, > + TP_ARGS( > + libcamera::Request *, req > + ) > +) > + > +TRACEPOINT_EVENT_INSTANCE( > + libcamera, > + request, > + request_destroy, > + TP_ARGS( > + libcamera::Request *, req > + ) > +) > + > +TRACEPOINT_EVENT_INSTANCE( > + libcamera, > + request, > + request_reuse, > + TP_ARGS( > + libcamera::Request *, req > + ) > +) > + > +TRACEPOINT_EVENT_INSTANCE( > + libcamera, > + request, > + request_complete, > + TP_ARGS( > + libcamera::Request *, req > + ) > +) > + > + > +TRACEPOINT_EVENT( > + libcamera, > + request_complete_buffer, > + TP_ARGS( > + libcamera::Request *, req, > + int, cancelled, > + libcamera::FrameBuffer *, buf > + ), > + TP_FIELDS( > + ctf_integer_hex(uintptr_t, request, reinterpret_cast<uintptr_t>(req)) > + ctf_integer(uint64_t, cookie, req->cookie()) > + ctf_integer(int, status, req->status()) > + ctf_integer(int, cancelled, cancelled) I think you could drop cancelled, as it's a result of the buffer status, which you also capture. > + ctf_integer_hex(uintptr_t, buffer, reinterpret_cast<uintptr_t>(buf)) > + ctf_enum(libcamera, buffer_status, uint32_t, buf_status, buf->metadata().status) > + ) > +) > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > index ae8b1660..d1b43697 100644 > --- a/src/libcamera/request.cpp > +++ b/src/libcamera/request.cpp > @@ -16,6 +16,7 @@ > > #include "libcamera/internal/camera_controls.h" > #include "libcamera/internal/log.h" > +#include "libcamera/internal/tracepoints.h" > > /** > * \file request.h > @@ -85,10 +86,14 @@ Request::Request(Camera *camera, uint64_t cookie) > * \todo: Add a validator for metadata controls. > */ > metadata_ = new ControlList(controls::controls); > + > + LIBCAMERA_TRACEPOINT(request_construct, this); > } > > Request::~Request() > { > + LIBCAMERA_TRACEPOINT(request_destroy, this); > + > delete metadata_; > delete controls_; > delete validator_; > @@ -106,6 +111,8 @@ Request::~Request() > */ > void Request::reuse(ReuseFlag flags) > { > + LIBCAMERA_TRACEPOINT(request_reuse, this); > + > pending_.clear(); > if (flags & ReuseBuffers) { > for (auto pair : bufferMap_) { > @@ -259,6 +266,8 @@ void Request::complete() > LOG(Request, Debug) > << "Request has completed - cookie: " << cookie_ > << (cancelled_ ? " [Cancelled]" : ""); > + > + LIBCAMERA_TRACEPOINT(request_complete, this); > } > > /** > @@ -276,6 +285,8 @@ void Request::complete() > */ > bool Request::completeBuffer(FrameBuffer *buffer) > { > + LIBCAMERA_TRACEPOINT(request_complete_buffer, this, cancelled_, buffer); > + > int ret = pending_.erase(buffer); > ASSERT(ret == 1); > It looks really nice :-)
diff --git a/include/libcamera/internal/tracepoints/meson.build b/include/libcamera/internal/tracepoints/meson.build index 2dd6733e..8410c205 100644 --- a/include/libcamera/internal/tracepoints/meson.build +++ b/include/libcamera/internal/tracepoints/meson.build @@ -1,4 +1,5 @@ # SPDX-License-Identifier: CC0-1.0 tracepoint_files = files([ + 'request.tp', ]) diff --git a/include/libcamera/internal/tracepoints/request.tp b/include/libcamera/internal/tracepoints/request.tp new file mode 100644 index 00000000..92308dcd --- /dev/null +++ b/include/libcamera/internal/tracepoints/request.tp @@ -0,0 +1,90 @@ +#include <libcamera/buffer.h> +#include <libcamera/request.h> + +TRACEPOINT_ENUM( + libcamera, + request_status, + TP_ENUM_VALUES( + ctf_enum_value("RequestPending", 0) + ctf_enum_value("RequestComplete", 1) + ctf_enum_value("RequestCancelled", 2) + ) +) + +TRACEPOINT_ENUM( + libcamera, + buffer_status, + TP_ENUM_VALUES( + ctf_enum_value("FrameSuccess", 0) + ctf_enum_value("FrameError", 1) + ctf_enum_value("FrameCancelled", 2) + ) +) + +TRACEPOINT_EVENT_CLASS( + libcamera, + request, + TP_ARGS( + libcamera::Request *, req + ), + TP_FIELDS( + ctf_integer_hex(uintptr_t, request, reinterpret_cast<uintptr_t>(req)) + ctf_integer(uint64_t, cookie, req->cookie()) + ctf_enum(libcamera, request_status, uint32_t, status, req->status()) + ) +) + +TRACEPOINT_EVENT_INSTANCE( + libcamera, + request, + request_construct, + TP_ARGS( + libcamera::Request *, req + ) +) + +TRACEPOINT_EVENT_INSTANCE( + libcamera, + request, + request_destroy, + TP_ARGS( + libcamera::Request *, req + ) +) + +TRACEPOINT_EVENT_INSTANCE( + libcamera, + request, + request_reuse, + TP_ARGS( + libcamera::Request *, req + ) +) + +TRACEPOINT_EVENT_INSTANCE( + libcamera, + request, + request_complete, + TP_ARGS( + libcamera::Request *, req + ) +) + + +TRACEPOINT_EVENT( + libcamera, + request_complete_buffer, + TP_ARGS( + libcamera::Request *, req, + int, cancelled, + libcamera::FrameBuffer *, buf + ), + TP_FIELDS( + ctf_integer_hex(uintptr_t, request, reinterpret_cast<uintptr_t>(req)) + ctf_integer(uint64_t, cookie, req->cookie()) + ctf_integer(int, status, req->status()) + ctf_integer(int, cancelled, cancelled) + ctf_integer_hex(uintptr_t, buffer, reinterpret_cast<uintptr_t>(buf)) + ctf_enum(libcamera, buffer_status, uint32_t, buf_status, buf->metadata().status) + ) +) diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp index ae8b1660..d1b43697 100644 --- a/src/libcamera/request.cpp +++ b/src/libcamera/request.cpp @@ -16,6 +16,7 @@ #include "libcamera/internal/camera_controls.h" #include "libcamera/internal/log.h" +#include "libcamera/internal/tracepoints.h" /** * \file request.h @@ -85,10 +86,14 @@ Request::Request(Camera *camera, uint64_t cookie) * \todo: Add a validator for metadata controls. */ metadata_ = new ControlList(controls::controls); + + LIBCAMERA_TRACEPOINT(request_construct, this); } Request::~Request() { + LIBCAMERA_TRACEPOINT(request_destroy, this); + delete metadata_; delete controls_; delete validator_; @@ -106,6 +111,8 @@ Request::~Request() */ void Request::reuse(ReuseFlag flags) { + LIBCAMERA_TRACEPOINT(request_reuse, this); + pending_.clear(); if (flags & ReuseBuffers) { for (auto pair : bufferMap_) { @@ -259,6 +266,8 @@ void Request::complete() LOG(Request, Debug) << "Request has completed - cookie: " << cookie_ << (cancelled_ ? " [Cancelled]" : ""); + + LIBCAMERA_TRACEPOINT(request_complete, this); } /** @@ -276,6 +285,8 @@ void Request::complete() */ bool Request::completeBuffer(FrameBuffer *buffer) { + LIBCAMERA_TRACEPOINT(request_complete_buffer, this, cancelled_, buffer); + int ret = pending_.erase(buffer); ASSERT(ret == 1);