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.

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

ipc4: mixin: Restore back gain support #9038

Conversation

serhiy-katsyuba-intel
Copy link
Contributor

Restore back gain support which was accidentally removed during previous HIFI optimization/refactoring.

This PR implements gain for "generic" and "HIFI3" mixin versions. Implementation for "HIFI5" version is a TODO.

Restore gain support which was accidentally removed during previous
HIFI optimization/refactoring.

This commit only adds gain supports to "generic" mixin implementation.

Signed-off-by: Serhiy Katsyuba <serhiy.katsyuba@intel.com>
Restore gain support which was accidentally removed during previous
HIFI optimization/refactoring.

This commit only adds gain supports to "HIFI3" mixin implementation.

Signed-off-by: Serhiy Katsyuba <serhiy.katsyuba@intel.com>
serhiy-katsyuba-intel added a commit to serhiy-katsyuba-intel/sof that referenced this pull request Apr 12, 2024
*** DO NOT MERGE ***

Temporary force to use "generic" mixin impl just for a CI run to check
thesofproject#9038.

Signed-off-by: Serhiy Katsyuba <serhiy.katsyuba@intel.com>
@serhiy-katsyuba-intel serhiy-katsyuba-intel marked this pull request as ready for review April 12, 2024 15:46
@serhiy-katsyuba-intel
Copy link
Contributor Author

Separate draft PR tests "generic" impl #9039 and "HIFI3" impl #9040.

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.

@singalsu pls review
@serhiy-katsyuba-intel I have a suspicion that some of the gains may have been removed to improve MCPS as they were always 0dB. It probably makes sense to do the gain only if gain != 0dB to save the MCPS since mixin/mixout are used in lots of places. This may be best performed with a mix_s16() and a mix_s16_gain() and we decide which to use at process() time.

@serhiy-katsyuba-intel
Copy link
Contributor Author

@lgirdwood , if we choose to have separate mix_s16() and mix_s16_gain() we'll have to maintain 6 functions instead of 3. I though applying gain to mixin sources might be quite popular use case as without it there is a chance for saturation and so "gain feature" should be used almost always. Do we have any assumptions how typical is using "gain feature" when mixing? If it is not needed most of the time -- I can redo to have separate mix_sXX() and mix_sXX_gain() versions.

@lgirdwood
Copy link
Member

@lgirdwood , if we choose to have separate mix_s16() and mix_s16_gain() we'll have to maintain 6 functions instead of 3. I though applying gain to mixin sources might be quite popular use case as without it there is a chance for saturation and so "gain feature" should be used almost always. Do we have any assumptions how typical is using "gain feature" when mixing? If it is not needed most of the time -- I can redo to have separate mix_sXX() and mix_sXX_gain() versions.

@singalsu can you comment here on usage.

@serhiy-katsyuba-intel I know its 6 funcs, but its not really 6 different funcs, they will be the same in pairs except for the gain block (and this could be explained in the comment blocks). e.g. duplication of mix_s16() for MCPS reasons.

@abonislawski
Copy link
Member

If there are many use cases without gain then its better to separate them.

@lgirdwood @singalsu is someone able to do this also for HiFi5 files?

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.

Interesting. Digging into this, the gain support was added with remapping support with
commit 9fc0bad ("ipc4: add mixin/mixout channel remapping support"). This had special casing was IPC4_MIXIN_UNITY_GAIN. Then when remapping feature was removed, applying gain was removed as well.

The Linux driver and topologies are not using non-unity gains (there is assumption these are only used in remapping mode, but I guess that's the wrong assumption).

@serhiy-katsyuba-intel
Copy link
Contributor Author

The Linux driver and topologies are not using non-unity gains (there is assumption these are only used in remapping mode, but I guess that's the wrong assumption).

Thank you. So I will re-do to have separate mixing function implementations: with and without gain.

@lgirdwood
Copy link
Member

If there are many use cases without gain then its better to separate them.

@lgirdwood @singalsu is someone able to do this also for HiFi5 files?

Yep, @singalsu can look at Hifi5 in Q3

serhiy-katsyuba-intel added a commit to serhiy-katsyuba-intel/sof that referenced this pull request Apr 19, 2024
*** DO NOT MERGE ***

Temporary force to use "generic" mixin impl just for a CI run to check
thesofproject#9038.

Signed-off-by: Serhiy Katsyuba <serhiy.katsyuba@intel.com>
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