[libcamera-devel,RFC,2/2,DO,NOT,MERGE] libcamera: uvcvideo, request: Add tracepoints

10093 diff mbox series

Add and use tracepoints in the uvcvideo pipeline handler and Request.
This is only meant to show how one would create and use tracepoints.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
Setting log level is also available. I haven't tested it yet, but in
theory is should Just Work (TM). It looks like you can set log levels on
the event definitions, and not when you emit the events.
---
 .../internal/tracepoints/meson.build          |  2 +
 .../libcamera/internal/tracepoints/request.tp | 91 +++++++++++++++++++
 .../internal/tracepoints/uvcvideo.tp          | 13 +++
 src/libcamera/pipeline/uvcvideo/uvcvideo.cpp  |  3 +
 src/libcamera/request.cpp                     | 16 ++++
 5 files changed, 125 insertions(+)
 create mode 100644 include/libcamera/internal/tracepoints/request.tp
 create mode 100644 include/libcamera/internal/tracepoints/uvcvideo.tp

Hi Paul,

Thank you for the patch.

On Mon, Oct 19, 2020 at 07:25:29PM +0900, Paul Elder wrote:
> Add and use tracepoints in the uvcvideo pipeline handler and Request.
> This is only meant to show how one would create and use tracepoints.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> Setting log level is also available. I haven't tested it yet, but in
> theory is should Just Work (TM). It looks like you can set log levels on
> the event definitions, and not when you emit the events.
> ---
>  .../internal/tracepoints/meson.build          |  2 +
>  .../libcamera/internal/tracepoints/request.tp | 91 +++++++++++++++++++
>  .../internal/tracepoints/uvcvideo.tp          | 13 +++
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp  |  3 +
>  src/libcamera/request.cpp                     | 16 ++++

I think you could already split this in two, one patch for the libcamera
core and one for the pipeline handler, as I believe the series will
graduate to non-RFC status fairly soon.

>  5 files changed, 125 insertions(+)
>  create mode 100644 include/libcamera/internal/tracepoints/request.tp
>  create mode 100644 include/libcamera/internal/tracepoints/uvcvideo.tp
> 
> diff --git a/include/libcamera/internal/tracepoints/meson.build b/include/libcamera/internal/tracepoints/meson.build
> index 2dd6733e..3ec53fbc 100644
> --- a/include/libcamera/internal/tracepoints/meson.build
> +++ b/include/libcamera/internal/tracepoints/meson.build
> @@ -1,4 +1,6 @@
>  # SPDX-License-Identifier: CC0-1.0
>  
>  tracepoint_files = files([
> +    'request.tp',
> +    'uvcvideo.tp',
>  ])
> diff --git a/include/libcamera/internal/tracepoints/request.tp b/include/libcamera/internal/tracepoints/request.tp
> new file mode 100644
> index 00000000..c64bbd43
> --- /dev/null
> +++ b/include/libcamera/internal/tracepoints/request.tp
> @@ -0,0 +1,91 @@
> +TRACEPOINT_EVENT_CLASS(
> +	libcamera,
> +	request,
> +	TP_ARGS(
> +		uint64_t, cookie,
> +		int, status,
> +		int, cancelled
> +	),
> +	TP_FIELDS(
> +		ctf_integer(uint64_t, cookie, cookie)
> +		ctf_integer(int, status, status)
> +		ctf_integer(int, cancelled, cancelled)
> +	)
> +)
> +
> +TRACEPOINT_EVENT_INSTANCE(
> +	libcamera,
> +	request,
> +	request_construct,
> +	TP_ARGS(
> +		uint64_t, cookie,
> +		int, status,
> +		int, cancelled
> +	)
> +)

If I understand this correctly, TRACEPOINT_EVENT_CLASS will generate one
serialization function, and TRACEPOINT_EVENT_INSTANCE, whose TP_ARGS
must match the class', then use it.

I think we need to think a bit about the naming scheme here. This code
initially made me believe the event class was a good match for the C++
class, but it's really about classes of events that take the same
arguments, not C++ classes. If we needed to add other arguments to some
of the tracepoints for Request, we would need different event classes,
for the C++ class. I thus wonder if the tracepoint class name should
contain more information.

Alternatively, is the provider name required to be the same for all our
tracepoints, or could it include the C++ class name (e.g.
libcamera_request) ? What impact would this have ?

I was also wondering if we could use names that would make the class
name more apparent (for instance Request::Request instead of
request_construct), but all names need to be valid C symbol names. We
could encode them using the C++ name encoding scheme, and decode them
when consuming the traces (which would require pre-processing the .tp
file with a custom tool). That's a possible improvement for (much) later
:-)

