Skip to content

Support for dynamic codec adapter id - Part 3 - #5596

Merged
lgirdwood merged 2 commits into
thesofproject:mainfrom
dbaluta:dyn_id3
Apr 27, 2022
Merged

Support for dynamic codec adapter id - Part 3#5596
lgirdwood merged 2 commits into
thesofproject:mainfrom
dbaluta:dyn_id3

Conversation

@dbaluta

@dbalutadbaluta commented Mar 24, 2022

Copy link
Copy Markdown
Collaborator

This is final part of series supporting dynamic codec id.

Codec id will be sent from the Linux host via sof_ipc_stream_params and kept in the extended data
structure.

First two series were previously sent for review at:

#5591 (1)
#5592 (2)

Current PR contains (1) and (2) above. Only last two patches are actually new in this PR.

lgirdwood
lgirdwood reviewed Mar 24, 2022
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.

Can we use the same ABI update for all parts (IIRC, we had an ABI update in part 2 also ?)
Also open on the dual license question - how do we say "using MIT" for the avoidance of any doubt.

@lgirdwoodlgirdwood added the ABI ABI change involved label Mar 24, 2022
@dbaluta

dbaluta commented Mar 24, 2022

Copy link
Copy Markdown
CollaboratorAuthor

@lgirdwood it is the same abi upgrade, i kept the patch from part 2 in order to avoid conflicts.

Yes, can we modify the license to only say MIT? The same file is exported to tinycompress library.

Cc: @plbossart

lgirdwood
lgirdwood approved these changes Mar 28, 2022
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.

Approved using the MIT license selection for compress_params.h.
We will also need to add the Khronos copyright to LICENSE (if its not already there - Intel copyright is already there).

@dbaluta

dbaluta commented Mar 28, 2022

Copy link
Copy Markdown
CollaboratorAuthor

@lgirdwood thanks. I have added now the license for Kronos in #5590.

I will take care of merging the PRs once Karolina gives her OK for Intel tests.

@dbalutadbaluta mentioned this pull request Mar 28, 2022
Merged
@sys-pt1s

sys-pt1s commented Apr 15, 2022

Copy link
Copy Markdown

Can one of the admins verify this patch?

@lgirdwood

lgirdwood commented Apr 25, 2022

Copy link
Copy Markdown
Member

@dbaluta part 2 merged.

@dbaluta
codec_adapter: cadence: Remove api_id parameter
149b7e6
api_id parameter will be dynamically updated at runtime. Remove api_id
parameter id and use the default value in order to make code easier
to update for dynamic id change.
Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
@dbaluta

dbaluta commented Apr 26, 2022

Copy link
Copy Markdown
CollaboratorAuthor

@lgirdwood rebased and pushed last part.

@lgirdwood

lgirdwood commented Apr 26, 2022

Copy link
Copy Markdown
Member

@dbaluta looks like something is leaking ?

17:00:14.901 => Post-iteration cleanup
17:00:14.901 => Verification
17:00:14.901 => Topology Iteration 0, Playback Iteration 0
17:00:14.901 => Topology Iteration 1, Playback Iteration 0
17:00:14.901 => Topology Iteration 2, Playback Iteration 0
17:00:14.901 => Verify memory usage
17:00:14.901 => Memory usage after topology iteration 0
17:00:14.948 Memory leak detected: BUFFER 0,reference used: 21504, used: 24576,reference free: 633856, free: 630784
17:00:14.948 => Memory usage after topology iteration 1
17:00:14.963 Memory leak detected: BUFFER 0,reference used: 21504, used: 27648,reference free: 633856, free: 627712
17:00:14.963 => Memory usage after topology iteration 2
17:00:14.979 Memory leak detected: BUFFER 0,reference used: 21504, used: 30720,reference free: 633856, free: 624640

lgirdwood
lgirdwood reviewed Apr 26, 2022
View reviewed changes
Comment threadsrc/audio/codec_adapter/codec/cadence.c Outdated

@lgirdwoodlgirdwoodApr 26, 2022

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.

could we be leaking here if we are calling this multiple times ?

@dbalutadbalutaApr 26, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

@lgirdwood yes, you are right. Latest update should take care of that.

@dbaluta
codec_adapter: cadence: Resolve api_id at runtime
960658a
Linux host will send codec_id at runtime via sof_ipc_stream_params
as part of extended data appended at the end.
When ext_data_length is non-zero this means that the kernel wants us
the use dynamic codec id. When ext_data_length is zero, use backward
use default codec id which means we keep compatibility with older
kernels.
The codec_id is sent using compress API interface. We need to convert
that to internal cadence numbering scheme.
And final step, in order to use the dynamic id we need to move
cadence_codec_post_init() from init() function to prepare() in order
to be sure that the kernel already sent us the parameters.
We need to this operation atomically in one patch in order to avoid
breaking the functionality between patches.
Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
lgirdwood
lgirdwood approved these changes Apr 26, 2022
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.

@ranj063 good for you ?

@lgirdwood

lgirdwood commented Apr 26, 2022

Copy link
Copy Markdown
Member

Looks like CI incomplete - rerun.

@lgirdwood

lgirdwood commented Apr 26, 2022

Copy link
Copy Markdown
Member

SOFCI TEST

@lgirdwood
lgirdwood merged commit 426542d into thesofproject:mainApr 27, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ranj063ranj063ranj063 approved these changes
@paulstelian97paulstelian97paulstelian97 approved these changes
@lgirdwoodlgirdwoodlgirdwood approved these changes
@cujomalaineycujomalaineyAwaiting requested review from cujomalainey
@mrajwamrajwaAwaiting requested review from mrajwa
@RanderWangRanderWangAwaiting requested review from RanderWang
@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

Assignees

No one assigned

Labels

ABIABI change involved

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@dbaluta@sys-pt1s@lgirdwood@ranj063@paulstelian97