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

ujfalusi · 2024-11-21T17:01:56Z

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

+
+	ret = sof_client_ipc_set_get_data(cdev, &msg, false);
+	if (ret) {
+		dev_err(dev, "%s: ipc failed %u", __func__, ret);


So, with old firmware this will keep printing errors (and the sof_ipc4_check_reply_status() also with "FW reported error: %u - Unknown\n")?

Ok, I'll make this a debug.

It might be OK to drop the print altogether? The core will print error anyways.

But, but. If this fails once, the chances to fail next time is pretty high, right? Should we disable the probes or fallback to what we had before?

There was nothing before. The callback just returned an empty array of probe points, so the only difference to the earlier behavior is the error message.

@jsarha, the difference is that this will introduce regression if you update the kernel w/o touching the probes, what worked will no longer works.
Albeit, it is a low impact regression, the probes are not used too frequently.

@ujfalusi there is some new error messages in the logs when reading "probe_points" debugfs or when sof_probes_compr_shutdown() is called, but there is no other difference in behavior. I'd say very low impact regression, as the only annoyance is couple of new error messages.

ujfalusi · 2024-11-22T06:21:07Z

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

+		else
+			ret = snprintf(buf + offset, remaining,
+				       "Id: %#010x  Purpose: %u  Node id: %#x\n",
+				       desc[i].buffer_id, desc[i].purpose, desc[i].stream_tag);


Instead, please add a new callback to struct sof_probes_ipc_ops, format/decode_point_information() or something simpler.

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

ujfalusi · 2024-11-22T06:44:25Z

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

+	int ret;
+
+	w = sof_ipc4_find_swidget_by_ids(sdev, SOF_IPC4_MOD_ID_GET(desc->buffer_id),
+					 SOF_IPC4_MOD_INSTANCE_GET(desc->buffer_id));


This is a grey area, I'm not sure if sof-clients should use swidget from sof proper.

When (If?) we move the audio cards to sof-clients then the sdev will not have a list of swidgets, all swidget will be local to the cdev, which means that you will have hard time to get this information

I think we should have a wrapper here for clients like we have for looking up the fw_module.
sof_client_ipc4_find_swidget_by_id()

The find_module() is a clean cut as the modules will always be managed by the sof core, the sof_client_ipc4_find_swidget_by_id() can be extended to search in sof-clients when the time comes and avoids using sdev directly.

Ok. If I write the sof_client_ipc4_find_swidget_by_id() wrapper in sof-client.c, and maybe a warning comment above, that don't build critical features on top of this function, then this is Ok 😅?

Probes are anyway a debug feature, and temporarily breaking it at some point in a future, is not catastrophic. Still I think this feature is very helpful if someone is really trying to use probes for someting, as at least I have no idea how find out a specific module- and instance-id of a specific probe point in the topology, without going through the widgets.

Ok. If I write the sof_client_ipc4_find_swidget_by_id() wrapper in sof-client.c, and maybe a warning comment above, that don't build critical features on top of this function, then this is Ok 😅?

I would not add any comment, not sure what it can help. You can only build the ipc4 variant of probes when IPC4 is enabled, so it will work.

Probes are anyway a debug feature, and temporarily breaking it at some point in a future, is not catastrophic. Still I think this feature is very helpful if someone is really trying to use probes for someting, as at least I have no idea how find out a specific module- and instance-id of a specific probe point in the topology, without going through the widgets.

Right, but it is a sof-client at the same time, it has to obey the rules set for such drivers. Debug, developer or user facing modules alike.

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

jsarha · 2024-11-22T11:18:18Z

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

+	int ret;
+
+	w = sof_ipc4_find_swidget_by_ids(sdev, SOF_IPC4_MOD_ID_GET(desc->buffer_id),
+					 SOF_IPC4_MOD_INSTANCE_GET(desc->buffer_id));


Ok. If I write the sof_client_ipc4_find_swidget_by_id() wrapper in sof-client.c, and maybe a warning comment above, that don't build critical features on top of this function, then this is Ok 😅?

Probes are anyway a debug feature, and temporarily breaking it at some point in a future, is not catastrophic. Still I think this feature is very helpful if someone is really trying to use probes for someting, as at least I have no idea how find out a specific module- and instance-id of a specific probe point in the topology, without going through the widgets.

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

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

Remove unused enum sof_ipc4_probe_type definition. There is no need for IPC4 specific probe types. Signed-off-by: Jyri Sarha <[email protected]>

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

jsarha force-pushed the probes_ipc4_get_config branch from c0b2ba6 to e0c8597 Compare November 24, 2024 21:13

Jyri Sarha added 6 commits November 25, 2024 09:34

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

4ff2882

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: ipc4-topology: Add sof_client_ipc4_find_swidget_by_id()

1938b03

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

ASoC: SOF: sof-client-probes: IPC4: Remove idle enum sof_ipc4_probe_type

ccc2da5

Remove unused enum sof_ipc4_probe_type definition. There is no need for IPC4 specific probe types. Signed-off-by: Jyri Sarha <[email protected]>

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

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

ujfalusi Nov 21, 2024

jsarha Nov 22, 2024

ujfalusi Nov 22, 2024

ujfalusi Nov 22, 2024

jsarha Nov 22, 2024

ujfalusi Nov 25, 2024

jsarha Nov 25, 2024

ujfalusi Nov 22, 2024

ujfalusi Nov 22, 2024

jsarha Nov 22, 2024

ujfalusi Nov 22, 2024

jsarha left a comment

jsarha Nov 22, 2024

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

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

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

jsarha commented Nov 20, 2024 •

edited

Loading