Skip to content

Conversation

@stolx
Copy link
Contributor

@stolx stolx commented Mar 23, 2021

Adding Waves codec to tgl-max98357a-rt5682 topology
Changes here are WIP and should be treated as reference
for Waves codec instantiation, startup payload,
max payload size, etc.

Signed-off-by: Oleksandr Strelchenko [email protected]

Adding Waves codec to tgl-max98357a-rt5682 topology
Changes here are WIP and should be treated as reference
for Waves codec instantiation, startup payload,
max payload size, etc.

Signed-off-by: Oleksandr Strelchenko <[email protected]>
@stolx
Copy link
Contributor Author

stolx commented Mar 23, 2021

@cujomalainey @RDharageswari
pinging you to start WIP topology review

@plbossart
Copy link
Member

@stolx @cujomalainey do you really want this merge in the SOF main branch?

This should be hosted somewhere else where the binaries are accessible.

@cujomalainey
Copy link
Contributor

given the upcoming topology change yes we want it here. It will be hidden behind a flag, but we figure it be best to keep everything upstream except libraries and tuning for maintenance sake. A topology is nothing special that someone could cook up anyways given the codec adapter source.


# Low Latency playback pipeline 2 on PCM 1 using max 2 channels of s24le.
# Schedule 48 frames per 1000us deadline on core 0 with priority 0
PIPELINE_PCM_ADD(sof/pipe-volume-playback.m4,
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good, we just need to hide this change behind a flag similar to line L#254

include(`muxdemux.m4')
include(`mixercontrol.m4')
include(`bytecontrol.m4')
include(`dai.m4')
Copy link
Contributor

Choose a reason for hiding this comment

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

for this file i think it might be better to just copy this file and make a "waves-demux" pipeline

@plbossart
Copy link
Member

given the upcoming topology change yes we want it here. It will be hidden behind a flag, but we figure it be best to keep everything upstream except libraries and tuning for maintenance sake. A topology is nothing special that someone could cook up anyways given the codec adapter source.

Sorry, not following. This is a topology that can be tested in CI and on my test Volteer device w/ Ubuntu + latest kernel, and this PR adds a pre-requisite that the binaries be part of the firmware. That breaks the public CI and tests for non-Chrome releases. That is a regression from my perspective, you are breaking my setup.

Again there should be nothing in the public tree that overrides a default behavior by assuming the presence of 3rd party libraries.

Move this file to a different Chome directory if you want to, but don't change the default please.

@cujomalainey
Copy link
Contributor

given the upcoming topology change yes we want it here. It will be hidden behind a flag, but we figure it be best to keep everything upstream except libraries and tuning for maintenance sake. A topology is nothing special that someone could cook up anyways given the codec adapter source.

Sorry, not following. This is a topology that can be tested in CI and on my test Volteer device w/ Ubuntu + latest kernel, and this PR adds a pre-requisite that the binaries be part of the firmware. That breaks the public CI and tests for non-Chrome releases. That is a regression from my perspective, you are breaking my setup.

Again there should be nothing in the public tree that overrides a default behavior by assuming the presence of 3rd party libraries.

Move this file to a different Chome directory if you want to, but don't change the default please.

Please see comments on PR, this is not going to replace the existing PR, hence why this is a draft. It will be going behind a flag. We are using drafts as cross-team work spaces, hence why it is not ready for review yet.

`dapm(N_BUFFER(0), N_PCMP(PCM_ID))',
`dapm(N_BUFFER(3), N_PCMP(PCM_ID))',
`dapm(N_CODEC_ADAPTER(0), N_BUFFER(3))',
`dapm(N_BUFFER(0), N_CODEC_ADAPTER(0))',
Copy link
Contributor

Choose a reason for hiding this comment

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

the pipe-volume-demux-playback.m4 is quite generic, adding codec_adapter to this makes confusion, as @cujomalainey commented, create a new .m4 for it is better choice.

@@ -0,0 +1,121 @@
# Low Latency Passthrough with codec_adapter Pipeline and PCM
# codec_adapter instantiates Waves codec
#
Copy link
Collaborator

@dbaluta dbaluta Mar 24, 2021

Choose a reason for hiding this comment

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

I wonder what is the utility of pipe-codec-adapter-playback.m4 if we create this kind of files for each new type of codec.

Cc: @johnylin76

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah good point that might be the better solution, good point for the generic pipeline, demux will likely sill need copying.

# PCM Configuration
#

PCM_CAPABILITIES(Passthrough Playback PCM_ID, CAPABILITY_FORMAT_NAME(PIPELINE_FORMAT), PCM_MIN_RATE, PCM_MAX_RATE, 2, PIPELINE_CHANNELS, 2, 16, 192, 16384, 65536, 65536) No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

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

fyi all files need a new line at the end.

@lgirdwood lgirdwood closed this Mar 28, 2021
@lgirdwood lgirdwood deleted the branch thesofproject:master March 28, 2021 13:44
@paulstelian97
Copy link
Collaborator

Please resubmit with "main" as PR base branch.

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