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.

ujfalusi · 2024-11-29T06:24:18Z

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

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


Should this be dev_dbg() if needed at all?

I'll make it debug. This print will only come if someone is reading the debugfs files, e.g. and then if someting unexpected happens, its comforting to read that atleast there were some probe points received from the FW.

ujfalusi · 2024-11-29T06:30:52Z

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

+	if (!w) {
+		dev_warn(dev, "matching widget to module_id %lu and instance_id %lu not found\n",
+			 SOF_IPC4_MOD_ID_GET(desc->buffer_id),
+			 SOF_IPC4_MOD_INSTANCE_GET(desc->buffer_id));


Can you use the same format for the print as it is used in sound/soc/sof/ipc4-control.c ?

dev_err(sdev->dev, "%s: Failed to find widget for module %u.%u\n", __func__, ndata->module_id, ndata->instance_id);

^ as in ipc4-control.c

ujfalusi · 2024-11-29T06:33:51Z

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

+				   struct sof_probe_point_desc *desc)
+{
+	struct device *dev = &cdev->auxdev.dev;
+	struct snd_sof_widget *w;


Can you use swidget for consistency with the rest of SOF code?

ujfalusi · 2024-11-29T06:39:41Z

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

+		       desc->buffer_id, desc->purpose, desc->stream_tag, w->widget->name,
+		       sof_probe_type_string(SOF_IPC4_PROBE_TYPE_GET(desc->buffer_id)),
+		       SOF_IPC4_PROBE_IDX_GET(desc->buffer_id),
+		       desc->stream_tag ? "(probed)" : "");


I would try to find a different phrase than probed here. It caught me every time I read the log: so this module is probed (as loaded, finished probing) but others are not? Oh no, it means that this is observed, under probe, probed. probing active / under probing / * perhaps?

I'll go with "connected" then.

ujfalusi · 2024-11-29T06:46:30Z

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

+#define SOF_IPC4_PROBE_IDX_SHIFT		26
+#define SOF_IPC4_PROBE_IDX_MASK			GENMASK(31, 26)
+#define SOF_IPC4_PROBE_IDX_GET(x)		(((x) & SOF_IPC4_PROBE_IDX_MASK) \
+						 >> SOF_IPC4_PROBE_IDX_SHIFT)


I would make these defines and the sof_probe_type_string() local to ipc4-probes, they are not generic.
The SOF_IPC4_PROBE_* defines might be better in the main IPC4 header?

Oh, for a moment I had mistaken that probe type was not ipc4 specific. Will do.

ujfalusi · 2024-11-29T06:52:07Z

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

@@ -75,7 +75,8 @@ static int sof_probes_compr_shutdown(struct snd_compr_stream *cstream,
 	int i, ret;

 	/* disconnect all probe points */
-	ret = ipc->points_info(cdev, &desc, &num_desc);
+	ret = ipc->points_info(cdev, &desc, &num_desc,
+			       PROBES_INFO_ACTIVE_PROBES);


Hrm, so this will introduce an error print with IPC3 as the ACTIVE_PROBES is handled as error in there?

With IPC3 we don't have distinction between active or available probes, right? Both case should be handled in a similar way, provide all the probes.

No. The active probes is the old feature that is supported also by ipc3. The available probes, giving all connection points availbale, is the new feature that is not supperted by ipc3. E.g. this call is equivalent to the old function that did not have the type argument.

If somebody tries to read "probe_points_available" in ipc3 system, then this funcion is called with PROBES_INFO_AVAILABLE_PROBES, and there will be a read error due to an ipc error. Then again nothing, but the lack of time, is stopping us from implementing available probes query for ipc3 too.

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]>

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]>

…be_points"

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

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

jsarha · 2024-11-29T08:09:31Z

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

+		       desc->buffer_id, desc->purpose, desc->stream_tag, w->widget->name,
+		       sof_probe_type_string(SOF_IPC4_PROBE_TYPE_GET(desc->buffer_id)),
+		       SOF_IPC4_PROBE_IDX_GET(desc->buffer_id),
+		       desc->stream_tag ? "(probed)" : "");


I'll go with "connected" then.

jsarha · 2024-11-29T09:39:45Z

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

+#define SOF_IPC4_PROBE_IDX_SHIFT		26
+#define SOF_IPC4_PROBE_IDX_MASK			GENMASK(31, 26)
+#define SOF_IPC4_PROBE_IDX_GET(x)		(((x) & SOF_IPC4_PROBE_IDX_MASK) \
+						 >> SOF_IPC4_PROBE_IDX_SHIFT)


Oh, for a moment I had mistaken that probe type was not ipc4 specific. Will do.

jsarha · 2024-11-29T12:32:22Z

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

@@ -75,7 +75,8 @@ static int sof_probes_compr_shutdown(struct snd_compr_stream *cstream,
 	int i, ret;

 	/* disconnect all probe points */
-	ret = ipc->points_info(cdev, &desc, &num_desc);
+	ret = ipc->points_info(cdev, &desc, &num_desc,
+			       PROBES_INFO_ACTIVE_PROBES);


No. The active probes is the old feature that is supported also by ipc3. The available probes, giving all connection points availbale, is the new feature that is not supperted by ipc3. E.g. this call is equivalent to the old function that did not have the type argument.

If somebody tries to read "probe_points_available" in ipc3 system, then this funcion is called with PROBES_INFO_AVAILABLE_PROBES, and there will be a read error due to an ipc error. Then again nothing, but the lack of time, is stopping us from implementing available probes query for ipc3 too.

jsarha · 2024-11-29T12:43:36Z

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

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


I'll make it debug. This print will only come if someone is reading the debugfs files, e.g. and then if someting unexpected happens, its comforting to read that atleast there were some probe points received from the FW.

jsarha · 2024-11-29T12:50:00Z

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

@@ -111,6 +113,11 @@ static int ipc3_probes_info(struct sof_client_dev *cdev, unsigned int cmd,
 	*params = NULL;
 	*num_params = 0;

+	if (type != PROBES_INFO_ACTIVE_PROBES) {


@ujfalusi its checked here that we only handle active probes request for ipc3, but there is no technical issue stopping us from implementing the "available" request in the FW for ipc3 and allowing it here.

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

Jyri Sarha added 6 commits November 29, 2024 14:46

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

40cbc3f

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()

5913a74

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]>

SQUASH ASoC: SOF: sof-client-probes-ipc4: Human readable debugfs "pro…

fafbb2f

…be_points"

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

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

ujfalusi Nov 29, 2024

jsarha Nov 29, 2024

ujfalusi Nov 29, 2024

ujfalusi Nov 29, 2024

ujfalusi Nov 29, 2024 •

edited

Loading

jsarha Nov 29, 2024

ujfalusi Nov 29, 2024

jsarha Nov 29, 2024

ujfalusi Nov 29, 2024

jsarha Nov 29, 2024

jsarha left a comment

jsarha Nov 29, 2024

jsarha Nov 29, 2024

jsarha Nov 29, 2024

jsarha Nov 29, 2024

jsarha Nov 29, 2024 •

edited

Loading

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

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

ujfalusi Nov 29, 2024 • edited Loading

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

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

Choose a reason for hiding this comment

jsarha Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

jsarha commented Nov 20, 2024 •

edited

Loading

ujfalusi Nov 29, 2024 •

edited

Loading

jsarha Nov 29, 2024 •

edited

Loading