Skip to content
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.

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

lib.sh: simplify the check for firmware rtos #973

Merged
merged 1 commit into from
Nov 7, 2022

Conversation

aiChaoSONG
Copy link

@aiChaoSONG aiChaoSONG commented Oct 20, 2022

Previously we do a lot of test to get the firmware path, it is not necessary, because we can acquire it simply with journalctl. Then we can test if the firmware loaded is using zephyr rtos by checking the occurrence of 'zephyr' in the firmware binary as before.

First (maybe the only) step to unlock sof-test for mtl

ranj063
ranj063 previously approved these changes Oct 20, 2022
Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

My understanding of @plbossart's comment in thesofproject/linux#3867 (comment) is that we should always parse the debug logs, end of story. So this PR should not exist and 3867 should be closed as wontfix?

Blocking this PR until we have clear, final agreement from everyone on what the final solution must be (even if we tolerate short term hacks in sof-test in the mean time).

case-lib/lib.sh Outdated Show resolved Hide resolved
@aiChaoSONG aiChaoSONG changed the title lib.sh: fix ipc4 firmware name lib.sh: simplify the check for firmware rtos Oct 24, 2022
@miRoox
Copy link
Contributor

miRoox commented Nov 2, 2022

We need this to enable mtrace on MTL

mengdonglin
mengdonglin previously approved these changes Nov 2, 2022
case-lib/lib.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

I think the patch looks good. The new dependency to

»       »       dev_dbg(sdev->dev, "request_firmware %s successful\n",
»       »       »       fw_filename);

... in SOF kernel driver should be more explicit though.

case-lib/lib.sh Show resolved Hide resolved
case-lib/lib.sh Show resolved Hide resolved
case-lib/lib.sh Outdated Show resolved Hide resolved
Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

Please create a new get_firmware_path() function that can be re-used elsewhere if needed (and easily changed to debugfs if/when thesofproject/linux#3867 happens)

Do not drop the URL to thesofproject/linux#3867, it has not been closed as wontfix.

The commit subject is too vague / misleading. How about: stop using PCI IDs to guess the firmware filename, always grep kernel logs instead.

awk has a built-in grep feature, use it. In fact awk can implement tail too, see example in tools/sof-get-default-tplg.sh where we already do something very similar. Also: add a comment referring to tools/sof-get-default-tplg.sh so they can be kept in sync (if thesofproject/linux#3867 or anything else happens)

This should do grep, tail, fix the conflict spotted by @libinyang and everything else with a single awk command but please test it again

journalctl_cmd -k |
  awk '/sof.*request_firmware/ { sub(/^.*request_firmware/,""); last_loaded_file=$1 } END { print last_loaded_file }'

case-lib/lib.sh Show resolved Hide resolved
@marc-hb
Copy link
Collaborator

marc-hb commented Nov 3, 2022

The commit subject is too vague / misleading. How about: stop using PCI IDs to guess the firmware filename, always grep kernel logs instead.

This patch stops using PCI IDs while PR #976 adds a new PCI ID at the same time. What is going on?

BTW have you looked for other places where sof-dump-status.py -p / -P are used? Don't they need the same fix?

@mengdonglin
Copy link
Contributor

The commit subject is too vague / misleading. How about: stop using PCI IDs to guess the firmware filename, always grep kernel logs instead.>
This patch stops using PCI IDs while PR #976 adds a new PCI ID at the same time. What is going on?

@marc-hb I think sof-dump-status.py is for dumping platform info to debug FW bugs. So PR #976 seems not relevant to this. We don't really care what SoC is used.

BTW have you looked for other places where sof-dump-status.py -p / -P are used? Don't they need the same fix?

kv2019i
kv2019i previously approved these changes Nov 4, 2022
Copy link
Contributor

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

Thanks @aiChaoSONG , looks good to me!

mengdonglin
mengdonglin previously approved these changes Nov 4, 2022
marc-hb
marc-hb previously approved these changes Nov 5, 2022
Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

Can someone check the internal test results for BOTH stable-2.2 and main before merging this?

@marc-hb I think sof-dump-status.py is for dumping platform info to debug FW bugs. So PR #976 seems not relevant to this. We don't really care what SoC is used.

sof-dump-status.py is still used in many sof-test places for many things including finding the firmware filename, either directly or indirectly.

Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

You dropped this

# FIXME: the kernel driver should give us the FW path
# https://github.com/thesofproject/linux/issues/3867

This patch simplifies the acquisition of firmware path
with journalctl, so that we can test if the firmware
loaded is using zephyr rtos by checking the occurrence
of 'zephyr' in the firmware binary.

Signed-off-by: Chao Song <[email protected]>
@keqiaozhang keqiaozhang merged commit 2e1573c into thesofproject:main Nov 7, 2022

firmware_path=$(get_firmware_path)
[ -n "$firmware_path" ] ||
die 'firmware path not found from journalctl, no firmware loaded or debug option disabled?'
Copy link
Contributor

@miRoox miRoox Nov 9, 2022

Choose a reason for hiding this comment

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

die here will lose "Test Result: FAIL!" and make our CI treat it as TIMEOUT. #979

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @miRoox! The func_exit_handler() should print "Test Result: something", did you find why it does not?

Copy link
Collaborator

Choose a reason for hiding this comment

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

To reproduce simply change the grep value to make it fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

The func_exit_handler() should print "Test Result: something", did you find why it does not?

As you said in #977, func_exit_handler will not be called again when exit inside it.

Copy link
Collaborator

@marc-hb marc-hb Nov 10, 2022

Choose a reason for hiding this comment

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

As you said in #977, func_exit_handler will not be called again when exit inside it.

Answering in bug #979 , let's move the discussion there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants