ext_manifest: ipc msg size and cavs lpro info #2893

abonislawski · 2020-04-29T08:22:00Z

This will add to ext_manifest ipc msg size and cavs lpro informations

src/include/kernel/ext_manifest.h

lgirdwood · 2020-05-25T10:40:56Z

src/include/kernel/ext_manifest.h

This cant be fixed size otherwise it's an ABI change. The number of elems needs to be variable size.

This is local variable, kernel will still use only .hdr.elem_size

ok, but we should use ARRAY_SIZE() instead of this.

hmm, we cant use ARRAY_SIZE() with enum. What about moving this define to enum list and there will be something like EXT_MAN_CONFIG_LAST_ELEM always at the end of enum? So this value will describe enum size and there will be no need to increase _NUM like now

@lgirdwood please check now

src/init/ext_manifest.c

src/include/kernel/ext_manifest.h

lgirdwood · 2020-06-01T09:44:27Z

@abonislawski do you have a PR for the kernel ?

abonislawski · 2020-06-01T11:34:14Z

Changed EXT_MAN_ELEM_CONFIG_DATA to 5 because there will be EXT_MAN_ELEM_DBG_ABI with 4

@lgirdwood no PR for kernel, need someone from kernel side to use this data.
As I know @ranj063 will need LPRO info for iccmax
not sure about ipc msg size

but thats not a blocker anyway

ktrzcinx · 2020-06-01T11:35:23Z

Changed EXT_MAN_ELEM_CONFIG_DATA to 5 because there will be EXT_MAN_ELEM_DBG_ABI with 4

@abonislawski Great, thanks

lgirdwood · 2020-06-11T10:18:26Z

@ranj063 @kv2019i can someone approve this on kernel side.

kv2019i · 2020-06-11T18:12:47Z

@lgirdwood wrote:

@ranj063 @kv2019i can someone approve this on kernel side.

Hmm, according to the old (but still effective) ABI process, we need to have kernel side update of headers merged at the same time and there is no kernel PR, so we can't progress.

It would seem this is a change, that as per the proposed new model, would qualify as something that can be merged without driver change, but alas the process update is not approved yet, so we should go with the old one still.

kv2019i · 2020-07-09T15:53:38Z

We have some piled ABI backlog. @plbossart @ranj063 @bardliao are you ok wtih moving this PR to the new ABI process? To me this seems ready to go and we do not need a matching kernel PR if we do the review here. But as this PR was started in "old times", I'd like to have an explicit ack from you. Normally a single kernel maintainer approval is ok to qualify.

plbossart · 2020-07-09T17:28:40Z

We have some piled ABI backlog. @plbossart @ranj063 @bardliao are you ok wtih moving this PR to the new ABI process? To me this seems ready to go and we do not need a matching kernel PR if we do the review here. But as this PR was started in "old times", I'd like to have an explicit ack from you. Normally a single kernel maintainer approval is ok to qualify.

go for it.

kv2019i

@lgirdwood we are good to go with this and we handle this in the new process. Kernel PR will come later when the feature is taken into use.

ranj063 · 2020-07-22T01:42:56Z

src/include/kernel/ext_manifest.h

@abonislawski while talking to @fredoh9 about his PR, we realized that since the elems array has a fixed size of EXT_MAN_CONFIG_LAST_ELEM, the array will have to populated with the right tokens to not parse invalid elems as CAVS_LPRO tokens. So I think the elem type should start with a 1 instead. What do you think?

@ranj063 yes, looks like a real issue. Thanks for spotting this, I will change to 1

I saw your change, I will update mine and test it again

@fredoh9 thanks

fredoh9 · 2020-07-27T16:44:18Z

src/include/kernel/ext_manifest.h

doesn't this structure have to be packed? and same question to const struct ext_man_config_data ext_man_config below

both of members are uint32_t so no issues but actually yes, it should, I will update

ranj063

@abonislawski @fredoh9 thinking more about this PR, I realized that we shouldnt be adding intel-specific types in the common layer for the ext manifest. These should be platform-specific in the FW and should be parsed in the platform-specific code in the kernel side as well. This way future platforms can also have their own set of elem types that wont be needed to be dealt with in current platforms,

lgirdwood · 2020-08-05T14:12:27Z

@abonislawski @fredoh9 ping - UUID is merged so we ABI merges happening now. Can you make the updates suggested by @ranj063 thanks

lgirdwood · 2020-08-10T13:03:51Z

@abonislawski ping

abonislawski · 2020-08-11T14:43:46Z

@ranj063 @lgirdwood sorry for late response, so in this case we want to move EXT_MAN_CONFIG_CAVS_LPRO to platform specific code?
In this case one simple EXT_MAN_ELEM_CONFIG_DATA for key-value fields will be not enough, we will need another elem like EXT_MAN_ELEM_CAVS_CONFIG_DATA which will contain only CAVS_LPRO, is that right?

ranj063 · 2020-08-11T15:21:57Z

@ranj063 @lgirdwood sorry for late response, so in this case we want to move EXT_MAN_CONFIG_CAVS_LPRO to platform specific code?
In this case one simple EXT_MAN_ELEM_CONFIG_DATA for key-value fields will be not enough, we will need another elem like EXT_MAN_ELEM_CAVS_CONFIG_DATA which will contain only CAVS_LPRO, is that right?

@abonislawski Im thinking that the CONFIG_DATA be key/value pairs up to a max number of elems. But whats written into the key, ex: CONFIG_CAVS_LPRO should be in platform specific code and the parsing for what the key stands for would be handled in the platform-specific code in the driver as well

