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.

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

Jump to bottom

[FEATURE] IPC4: verify-firmware-presence.sh: use actual firmware #841

Closed
plbossart opened this issue Jan 28, 2022 · 12 comments
Closed

[FEATURE] IPC4: verify-firmware-presence.sh: use actual firmware #841

plbossart opened this issue Jan 28, 2022 · 12 comments
Assignees
@marc-hb
Labels
P1 Blocker bugs or important features

Comments

@plbossart
Copy link
Member

With the IPC4 file path and filename changes, this test needs to be updated:

[   40.931241] sof-audio-pci-intel-tgl 0000:00:1f.3: request_firmware intel/avs/tgl/dsp_basefw.bin successful

2022-01-27 23:55:25 UTC Sub-Test: [INFO] Checking file: /lib/firmware/intel/sof/sof-tgl.ri

The paths and filenames need to be updated.
@plbossart plbossart added the IPC4 label Jan 28, 2022
@marc-hb
Copy link
Collaborator

marc-hb commented Jan 28, 2022

Checking file: /lib/firmware/intel/sof/community/sof-tgl.ri

The /lib/firmware/intel/sof/community/ part comes from ./sof-test/tools/sof-dump-status.py --fwpath
The tgl part comes from ./sof-test/tools/sof-dump-status.py --platform

--platform output comes from a table of PCI and DMI identifiers embedded in sof-dump-status.py + some /sys/ exploration.

--fwpath output is mostly hardcoded except for the community part that is added based on matching DMI System Information -> Manufacturer, Product Name against a list of values embedded in the script.

Is intel/avs/tgl/dsp_basefw.bin a good model / good filename example representative of all configurations? For both open and closed source?

This test is designed to PASS before the firmware is loaded. I doubt it's possible to predict the IPC version before the firmware is loaded, is there? So should the test just look for both and pass when any is found?

If this gets too complicated this test should simply be removed from IPC4 test plans. It's never been terribly useful IMHO.

@marc-hb
Copy link
Collaborator

marc-hb commented Jan 28, 2022

Is intel/avs/tgl/dsp_basefw.bin a good model / good filename example representative of all configurations? For both open and closed source?

Thanks @ranj063 for answering this: yes this is a good template for closed source. Open source will hopefully have a template too but a different one, probably something like intel/sof/community/sof-abc4.ri

@plbossart
Copy link
Member Author

we are going to use a different path for SOF w/ IPC4, e.g. inte/sof-ipc4/. So I wonder if this test should be removed completely.
It makes no sense anyway because the distribution typically has firmwares installed, but likely not the ones you want to test. This model of hard-coding path and overriding distribution files is not good. We should use /lib/firmware/updates for CI and tests.

@marc-hb marc-hb mentioned this issue Jan 29, 2022
Merged
marc-hb added a commit to marc-hb/sof-test that referenced this issue Feb 2, 2022
@marc-hb
run-all-tests.sh: don't test that the firmware is loaded
52fd945 
Don't run verify-firmware-presence and verify-firmware-load by default.
The run-all-tests.sh "harcoded test plan" is a bit of a hack, purely for
interactive use. Not used as a test runner by any automation any time
soon.

When run without firmware the script now fails like this:

+ sof-test/test-case/verify-pcm-list.sh
grep: /proc/asound/cards: No such file or directory

For interactive users who forget to load the firmware and have direct
access to every log file and everything else this is a good enough error
message.

This makes testing IPC4 firmware easier as it mitigates issues thesofproject#841 and

Signed-off-by: Marc Herbert <[email protected]>
marc-hb added a commit to marc-hb/sof-test that referenced this issue Feb 2, 2022
@marc-hb
run-all-tests.sh: don't test that the firmware is loaded
0d90f61 
Don't run verify-firmware-presence and verify-firmware-load by default.
The run-all-tests.sh "harcoded test plan" is a bit of a hack, purely for
interactive use. Not used as a test runner by any automation any time
soon.

