Skip to content

Conversation

@abonislawski
Copy link
Member

This patch will allow to keep cavs-specific elements

This patch will allow to keep cavs-specific elements

Signed-off-by: Adrian Bonislawski <adrian.bonislawski@linux.intel.com>
@fredoh9
Copy link
Contributor

fredoh9 commented Sep 25, 2020

I can see ext_man_cavs_config in .fw_metadata now.

 .fw_metadata   0x220001a0       0x28 src/platform/intel/cavs/libcavs_ext_manifest.a(ext_manifest.c.o)
                0x220001a0                ext_man_cavs_config

@fredoh9
Copy link
Contributor

fredoh9 commented Sep 25, 2020

I have a question on .fw_ready section as well.
I used to see ext_man_config in fw_ready as well. I only see up to user_abi_version. Can we include ext_man_config and ext_man_cavs_config in fw_ready section. This doesn't mandate to parse that data.

.fw_ready       0xbe05e518      0x284
 *(.fw_ready)
 .fw_ready      0xbe05e518       0x6c CMakeFiles/sof.dir/src/platform/intel/cavs/platform.c.o
 *(.fw_ready_metadata)
 .fw_ready_metadata
                0xbe05e584      0x190 CMakeFiles/sof.dir/src/platform/intel/cavs/platform.c.o
 .fw_ready_metadata
                0xbe05e714       0x5c src/ipc/libdata_structs.a(cc_version.c.o)
                0xbe05e714                cc_version
 .fw_ready_metadata
                0xbe05e770       0x1c src/ipc/libdata_structs.a(probe_support.c.o)
                0xbe05e770                probe_support
 .fw_ready_metadata
                0xbe05e78c       0x10 src/ipc/libdata_structs.a(user_abi_version.c.o)
                0xbe05e78c                user_abi_version

And 2nd question, parsing extended manifest from linux driver is broken. firmware doesn't contain extended manifest

It reads wrong MAGIC NUMBER and return failure. We should get 0x6e614d58 but 0x44504324. After inspection of the data, it has totally corrupted data. (I'm testing with CML Helios)

/* In ASCII `XMan` */
#define SOF_EXT_MAN_MAGIC_NUMBER	0x6e614d58

From my kernel debugging log,

[    2.727552] sof-audio-pci 0000:00:1f.3: request_firmware intel/sof/community/sof-cml.ri successful
[    2.727554] Error: MAGIC NUMBER is wrong, 0x44504324
[    2.727559] sof-audio-pci 0000:00:1f.3: magic=0x44504324 full_size=3 header_size=3507486977 header_version=50534441
[    2.727562] sof-audio-pci 0000:00:1f.3: remaining=787480322 ext_man_size=0
[    2.727564] sof-audio-pci 0000:00:1f.3: firmware doesn't contain extended manifest
[    2.727567] sof-audio-pci 0000:00:1f.3: sof_probe_continue: call snd_sof_run_firmware

@ktrzcinx
Copy link
Member

ktrzcinx commented Sep 28, 2020

@fredoh9

  1. Can we include ext_man_config and ext_man_cavs_config in fw_ready section

Why? We should rather prepare to removing data from there, it's why data from fw_ready/fw_ready_metadata has been ported to ext_manifest to make it fully functional. Adding compile-time known elements to fw_ready_metadata is deprecated (leads to unnecessary FW binary growth). Some exception may be, when something is not known at compilation time, eg. some HW specific staff, then fw_ready_metadata may be right place for such a thing.

  1. Error: MAGIC NUMBER is wrong, 0x44504324

This number it's $CPD in ascii, did you cherry-pick this patch: #2957 before FW compilation? List of included ext_man elements can be found then in rimage output logs, something like:

Firmware manifest completed!
Extended manifest found module, type: 0x0001 size: 0x01A0 ( 416) offset: 0x0000
Extended manifest found module, type: 0x0000 size: 0x0050 (  80) offset: 0x01A0
Extended manifest found module, type: 0x0002 size: 0x0070 ( 112) offset: 0x01F0
Extended manifest found module, type: 0x0003 size: 0x0030 (  48) offset: 0x0260
Extended manifest found module, type: 0x0004 size: 0x0020 (  32) offset: 0x0290
Extended manifest found module, type: 0x0005 size: 0x0020 (  32) offset: 0x02B0
Extended manifest saved to file sof-tgl.ri.xman size 0x02E0 (736) bytes

Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

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

@abonislawski Not following the commit message. This looks like we are adding config to a linker section (which one) for parsing by the kernel at FW load ?
@kv2019i @fredoh9 are all the kernel pieces in place ?

@abonislawski
Copy link
Member Author

abonislawski commented Sep 28, 2020

@lgirdwood we are adding EXTERN info, to inform linker that this variable is used and prevent from deleting unused variables because in fact ext manifest structs are not used anywhere and linker will not include them.
Currently this works with the same way (EXTERN(ext_man_fw_ver)) and Im adding another extern because introduced cavs part of ext manifest is compiled into another lib.
There is no need to add more configs to linker, for example if we will add more elements to cavs part of ext manifest

@lgirdwood
Copy link
Member

@abonislawski sorry, I was asking why we are adding this data (who will use it). My understanding is that @kv2019i would like to integrate the kernel parts of the extended manifest after v1.6 (so this may pend on a kernel patch too) ?

@abonislawski
Copy link
Member Author

@lgirdwood this PR is more like fix for this PR #2893 to work properly, we can merge this anytime we want because ext_manifest is not included without PR #2957 (this one is waiting for a proper time)

@fredoh9
Copy link
Contributor

fredoh9 commented Sep 28, 2020

I wasn't clear about FW_READY metadata vs extended manifest. Ever since i saw firmware doesn't contain extended manifest, I was referring FW_READY metadata also part of ext manifest.

Without PR #2957, if ext manifest not included, then thesofproject/linux#2434 is not v1.6 release blocker. I agree, it won't break any kernel driver. I will remove "DON'T merge this PR yet" comment.

@lgirdwood lgirdwood merged commit 96474cd into thesofproject:master Sep 29, 2020
@lgirdwood
Copy link
Member

@abonislawski ok, whats the plan for #2957 ? @kv2019i is now good or extended manifest ?

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.

4 participants