Skip to content

Pretend unused slots presence in dictionaries from extended manifest - #3691

Merged
lgirdwood merged 3 commits into
thesofproject:masterfrom
ktrzcinx:ext-man-dict-cnt
Jan 7, 2021
Merged

Pretend unused slots presence in dictionaries from extended manifest#3691
lgirdwood merged 3 commits into
thesofproject:masterfrom
ktrzcinx:ext-man-dict-cnt

Conversation

@ktrzcinx

@ktrzcinxktrzcinx commented Dec 15, 2020

Copy link
Copy Markdown
Member

Solution extracted from discussion from #3527

@lgirdwoodlgirdwood mentioned this pull request Dec 17, 2020
Closed
lgirdwood
lgirdwood reviewed Dec 17, 2020
View reviewed changes
Comment threadsrc/init/ext_manifest.c Outdated

@lgirdwoodlgirdwoodDec 17, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just wondering here if we can use a linker variable here for CONFIG_ELEM_COUNT as a way to automate this as it may be error prone for developers i.e. something cmake can pass into the linker pre-processor and have the linker set this based on the FW binary build ???

@xiulipanxiulipanDec 21, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ktrzcinx agreed with Liam, we can try to use some trick with enum like

Suggested change
sizeof(structconfig_elem) *CONFIG_ELEM_CNT,
sizeof(structconfig_elem) *(EXT_MAN_CONFIG_LAST_ELEM-1),

You do not need to update the EXT_MAN_CONFIG_LAST_ELEM or CONFIG_ELEM_CNT if we add a new type

xiulipan
xiulipan suggested changes Dec 21, 2020
View reviewed changes

@xiulipanxiulipan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we can keep EXT_MAN_*_LAST_ELEM and calculated the CNT with (LAST_ELEM - 1)
No need to update the CNT anymore if we add new enum.

Comment threadsrc/init/ext_manifest.c Outdated

@xiulipanxiulipanDec 21, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ktrzcinx agreed with Liam, we can try to use some trick with enum like

Suggested change
sizeof(structconfig_elem) *CONFIG_ELEM_CNT,
sizeof(structconfig_elem) *(EXT_MAN_CONFIG_LAST_ELEM-1),

You do not need to update the EXT_MAN_CONFIG_LAST_ELEM or CONFIG_ELEM_CNT if we add a new type

Comment threadsrc/platform/intel/cavs/ext_manifest.c Outdated
@ktrzcinx
ext_man: Remove dictionary elements counter from ABI for ext_man_conf…
6297906
…ig_data
This value shouldn't be included in any ABI header, because it may
vary between firmware configurations.
This change makes header file more similar to version provided by kernel.
Signed-off-by: Karol Trzcinski <karolx.trzcinski@linux.intel.com>
@ktrzcinx
ext_man: cavs: Always include EXT_MAN_CAVS_CONFIG_LPRO element to man…
e703310
…ifest
This value should be included to output image with proper value instead
of including only for some configurations. It will reduce dictionary
element counting complexity.
Signed-off-by: Karol Trzcinski <karolx.trzcinski@linux.intel.com>
@ktrzcinx
ext_man: Remove dictionary elements counter from ABI for ext_man_cavs…
7e9fd5d
…_config_data
This value shouldn't be included in any ABI header, because it may
vary between firmware configurations.
This change makes header file more similar to version provided by kernel.
Signed-off-by: Karol Trzcinski <karolx.trzcinski@linux.intel.com>
@ktrzcinx
ktrzcinx requested a review from xiulipanJanuary 4, 2021 07:27
xiulipan
xiulipan approved these changes Jan 5, 2021
View reviewed changes

@xiulipanxiulipan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM not. I am OK to have a more clear define for the COUNT.

lgirdwood
lgirdwood reviewed Jan 5, 2021
View reviewed changes
Comment threadsrc/include/kernel/ext_manifest.h
@lgirdwood

lgirdwood commented Jan 6, 2021

Copy link
Copy Markdown
Member

@ktrzcinx can you check internal CI, looks like tests are failing ?

@xiulipan

xiulipan commented Jan 7, 2021

Copy link
Copy Markdown
Contributor

@lgirdwood@ktrzcinx I re-check our discussion in #3527
I will trigger a new round test with GCC built FW binary to verify this change won't break GCC compiler.

@xiulipan

xiulipan commented Jan 7, 2021

Copy link
Copy Markdown
Contributor

@lgirdwood@ktrzcinx Good to go, simple on-device test did not show issue with GCC FW binary.

@ktrzcinx

ktrzcinx commented Jan 7, 2021

Copy link
Copy Markdown
MemberAuthor

@lgirdwood in internal CI single test is failing with message ERROR test_keyword_get_threshold(), unsupported sample width: 0, but it is unrelated with proposed change - system boot-up properly.

@lgirdwood

lgirdwood commented Jan 7, 2021

Copy link
Copy Markdown
Member

@zrombel good to merge, this error is unrelated ?

@zrombel

zrombel commented Jan 7, 2021

Copy link
Copy Markdown

It seemed unrelated. I run tests one again to be sure and now it's all green. Good to merge.

@lgirdwood
lgirdwood merged commit 5a1078d into thesofproject:masterJan 7, 2021
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lgirdwoodlgirdwoodlgirdwood approved these changes
@dbalutadbalutaAwaiting requested review from dbaluta
@jajanuszjajanuszAwaiting requested review from jajanusz
@lbetlejlbetlejAwaiting requested review from lbetlej
@mmaka1mmaka1Awaiting requested review from mmaka1
@plbossartplbossartAwaiting requested review from plbossart
+1 more reviewer
@xiulipanxiulipanxiulipan approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ktrzcinx@lgirdwood@xiulipan@zrombel