Skip to content

IPC4: simplify pcm conversion function and add LSB & MSB support - #7959

Merged
kv2019i merged 5 commits into
thesofproject:mainfrom
RanderWang:lsb_support
Aug 22, 2023
Merged

IPC4: simplify pcm conversion function and add LSB & MSB support#7959
kv2019i merged 5 commits into
thesofproject:mainfrom
RanderWang:lsb_support

Conversation

@RanderWang

@RanderWangRanderWang commented Jul 18, 2023

Copy link
Copy Markdown
Collaborator

Currently cSOF only supports S24_LE, now add S24_BE to support sample type IPC4_TYPE_MSB_INTEGER

@RanderWang
RanderWang requested a review from kv2019iJuly 18, 2023 07:01
kv2019i
kv2019i requested changes Jul 18, 2023
View reviewed changes

@kv2019ikv2019i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I like the approach to move the DAI-specific overrides to copier, but I think the confusion of what LSB/MSB means is still causing problems here. LE/BE (endianess) is not the same thing at all and we shouldn't mix these. Please see comments inline.

Comment threadsrc/audio/Kconfig Outdated
Comment threadsrc/audio/pcm_converter/pcm_converter_hifi3.c Outdated
btian1
btian1 reviewed Jul 19, 2023
View reviewed changes
Comment threadsrc/include/sof/audio/audio_stream.h Outdated
@RanderWang
RanderWangforce-pushed the lsb_support branch 2 times, most recently from 493dd4c to 2a5e466CompareJuly 20, 2023 09:14
kv2019i
kv2019i approved these changes Jul 20, 2023
View reviewed changes

@kv2019ikv2019i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks @RanderWang , looks good now!

Comment threadsrc/include/sof/audio/audio_stream.h Outdated
Comment threadsrc/audio/Kconfig Outdated
@RanderWang
RanderWang requested a review from ranj063July 21, 2023 02:46
btian1
btian1 reviewed Jul 25, 2023
View reviewed changes
Comment threadsrc/include/sof/audio/audio_stream.h
lyakh
lyakh reviewed Jul 26, 2023
View reviewed changes
Comment threadsrc/audio/copier/copier_generic.c Outdated
lgirdwood
lgirdwood requested changes Jul 31, 2023
View reviewed changes

@lgirdwoodlgirdwood left a comment

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.

New enum values need to go at end and align with kernel.

Comment threadsrc/include/ipc/stream.h
@lgirdwoodlgirdwood added the ABI ABI change involved label Jul 31, 2023
@mengdonglinmengdonglin mentioned this pull request Aug 9, 2023
Closed
@mengdonglin

mengdonglin commented Aug 9, 2023

Copy link
Copy Markdown
Collaborator

Part of fixes for #7697

@lgirdwood

lgirdwood commented Aug 9, 2023

Copy link
Copy Markdown
Member

SOFCI TEST

@RanderWang
RanderWangforce-pushed the lsb_support branch 2 times, most recently from c1c515a to acbdd82CompareAugust 17, 2023 03:21
@RanderWang

RanderWang commented Aug 17, 2023

Copy link
Copy Markdown
CollaboratorAuthor

sorry, I am still checking the error

@RanderWang
RanderWang requested review from aiChaoSONG and removed request for ranj063August 21, 2023 01:06
@RanderWang
RanderWangforce-pushed the lsb_support branch 2 times, most recently from 36916ab to c96bb37CompareAugust 21, 2023 05:25
@RanderWang
audio: add fixup for S24_3LE
010f028
It is missed in kconfig.
Signed-off-by: Rander Wang <rander.wang@intel.com>
@RanderWang
ipc4: add S24_4LE_MSB format support
c2349c7
For sample format with 24 bits valid sample bit and 32 bits container,
valid sample is at msb 24bits if IPC4_TYPE_MSB_INTEGER is set.
Signed-off-by: Rander Wang <rander.wang@intel.com>
@RanderWang
pcm:_converter: simplify the pcm conversion algorithm
d7e09f8
Remove gateway type and direction for pcm conversion check and will move
them to copier module. This will make pcm conversion check more general
and simple. Also remove s16/c32 for gateway since it is never used.
Signed-off-by: Rander Wang <rander.wang@intel.com>
@RanderWang
topology2: change the sample type for Linux audio system
96ed600
At first the sample type is set to MSB_INTEGER to follow windows
settings, but actually we use LSB_INTERGER type such as S24_4LE.
Now change the default sample type to LSB_INTERGER to align with
FW usage. For DAI copier we need to use MSB_INTERGER for hardware
requirement. Currently sample type only affect s24/c32 case, so only
change sample type in dai for this format config.
FW will use sample type to choose correct format conversion
function and can deal with Windows audio stream correctly with MSB
s24/c32 format.
out_fmt_cfg is redefined for a alsa-lib bug. Alsa-lib will first process
out_fmt_cfg = '$[($out_channels | ($out_valid_bit_depth * 256)) |
($out_sample_type * 65536)]' in base class and then deal with
out_sample_type, so error is reported. Now first define out_sample_type
and then out_fmt_cfg, everything works.
Signed-off-by: Rander Wang <rander.wang@intel.com>
@RanderWang
copier: adjust valid format based on sample type
f7956e4
Adjust valid format in copier for some types of dai gateway which
need to use MSB type.
Currently sample type only affect the copier module so we don't do
it in audio_stream_fmt_conversion.
Signed-off-by: Rander Wang <rander.wang@intel.com>
@lgirdwood

