[libcamera-devel] [PATCH v3 2/6] libcamera: request: Add tracepoints

[Laurent Pinchart]

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 at ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at 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 :-)

-- 
Regards,

Laurent Pinchart