ASoC: SOF: add ext manifest 4 firmware support by bardliao · Pull Request #3176 · thesofproject/linux

bardliao · 2021-09-27T08:33:33Z

SOF can support a new firmware extension manifest from ABI version 4. This
patch adds extension manifest 4 parser.

Signed-off-by: Rander Wang rander.wang@intel.com

@plbossart @ujfalusi I am not sure can we use bitfields in extension manifest struct. I will modify it if it is not suitable.

plbossart

Confused on what parts are actually needed by the kernel.

bardliao · 2021-09-30T08:58:17Z

Confused on what parts are actually needed by the kernel.

Thanks @plbossart. I checked those elements and we don't need most of them.

ranj063 · 2021-10-08T16:03:50Z

+	uint32_t	ibs;		/* input buffer size (bytes) */
+	uint32_t	obs;		/* output buffer size (bytes) */
+	uint32_t	module_flags;	/* flags, res for future use */
+	uint32_t	cpc;		/* cycles per single run */


single run of what? sorry the comment is quite vague, could you please elaborate?

single run of what? sorry the comment is quite vague, could you please elaborate?

@RanderWang Could you comment on this?

it is related to schedule. In each schedule period, each module is scheduled to process a batch of sample data, generally 1ms data. Here single run means the time span that each module used.

then 'max cycles per scheduling period'?

then 'max cycles per scheduling period'?

Updated, thanks

ujfalusi · 2021-10-12T09:38:14Z

