Probes ipc4 get config #5249

jsarha · 2024-11-20T20:43:18Z

Some trivial fixes
proper sof_probes_ipc_ops points_info() implementation for ipc4
more informative debufs read output for ipc4
all availabe probe points option to points_info() and a separate debugfs file for it

The functionality of the new features depend on thesofproject/sof#9669

lgirdwood

LGTM, nice addition to make this way easier to use.

jsarha · 2024-11-21T20:48:03Z

The small updates are for fixing sparse warnings from exotic builds. The check fails at first failed file and commit, so there is many iterations. Missing documentation, wrong format string for 32-bit build, etc. etc.

ujfalusi

@jsarha, you are flexing again the boundaries of what sof-clients can do ;)
There are couple of places that we need to align with the definition of sof-client, I know it makes things a bit harder, but I want to keep the rules applicable.

sound/soc/sof/sof-client-probes-ipc4.c

sound/soc/sof/sof-client-probes.c

jsarha

Thanks @ujfalusi for feedback! I'll drop the first commit, implement the find widgets wrapper, and add the new sof_probes_ipc_ops callback.

sound/soc/sof/sof-client-probes-ipc4.c

sound/soc/sof/sof-client-probes.c

ujfalusi

@jsarha, this version looks much cleaner but there are few things that should be checked.

sound/soc/sof/sof-client-probes-ipc4.c

sound/soc/sof/sof-client-probes.h

sound/soc/sof/sof-client-probes.c

jsarha

Thanks again. Hope its now good. At least its converging.

sound/soc/sof/sof-client-probes-ipc4.c

sound/soc/sof/sof-client-probes.h

sound/soc/sof/sof-client-probes.c

sound/soc/sof/sof-client-probes-ipc4.c

sound/soc/sof/sof-client-probes-ipc3.c

ujfalusi · 2024-12-02T09:01:33Z

@jsarha, can you check the W=1 build errors?

jsarha · 2024-12-09T09:43:30Z

@jsarha, can you check the W=1 build errors?

Oops, there were other short comings in the previous version too. I'll ping you once the compile CI tests are ready, if they are now OK.

jsarha · 2024-12-09T19:29:26Z

@ujfalusi , I think this is finally good to go.

ujfalusi

@jsarha, looks good to my eyes, but I will re-check it on Monday with somewhat fresher mind.
Thank you for the updates!

lgirdwood

Just one pointer check.

lgirdwood · 2024-12-16T16:08:13Z

sound/soc/sof/sof-client-probes-ipc4.c

+		return ret;
+	}
+	info = msg.data_ptr;
+	*num_desc =  info->num_elems;


do we check if this is NULL ?

Which one? num_descs comes from these calls: https://github.com/jsarha/linux-sof/blob/f5bbc39177bbfcd1753a7fef69043f69e3b5a540/sound/soc/sof/sof-client-probes.c#L78 and https://github.com/jsarha/linux-sof/blob/f5bbc39177bbfcd1753a7fef69043f69e3b5a540/sound/soc/sof/sof-client-probes.c#L227 , and info is assigned from msg.data_ptr which is allocated and checked couple of lines earlier (on line 199). There should be no need to add any more checks for either one being NULL, should there?

jsarha · 2024-12-27T11:02:44Z

sound/soc/sof/sof-client-probes-ipc4.c

+		return ret;
+	}
+	info = msg.data_ptr;
+	*num_desc =  info->num_elems;


Which one? num_descs comes from these calls: https://github.com/jsarha/linux-sof/blob/f5bbc39177bbfcd1753a7fef69043f69e3b5a540/sound/soc/sof/sof-client-probes.c#L78 and https://github.com/jsarha/linux-sof/blob/f5bbc39177bbfcd1753a7fef69043f69e3b5a540/sound/soc/sof/sof-client-probes.c#L227 , and info is assigned from msg.data_ptr which is allocated and checked couple of lines earlier (on line 199). There should be no need to add any more checks for either one being NULL, should there?

