[libcamera-devel] [PATCH v4 2/5] libcamera: request: Add tracepoints

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Fri, Oct 30, 2020 at 05:57:53PM +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>
> 
> ---
> Changes in v4:
> - move enum tp definitions to "header" tp files
>   - add them to meson accordingly
> - remove cancelled field from request_complete_buffer tracepoint
> 
> 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/buffer_enums.tp      |  9 +++
>  .../internal/tracepoints/meson.build          |  5 ++
>  .../libcamera/internal/tracepoints/request.tp | 68 +++++++++++++++++++
>  .../internal/tracepoints/request_enums.tp     |  9 +++
>  src/libcamera/request.cpp                     | 11 +++
>  5 files changed, 102 insertions(+)
>  create mode 100644 include/libcamera/internal/tracepoints/buffer_enums.tp
>  create mode 100644 include/libcamera/internal/tracepoints/request.tp
>  create mode 100644 include/libcamera/internal/tracepoints/request_enums.tp
> 
> diff --git a/include/libcamera/internal/tracepoints/buffer_enums.tp b/include/libcamera/internal/tracepoints/buffer_enums.tp
> new file mode 100644
> index 00000000..fdbea69a
> --- /dev/null
> +++ b/include/libcamera/internal/tracepoints/buffer_enums.tp
> @@ -0,0 +1,9 @@
> +TRACEPOINT_ENUM(
> +	libcamera,
> +	buffer_status,
> +	TP_ENUM_VALUES(
> +		ctf_enum_value("FrameSuccess", 0)
> +		ctf_enum_value("FrameError", 1)
> +		ctf_enum_value("FrameCancelled", 2)
> +	)
> +)
> diff --git a/include/libcamera/internal/tracepoints/meson.build b/include/libcamera/internal/tracepoints/meson.build
> index 2dd6733e..fee0758f 100644
> --- a/include/libcamera/internal/tracepoints/meson.build
> +++ b/include/libcamera/internal/tracepoints/meson.build
> @@ -1,4 +1,9 @@
>  # SPDX-License-Identifier: CC0-1.0
>  
> +# enum files must go first
>  tracepoint_files = files([
> +    'buffer_enums.tp',
> +    'request_enums.tp',
> +
> +    'request.tp',
>  ])

How about

# enum files must go first
tracepoint_files = files([
    'buffer_enums.tp',
    'request_enums.tp',
])

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..dd28a714
> --- /dev/null
> +++ b/include/libcamera/internal/tracepoints/request.tp
> @@ -0,0 +1,68 @@
> +#include <libcamera/buffer.h>
> +#include <libcamera/request.h>
> +
> +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,
> +		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_hex(uintptr_t, buffer, reinterpret_cast<uintptr_t>(buf))
> +		ctf_enum(libcamera, buffer_status, uint32_t, buf_status, buf->metadata().status)
> +	)
> +)
> diff --git a/include/libcamera/internal/tracepoints/request_enums.tp b/include/libcamera/internal/tracepoints/request_enums.tp
> new file mode 100644
> index 00000000..371f6544
> --- /dev/null
> +++ b/include/libcamera/internal/tracepoints/request_enums.tp
> @@ -0,0 +1,9 @@
> +TRACEPOINT_ENUM(
> +	libcamera,
> +	request_status,
> +	TP_ENUM_VALUES(
> +		ctf_enum_value("RequestPending", 0)
> +		ctf_enum_value("RequestComplete", 1)
> +		ctf_enum_value("RequestCancelled", 2)
> +	)
> +)
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index ae8b1660..a68684ef 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, buffer);
> +
>  	int ret = pending_.erase(buffer);
>  	ASSERT(ret == 1);
>  

-- 
Regards,

Laurent Pinchart