Skip to content

Telemetry2 debugfs #4956

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Telemetry2 debugfs #4956

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5128356
ASoC: SOF: IPC4: Add telemetry2 debug slot type
Mar 12, 2024
211db10
ASoC: SOF: IPC4: Simple debugfs file for telemetry2 debug window slot
Mar 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions include/sound/sof/ipc4/header.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,7 @@ struct sof_ipc4_notify_resource_data {
#define SOF_IPC4_DEBUG_SLOT_DEBUG_LOG 0x474f4c00 /* byte 0: core ID */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jsarha can you please elaborate a bit on what telemetry2 is, the need for it and how it is different from the existing telemetry?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It tries to be what Liam has described here:
thesofproject/sof#5521 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, but no one can figure this out. You have to add a simple enough abstract so that the PR is self-contained and self-explanatory.

#define SOF_IPC4_DEBUG_SLOT_GDB_STUB 0x42444700
#define SOF_IPC4_DEBUG_SLOT_TELEMETRY 0x4c455400
#define SOF_IPC4_DEBUG_SLOT_TELEMETRY2 0x4c455500
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commit message: a 'second telemetry slot'?

so we can have two slots for telemetry used concurrently? if not, how do we know which one is valid?

That's not a good start for a review in general.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd assume this would require some discussion. The old telemetry slot is not very extendable so something new is needed to pass more debugging information to SOF side.

#define SOF_IPC4_DEBUG_SLOT_BROKEN 0x44414544

/**
Expand Down
2 changes: 1 addition & 1 deletion sound/soc/sof/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ snd-sof-objs += ipc3.o ipc3-loader.o ipc3-topology.o ipc3-control.o ipc3-pcm.o\
endif
ifneq ($(CONFIG_SND_SOC_SOF_IPC4),)
snd-sof-objs += ipc4.o ipc4-loader.o ipc4-topology.o ipc4-control.o ipc4-pcm.o\
ipc4-mtrace.o ipc4-telemetry.o
ipc4-mtrace.o ipc4-telemetry.o ipc4-telemetry2.o
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the 2 in telemetry2?

Does this invalidate or exist concurrently with ipc4-telemetry?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried to answer to this question here: thesofproject/sof#9084 (comment)

endif

# SOF client support
Expand Down
76 changes: 76 additions & 0 deletions sound/soc/sof/ipc4-telemetry2.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
// SPDX-License-Identifier: GPL-2.0-only
//
// Copyright(c) 2024 Intel Corporation.

#include <linux/debugfs.h>
#include <linux/io.h>
#include <linux/pm_runtime.h>
#include <sound/sof/debug.h>
#include <sound/sof/ipc4/header.h>
#include "sof-priv.h"
#include "ops.h"
#include "ipc4-telemetry2.h"
#include "ipc4-priv.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commit message "left to user-space tools"

What tools are you referring to? are they open-source?

Copy link
Author

@jsarha jsarha May 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first prototype tool for extracting the thread information from the telemetry2 slot is implemented as third commit in this PR:
thesofproject/sof#9084

But I am vague on purpose. This debugfs file just shows the debug slot contents as it is in a file, and thus the Linux driver part can be quite agnostic of the both the FW and the tools to decode the information. The tool should then check that the ABI version in the debug window is something it can understand.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At minimum you should state that the tools to extract the thread information will be open-sourced. That's a common concern for reviewers/maintainers that a new kernel interface is exposed but it can only used with private tools. In this case this is not the direction at all so an additional comment would help.


static ssize_t sof_telemetry2_entry_read(struct file *file, char __user *buffer,
size_t count, loff_t *ppos)
{
struct snd_sof_dfsentry *dfse = file->private_data;
struct snd_sof_dev *sdev = dfse->sdev;
u32 type = SOF_IPC4_DEBUG_SLOT_TELEMETRY2;
loff_t pos = *ppos;
size_t size_ret;
u32 offset;
u8 *buf;

if (pos < 0)
return -EINVAL;
if (pos >= SOF_IPC4_DEBUG_SLOT_SIZE || !count)
return 0;
if (count > SOF_IPC4_DEBUG_SLOT_SIZE - pos)
count = SOF_IPC4_DEBUG_SLOT_SIZE - pos;

offset = sof_ipc4_find_debug_slot_offset_by_type(sdev, type);
if (!offset)
return -EFAULT;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this even remotely backwards compatible with previous generations, e.g. if you update the kernel but not the firmware?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure it is, as much as it can be. There of course is no previous generations of this particular feature, but its using the old debug slot reservation mechanism on the FW side and using the same function as mtrace and telemetry(1) to look for the specific slot based on the id. Naturally, there is not much we can do if the ID is not found.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the kernel should continue if it doesn't find an ID that only supported in newer versions of firmware.
I think this feature should be clearly advertised in the extended manifest, checking for the presence of an ID is a bit weird.

In this case it's not lethal because we actually ignore ALL errors from this function, so even returning -EFAULT is useless. You don't need an error code...


buf = kzalloc(SOF_IPC4_DEBUG_SLOT_SIZE, GFP_KERNEL);
if (!buf)
return -ENOMEM;

sof_mailbox_read(sdev, offset, buf, SOF_IPC4_DEBUG_SLOT_SIZE);
size_ret = copy_to_user(buffer, buf + pos, count);
if (size_ret) {
kfree(buf);
return -EFAULT;
}

*ppos = pos + count;
kfree(buf);

return count;
}

static const struct file_operations sof_telemetry2_fops = {
.open = simple_open,
.read = sof_telemetry2_entry_read,
.llseek = default_llseek,
};

void sof_ipc4_create_telemetry2_debugfs_node(struct snd_sof_dev *sdev)
{
struct snd_sof_dfsentry *dfse;

dfse = devm_kzalloc(sdev->dev, sizeof(*dfse), GFP_KERNEL);
if (!dfse)
return;

dfse->type = SOF_DFSENTRY_TYPE_IOMEM;
dfse->size = SOF_IPC4_DEBUG_SLOT_SIZE;
dfse->access_type = SOF_DEBUGFS_ACCESS_ALWAYS;
dfse->sdev = sdev;

list_add(&dfse->list, &sdev->dfsentry_list);

debugfs_create_file("telemetry2", 0444, sdev->debugfs_root, dfse, &sof_telemetry2_fops);
}
11 changes: 11 additions & 0 deletions sound/soc/sof/ipc4-telemetry2.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/* SPDX-License-Identifier: GPL-2.0-only */
/*
* Copyright(c) 2024 Intel Corporation.
*/

#ifndef __SOUND_SOC_SOF_IPC4_TELEMETRY2_H
#define __SOUND_SOC_SOF_IPC4_TELEMETRY2_H

void sof_ipc4_create_telemetry2_debugfs_node(struct snd_sof_dev *sdev);

#endif
3 changes: 3 additions & 0 deletions sound/soc/sof/ipc4.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include "ipc4-fw-reg.h"
#include "ipc4-priv.h"
#include "ipc4-telemetry.h"
#include "ipc4-telemetry2.h"
#include "ops.h"

static const struct sof_ipc4_fw_status {
Expand Down Expand Up @@ -582,6 +583,8 @@ static int ipc4_fw_ready(struct snd_sof_dev *sdev, struct sof_ipc4_msg *ipc4_msg

sof_ipc4_create_exception_debugfs_node(sdev);

sof_ipc4_create_telemetry2_debugfs_node(sdev);

return sof_ipc4_init_msg_memory(sdev);
}

Expand Down