> +
> +TRACEPOINT_EVENT_INSTANCE(
> +	libcamera,
> +	request,
> +	request_deconstruct,
> +	TP_ARGS(
> +		uint64_t, cookie,
> +		int, status,
> +		int, cancelled
> +	)
> +)
> +
> +TRACEPOINT_EVENT_INSTANCE(
> +	libcamera,
> +	request,
> +	request_reuse,
> +	TP_ARGS(
> +		uint64_t, cookie,
> +		int, status,
> +		int, cancelled
> +	)
> +)
> +
> +TRACEPOINT_EVENT_INSTANCE(
> +	libcamera,
> +	request,
> +	request_add_buffer,
> +	TP_ARGS(
> +		uint64_t, cookie,
> +		int, status,
> +		int, cancelled
> +	)
> +)
> +
> +TRACEPOINT_EVENT_INSTANCE(
> +	libcamera,
> +	request,
> +	request_find_buffer,
> +	TP_ARGS(
> +		uint64_t, cookie,
> +		int, status,
> +		int, cancelle
> +	)
> +)
> +
> +TRACEPOINT_EVENT_INSTANCE(
> +	libcamera,
> +	request,
> +	request_complete,
> +	TP_ARGS(
> +		uint64_t, cookie,
> +		int, status,
> +		int, cancelled
> +	)
> +)
> +
> +TRACEPOINT_EVENT_INSTANCE(
> +	libcamera,
> +	request,
> +	request_complete_buffer,
> +	TP_ARGS(
> +		uint64_t, cookie,
> +		int, status,
> +		int, cancelled
> +	)
> +)
> diff --git a/include/libcamera/internal/tracepoints/uvcvideo.tp b/include/libcamera/internal/tracepoints/uvcvideo.tp
> new file mode 100644
> index 00000000..61592ce5
> --- /dev/null
> +++ b/include/libcamera/internal/tracepoints/uvcvideo.tp
> @@ -0,0 +1,13 @@
> +TRACEPOINT_EVENT_CLASS(
> +	libcamera,
> +	uvcvideo,
> +	TP_ARGS(),
> +	TP_FIELDS()
> +)
> +
> +TRACEPOINT_EVENT_INSTANCE(
> +	libcamera,
> +	uvcvideo,
> +	uvcvideo_contruct,
> +	TP_ARGS()
> +)

With a single instance, you can use TRACEPOINT_EVENT().

> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index 8ec0dac1..bc01a78c 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -27,6 +27,8 @@
>  #include "libcamera/internal/v4l2_controls.h"
>  #include "libcamera/internal/v4l2_videodevice.h"
>  
> +#include "libcamera/internal/tracepoints.h"

No need to keep this header separate.

> +
>  namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(UVC)
> @@ -171,6 +173,7 @@ CameraConfiguration::Status UVCCameraConfiguration::validate()
>  PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)
>  	: PipelineHandler(manager)
>  {
> +	LIBCAMERA_TRACEPOINT(uvcvideo_contruct);
>  }
>  
>  CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera,
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index ae8b1660..74c1a7b0 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -17,6 +17,8 @@
>  #include "libcamera/internal/camera_controls.h"
>  #include "libcamera/internal/log.h"
>  
> +#include "libcamera/internal/tracepoints.h"

Does generating a single header with all tracepoints risk increasing
compilation time ? I wonder if one libcamera/tracepoints/<name>.h for
each <name>.tp would be better.

> +
>  /**
>   * \file request.h
>   * \brief Describes a frame capture request to be processed by a camera
> @@ -85,10 +87,14 @@ Request::Request(Camera *camera, uint64_t cookie)
>  	 * \todo: Add a validator for metadata controls.
>  	 */
>  	metadata_ = new ControlList(controls::controls);
> +
> +	LIBCAMERA_TRACEPOINT(request_construct, cookie_, status_, cancelled_);

The Request class is, I think, a good candidate for tracepoints. For the
next example, I think you could drop some of the tracepoints (I'm about
findBuffer for instance), and possibly tailor the argument to the
tracepoint event if other information would be useful to capture.

Would it also make sense to capture a pointer to the request, to match
the different events ? That's probably a question that applies to all,
or most, of the events we will have.

>  }
>  
>  Request::~Request()
>  {
> +	LIBCAMERA_TRACEPOINT(request_deconstruct, cookie_, status_, cancelled_);
> +
>  	delete metadata_;
>  	delete controls_;
>  	delete validator_;
> @@ -106,6 +112,8 @@ Request::~Request()
>   */
>  void Request::reuse(ReuseFlag flags)
>  {
> +	LIBCAMERA_TRACEPOINT(request_reuse, cookie_, status_, cancelled_);
> +
>  	pending_.clear();
>  	if (flags & ReuseBuffers) {
>  		for (auto pair : bufferMap_) {
> @@ -167,6 +175,8 @@ void Request::reuse(ReuseFlag flags)
>   */
>  int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)
>  {
> +	LIBCAMERA_TRACEPOINT(request_add_buffer, cookie_, status_, cancelled_);
> +
>  	if (!stream) {
>  		LOG(Request, Error) << "Invalid stream reference";
>  		return -EINVAL;
> @@ -202,6 +212,8 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)
>   */
>  FrameBuffer *Request::findBuffer(const Stream *stream) const
>  {
> +	LIBCAMERA_TRACEPOINT(request_find_buffer, cookie_, status_, cancelled_);
> +
>  	const auto it = bufferMap_.find(stream);
>  	if (it == bufferMap_.end())
>  		return nullptr;
> @@ -253,6 +265,8 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const
>   */
>  void Request::complete()
>  {
> +	LIBCAMERA_TRACEPOINT(request_complete, cookie_, status_, cancelled_);
> +
>  	ASSERT(!hasPendingBuffers());
>  	status_ = cancelled_ ? RequestCancelled : RequestComplete;
>  
> @@ -276,6 +290,8 @@ void Request::complete()
>   */
>  bool Request::completeBuffer(FrameBuffer *buffer)
>  {
> +	LIBCAMERA_TRACEPOINT(request_complete_buffer, cookie_, status_, cancelled_);
> +
>  	int ret = pending_.erase(buffer);
>  	ASSERT(ret == 1);
>