jsarha · 2024-12-27T11:03:21Z

sound/soc/sof/sof-client-probes-ipc4.c

+	info = msg.data_ptr;
+	*num_desc =  info->num_elems;
+	dev_dbg(dev, "%s: got %zu probe points", __func__, *num_desc);
+	info = msg.data_ptr;


Hmm, this superfluous assignment should go.

Upgrade the struct sof_probes_ipc_ops points_info() method from dummy implementation to a working implementation. The actual functionality requires that the DSP FW supports the IPC request. The support was just recently added. If its not there an IPC failure is reported in the logs. Signed-off-by: Jyri Sarha <[email protected]>

Add SOF_IPC4_MOD_INSTANCE_GET() and SOF_IPC4_MOD_ID_GET() for getting the ids from ipc4 header presentation. Signed-off-by: Jyri Sarha <[email protected]>

Add sof_client_ipc4_find_swidget_by_id() for finding widgets from SOF client devices. The motivation is to decode probes debugfs output to be more readable. Signed-off-by: Jyri Sarha <[email protected]>

jsarha · 2024-12-30T17:44:44Z

Rebased on top of 6.13 based sof-dev.

jsarha · 2025-01-07T13:24:44Z

@lgirdwood & @ujfalusi, a gentle ping!

lgirdwood · 2025-01-08T17:01:23Z

@ujfalusi @ranj063 pls review.

ranj063 · 2025-01-08T19:18:02Z

sound/soc/sof/sof-client-probes.h

@@ -34,13 +34,18 @@ struct sof_probe_point_desc {
 	unsigned int stream_tag;
 } __packed;

