Skip to content

Conversation

@stolx
Copy link
Contributor

@stolx stolx commented Mar 29, 2021

Add pipe for Waves codec playback
Add pipe for Waves codec demux playback
Modify sof-tgl-max98357a-rt5682 topology so Waves codec can be added
to playback and headphones depending on -DWAVES value

Signed-off-by: Oleksandr Strelchenko oleksandr.strelchenko@waves.com

@stolx
Copy link
Contributor Author

stolx commented Mar 29, 2021

@cujomalainey @dbaluta @keyonjie
comments from #3951 are addressed, thank you

@lgirdwood lgirdwood requested a review from RDharageswari March 29, 2021 20:58
Copy link
Contributor

@cujomalainey cujomalainey left a comment

Choose a reason for hiding this comment

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

Thanks for the update LGTM

Copy link
Contributor

@keyonjie keyonjie left a comment

Choose a reason for hiding this comment

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

looks good, thanks.

Copy link
Member

@plbossart plbossart left a comment

Choose a reason for hiding this comment

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

couple of nit-picks below.

Copy link
Member

Choose a reason for hiding this comment

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

does this mean that this will create controls "MaxxChrome Runtime %d" and "MaxChrome Setup %d"?

If yes, it's not clear to me what Runtime and Setup mean for a control? Adding some comments wouldn't hurt on the intended purpose and behavior

Copy link
Contributor

Choose a reason for hiding this comment

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

These are controls dictated by the codec adapter and we have no control over.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, what? Nothing prevents us from creating the controls we see fit in the topology. The codec adapter is just another module which knows nothing about controls. The codec adapter defines an IPC, but not how those controls are named and used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, but the names correspond to the naming in the codec adapter. Codec adapter has explicitly a "setup config" handler and a "runtime config" handler. I am down for comments I am just hesitant to deviate to far from the source code.

Copy link
Contributor Author

@stolx stolx Mar 31, 2021

Choose a reason for hiding this comment

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

@plbossart

does this mean that this will create controls "MaxxChrome Runtime %d" and "MaxChrome Setup %d"?

yes, the topology will create:
for speakers: MaxxChrome Runtime 1 MaxxChrome Setup 1
for headphones: MaxxChrome Runtime 2 MaxxChrome Setup 2
difference between them is defined by codec_adapter api. For example, Setup, as opposed to Runtime, has a codec_adapter-related part that holds codec id (used to select between possible codecs), and a bunch of actually unused fields.
AFAIK having setup and runtime configs is a must for a codec that implements codec_adapter interface.
So correct description of Setup and Runtime is some kind of a reference to codec_adapter api, which I can add as a comment to topology but not sure it is needed.

Or maybe I did not understand your point correctly?

Copy link
Member

Choose a reason for hiding this comment

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

the point was that it would be more intuitive to have

for speakers: MaxxChrome Runtime MaxxChrome Setup Speaker
for headphones: MaxxChrome Runtime MaxxChrome Setup Headphones

and as discussed with Curtis this can be done via macros that are set in the caller

Copy link
Contributor

Choose a reason for hiding this comment

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

@stolx see here how all these params are defined in top level m4 then used as variables in included m4? @plbossart is suggesting something similar so we can pass in names to codec adapter where relevant. Just to make the names more intuitive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@plbossart @cujomalainey
added macros to make more intuitive name for controls, please check if this will work

@marc-hb
Copy link
Collaborator

marc-hb commented Mar 31, 2021

Github Actions hang fixed by #3992 , please git commit --amend and force push if you want to retrigger

@lgirdwood
Copy link
Member

lgirdwood commented Apr 1, 2021

Github Actions hang fixed by #3992 , please git commit --amend and force push if you want to retrigger

@stolx can you make sure you rebase on top of latest main branch when pushing update as it include a CI fix (otherwise the CI shows many failures).

@stolx stolx force-pushed the add-waves-topology branch from 1c916b7 to 37d72bc Compare April 5, 2021 15:15
Copy link
Member

@plbossart plbossart left a comment

Choose a reason for hiding this comment

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

looks mostly good, I suggested an improvement below and I have a question as well.
Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

I don't fully follow why here you have a PIPELINE ID but not in the other file without demux?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pipe-waves-codec-playback.m4 defines CA_RUNTIME_CONTROLBYTES_NAME and CA_SETUP_CONTROLBYTES_NAME
then sof/pipe-codec-adapter-playback.m4 adds pipeline ID to it:
https://github.com/thesofproject/sof/blob/main/tools/topology/sof/pipe-codec-adapter-playback.m4#L73

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so here I am aligning with sof/pipe-codec-adapter-playback.m4 implementation adding pipeline ID to name of a control

@dbaluta
Copy link
Collaborator

dbaluta commented Apr 6, 2021

@stolx this should be merged after #3999. Please mind that you need to define waves uuid
according to PR #3999

@stolx stolx force-pushed the add-waves-topology branch from 37d72bc to 2bc6d28 Compare April 6, 2021 10:33
@lgirdwood
Copy link
Member

@stolx #3999 is now merged.

Add pipe for Waves codec playback
Add pipe for Waves codec demux playback
Modify sof-tgl-max98357a-rt5682 topology so Waves codec can be added
to playback and headphones in case 'WAVES' is defined

Signed-off-by: Oleksandr Strelchenko <oleksandr.strelchenko@waves.com>
@stolx stolx force-pushed the add-waves-topology branch from 2bc6d28 to 35426f2 Compare April 6, 2021 17:16
@stolx
Copy link
Contributor Author

stolx commented Apr 6, 2021

@lgirdwood @dbaluta
I merged this PR with #3999 and added Waves codec UUID to custom topologies

@dbaluta
Copy link
Collaborator

dbaluta commented Apr 6, 2021

@stolx thanks. That part looks good to me. For the rest I'm not able to comment as they are Intel specific stuff.

@lgirdwood
Copy link
Member

SOFCI TEST

@lgirdwood
Copy link
Member

Rerun CI as build reported a TGL-H failure, but status was all green ? Could be a CI sync issue. Rerun to be sure.

@stolx stolx requested a review from plbossart April 8, 2021 05:49
@lgirdwood
Copy link
Member

Usual CI timeout on suspend/resume.

@lgirdwood lgirdwood merged commit 72121fb into thesofproject:main Apr 8, 2021
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.

7 participants