Skip to content

[DO NOT MERGE] [DO NOT REVIEW] pcm_converter: Replace asserts with sample number check to if instruction - #3156

Closed
ktrzcinx wants to merge 3 commits into
thesofproject:masterfrom
ktrzcinx:pcm-onverter-remove-assert
Closed

[DO NOT MERGE] [DO NOT REVIEW] pcm_converter: Replace asserts with sample number check to if instruction#3156
ktrzcinx wants to merge 3 commits into
thesofproject:masterfrom
ktrzcinx:pcm-onverter-remove-assert

Conversation

@ktrzcinx

Copy link
Copy Markdown
Member

Using if with return instruction is not so fatal consequences
as assertion, so fw will keep running after such an error.

Signed-off-by: Karol Trzcinski karolx.trzcinski@linux.intel.com

Fix for #3146

Comment threadsrc/audio/pcm_converter/pcm_converter.c Outdated
Comment threadsrc/audio/pcm_converter/pcm_converter.c Outdated
Comment threadsrc/audio/pcm_converter/pcm_converter.c Outdated
@ktrzcinx
ktrzcinxforce-pushed the pcm-onverter-remove-assert branch 2 times, most recently from d093350 to 56b0b84CompareJuly 9, 2020 13:21

@plbossartplbossart 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.

Something fishy is going on, all legacy platforms fail alsa-bat tests and the logger shows tons of errors

[4338062316.041667] ( nan) c0 pcm_conv ......../pcm_converter.c:48 ERROR Free samples 0 in sink < samples to convert 96 + offset 0

see https://sof-ci.01.org/sofpr/PR3156/build6552/devicetest/ and e.g. look at the results for Broadwell.

@paulstelian97

Copy link
Copy Markdown
Collaborator

Something fishy is going on, all legacy platforms fail alsa-bat tests and the logger shows tons of errors

[4338062316.041667] ( nan) c0 pcm_conv ......../pcm_converter.c:48 ERROR Free samples 0 in sink < samples to convert 96 + offset 0

see https://sof-ci.01.org/sofpr/PR3156/build6552/devicetest/ and e.g. look at the results for Broadwell.

This change shouldn't actually affect what happens, this error should only show up when there was a panic before. Surely wasn't there some recovery code that detected the panic and reloaded the firmware when this happened and the reload made everything happen successfully?

There is indeed a problem, though I feel like this patch only unmasks it rather than causing it.

@paulstelian97

Copy link
Copy Markdown
Collaborator

Maybe the biggest issue is that pcm_convert_as_linear returns void and the error is traced and swallowed instead of reported in the upper layers. I'd suggest you take a look into that instead, see if there is a way to actually do the error propagation.

@ktrzcinx
ktrzcinx requested a review from libinyang as a code ownerJuly 10, 2020 08:37
@ktrzcinxktrzcinx changed the title pcm_converter: Replace asserts with sample number check to if instruction[WIP] pcm_converter: Replace asserts with sample number check to if instructionJul 10, 2020
@ktrzcinx
ktrzcinxforce-pushed the pcm-onverter-remove-assert branch from 0db109b to 3e5995aCompareJuly 10, 2020 10:41
@ktrzcinx
ktrzcinxforce-pushed the pcm-onverter-remove-assert branch from 3e5995a to 0932e46CompareJuly 10, 2020 13:29
@ktrzcinxktrzcinx mentioned this pull request Jul 10, 2020
@ktrzcinx
ktrzcinxforce-pushed the pcm-onverter-remove-assert branch from 0932e46 to 410d205CompareJuly 10, 2020 15:01
@lgirdwood

Copy link
Copy Markdown
Member

@ktrzcinx I've reverted the original PR, can we rework this on top.

@ktrzcinx
ktrzcinxforce-pushed the pcm-onverter-remove-assert branch 2 times, most recently from ba0a74c to b4779aeCompareJuly 27, 2020 09:53
@ktrzcinx
ktrzcinxforce-pushed the pcm-onverter-remove-assert branch from b4779ae to 7c76375CompareJuly 27, 2020 10:11

@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.

Is the last patch temp or for upsgtream ?