+	const struct sof_ext_manifest4_hdr *hdr;
+
+	hdr = (struct sof_ext_manifest4_hdr *)fw->data;
+	if (hdr->id == SOF_EXT_MAN4_MAGIC_NUMBER) {


hdr->id is magic, right?

hdr->id is magic, right?

Yes, hdr->id is a magic number

ujfalusi · 2021-10-12T09:44:35Z

+
+struct sof_ext_manifest4_hdr {
+	u32 id;
+	u32 len;


Length of what?

Length of what?

length of extension manifest. I will add a comment here.

ujfalusi · 2021-10-12T09:49:34Z

+	fw_offset = hdr->len;
+
+	fw_header = (struct sof_man4_fw_binary_header *)(fw->data + fw_offset +
+							 SOF_MAN4_FW_HDR_OFFSET);


we have so many offsets that it is hard to follow, can you use hdr->len instead of fw_offset, it is only used here and then returned at the end.
between hdr->len and SOF_MAN4_FW_HDR_OFFSET we have grey area? 0x2000 is a lot of wasted space.

we have so many offsets that it is hard to follow, can you use hdr->len instead of fw_offset, it is only used here and then returned at the end.

Will use hdr->len instead of fw_offset.

between hdr->len and SOF_MAN4_FW_HDR_OFFSET we have grey area? 0x2000 is a lot of wasted space.

I checked the firmware binary, there are some data followed by a lot of 0s in the SOF_MAN4_FW_HDR_OFFSET area. I believe that area is used by some struct.

This 0x2000 has been a constant for a long time: thesofproject/sof@8885b465df53

To test thesofproject/sof#4889 I ran the latest tools/sof_ri_info/sof_ri_info.py on all platform binaries since v1.7 and it parsed all of them. Example:

Reading SOF ri image sof-bin/v1.9.x/sof-v1.9/intel-signed/sof-tgl-h.ri File size 0x68300 (426752) 0x0 Looking for Extended Manifest 0x0 Extended Manifest (XMan) 0x300 Looking for CSE Manifest 0x300 CSE Manifest ($CPD) 0x308 # of entries 3 0x314 Reading CSE Manifest entry 0... 0x32c ... CSE Entry name ADSP.man file offset 0x35c length 1124 ... 0x2300 ADSP Manifest ($AM1) 0x2334 Module Entry signature found ($AME) 0x23a8 Module Entry signature found ($AME) Parsing finished SOF Binary sof-bin/v1.9.x/sof-v1.9/intel-signed/sof-tgl-h.ri size 0x68300 Extended Manifest ver 1.0.0 length 768 CSE Manifest ver 0x102 checksum 0x0 partition name ADSP ADSP.man (CSS Manifest) type 0x4 file offset 0x35c hdr_len 900 ver 0x21000 date 2021/09/23 Rsvd0 0x1 Modulus size (dwords) 96 d3 72 92 99 4e b9 cd 67 ... 5d 62 4a 80 96 31 f8 b5 (TGL Intel prod key) Exponent size (dwords) 1 .... .... cavs0015 (ADSP Manifest) file offset 0x2300 name ADSPFW build ver 1.9.0.1 feature mask 0xffff image flags 0x0 HW buffers base address 0x0 length 0x0 Load offset 0x30000

kv2019i

If I compare this to include/sound/sof/ext_manifest.h (for v3), this is quite specific (for example many fields for ROM only usage). I wonder if this should be hidden as a vendor manifest instead, e.g. Intel specific manifest.

If the amount of data SOF driver needs to get from manifest stays low, we could have a function interface to get data (like FW version and max IPC MSG size) and this could be coded in a vendor specific manifest structure.

Of course, if SOF core needs to get more data for IPC4 (like the module description data), then this become more complex. At minimum in this case, we should have clear documentation which of the manifest fields are relevant to driver and describe them better (like in include/sound/sof/ext_manifest.h -> all fields documented).

kv2019i

Closer but still a few opens

kv2019i · 2021-10-27T11:39:02Z

+	dev_dbg(sdev->dev, " fw %s: header length %x, module num %d", fw_header->name,
+		fw_header->len, fw_header->num_module_entries);
+
+	fm_entry = (struct sof_man4_module *)((u8 *)fw_header + fw_header->len);


You don't need fm_entry here yet, so this could be before L415. This is really the first place you need fm_entry. This will not impact the sizeof calculations (L401 et al, this just need to know the type).

@bardliao , can you address @kv2019i's comment?

@bardliao , can you address @kv2019i's comment?

Yes, I moved fm_entry = (struct sof_man4_module *)((u8 *)fw_header + fw_header->len); and the remaining calculation right before fm_config = (struct sof_man4_module_config *)(fm_entry + fw_header->num_module_entries);. Or are you suggesting that moving fm_entry only but not the remaining calculation?
Like

remaining -= fw_header->len; if (remaining < fw_header->num_module_entries * sizeof(*fm_entry)) { dev_err(sdev->dev, "Invalid num_module_entries %d\n", fw_header->num_module_entries); return -EINVAL; } fm_entry = (struct sof_man4_module *)((u8 *)fw_header + fw_header->len); fm_config = (struct sof_man4_module_config *)(fm_entry + fw_header->num_module_entries);

The reason I kept the remaining calculation right after pointer assignments was that I think it is more readable.

I'm good with this. Strictly speaking, you only consume/access the data when the pointer is dereferences (at L415), so in that sense more correctly place to adjust remaining would be L416. But I think this is now close enough and may be easier to read.

kv2019i · 2021-10-27T11:49:42Z

+		memcpy(&module_entry->man4_module_entry, fm_entry, sizeof(*fm_entry));
+
+		if (fm_entry->cfg_count)
+			module_entry->bss_size = fm_config[fm_entry->cfg_offset].is_bytes;


You are correct @bardliao , keep fm_config init in the original place.

This segment is difficult to follow as each fm_entry will also access fm_config, so you are consuming data from two places in the binary in this loop. So it's not so clear where to actually modify remaining as you are going back and forth in the blob. And you can do all the checks outside the loop either as you need to parse each fm_entry to see how many fm_config entries is has.

I think the current code is probably the best compromise.

kv2019i

Thanks @bardliao , looks good now! I have one style related issue for snd_sof_fw_ext_man_parse() implementation, see code alternative in the comment. But that's not a blocking concern, so I'm ok to approve.

ujfalusi

@bardliao , I only have few points to clarify, other than that it looks pretty good.

ujfalusi · 2021-10-29T12:36:20Z

+	dev_dbg(sdev->dev, " fw %s: header length %x, module num %d", fw_header->name,
+		fw_header->len, fw_header->num_module_entries);
+
+	fm_entry = (struct sof_man4_module *)((u8 *)fw_header + fw_header->len);


@bardliao , can you address @kv2019i's comment?

kv2019i · 2021-11-02T12:31:11Z

+	dev_dbg(sdev->dev, " fw %s: header length %x, module num %d", fw_header->name,
+		fw_header->len, fw_header->num_module_entries);
+
+	fm_entry = (struct sof_man4_module *)((u8 *)fw_header + fw_header->len);


I'm good with this. Strictly speaking, you only consume/access the data when the pointer is dereferences (at L415), so in that sense more correctly place to adjust remaining would be L416. But I think this is now close enough and may be easier to read.

ujfalusi

@bardliao, looks good, thank you for the updates.

bardliao · 2021-11-05T11:57:48Z

@plbossart I addressed all comments except the endianness issue. I need more discussion on the endianness issue.

plbossart · 2021-11-05T15:49:12Z

@plbossart I addressed all comments except the endianness issue. I need more discussion on the endianness issue.

Sorry I wasn't asking for a change, it would be fine to just have a comment that this assumes little-endian order for compatibility with Windows firmware. That way upstream reviewers will know it's by-design and not a mistake.

bardliao · 2021-11-08T08:16:05Z

@plbossart I addressed all comments except the endianness issue. I need more discussion on the endianness issue.

Sorry I wasn't asking for a change, it would be fine to just have a comment that this assumes little-endian order for compatibility with Windows firmware. That way upstream reviewers will know it's by-design and not a mistake.

Oh, I see. Adding a sentence Note that not all items in the struct are 32 bits length, and we assume the firmware is little-endian. at the beginning of ext_manifest4.h now.

ranj063 · 2021-11-15T02:55:30Z

+#define SOF_MAN4_DEFAULT_HASH_SHA256_LEN 32
+#define SOF_MAN4_SEGMENT_NUM 3
+#define SOF_MAN4_MODULE_PARAMS_NUM 4
+#define SOF_MAN4_FW_HDR_OFFSET 0x2000


@bardliao the FW_HDR_OFFSET is not the same for cavs1.5 abd cavs1.8. Can we please abstract this?

@bardliao the FW_HDR_OFFSET is not the same for cavs1.5 abd cavs1.8. Can we please abstract this?

Thanks @ranj063. I did some change, could you review it? Thanks.

ranj063 · 2021-11-15T16:33:30Z

+#define SOF_MAN4_DEFAULT_HASH_SHA256_LEN 32
+#define SOF_MAN4_SEGMENT_NUM 3
+#define SOF_MAN4_MODULE_PARAMS_NUM 4
+#define SOF_MAN4_FW_HDR_OFFSET 0x2000


@bardliao thanks but you didnt use the struct defined in the first patch?

@bardliao thanks but you didnt use the struct defined in the first patch?

Yes, I did. See snd_sof_fw_ext_man4_parse

static int snd_sof_fw_ext_man4_parse(struct snd_sof_dev *sdev, const struct firmware *fw) { struct snd_sof_pdata *sof_pdata = sdev->pdata; const struct sof_dev_desc *desc = sof_pdata->desc; const struct snd_sof_ext_man_pdata *ext_man_pdata = desc->ext_man_pdata; ... if (ext_man_pdata) fw_hdr_offset = ext_man_pdata->fw_hdr_offset; else fw_hdr_offset = SOF_MAN4_FW_HDR_OFFSET; ... }

Some extension manifest datas are platform dependency. Add a new struct allow us to define those datas for each platform. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

SOF can support a new firmware extension manifest from ABI version 4. This patch adds extension manifest 4 parser. Signed-off-by: Rander Wang <rander.wang@intel.com>

bardliao requested review from RanderWang, kv2019i, plbossart, ranj063 and ujfalusi September 27, 2021 08:33

bardliao requested review from dbaluta, juimonen, keyonjie, lgirdwood, libinyang and lyakh as code owners September 27, 2021 08:33

bardliao force-pushed the manifest4 branch 2 times, most recently from 76d1195 to 6194012 Compare September 29, 2021 05:25

bardliao changed the title ~~ASoC: SOF: add manifest 4 firmware support~~ ASoC: SOF: add ext manifest 4 firmware support Sep 29, 2021

RanderWang reviewed Sep 29, 2021

View reviewed changes

Comment thread include/sound/sof/ext_manifest4.h Resolved

ujfalusi requested changes Sep 29, 2021

View reviewed changes

Comment thread include/sound/sof/ext_manifest4.h Resolved

plbossart reviewed Sep 29, 2021

View reviewed changes

Comment thread include/sound/sof/ext_manifest4.h Resolved

Comment thread include/sound/sof/ext_manifest4.h Resolved

Comment thread include/sound/sof/ext_manifest4.h Resolved

Comment thread include/sound/sof/ext_manifest4.h Resolved

ranj063 reviewed Sep 29, 2021

View reviewed changes

Comment thread sound/soc/sof/loader.c Resolved

Comment thread sound/soc/sof/loader.c Resolved

bardliao force-pushed the manifest4 branch from 6194012 to 8d7a1ce Compare September 30, 2021 08:48

bardliao force-pushed the manifest4 branch from 8d7a1ce to 6a380dd Compare September 30, 2021 09:42

bardliao requested review from RanderWang, plbossart, ranj063 and ujfalusi October 7, 2021 06:51

ranj063 reviewed Oct 8, 2021

View reviewed changes

bardliao force-pushed the manifest4 branch from 6a380dd to d1c5726 Compare October 12, 2021 10:12

ujfalusi requested changes Oct 12, 2021

View reviewed changes

kv2019i requested changes Oct 12, 2021

View reviewed changes

Comment thread include/sound/sof/ext_manifest4.h Resolved

Comment thread include/sound/sof/ext_manifest4.h Resolved

bardliao force-pushed the manifest4 branch from d1c5726 to 16d7967 Compare October 13, 2021 10:20

bardliao force-pushed the manifest4 branch from d6bbbd0 to b2838e8 Compare October 26, 2021 02:09

bardliao requested a review from kv2019i October 27, 2021 08:41

bardliao force-pushed the manifest4 branch from b2838e8 to 03fa53b Compare October 27, 2021 08:45

kv2019i requested changes Oct 27, 2021

View reviewed changes

bardliao force-pushed the manifest4 branch from 03fa53b to ea2e3e8 Compare October 28, 2021 02:11

bardliao requested a review from kv2019i October 29, 2021 05:03

kv2019i previously approved these changes Oct 29, 2021

View reviewed changes

Comment thread sound/soc/sof/loader.c Resolved

ujfalusi requested changes Oct 29, 2021

View reviewed changes

bardliao dismissed kv2019i’s stale review via 9b0dc33 November 1, 2021 02:12

bardliao force-pushed the manifest4 branch from ea2e3e8 to 9b0dc33 Compare November 1, 2021 02:12

bardliao requested review from kv2019i and ujfalusi November 2, 2021 09:08

kv2019i previously approved these changes Nov 2, 2021

View reviewed changes

ujfalusi previously approved these changes Nov 4, 2021

View reviewed changes

plbossart requested changes Nov 4, 2021

View reviewed changes

bardliao dismissed stale reviews from ujfalusi and kv2019i via b1c2a5a November 5, 2021 11:55

bardliao force-pushed the manifest4 branch from 9b0dc33 to b1c2a5a Compare November 5, 2021 11:55

bardliao force-pushed the manifest4 branch from b1c2a5a to 83a6f82 Compare November 8, 2021 08:12

ranj063 reviewed Nov 15, 2021

View reviewed changes

bardliao force-pushed the manifest4 branch from 83a6f82 to 41951df Compare November 15, 2021 10:05

ranj063 reviewed Nov 15, 2021

View reviewed changes

bardliao force-pushed the manifest4 branch from 41951df to 68d2c67 Compare November 16, 2021 07:25

bardliao and others added 2 commits November 17, 2021 19:44

ASoC: SOF: add struct snd_sof_ext_man_pdata into struct sof_dev_desc

da1c3fc

Some extension manifest datas are platform dependency. Add a new struct allow us to define those datas for each platform. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

ASoC: SOF: add ext manifest 4 firmware support

6a9cf32

SOF can support a new firmware extension manifest from ABI version 4. This patch adds extension manifest 4 parser. Signed-off-by: Rander Wang <rander.wang@intel.com>

bardliao force-pushed the manifest4 branch from 68d2c67 to 6a9cf32 Compare November 17, 2021 11:46

bardliao closed this Dec 17, 2021

Conversation

bardliao commented Sep 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bardliao commented Sep 30, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ujfalusi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bardliao commented Sep 27, 2021 •

edited

Loading