Skip to content

Topology: sof-icl-rt711-rt1308-rt715-hdmi: Use demux for platforms with multiple speakers#2603

Merged
plbossart merged 2 commits intothesofproject:masterfrom
bardliao:fix-2570
Mar 25, 2020
Merged

Topology: sof-icl-rt711-rt1308-rt715-hdmi: Use demux for platforms with multiple speakers#2603
plbossart merged 2 commits intothesofproject:masterfrom
bardliao:fix-2570

Conversation

@bardliao
Copy link
Collaborator

@bardliao bardliao commented Mar 23, 2020

We only need demux for two speaker dais case.

Fixes: 5e8de9f "topology: sof-icl-rt711-rt1308-rt715-hdmi: Merge two pipeline with demux"
Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com

Fixes #2570

@RanderWang
Copy link
Collaborator

RanderWang commented Mar 23, 2020

@bardliao How about change your title to reflect your change ? "use demux for platforms with multiple speakers" And commit message may disclose how to do in FW for this case.

Copy link

@juimonen juimonen left a comment

Choose a reason for hiding this comment

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

@bardliao so where is the "MONO" coming from?

@bardliao
Copy link
Collaborator Author

@bardliao so where is the "MONO" coming from?

We will define PLATFORM in CMakeLists.txt, and define "MONO" in specific platform m4 file. e.g. cml-mono.m4

@bardliao bardliao changed the title Topology: sof-icl-rt711-rt1308-rt715-hdmi: Don't use demux if there is only one speaker dai Topology: sof-icl-rt711-rt1308-rt715-hdmi: Use demux for platforms with multiple speakers Mar 23, 2020
@bardliao
Copy link
Collaborator Author

@bardliao How about change your title to reflect your change ? "use demux for platforms with multiple speakers" And commit message may disclose how to do in FW for this case.

Done

@plbossart
Copy link
Member

To answer to @juimonen there is an abuse of language. MONO here means single amplifier. That RT1308 is perfectly capable of playing stereo sounds. Not MONO means 2 amps and sometimes 2 speakers and sometimes 4, it's a topology indication.
I've been wondering if it's too late to remove this misleading MONO wording. We'd need to change the topology names used in the kernel soc-acpi matching tables.

Copy link
Member

Choose a reason for hiding this comment

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

there's already an ifdef on line 61, it'd probably be a good thing to have clearer separation between the cases.
Also we may want to conditionally disable aggregation, as we do e.g. for the kernel.

Copy link
Member

Choose a reason for hiding this comment

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

@bardliao what I meant was

if MONO
   pipelines, etc
else
  if aggregation
      pipelines, etc
  else 
      pipelines, pcm, etc.
  endif
endif

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@plbossart Done. But a little bit different to your example.

…th multiple speakers

We only need demux for two speaker dais case.

Fixes: 5e8de9f "topology: sof-icl-rt711-rt1308-rt715-hdmi: Merge
two pipeline with demux"

Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
We will create individual PCM for each speaker when NO_AGGREGATION
is defined

Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
PIPELINE_SOURCE_4, 2, s24le,
1000, 0, 0, SCHEDULE_TIME_DOMAIN_TIMER)',
`ifdef(`MONO', `',
`DAI_ADD_SCHED(sof/pipe-dai-sched-playback.m4,
Copy link
Member

Choose a reason for hiding this comment

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

the mix of ifdef is really hard to follow. And on line 148 it's rather suspicious to have SDW1-playback with index 0x202. That looks like a copy paste mistake to me, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the mix of ifdef is really hard to follow. And on line 148 it's rather suspicious to have SDW1-playback with index 0x202. That looks like a copy paste mistake to me, no?

No, it is intentional. ALH0x202 is the 2nd DAI for SDW1-Playback stream.
From line 54 to 79:

if NOMO (1 rt1308)
    Create a normal pipeline 3
else
    if NO_AGGREGATION (2 rt1308s but no aggregation)
        Create a normal pipeline 3 and normal pipeline 4
    else (2 rt1308s and aggregation)
        Create a demux pipeline 3 and DAI endpoint 4

From line 139 to 162:

if NO_AGGREGATION (2 rt1308s, but no aggregation)
    ALH0x202 DAI is for SDW2-Playback stream
else
    if MONO (No ALH0x202 is there is only 1 rt1308)
    else (2 rt1308s and aggregation)
        ALH0x202 is the 2nd DAI for SDW1-Playback stream and connect pipeline 4 source to the demux

Copy link
Member

Choose a reason for hiding this comment

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

ok, but then let's stop using SDW1-playback, it's confusing. In some cases it does mean playback on Link1 and other it's just the playback stream1...

We can fix this in a follow-up, along with the rename of MONO which is misleading - should be SINGLE_AMP

@plbossart
Copy link
Member

plbossart commented Mar 25, 2020

merging to keep topology and kernel aligned

@plbossart plbossart merged commit ae53aeb into thesofproject:master Mar 25, 2020
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.

[BUG][CML]" error: ipc error for 0x60010000 size 20" display when aplay via speaker0,2.

5 participants