+enum sof_probe_info_type {
+	PROBES_INFO_ACTIVE_PROBES = 0,


i dont think you need the = 0

I usually like to explicitely show how an enum value will behave when interpreted as a boolean or what enum symbol a zeroed memory will have, but here there really is no need to care about what the numerical value is (I think I was expecting to use the value as a boolean).

ranj063

just one nit-pick, looks good otherwise

ujfalusi

@jsarha, sorry for the delay, I have only one minor comment.

ujfalusi · 2025-01-14T13:00:04Z

sound/soc/sof/sof-client-probes-ipc4.c

+		dev_err(dev, "%s: Failed to find widget for module %lu.%lu\n",
+			__func__, SOF_IPC4_MOD_ID_GET(desc->buffer_id),
+			SOF_IPC4_MOD_INSTANCE_GET(desc->buffer_id));
+		ret = snprintf(buf, size, "%u,%u,%u\nUNKNOWN buffer_id 0x%08x\n",


can we keep this in one line ?

"%u,%u,%u\tUNKNOWN buffer_id 0x%08x\n"

and you are printing the desc->buffer_id twice?

Sure will "\n" to "\t". I am using different macros in different times I am printing desc->buffer_id. The module id and instance id is printed separately.

jsarha

Fixes recommended in last review round applied.

jsarha · 2025-01-09T18:20:05Z

sound/soc/sof/sof-client-probes.h

@@ -34,13 +34,18 @@ struct sof_probe_point_desc {
 	unsigned int stream_tag;
 } __packed;

+enum sof_probe_info_type {
+	PROBES_INFO_ACTIVE_PROBES = 0,


I usually like to explicitely show how an enum value will behave when interpreted as a boolean or what enum symbol a zeroed memory will have, but here there really is no need to care about what the numerical value is (I think I was expecting to use the value as a boolean).

jsarha · 2025-01-15T10:58:09Z

sound/soc/sof/sof-client-probes-ipc4.c

+		dev_err(dev, "%s: Failed to find widget for module %lu.%lu\n",
+			__func__, SOF_IPC4_MOD_ID_GET(desc->buffer_id),
+			SOF_IPC4_MOD_INSTANCE_GET(desc->buffer_id));
+		ret = snprintf(buf, size, "%u,%u,%u\nUNKNOWN buffer_id 0x%08x\n",


Sure will "\n" to "\t". I am using different macros in different times I am printing desc->buffer_id. The module id and instance id is printed separately.

ujfalusi · 2025-01-15T11:14:11Z

sound/soc/sof/sof-client-probes-ipc4.c

+			SOF_IPC4_MOD_INSTANCE_GET(desc->buffer_id));
+		ret = snprintf(buf, size, "%u,%u,%u\tUNKNOWN buffer_id 0x%08x\n",
+			       desc->buffer_id, desc->purpose, desc->stream_tag,
+			       desc->buffer_id);


I think I saw it before, but why are we printing the desc->buffer_id twice if swidget is not found?
Why not:

ret = snprintf(buf, size, "%u,%u,%u\t<unknown> %s buf idx %lu %s\n", desc->buffer_id, desc->purpose, desc->stream_tag, sof_probe_ipc4_type_string(SOF_IPC4_PROBE_TYPE_GET(desc->buffer_id)), SOF_IPC4_PROBE_IDX_GET(desc->buffer_id), desc->stream_tag ? "(connected)" : "");

or combine the two prints into one:

ret = snprintf(buf, size, "%u,%u,%u\t%s %s buf idx %lu %s\n", desc->buffer_id, desc->purpose, desc->stream_tag, swidget ? swidget->widget->name : "<unknown>", sof_probe_ipc4_type_string(SOF_IPC4_PROBE_TYPE_GET(desc->buffer_id)), SOF_IPC4_PROBE_IDX_GET(desc->buffer_id), desc->stream_tag ? "(connected)" : "");

The current output of three integers is not very human readable. Use ipc4 functions to describe in more detail what the struct sof_probe_point_desc buffer_id is actually referring to in an ipc4 SOF system. Before this commit the "probe_points" debugfs file could read as: Id: 0x01000004 Purpose: 0 Node id: 0x100 Id: 0x00000006 Purpose: 0 Node id: 0x100 And after in the same situation in an ipc4 system it reads: 16777220,0,256 host-copier.0.playback output buf idx 0 (probed) 6,0,256 gain.1.1 input buf idx 0 (probed) The triplet in the beginning of the line can be used to reinserted the probe point again by writing it into "probe_points" debugfs file, if its first removed by writing the fist number in "probe_points_remove". The last number is ignored when creating a probe point. Signed-off-by: Jyri Sarha <[email protected]>

Add another debugfs file, "probe_points_available", that shows all the available probe points in the SOF FW at the time of query. The probe points are there only when an active SOF stream exists in the system. However, the stream identifiers are persistent in the sense that the same probe point identifiers always appear with the same playback or capture command in the same system configuration. The output, when reading "probe_points_available", may look like this: 16777220,0,256 host-copier.0.playback output 1 buf idx 0 (probed) 6,0,256 gain.1.1 input buf idx 0 (probed) 16777222,0,0 gain.1.1 output buf idx 0 2,0,0 mixin.1.1 input buf idx 0 16777218,0,0 mixin.1.1 output buf idx 0 3,0,0 mixout.2.1 input buf idx 0 16777219,0,0 mixout.2.1 output buf idx 0 65542,0,0 gain.2.1 input 0 buf idx 0 16842758,0,0 gain.2.1 output buf idx 0 15,0,0 smart_amp.2.1 input buf idx 0 16777231,0,0 smart_amp.2.1 output buf idx 0 65540,0,0 dai-copier.SSP.NoCodec-0.playback input buf idx 0 The triplet at the beginning of a line can be copy-pasted as such to "probe_points" debugfs file for adding a probe point. The rest of the line tries to give human readable explanation of what this probe point is. Signed-off-by: Jyri Sarha <[email protected]>

jsarha · 2025-01-15T18:20:00Z

@ujfalusi & @ranj063 , hope this is now good.

lgirdwood

Almost good to go for me, lets use HEX for our IDs though.

lgirdwood · 2025-01-16T11:56:50Z

sound/soc/sof/sof-client-probes-ipc4.c

@@ -8,6 +8,7 @@
 #include <sound/soc.h>
 #include <sound/sof/ipc4/header.h>
 #include <uapi/sound/sof/header.h>
+#include "sof-audio.h"


@jsarha btw the 1st triplet above 16777220,0,256 is better in HEX as 0x1000004, 0x0, 0x100

@lgirdwood , the reason for the decimal format is the the fackt that its decimal numbers that are expected when defining the probe points. So if we want to be able to copy-paste the triplets to probe_points debgfs file, then we want to print decimal numbers. We can of course change the write interface too so that it expect hex-numbers or is sensitive to "0x" prefix. It would take a day or two to implement and properly test that, should I go that way?

Pls make hex, the hex value also make sense when introspection is performed by anyone debugging as they map to bit masks within the IDs, i.e. its not a coincidence we have 0x1000004 and 0x100 as values.

jsarha marked this pull request as ready for review November 20, 2024 20:43

jsarha requested review from lgirdwood, plbossart, ranj063, kv2019i, dbaluta, bardliao and ujfalusi as code owners November 20, 2024 20:43

jsarha force-pushed the probes_ipc4_get_config branch 5 times, most recently from 39862fe to 32b8154 Compare November 21, 2024 15:46

lgirdwood previously approved these changes Nov 21, 2024

View reviewed changes

jsarha dismissed lgirdwood’s stale review via c5ea419 November 21, 2024 21:03

jsarha force-pushed the probes_ipc4_get_config branch 2 times, most recently from c5ea419 to c0b2ba6 Compare November 21, 2024 21:25

ujfalusi requested changes Nov 22, 2024

View reviewed changes

jsarha commented Nov 22, 2024

View reviewed changes

sound/soc/sof/sof-client-probes-ipc4.c Outdated Show resolved Hide resolved

sound/soc/sof/sof-client-probes.c Outdated Show resolved Hide resolved

jsarha force-pushed the probes_ipc4_get_config branch 2 times, most recently from e0c8597 to ccc2da5 Compare November 25, 2024 08:16

lgirdwood previously approved these changes Nov 28, 2024

View reviewed changes

ujfalusi requested changes Nov 29, 2024

View reviewed changes

jsarha dismissed lgirdwood’s stale review via 42620ca November 29, 2024 12:50

jsarha force-pushed the probes_ipc4_get_config branch from ccc2da5 to 42620ca Compare November 29, 2024 12:50

jsarha commented Nov 29, 2024

View reviewed changes

jsarha force-pushed the probes_ipc4_get_config branch from 42620ca to f5bbc39 Compare December 9, 2024 09:42

ujfalusi previously approved these changes Dec 13, 2024

View reviewed changes

lgirdwood reviewed Dec 16, 2024

View reviewed changes

jsarha commented Dec 27, 2024

View reviewed changes

jsarha dismissed ujfalusi’s stale review via c3528ea December 29, 2024 18:15

jsarha force-pushed the probes_ipc4_get_config branch from f5bbc39 to c3528ea Compare December 29, 2024 18:15

Jyri Sarha added 3 commits December 30, 2024 18:59

ASoC: SOF: IPC4: Add GET macros for module id and module instance id

ed3240c

Add SOF_IPC4_MOD_INSTANCE_GET() and SOF_IPC4_MOD_ID_GET() for getting the ids from ipc4 header presentation. Signed-off-by: Jyri Sarha <[email protected]>

ASoC: SOF: client: Add sof_client_ipc4_find_swidget_by_id()

7b0b5e7

Add sof_client_ipc4_find_swidget_by_id() for finding widgets from SOF client devices. The motivation is to decode probes debugfs output to be more readable. Signed-off-by: Jyri Sarha <[email protected]>

jsarha force-pushed the probes_ipc4_get_config branch from c3528ea to 4276e4f Compare December 30, 2024 17:44

ranj063 reviewed Jan 8, 2025

View reviewed changes

ranj063 approved these changes Jan 8, 2025

View reviewed changes

ranj063 previously approved these changes Jan 8, 2025

View reviewed changes

ujfalusi reviewed Jan 14, 2025

View reviewed changes

lgirdwood previously approved these changes Jan 14, 2025

View reviewed changes

jsarha dismissed stale reviews from lgirdwood and ranj063 via cd94f96 January 15, 2025 11:04

jsarha force-pushed the probes_ipc4_get_config branch from 4276e4f to cd94f96 Compare January 15, 2025 11:04

jsarha commented Jan 15, 2025

View reviewed changes

ujfalusi reviewed Jan 15, 2025

View reviewed changes

Jyri Sarha added 2 commits January 15, 2025 19:09

jsarha force-pushed the probes_ipc4_get_config branch from cd94f96 to e6cd124 Compare January 15, 2025 17:18

ranj063 approved these changes Jan 15, 2025

View reviewed changes

ranj063 requested a review from ujfalusi January 15, 2025 19:37

lgirdwood reviewed Jan 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Probes ipc4 get config #5249

Probes ipc4 get config #5249

jsarha commented Nov 20, 2024 •

edited

Loading

lgirdwood left a comment

jsarha commented Nov 21, 2024

ujfalusi left a comment

jsarha left a comment

ujfalusi left a comment

jsarha left a comment

ujfalusi commented Dec 2, 2024

jsarha commented Dec 9, 2024

jsarha commented Dec 9, 2024 •

edited

Loading

ujfalusi left a comment

lgirdwood left a comment

lgirdwood Dec 16, 2024

jsarha Dec 27, 2024 •

edited

Loading

jsarha Dec 27, 2024 •

edited

Loading

jsarha Dec 27, 2024

jsarha commented Dec 30, 2024

jsarha commented Jan 7, 2025

lgirdwood commented Jan 8, 2025

ranj063 Jan 8, 2025

jsarha Jan 9, 2025

ranj063 left a comment

ujfalusi left a comment

ujfalusi Jan 14, 2025

jsarha Jan 15, 2025

jsarha left a comment

jsarha Jan 9, 2025

jsarha Jan 15, 2025

ujfalusi Jan 15, 2025

jsarha commented Jan 15, 2025

lgirdwood left a comment

lgirdwood Jan 16, 2025

jsarha Jan 16, 2025

lgirdwood Jan 16, 2025

Probes ipc4 get config #5249

Are you sure you want to change the base?

Probes ipc4 get config #5249

Conversation

jsarha commented Nov 20, 2024 • edited Loading

lgirdwood left a comment

Choose a reason for hiding this comment

jsarha commented Nov 21, 2024

ujfalusi left a comment

Choose a reason for hiding this comment

jsarha left a comment

Choose a reason for hiding this comment

ujfalusi left a comment

Choose a reason for hiding this comment

jsarha left a comment

Choose a reason for hiding this comment

ujfalusi commented Dec 2, 2024

jsarha commented Dec 9, 2024

jsarha commented Dec 9, 2024 • edited Loading

ujfalusi left a comment

Choose a reason for hiding this comment

lgirdwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsarha Dec 27, 2024 • edited Loading

Choose a reason for hiding this comment

jsarha Dec 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsarha commented Dec 30, 2024

jsarha commented Jan 7, 2025

lgirdwood commented Jan 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranj063 left a comment

Choose a reason for hiding this comment

ujfalusi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsarha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsarha commented Jan 15, 2025

lgirdwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsarha commented Nov 20, 2024 •

edited

Loading

jsarha commented Dec 9, 2024 •

edited

Loading

jsarha Dec 27, 2024 •

edited

Loading

jsarha Dec 27, 2024 •

edited

Loading