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

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Wed, Oct 28, 2020 at 07:31:47PM +0900, Paul Elder wrote:
> Add and use tracepoints in Request.

A rationale would be useful here. It can probably be as simple as
explaining that requests are core to libcamera operation, that detecting
delays in their processing is important, and that this thus serves as a
good usage example of tracepoints.

> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> 
> ---
> 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 | 85 +++++++++++++++++++
>  src/libcamera/request.cpp                     | 15 ++++
>  3 files changed, 101 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..481a3670
> --- /dev/null
> +++ b/include/libcamera/internal/tracepoints/request.tp
> @@ -0,0 +1,85 @@
> +#include <libcamera/request.h>
> +
> +TRACEPOINT_EVENT_CLASS(
> +	libcamera,
> +	request,
> +	TP_ARGS(
> +		libcamera::Request *, req,
> +		int, cancelled

I suppose the reason why you can't use req->cancelled_ is because it's
private ? I wonder if it's a useful field, given that it's only set in
Request::completeBuffer(), and after that it influences status_ in
Request::complete(), and status_ captured through req->status() below.
I'd drop it, and instead create another event for
Request::completeBuffer() that takes the buffer status as an additional
argument.

> +	),
> +	TP_FIELDS(
> +		ctf_integer(uint64_t, cookie, req->cookie())
> +		ctf_integer(int, status, req->status())

Oh, that's interesting.

This means that only the cookie and status (and cancelled below) are
recorded by the trace, right ?

> +		ctf_integer(int, cancelled, cancelled)
> +	)
> +)
> +
> +TRACEPOINT_EVENT_INSTANCE(
> +	libcamera,
> +	request,
> +	request_construct,
> +	TP_ARGS(
> +		libcamera::Request *, req,
> +		int, cancelled
> +	)
> +)
> +
> +TRACEPOINT_EVENT_INSTANCE(
> +	libcamera,
> +	request,
> +	request_deconstruct,

I'd name this request_destroy as C++ has destructors.

> +	TP_ARGS(
> +		libcamera::Request *, req,
> +		int, cancelled
> +	)
> +)
> +
> +TRACEPOINT_EVENT_INSTANCE(
> +	libcamera,
> +	request,
> +	request_reuse,
> +	TP_ARGS(
> +		libcamera::Request *, req,
> +		int, cancelled
> +	)
> +)
> +
> +TRACEPOINT_EVENT_INSTANCE(
> +	libcamera,
> +	request,
> +	request_add_buffer,
> +	TP_ARGS(
> +		libcamera::Request *, req,
> +		int, cancelled
> +	)
> +)
> +
> +TRACEPOINT_EVENT_INSTANCE(
> +	libcamera,
> +	request,
> +	request_find_buffer,
> +	TP_ARGS(
> +		const libcamera::Request *, req,
> +		int, cancelled
> +	)
> +)

I'd drop those two for now, I don't think they're really useful to trace
request timings.

> +
> +TRACEPOINT_EVENT_INSTANCE(
> +	libcamera,
> +	request,
> +	request_complete,
> +	TP_ARGS(
> +		libcamera::Request *, req,
> +		int, cancelled
> +	)
> +)
> +
> +TRACEPOINT_EVENT_INSTANCE(
> +	libcamera,
> +	request,
> +	request_complete_buffer,
> +	TP_ARGS(
> +		libcamera::Request *, req,
> +		int, cancelled
> +	)
> +)

This one I think is useful, but I wonder if we shouldn't add information
about the buffer being completed to make real use of it. The buffer
status immediately come to mind, identifying the buffer would also be
useful I think. The second point can be addressed later.

> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index ae8b1660..bec2cab5 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, cancelled_);
>  }
>  
>  Request::~Request()
>  {
> +	LIBCAMERA_TRACEPOINT(request_deconstruct, this, cancelled_);
> +
>  	delete metadata_;
>  	delete controls_;
>  	delete validator_;
> @@ -106,6 +111,8 @@ Request::~Request()
>   */
>  void Request::reuse(ReuseFlag flags)
>  {
> +	LIBCAMERA_TRACEPOINT(request_reuse, this, cancelled_);
> +
>  	pending_.clear();
>  	if (flags & ReuseBuffers) {
>  		for (auto pair : bufferMap_) {
> @@ -167,6 +174,8 @@ void Request::reuse(ReuseFlag flags)
>   */
>  int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)
>  {
> +	LIBCAMERA_TRACEPOINT(request_add_buffer, this, cancelled_);
> +
>  	if (!stream) {
>  		LOG(Request, Error) << "Invalid stream reference";
>  		return -EINVAL;
> @@ -202,6 +211,8 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)
>   */
>  FrameBuffer *Request::findBuffer(const Stream *stream) const
>  {
> +	LIBCAMERA_TRACEPOINT(request_find_buffer, this, cancelled_);
> +
>  	const auto it = bufferMap_.find(stream);
>  	if (it == bufferMap_.end())
>  		return nullptr;
> @@ -253,6 +264,8 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const
>   */
>  void Request::complete()
>  {
> +	LIBCAMERA_TRACEPOINT(request_complete, this, cancelled_);
> +

I'd move this after status_ to capture the correct status.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

>  	ASSERT(!hasPendingBuffers());
>  	status_ = cancelled_ ? RequestCancelled : RequestComplete;
>  
> @@ -276,6 +289,8 @@ void Request::complete()
>   */
>  bool Request::completeBuffer(FrameBuffer *buffer)
>  {
> +	LIBCAMERA_TRACEPOINT(request_complete_buffer, this, cancelled_);
> +
>  	int ret = pending_.erase(buffer);
>  	ASSERT(ret == 1);
>  

-- 
Regards,

Laurent Pinchart