To avoid lots of single value structures this will introduce config data element which will contain several simple token/value pairs At initial version it will contain IPC_MSG_SIZE elem which will inform kernel about maximum IPC msg size Signed-off-by: Adrian Bonislawski <adrian.bonislawski@linux.intel.com>

At initial version it will add CAVS_LPRO for CAVS platforms, OUTBOX_SIZE and INBOX_SIZE Signed-off-by: Adrian Bonislawski <adrian.bonislawski@linux.intel.com>

abonislawski · 2020-08-11T17:59:07Z

@ranj063 @fredoh9 PR updated

ranj063

LGTM now, @abonislawski . Thanks again!

abonislawski requested a review from ktrzcinx April 29, 2020 08:22

abonislawski requested review from a team, dbaluta, jajanusz, lbetlej, lgirdwood, mmaka1, plbossart and tlauda as code owners April 29, 2020 08:22

abonislawski changed the title ~~etx_manifest: ipc msg size and cavs lpro info~~ ext_manifest: ipc msg size and cavs lpro info Apr 29, 2020

ktrzcinx approved these changes Apr 29, 2020

View reviewed changes

lgirdwood requested changes May 7, 2020

View reviewed changes

src/include/kernel/ext_manifest.h Outdated Show resolved Hide resolved

src/include/kernel/ext_manifest.h Outdated Show resolved Hide resolved

abonislawski force-pushed the ext_man_data branch from 55ae488 to f43fb16 Compare May 13, 2020 08:15

lgirdwood requested changes May 18, 2020

View reviewed changes

src/include/kernel/ext_manifest.h Outdated Show resolved Hide resolved

abonislawski force-pushed the ext_man_data branch from f43fb16 to a58de06 Compare May 20, 2020 08:14

lgirdwood reviewed May 20, 2020

View reviewed changes

src/include/kernel/ext_manifest.h Outdated Show resolved Hide resolved

abonislawski force-pushed the ext_man_data branch from a58de06 to 04ecc0e Compare May 22, 2020 07:54

lgirdwood requested changes May 25, 2020

View reviewed changes

ktrzcinx reviewed May 27, 2020

View reviewed changes

src/include/kernel/ext_manifest.h Outdated Show resolved Hide resolved

abonislawski force-pushed the ext_man_data branch from 04ecc0e to d66c713 Compare June 1, 2020 07:19

lgirdwood approved these changes Jun 1, 2020

View reviewed changes

lgirdwood added the ABI ABI change involved label Jun 1, 2020

abonislawski force-pushed the ext_man_data branch from d66c713 to e166399 Compare June 1, 2020 11:23

lgirdwood added this to the ABI-3.17 milestone Jun 12, 2020

kv2019i approved these changes Jul 17, 2020

View reviewed changes

ranj063 approved these changes Jul 17, 2020

View reviewed changes

fredoh9 mentioned this pull request Jul 17, 2020

Support extended manifest config data for clock config thesofproject/linux#2297

Closed

ranj063 reviewed Jul 22, 2020

View reviewed changes

abonislawski force-pushed the ext_man_data branch from e166399 to 922005d Compare July 22, 2020 18:50

fredoh9 reviewed Jul 27, 2020

View reviewed changes

abonislawski force-pushed the ext_man_data branch from 922005d to 61fb733 Compare July 28, 2020 07:35

ranj063 requested changes Jul 30, 2020

View reviewed changes

abonislawski force-pushed the ext_man_data branch from 61fb733 to e3dad31 Compare August 11, 2020 17:27

abonislawski added 2 commits August 11, 2020 19:45

ext_manifest: add platform-specific config elem

07775da

At initial version it will add CAVS_LPRO for CAVS platforms, OUTBOX_SIZE and INBOX_SIZE Signed-off-by: Adrian Bonislawski <adrian.bonislawski@linux.intel.com>

abonislawski force-pushed the ext_man_data branch from e3dad31 to 07775da Compare August 11, 2020 17:57

ranj063 approved these changes Aug 11, 2020

View reviewed changes

lgirdwood approved these changes Aug 14, 2020

View reviewed changes

lgirdwood merged commit 3049956 into thesofproject:master Aug 14, 2020

abonislawski mentioned this pull request Sep 28, 2020

ext_man: prevent removal of cavs-specific element #3473

Merged

ext_manifest: ipc msg size and cavs lpro info #2893

ext_manifest: ipc msg size and cavs lpro info #2893

Uh oh!

Conversation

abonislawski commented Apr 29, 2020

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

abonislawski May 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lgirdwood commented Jun 1, 2020

Uh oh!

abonislawski commented Jun 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ktrzcinx commented Jun 1, 2020

Uh oh!

lgirdwood commented Jun 11, 2020

Uh oh!

kv2019i commented Jun 11, 2020

Uh oh!

kv2019i commented Jul 9, 2020

Uh oh!

plbossart commented Jul 9, 2020

Uh oh!

kv2019i left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranj063 left a comment

Choose a reason for hiding this comment

Uh oh!

lgirdwood commented Aug 5, 2020

Uh oh!

lgirdwood commented Aug 10, 2020

Uh oh!

abonislawski commented Aug 11, 2020

Uh oh!

ranj063 commented Aug 11, 2020

Uh oh!

abonislawski commented Aug 11, 2020

Uh oh!

ranj063 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

abonislawski May 27, 2020 •

edited

Loading

abonislawski commented Jun 1, 2020 •

edited

Loading