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

basefw: add support for fw config query for context save #8428

Merged
merged 2 commits into from
Nov 24, 2023
Merged

basefw: add support for fw config query for context save #8428

merged 2 commits into from
Nov 24, 2023

Conversation

RanderWang
Copy link
Collaborator

@RanderWang RanderWang commented Nov 1, 2023 •
edited
Loading

If CONFIG_ADSP_IMR_CONTEXT_SAVE is enabled, base fw will report fw_context_save is supported to host

kernel pr: thesofproject/linux#4680

@RanderWang RanderWang requested a review from ujfalusi November 1, 2023 05:17
@RanderWang RanderWang mentioned this pull request Nov 1, 2023
Merged
ujfalusi
ujfalusi requested changes Nov 1, 2023
View reviewed changes
src/audio/base_fw.c Outdated Show resolved Hide resolved
@RanderWang
Copy link
Collaborator Author

on kernel side, I change the default value of fw_context_save on TGL & MTL to ture, unless fw overrides it

btian1
btian1 approved these changes Nov 2, 2023
View reviewed changes
Copy link
Contributor

@btian1 btian1 left a comment

Choose a reason for hiding this comment

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

just a suggestion in comments:
If CONFIG_ADSP_IMR_CONTEXT_SAVE is enabled, base fw will report
fw_context_save is supported to driver. Driver will assume fw doesn't
support this feature if it is not reported.

@ujfalusi
Copy link
Contributor

ujfalusi commented Nov 2, 2023

just a suggestion in comments: If CONFIG_ADSP_IMR_CONTEXT_SAVE is enabled, base fw will report fw_context_save is supported to driver. Driver will assume fw doesn't support this feature if it is not reported.

The assumption is that the context save is supported if IMR is supported. We need negative flagging for this.

lyakh
lyakh reviewed Nov 2, 2023
View reviewed changes
src/audio/base_fw.c Outdated Show resolved Hide resolved
@RanderWang
Copy link
Collaborator Author

just a suggestion in comments: If CONFIG_ADSP_IMR_CONTEXT_SAVE is enabled, base fw will report fw_context_save is supported to driver. Driver will assume fw doesn't support this feature if it is not reported.

The assumption is that the context save is supported if IMR is supported. We need negative flagging for this.

@ujfalusi how about ID now ? We don't need negative flag sine the default value in kernel is true, not false.

lgirdwood
lgirdwood reviewed Nov 3, 2023
View reviewed changes
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.

Just need to align new IDs

src/include/ipc4/base_fw.h Outdated
/* Max config parameter id */
IPC4_MAX_FW_CFG_PARAM = IPC4_FW_CFG_PARAMS_COUNT - 1,
IPC4_MAX_FW_CFG_PARAM = IPC4_FW_CONTEXT_SAVE - 1,
Copy link
Member

Choose a reason for hiding this comment

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

@RanderWang @ujfalusi @mwasko @marcinszkudlinski to be on the safe side - can we add our new Intel reserved IDs as part of this PR and I think its going to be better if we also assign the numbers when updating this enum. i.e there is no ambiguos enum, developers should always see

IPC4_SOME_ENUM = NUMBER,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good idea! I use 100, big enough for 10 years.

Copy link
Member

Choose a reason for hiding this comment

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

good idea! I use 100, big enough for 10 years.

be careful, our industry has a bad experinces with quotes like these ;)

@RanderWang RanderWang force-pushed the lib_reload_config branch 2 times, most recently from 456d157 to 1ff2579 Compare November 6, 2023 06:13
@RanderWang
Copy link
Collaborator Author

reserve 100 for this config

@RanderWang
Copy link
Collaborator Author

@lgirdwood sync latest config from ref fw.

@RanderWang RanderWang force-pushed the lib_reload_config branch 2 times, most recently from 6b78836 to ffb1746 Compare November 13, 2023 09:08
@RanderWang
Copy link
Collaborator Author

@lgirdwood I reserved 3 id, how about it ? Thanks!

