2024-01-18 03:53:26

by Bjorn Andersson

[permalink] [raw]
Subject: [PATCH] soc: qcom: aoss: Add tracepoints in qmp_send()

Add tracepoint for tracing the messages being sent and the success
thereof. This is useful as the system has a variety of clients sending
requests to the always-on subsystem.

Signed-off-by: Bjorn Andersson <[email protected]>
---
drivers/soc/qcom/Makefile | 1 +
drivers/soc/qcom/qcom_aoss.c | 7 +++++++
drivers/soc/qcom/trace-aoss.h | 48 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 56 insertions(+)

diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 05b3d54e8dc9..9268e15e1719 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -1,5 +1,6 @@
# SPDX-License-Identifier: GPL-2.0
CFLAGS_rpmh-rsc.o := -I$(src)
+CFLAGS_qcom_aoss.o := -I$(src)
obj-$(CONFIG_QCOM_AOSS_QMP) += qcom_aoss.o
obj-$(CONFIG_QCOM_GENI_SE) += qcom-geni-se.o
obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o
diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
index aff0cfb71482..db94b2d17e7d 100644
--- a/drivers/soc/qcom/qcom_aoss.c
+++ b/drivers/soc/qcom/qcom_aoss.c
@@ -13,6 +13,9 @@
#include <linux/slab.h>
#include <linux/soc/qcom/qcom_aoss.h>

+#define CREATE_TRACE_POINTS
+#include "trace-aoss.h"
+
#define QMP_DESC_MAGIC 0x0
#define QMP_DESC_VERSION 0x4
#define QMP_DESC_FEATURES 0x8
@@ -235,6 +238,8 @@ int qmp_send(struct qmp *qmp, const char *fmt, ...)

mutex_lock(&qmp->tx_lock);

+ trace_aoss_send(buf);
+
/* The message RAM only implements 32-bit accesses */
__iowrite32_copy(qmp->msgram + qmp->offset + sizeof(u32),
buf, sizeof(buf) / sizeof(u32));
@@ -256,6 +261,8 @@ int qmp_send(struct qmp *qmp, const char *fmt, ...)
ret = 0;
}

+ trace_aoss_send_done(buf, ret);
+
mutex_unlock(&qmp->tx_lock);

return ret;
diff --git a/drivers/soc/qcom/trace-aoss.h b/drivers/soc/qcom/trace-aoss.h
new file mode 100644
index 000000000000..48cd3f0f4cb8
--- /dev/null
+++ b/drivers/soc/qcom/trace-aoss.h
@@ -0,0 +1,48 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM qcom_aoss
+
+#if !defined(_TRACE_RPMH_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_RPMH_H
+
+#include <linux/tracepoint.h>
+
+TRACE_EVENT(aoss_send,
+ TP_PROTO(const char *msg),
+ TP_ARGS(msg),
+ TP_STRUCT__entry(
+ __string(msg, msg)
+ ),
+ TP_fast_assign(
+ __assign_str(msg, msg);
+ ),
+ TP_printk("%s", __get_str(msg))
+);
+
+TRACE_EVENT(aoss_send_done,
+ TP_PROTO(const char *msg, int ret),
+ TP_ARGS(msg, ret),
+ TP_STRUCT__entry(
+ __string(msg, msg)
+ __field(int, ret)
+ ),
+ TP_fast_assign(
+ __assign_str(msg, msg);
+ __entry->ret = ret;
+ ),
+ TP_printk("%s: %d", __get_str(msg), __entry->ret)
+);
+
+#endif /* _TRACE_RPMH_H */
+
+#undef TRACE_INCLUDE_PATH
+#define TRACE_INCLUDE_PATH .
+
+#undef TRACE_INCLUDE_FILE
+#define TRACE_INCLUDE_FILE trace-aoss
+
+#include <trace/define_trace.h>

---
base-commit: 943b9f0ab2cfbaea148dd6ac279957eb08b96904
change-id: 20230927-qcom-aoss-tracepoints-c5832ca2eb29