When run without firmware the script now fails like this:

+ sof-test/test-case/verify-pcm-list.sh
grep: /proc/asound/cards: No such file or directory

For interactive users who forget to load the firmware and have direct
access to every log file and everything else this is a good enough error
message.

This makes testing IPC4 firmware easier as it mitigates issues thesofproject#841 and thesofproject#842

Signed-off-by: Marc Herbert <[email protected]>
marc-hb added a commit that referenced this issue Feb 2, 2022
@marc-hb
run-all-tests.sh: don't test that the firmware is loaded
7ce5bcb 
Don't run verify-firmware-presence and verify-firmware-load by default.
The run-all-tests.sh "harcoded test plan" is a bit of a hack, purely for
interactive use. Not used as a test runner by any automation any time
soon.

When run without firmware the script now fails like this:

+ sof-test/test-case/verify-pcm-list.sh
grep: /proc/asound/cards: No such file or directory

For interactive users who forget to load the firmware and have direct
access to every log file and everything else this is a good enough error
message.

This makes testing IPC4 firmware easier as it mitigates issues #841 and #842

Signed-off-by: Marc Herbert <[email protected]>
@mengdonglin mengdonglin added the P1 Blocker bugs or important features label Feb 10, 2022
@mengdonglin
Copy link
Contributor

we are going to use a different path for SOF w/ IPC4, e.g. inte/sof-ipc4/. So I wonder if this test should be removed completely. It makes no sense anyway because the distribution typically has firmwares installed, but likely not the ones you want to test. This model of hard-coding path and overriding distribution files is not good. We should use /lib/firmware/updates for CI and tests.

@miRoox I think we need to revise our DUT deployment script? What do you think?

@marc-hb
Copy link
Collaborator

marc-hb commented Feb 10, 2022 •
edited
Loading

I will upgrade the installer/Makefile so it supports the new /lib/firmware layout for IPC4

We should all use the same scripts, laying out files in /lib/firmware/ is not a CI-specific task.

Probably off-topic.

@mengdonglin
Copy link
Contributor

@marc-hb Are you good to be the feature owner?

@marc-hb marc-hb self-assigned this Feb 14, 2022
@marc-hb
Copy link
Collaborator

marc-hb commented Feb 14, 2022

Yes and the plan is in #855

@marc-hb
Copy link
Collaborator

marc-hb commented Feb 16, 2022 •
edited
Loading

Sorry I got confused: the plan in #855 (PR #860) was for bugs #842 and #845, not for this.

... but I will have a look a this anyway tomorrow (I was focused on #860 and others)

@marc-hb
Copy link
Collaborator

marc-hb commented Feb 16, 2022 •
edited
Loading

I think we should get rid of this test. At the SOF level we simply have no control on what the request_firmware() loader does in the kernel. request_firmware does not provide any feedback either and never tells what it loaded and where from (which is IMHO a security problem, I digress). I mean it never tells which of multiple paths it picked https://www.kernel.org/doc/html/latest/driver-api/firmware/fw_search_path.html

The good news is ./sof-test/tools/sof-dump-status.py --fwpath is used only once in sof-test, only in that test. We can change to print community/ or a blank string so it mimics the SOF driver exactly. Callers can then hardcode anything in they want in front of that if they so desire.

@plbossart
Copy link
Member Author

we could also check that the request_firmware succeeds, but if that fails we'll also never see the boot complete stage, so no problem with removing this test.

@plbossart
Copy link
Member Author

@marc-hb I can't recall if we ever reached a consensus on this, and if we even have an issue to fix.

@marc-hb
Copy link
Collaborator

marc-hb commented Mar 8, 2022

I think people either agree to remove this test from every test plan where it is an issue, or they don't care. No one will miss it.

@thesofproject/test-maintainers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@marc-hb marc-hb

Labels
P1 Blocker bugs or important features
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@plbossart @mengdonglin @marc-hb