Comment threadsrc/audio/dai.c Outdated
Comment threadsrc/audio/host.c Outdated
Comment threadsrc/audio/pcm_converter/pcm_converter.c Outdated
@ktrzcinxktrzcinx changed the title [WIP] pcm_converter: Replace asserts with sample number check to if instruction[DO NOT MERGE] [DO NOT REVIEW] pcm_converter: Replace asserts with sample number check to if instructionJul 28, 2020
@ktrzcinx
ktrzcinx marked this pull request as draft July 29, 2020 13:00
@ktrzcinx
ktrzcinxforce-pushed the pcm-onverter-remove-assert branch from 7c76375 to d70a862CompareJuly 29, 2020 13:01
@ktrzcinx

Copy link
Copy Markdown
MemberAuthor

SOFCI TEST

@lgirdwood

Copy link
Copy Markdown
Member

@ktrzcinx looks like it needs a rebase prior to a retest.

@ktrzcinx
ktrzcinxforce-pushed the pcm-onverter-remove-assert branch from d70a862 to b3fcf62CompareAugust 17, 2020 11:54
@ktrzcinx
ktrzcinxforce-pushed the pcm-onverter-remove-assert branch from b3fcf62 to 293f65fCompareSeptember 2, 2020 07:18
Usage of pcm_convert_as_linear() function speed up conversion
and make core convert function easier to implement.
Signed-off-by: Karol Trzcinski <karolx.trzcinski@linux.intel.com>
Raw stream operations may lead to wrong data reading and hurt
code hermetization. Extract local stream variable, to keep limit
of columns in line.
Signed-off-by: Karol Trzcinski <karolx.trzcinski@linux.intel.com>
Signed-off-by: Karol Trzcinski <karolx.trzcinski@linux.intel.com>
@ktrzcinx
ktrzcinxforce-pushed the pcm-onverter-remove-assert branch from c661c21 to 07ce749CompareSeptember 2, 2020 09:20

@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.

@ktrzcinx I checked the CI failures and all I could see was

449252956.354167] (449252960.000000) c0 hda-dma ..../intel/hda/hda-dma.c:440 hda-dmac: 4 channel 0 -> copy 0x1060 bytes
[449254143.437500] ( 1187.083374) c0 hda-dma ..../intel/hda/hda-dma.c:440 hda-dmac: 5 channel 0 -> copy 0x180 bytes
[449254183.593750] ( 40.156250) c0 hda-dma ..../intel/hda/hda-dma.c:334 hda-dmac: 7 channel 0 -> copy 0x170 bytes
[449255393.854167] ( 1210.260376) c0 hda-dma ..../intel/hda/hda-dma.c:440 hda-dmac: 5 channel 0 -> copy 0x180 bytes
[449255434.583333] ( 40.729168) c0 hda-dma ..../intel/hda/hda-dma.c:334 hda-dmac: 7 channel 0 -> copy 0x190 bytes
[449256644.010417] ( 1209.427124) c0 hda-dma ..../intel/hda/hda-dma.c:440 hda-dmac: 5 channel 0 -> copy 0x180 bytes
[449256684.531250] ( 40.520832) c0 hda-dma ..../intel/hda/hda-dma.c:334 hda-dmac: 7 channel 0 -> copy 0x180 bytes
[449257893.437500] ( 1208.906250) c0 hda-dma ..../intel/hda/hda-dma.c:440 hda-dmac: 5 channel 0 -> copy 0x180 bytes
[449257934.166667] ( 40.729168) c0 hda-dma ..../intel/hda/hda-dma.c:334 hda-dmac: 7 channel 0 -> copy 0x180 bytes

I would have expected the timing to be on 1ms instead of 1.2ms ? Any clues ?

@lgirdwood
lgirdwood deleted the branch thesofproject:masterMarch 28, 2021 13:44
@paulstelian97

Copy link
Copy Markdown
Collaborator

@ktrzcinx If this PR is still applicable, I recommend you send it again based on the "main" branch.

Sign up for freeto 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.

6 participants

@ktrzcinx@paulstelian97@lgirdwood@dbaluta@plbossart@mmaka1