lgirdwood commented Aug 21, 2023

Copy link
Copy Markdown
Member

@RanderWang can you check internal CI. thanks !

@RanderWang

RanderWang commented Aug 22, 2023•
edited
Loading

Copy link
Copy Markdown
CollaboratorAuthor

@lgirdwood I checked and tried to modify it for a few days. 4 tests are failed
: TestEnterD3OnMasterCore TestD3StreamCreation TestD3StreamResume TestMemoryStateInfoGet. Most tests are related to power test.

All are failed with FW exception. I have no idea that why my PR can affect the SetDX.

2023-08-21 05:02:09,997 | Sending SetDx (C7000000, 00000000)
Payload:
00000001 00000000 2023-08-21 05:02:10,137 | Notification FwExceptionNotification (9B0A0000, 00000000)

@RanderWang

RanderWang commented Aug 22, 2023

Copy link
Copy Markdown
CollaboratorAuthor

@lgirdwood I set up the CI internal test in my environment and check the failed cases:
TestD3StreamCreation TestD3StreamResume can work with my PR
TestEnterD3OnMasterCore TestMemoryStateInfoGet can't pass without my PR, so the failure are not caused by my PR.

@lgirdwood

lgirdwood commented Aug 22, 2023

Copy link
Copy Markdown
Member

@wszypelt@lrudyX can someone check CI results, @RanderWang has checked and cant explain why this PR would fail PM tests when its related to audio format converion. Could be a false positive ? Thanks !

@lrudyX

lrudyX commented Aug 22, 2023

Copy link
Copy Markdown

@wszypelt@lrudyX can someone check CI results, @RanderWang has checked and cant explain why this PR would fail PM tests when its related to audio format converion. Could be a false positive ? Thanks !

CI results are unclear, lets wait for rerun.

@lrudyX

lrudyX commented Aug 22, 2023

Copy link
Copy Markdown

@wszypelt@lrudyX can someone check CI results, @RanderWang has checked and cant explain why this PR would fail PM tests when its related to audio format converion. Could be a false positive ? Thanks !

CI results are unclear, lets wait for rerun.

Internal CI shows green :)

@kv2019i

kv2019i commented Aug 22, 2023

Copy link
Copy Markdown
Collaborator

Known fail in https://sof-ci.01.org/sofpr/PR7959/build11852/devicetest/index.html , rest is good, proceeding with merge.

Thanks @RanderWang , this was a long one. Thanks @lrudyX for checking the test results.

@kv2019i
kv2019i merged commit 5e1a9c0 into thesofproject:mainAug 22, 2023
@kv2019ikv2019i mentioned this pull request Aug 24, 2023
Closed
@marcinszkudlinskimarcinszkudlinski mentioned this pull request Aug 24, 2023
Merged
@wszypeltwszypelt mentioned this pull request Sep 4, 2023
Closed
@RanderWangRanderWang mentioned this pull request Sep 5, 2023
Merged
@bardliaobardliao mentioned this pull request Jan 29, 2024
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lyakhlyakhlyakh left review comments
@btian1btian1btian1 approved these changes
@kv2019ikv2019ikv2019i approved these changes
@lgirdwoodlgirdwoodlgirdwood approved these changes
@tmlemantmlemantmleman approved these changes
@abonislawskiabonislawskiAwaiting requested review from abonislawskiabonislawski is a code owner
@pblaszkopblaszkoAwaiting requested review from pblaszkopblaszko is a code owner
@marcinszkudlinskimarcinszkudlinskiAwaiting requested review from marcinszkudlinski
@plbossartplbossartAwaiting requested review from plbossartplbossart is a code owner
@mmaka1mmaka1Awaiting requested review from mmaka1mmaka1 is a code owner
@lbetlejlbetlejAwaiting requested review from lbetlejlbetlej is a code owner
@dbalutadbalutaAwaiting requested review from dbalutadbaluta is a code owner
@jsarhajsarhaAwaiting requested review from jsarhajsarha is a code owner
@aiChaoSONGaiChaoSONGAwaiting requested review from aiChaoSONG
@ranj063ranj063Awaiting requested review from ranj063ranj063 is a code owner

Assignees

No one assigned

Labels

ABIABI change involved

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@RanderWang@mengdonglin@lgirdwood@tmleman@btian1@lrudyX@kv2019i@lyakh