lgirdwood
lgirdwood reviewed Nov 13, 2023
View reviewed changes
src/include/ipc4/base_fw.h Show resolved Hide resolved
src/include/ipc4/base_fw.h Outdated
/* Intel reserved */
IPC4_INTEL_RESRVED_29 = 29,
IPC4_INTEL_RESRVED_30 = 30,
IPC4_INTEL_RESRVED_31 = 31,
Copy link
Member

Choose a reason for hiding this comment

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

If we are using 31 we can name it using its real name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure

@RanderWang
Copy link
Collaborator Author

@lgirdwood update the ID name

lgirdwood
lgirdwood approved these changes Nov 14, 2023
View reviewed changes
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.

@mwasko this PR also reserves the in-use IDs until some process is created to align.

@lgirdwood
Copy link
Member

@ujfalusi pls review, does kernel align or does it require a similar PR ?

kv2019i
kv2019i reviewed Nov 14, 2023
View reviewed changes
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

A few comments on the code documentation, otherwise looks good.

src/include/ipc4/base_fw.h Outdated Show resolved Hide resolved
src/include/ipc4/base_fw.h Outdated Show resolved Hide resolved
@RanderWang
base_fw: sync config from ref fw
1b80c83 
Update the basefw config query.

Signed-off-by: Rander Wang <[email protected]>
@RanderWang RanderWang force-pushed the lib_reload_config branch 2 times, most recently from a603a93 to c488070 Compare November 15, 2023 01:22
@lgirdwood
Copy link
Member

@wszypelt @lrudyX not expecting an ID update to break, good to merge ? Thanks !

@lgirdwood lgirdwood added this to the v2.8 milestone Nov 17, 2023
kv2019i
kv2019i approved these changes Nov 20, 2023
View reviewed changes
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

@mwasko @mmaka1 good to go (this reserves 29 for the new cfg id)?

@kv2019i
Copy link
Collaborator

kv2019i commented Nov 20, 2023

@RanderWang can you check the CI (and/or repush if this is transient as probably is the case)?

@RanderWang
Copy link
Collaborator Author

SOFCI TEST

@RanderWang
Copy link
Collaborator Author

Will update it according to ref fw which has merged a config

@RanderWang
basefw: add support for fw config query for context save
7b2cd87 
If CONFIG_ADSP_IMR_CONTEXT_SAVE is enabled, base fw will report
fw_context_save is supported to host. Driver will assume fw doesn't
support this feature if it is not reported.

Signed-off-by: Rander Wang <[email protected]>
@RanderWang
Copy link
Collaborator Author

@lgirdwood @kv2019i update PR to follow ref fw https://github.com/intel-innersource/drivers.audio.firmware.cavs-ace/pull/1533

@kv2019i
Copy link
Collaborator

kv2019i commented Nov 24, 2023

SOFCI TEST

@lgirdwood lgirdwood merged commit 472458e into thesofproject:main Nov 24, 2023
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lyakh lyakh lyakh left review comments

@tmleman tmleman tmleman approved these changes

@kv2019i kv2019i kv2019i approved these changes

@abonislawski abonislawski abonislawski approved these changes

@btian1 btian1 btian1 approved these changes

@ujfalusi ujfalusi ujfalusi approved these changes

@lgirdwood lgirdwood lgirdwood approved these changes

@marcinszkudlinski marcinszkudlinski Awaiting requested review from marcinszkudlinski marcinszkudlinski is a code owner

@pblaszko pblaszko Awaiting requested review from pblaszko pblaszko is a code owner

@plbossart plbossart Awaiting requested review from plbossart plbossart is a code owner

@mmaka1 mmaka1 Awaiting requested review from mmaka1 mmaka1 is a code owner

@lbetlej lbetlej Awaiting requested review from lbetlej lbetlej is a code owner

@dbaluta dbaluta Awaiting requested review from dbaluta dbaluta is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.8
Development

Successfully merging this pull request may close these issues.
8 participants
@RanderWang @ujfalusi @lgirdwood @kv2019i @lyakh @btian1 @abonislawski @tmleman