Best regards,
--
Bjorn Andersson <[email protected]>



2024-01-23 19:33:58

by Chris Lew

[permalink] [raw]
Subject: Re: [PATCH] soc: qcom: aoss: Add tracepoints in qmp_send()



On 1/17/2024 7:52 PM, Bjorn Andersson wrote:
> @@ -235,6 +238,8 @@ int qmp_send(struct qmp *qmp, const char *fmt, ...)
>
> mutex_lock(&qmp->tx_lock);
>
> + trace_aoss_send(buf);
> +
> /* The message RAM only implements 32-bit accesses */
> __iowrite32_copy(qmp->msgram + qmp->offset + sizeof(u32),
> buf, sizeof(buf) / sizeof(u32));
> @@ -256,6 +261,8 @@ int qmp_send(struct qmp *qmp, const char *fmt, ...)
> ret = 0;
> }
>
> + trace_aoss_send_done(buf, ret);
> +

As a side note, another place where we've traced before is on the
receiving irq to get the full timing of how long AOSS takes to process a
command. I think we've discussed this in the past and decided that we
can use kprobes if that need occurs.

> mutex_unlock(&qmp->tx_lock);
>
> return ret;
> diff --git a/drivers/soc/qcom/trace-aoss.h b/drivers/soc/qcom/trace-aoss.h
> new file mode 100644
> index 000000000000..48cd3f0f4cb8
> --- /dev/null
> +++ b/drivers/soc/qcom/trace-aoss.h
> @@ -0,0 +1,48 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM qcom_aoss
> +
> +#if !defined(_TRACE_RPMH_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_RPMH_H

Any Reason for this to be _TRACE_RPMH_H instead of _TRACE_AOSS_H?


2024-01-23 22:10:41

by Bjorn Andersson

[permalink] [raw]
Subject: Re: [PATCH] soc: qcom: aoss: Add tracepoints in qmp_send()

On Tue, Jan 23, 2024 at 10:54:16AM -0800, Chris Lew wrote:
>
>
> On 1/17/2024 7:52 PM, Bjorn Andersson wrote:
> > @@ -235,6 +238,8 @@ int qmp_send(struct qmp *qmp, const char *fmt, ...)
> > mutex_lock(&qmp->tx_lock);
> > + trace_aoss_send(buf);
> > +
> > /* The message RAM only implements 32-bit accesses */
> > __iowrite32_copy(qmp->msgram + qmp->offset + sizeof(u32),
> > buf, sizeof(buf) / sizeof(u32));
> > @@ -256,6 +261,8 @@ int qmp_send(struct qmp *qmp, const char *fmt, ...)
> > ret = 0;
> > }
> > + trace_aoss_send_done(buf, ret);
> > +
>
> As a side note, another place where we've traced before is on the receiving
> irq to get the full timing of how long AOSS takes to process a command. I
> think we've discussed this in the past and decided that we can use kprobes
> if that need occurs.
>

Right, these two tracepoints would not provide an accurate measurement
for the that time. But as we discussed, we can use this in combination
with other events to acquire such measurements.

> > mutex_unlock(&qmp->tx_lock);
> > return ret;
> > diff --git a/drivers/soc/qcom/trace-aoss.h b/drivers/soc/qcom/trace-aoss.h
> > new file mode 100644
> > index 000000000000..48cd3f0f4cb8
> > --- /dev/null
> > +++ b/drivers/soc/qcom/trace-aoss.h
> > @@ -0,0 +1,48 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
> > + */
> > +
> > +#undef TRACE_SYSTEM
> > +#define TRACE_SYSTEM qcom_aoss
> > +
> > +#if !defined(_TRACE_RPMH_H) || defined(TRACE_HEADER_MULTI_READ)
> > +#define _TRACE_RPMH_H
>
> Any Reason for this to be _TRACE_RPMH_H instead of _TRACE_AOSS_H?
>

That's what copy-paste gave me... Thanks for spotting that!

Regards,
Bjorn