Telemetry2 debugfs #4956
Telemetry2 debugfs #4956
Conversation
Add new debug window slot type for second telemetry data slot. Signed-off-by: Jyri Sarha <[email protected]>
| /* debug log slot types */ | ||
| #define SOF_IPC4_DEBUG_SLOT_UNUSED 0x00000000 | ||
| #define SOF_IPC4_DEBUG_SLOT_CRITICAL_LOG 0x54524300 /* byte 0: core ID */ | ||
| #define SOF_IPC4_DEBUG_SLOT_DEBUG_LOG 0x474f4c00 /* byte 0: core ID */ |
There was a problem hiding this comment.
@jsarha can you please elaborate a bit on what telemetry2 is, the need for it and how it is different from the existing telemetry?
There was a problem hiding this comment.
It tries to be what Liam has described here:
thesofproject/sof#5521 (comment)
There was a problem hiding this comment.
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.
sound/soc/sof/ipc4-telemetry2.c
Outdated
There was a problem hiding this comment.
@plbossart this should be GPL-2.0 only right?
There was a problem hiding this comment.
Ok, just copy-pasted this from somewhere. I try to find more suitable source.
sound/soc/sof/ipc4-telemetry2.h
Outdated
There was a problem hiding this comment.
@jsarha also drop the 'All rights reserved'. We removed all this recently.
| #define SOF_IPC4_DEBUG_SLOT_DEBUG_LOG 0x474f4c00 /* byte 0: core ID */ | ||
| #define SOF_IPC4_DEBUG_SLOT_GDB_STUB 0x42444700 | ||
| #define SOF_IPC4_DEBUG_SLOT_TELEMETRY 0x4c455400 | ||
| #define SOF_IPC4_DEBUG_SLOT_TELEMETRY2 0x4c455500 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
sound/soc/sof/ipc4-telemetry2.c
Outdated
sound/soc/sof/ipc4-telemetry2.c
Outdated
There was a problem hiding this comment.
is this even remotely backwards compatible with previous generations, e.g. if you update the kernel but not the firmware?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
sound/soc/sof/ipc4-telemetry2.c
Outdated
There was a problem hiding this comment.
commit message "left to user-space tools"
What tools are you referring to? are they open-source?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
sound/soc/sof/Makefile
Outdated
There was a problem hiding this comment.
what is the 2 in telemetry2?
Does this invalidate or exist concurrently with ipc4-telemetry?
There was a problem hiding this comment.
Tried to answer to this question here: thesofproject/sof#9084 (comment)
The "telemetry2" debugfs file simply maps the telemetry2 debug slot as a debugfs file. The decoding is left to user-space tools. The telemetry2 debug window contents is encoded in SOF FW code found under sof/src/debug/telemetry directory in the SOF FW sources. Signed-off-by: Jyri Sarha <[email protected]>
jsarha
force-pushed
the
thread-analyzer-telemetry2-debugfs
branch
from
c8ddb5b to
211db10
Compare
|
Updated the license sections and added one something more to the commit message. But the number one question still is, what to call this debug window slot, if not telemetry2? |
|
Closing this, as there is replacement PR here: #5154 |
3 participants
No description provided.