topology1: add sof-icl-rt711-rt1308-rt715-hdmi NOMIC support #5367

libinyang · 2022-02-16T02:17:47Z

add sof-icl-rt711-rt1308-rt715-hdmi NOMIC support and add CMake
definition for sof-adl-rt711-l2-rt1316-l01

Signed-off-by: Libin Yang libin.yang@intel.com

plbossart · 2022-02-16T03:11:38Z

tools/topology/topology1/sof-icl-rt711-rt1308-rt715-hdmi.m4

@@ -77,7 +77,8 @@ ifdef(`NOJACK', `',
 # PCM2 ---> volume ----> ALH 2 BE dailink 2
 ifdef(`MONO', `',
 `# PCM40 ---> volume ----> ALH 2 BE dailink 3')
-# PCM4 <--- volume <---- ALH 2 BE dailink 4
+ifdef(`NOMIC', `',
+`# PCM4 <--- volume <---- ALH 2 BE dailink 4')


well, no this looks wrong. When the RT715 is not present, then we can still have a PCM4 pipeline that takes the data from ALH2 BE dailink 4

It's really not a 'NOMIC" solution, rather that the mics are handled in a different way.

@plbossart I mean no dmic, not RT715 in this patch. On some platforms, there is no DMIC and only analog mic is used. There is no any codec connected to the BE dailink 4. I will change NOMIC to NODMIC.

BTW: And I found the comments are wrong. It should not be "ALH2 BE dailink4" but "ALH2 BE $MIC_LINK" (more precisely, it should be "ALH2 BE eval(MIC_LINK)" ).

I still think you need to macros

NO_MIC_CODEC: tells that there is no RT715 on this skew
RT711_MIC_SUPPORT: tells that the local mics are connected to RT711.

you should really disable the mic pipeline only when the RT711_MIC_SUPPORT is not defined, otherwise the mic pipeline needs to rely on a capture port of the RT711 and the ALH,

NO_MIC_CODEC: tells that there is no RT715 on this skew RT711_MIC_SUPPORT: tells that the local mics are connected to RT711.

What about use NOOBMIC (no onboard mic) instead of NODMIC? Let consider the scenarios:

neither rt711 onboard mic nor rt715 dmic is used, this NOOBMIC will be defined.

either rt711 onboard mic or rt715 dmic is used, NOOBMIC will not be defined. Then we don't define NOOBMIC and use MIC_LINK to define the link id just like what we do now. (If we use rt711 onboard MIC, we will still need to define PIN_ID. And we can do it when we use rt711 onboard mic)

Both rt711 on board mic and rt715 dmic are used. This should not happen. But if it happens, we need create a new pipeline and assign a new PCM number for it. We can do it when we meet such configuration in the future.

With this assumption, the patch still can be used, the only change is NODMIC -> NOOBMIC.

an acronym starting with "NOOB" doesn't sound very good in English, it's Internet slang for 'novice', 'newcomer' or 'inexperienced'. Consider a different term, such as NO_LOCAL_MICS or something.

I don't understand how you want to manage the case 2). You will need to use different branches to create the topology, and you need a variable to figure out if you are going to add the RT711 microphone path or the RT715 mic path. in existing solutions we use AMP_LINK as an integer, whereas here you need a true/false case.

The example I had in mind is the 'NO_JACK' case. We can have a similar case with NO_MIC_CODEC.

I will use NO_LOCAL_MICS. Thanks for pointing it out.

When creating a pipeline for RT715 or RT711 local mic, we can both use:

ifdef(`NO_LOCAL_MIC', `' , ` # pipeline PIPELINE_PCM_ADD(sof/pipe-highpass-switch-capture.m4, 5, 4, 2, s32le, 1000, 0, 0, 48000, 48000, 48000) # dai DAI_ADD(sof/pipe-dai-capture.m4, 5, ALH, eval(MIC_LINK * 256 + **PIN_ID**), `SDW'eval(MIC_LINK)`-Capture', PIPELINE_SINK_5, 2, s24le, 1000, 0, 0, SCHEDULE_TIME_DOMAIN_TIMER) # pcm PCM_CAPTURE_ADD(Microphone, 4, PIPELINE_PCM_5) # dai config DAI_CONFIG(ALH, eval(MIC_LINK * 256 + **PIN_ID**), 4, `SDW'eval(MIC_LINK)`-Capture', ALH_CONFIG(ALH_CONFIG_DATA(ALH, eval(MIC_LINK * 256 + **PIN_ID**), 48000, 2))) '

We don't need to care whether it is a rt715 codec or rt711 codec, do we?

Thanks @libinyang, we don't need to know what the actual codec is in the topology file, all we see in ALH channels and PDI/port interfaces.

plbossart · 2022-02-18T20:49:31Z

tools/topology/topology1/sof-icl-rt711-rt1308-rt715-hdmi.m4

-# PCM0 ---> volume ----> mixer --->ALH 2 BE dailink 0
-# PCM1 <--- volume <---- ALH 3 BE dailink 1
+# PCM0 ---> volume ----> mixer --->ALH 2 BE $UAJ_LINK
+# PCM1 <--- volume <---- ALH 3 BE $UAJ_LINK


@libinyang the use of a $ does not do anything in m4, and I am not sure what this is trying to accomplish?
There is no other use of this.

@plbossart '$' is in the comments line, just telling readers this is a variable. I will remove the '$' in the update patch.

plbossart · 2022-02-18T20:51:30Z

tools/topology/topology1/sof-icl-rt711-rt1308-rt715-hdmi.m4

@@ -77,7 +77,8 @@ ifdef(`NOJACK', `',
 # PCM2 ---> volume ----> ALH 2 BE $AMP_1_LINK
 ifdef(`MONO', `',
 `# PCM40 ---> volume ----> ALH 2 BE $AMP_2_LINK')
-# PCM4 <--- volume <---- ALH 2 BE $MIC_LINK
+ifdef(`NODMIC', `',


I thought you wanted to call this NO_LOCAL_MIC?

Will update it in the new patch

libinyang · 2022-02-21T02:26:35Z

Patch is updated.

remove the'$' in the comments
use NO_LOCAL_MIC

libinyang · 2022-02-21T06:53:38Z

@plbossart Please hold. I found sof-tgl-rt711-rt1308.m4 may be a better one for this scenario

libinyang · 2022-02-21T13:31:06Z

@plbossart Please hold. I found sof-tgl-rt711-rt1308.m4 may be a better one for this scenario

After doing the investigation. The sof-tgl-rt711-rt1308.m4 doesn't meet our scenario. I will continue using sof-icl-rt711-rt1308-rt715-hdmi. But as our driver has the rule to calculate the BE id. The Jack BE id is from 0. The AMP BE id is from 2. If there is second AMP codec, the second AMP BE id will be 3. The DMIC BE id is from 4. And the HMDI BE id is increasing dynamically. So the calculation for HDMI BE id should be:

if (DMIC exists)
    HDMI1_BE_ID = 5
elseif (there are 2 AMPs)
    HDMI1_BE_ID = 4
elseif (there is 1 AMP)
    HDMI1_BE_ID = 3
elseif (there is Jack)
    HDMI1_BE_ID = 2
else
   HDMI1_BE_ID = 0

@bardliao Could you please help confirm?

bardliao · 2022-02-22T01:20:46Z

@plbossart Please hold. I found sof-tgl-rt711-rt1308.m4 may be a better one for this scenario

After doing the investigation. The sof-tgl-rt711-rt1308.m4 doesn't meet our scenario. I will continue using sof-icl-rt711-rt1308-rt715-hdmi. But as our driver has the rule to calculate the BE id. The Jack BE id is from 0. The AMP BE id is from 2. If there is second AMP codec, the second AMP BE id will be 3. The DMIC BE id is from 4. And the HMDI BE id is increasing dynamically. So the calculation for HDMI BE id should be:
if (DMIC exists)
    HDMI1_BE_ID = 5
elseif (there are 2 AMPs)
    HDMI1_BE_ID = 4
elseif (there is 1 AMP)
    HDMI1_BE_ID = 3
elseif (there is Jack)
    HDMI1_BE_ID = 2
else
   HDMI1_BE_ID = 0
@bardliao Could you please help confirm?

@libinyang That would be very complicated if you want to calculate the HDMI1_BE_ID, for example, we could have more than one DMIC. Why not just specify HDMI1_BE_ID in CMakeLists.txt? We can have a default HDMI1_BE_ID if HDMI1_BE_ID is not specified.

libinyang · 2022-02-22T01:27:09Z

@bardliao Could you please help confirm?

@libinyang That would be very complicated if you want to calculate the HDMI1_BE_ID, for example, we could have more than one DMIC. Why not just specify HDMI1_BE_ID in CMakeLists.txt? We can have a default HDMI1_BE_ID if HDMI1_BE_ID is not specified.

@bardliao Specifying the HDMI1_BE_ID is deferring the effort and we need to calculate it every time and it can be calculated wrong. I prefer to calculate it by the m4 itself. For the 2 DMICs, we can modify the code when we really meet it. It should be easy to support 2 DMICs.

bardliao · 2022-02-22T02:58:16Z

@bardliao Could you please help confirm?

@libinyang That would be very complicated if you want to calculate the HDMI1_BE_ID, for example, we could have more than one DMIC. Why not just specify HDMI1_BE_ID in CMakeLists.txt? We can have a default HDMI1_BE_ID if HDMI1_BE_ID is not specified.

@bardliao Specifying the HDMI1_BE_ID is deferring the effort and we need to calculate it every time and it can be calculated wrong. I prefer to calculate it by the m4 itself. For the 2 DMICs, we can modify the code when we really meet it. It should be easy to support 2 DMICs.

@libinyang Given that the topology is for SDW + HDMI only. We can go with your proposal. Some comments for your if condition

if (DMIC exists) -> DMIC here means SDW DMIC, not PCH DMIC. We define #define SDW_DMIC_DAI_ID 4 in machine driver, which apply to sdw DMIC only.
there are 2 AMPs or there is 1 AMP -> We should test NO_AGGREGATION instead. There will be only one PCM present if two AMPs are aggregated.
I would recommend to use a simple m4 for the HDMI only case. ie. No SDW/SSP/DMIC etc.

libinyang · 2022-02-22T03:10:18Z

@bardliao

@libinyang Given that the topology is for SDW + HDMI only. We can go with your proposal. Some comments for your if condition

if (DMIC exists) -> DMIC here means SDW DMIC, not PCH DMIC. We define #define SDW_DMIC_DAI_ID 4 in machine driver, which apply to sdw DMIC only.

This is a good question. Actually, this file won't handle PCH DMIC platform. The SDW + PCH DMIC is handled by sof-tgl-rt711-rt1308.m4.

there are 2 AMPs or there is 1 AMP -> We should test NO_AGGREGATION instead. There will be only one PCM present if two AMPs are aggregated.

I actually test 'MONO'. MONO means there is a AMP or 2 AMPs. 'NO_AGGREGATION' is used when there are 2 AMPs. In the TPLG, we don't care there is one or two PCMs, but we can the BE ID. If the 2 speaker are aggregated, the second AMP should also occupies one BE ID, doesn't it?

I would recommend to use a simple m4 for the HDMI only case. ie. No SDW/SSP/DMIC etc.

Yes, this is a good suggestion. However my patch may still need to take care this situation, otherwise, the condition won't be gone through totally, which may cause corner issues.

libinyang · 2022-02-22T08:02:58Z

@bardliao

@libinyang Given that the topology is for SDW + HDMI only. We can go with your proposal. Some comments for your if condition

if (DMIC exists) -> DMIC here means SDW DMIC, not PCH DMIC. We define #define SDW_DMIC_DAI_ID 4 in machine driver, which apply to sdw DMIC only.

This is a good question. Actually, this file won't handle PCH DMIC platform. The SDW + PCH DMIC is handled by sof-tgl-rt711-rt1308.m4.

there are 2 AMPs or there is 1 AMP -> We should test NO_AGGREGATION instead. There will be only one PCM present if two AMPs are aggregated.

I actually test 'MONO'. MONO means there is a AMP or 2 AMPs. 'NO_AGGREGATION' is used when there are 2 AMPs. In the TPLG, we don't care there is one or two PCMs, but we can the BE ID. If the 2 speaker are aggregated, the second AMP should also occupies one BE ID, doesn't it?

I would recommend to use a simple m4 for the HDMI only case. ie. No SDW/SSP/DMIC etc.

Yes, this is a good suggestion. However my patch may still need to take care this situation, otherwise, the condition won't be gone through totally, which may cause corner issues.

After going through the driver code with @bardliao , it is confirmed that NO_AGGREGATION should be used.

libinyang · 2022-02-22T08:09:01Z

PR is updated:

fix the no aggregation issue in sof-icl-rt711-rt1308-rt715-hdmi
calculate the HDMI BE ID dynamically based on the platform configuration.

We could have a capture dai link for echo reference, so the test condition will not work

bardliao · 2022-02-22T12:53:36Z

PR is updated:

fix the no aggregation issue in sof-icl-rt711-rt1308-rt715-hdmi

calculate the HDMI BE ID dynamically based on the platform configuration.

Sorry, @libinyang we could have too many possible combination conditions. For example, we will have 1 playback and 1 capture dai link for rt1316, and we didn't consider this in your test condition. I can imagine that we will have a very complicated condition if we want to cover all possible cases. So, I think just define the starting HDMI ID in CMakeLists.txt is the simplest and most scalable way to fix the issue.

libinyang · 2022-02-22T15:17:01Z

PR is updated:

fix the no aggregation issue in sof-icl-rt711-rt1308-rt715-hdmi

calculate the HDMI BE ID dynamically based on the platform configuration.

The new tplg id still doesn't meet the driver. I will check the driver BE id.

libinyang · 2022-02-22T15:19:44Z

Sorry, @libinyang we could have too many possible combination conditions. For example, we will have 1 playback and 1 capture dai link for rt1316, and we didn't consider this in your test condition. I can imagine that we will have a very complicated condition if we want to cover all possible cases. So, I think just define the starting HDMI ID in CMakeLists.txt is the simplest and most scalable way to fix the issue.

@bardliao Define the ID in CMakefile will make the thing easy. But each tplg needs extra effort. @plbossart What's you option?

plbossart · 2022-02-22T17:00:13Z

Sorry @libinyang @bardliao, what makes you think, NO_AGGREGATION is relevant here?

You have two rt1316 amplifiers on different links for this device and for all products using this configuration we use aggregation. NO_AGGREGATION was a test mode added initially, but it shall not be used by anyone. I think we want to remove it from topologies because it adds a level of complexity for no good reason.

libinyang · 2022-02-23T01:16:31Z

Sorry @libinyang @bardliao, what makes you think, NO_AGGREGATION is relevant here?

You have two rt1316 amplifiers on different links for this device and for all products using this configuration we use aggregation. NO_AGGREGATION was a test mode added initially, but it shall not be used by anyone. I think we want to remove it from topologies because it adds a level of complexity for no good reason.

@plbossart Got it. Thanks. We found even with the latest code of this PR. It still doesn't work. This is because it is not sync between driver and tplg. Driver enables capture on rt1316 for smart AMP reference. But our tplg doesn't contain any information for it. I think @bardliao 's suggestion is easiest way to implement. I will take @bardliao 's advice as we need to give a solution quickly.

plbossart · 2022-02-23T02:26:33Z

Sorry @libinyang @bardliao, what makes you think, NO_AGGREGATION is relevant here?
You have two rt1316 amplifiers on different links for this device and for all products using this configuration we use aggregation. NO_AGGREGATION was a test mode added initially, but it shall not be used by anyone. I think we want to remove it from topologies because it adds a level of complexity for no good reason.

@plbossart Got it. Thanks. We found even with the latest code of this PR. It still doesn't work. This is because it is not sync between driver and tplg. Driver enables capture on rt1316 for smart AMP reference. But our tplg doesn't contain any information for it. I think @bardliao 's suggestion is easiest way to implement. I will take @bardliao 's advice as we need to give a solution quickly.

I don't see the problem @libinyang. The machine driver can expose dai links that the topology does not use, we do this all the time. The problem is when the topology uses a link that is not created by the machine driver, that would prevent the card from probing.

bardliao · 2022-02-23T07:23:04Z

@plbossart The issue is that the HDMI id is fixed in sof-icl-rt711-rt1308-rt715-hdmi.m4, but it is dynamic in machine driver. It will be the last SDW ID + 1. In this case, the IDs are

[    5.854750] sof_sdw sof_sdw: create dai link SDW2-Playback, id 0
[    5.854760] sof_sdw sof_sdw: create dai link SDW2-Capture, id 1
[    5.854763] sof_sdw sof_sdw: create dai link SDW0-Playback, id 2
[    5.854765] sof_sdw sof_sdw: create dai link SDW0-Capture, id 3
[    5.854766] sof_sdw sof_sdw: create dai link iDisp1, id 4
[    5.854767] sof_sdw sof_sdw: create dai link iDisp2, id 5
[    5.854768] sof_sdw sof_sdw: create dai link iDisp3, id 6
[    5.854769] sof_sdw sof_sdw: create dai link iDisp4, id 7

@libinyang is trying to work out a general formula that can fit all cases. My opinion is that we can just specify the HDMI id on CMakeList.txt.

plbossart · 2022-02-23T15:43:11Z

@plbossart The issue is that the HDMI id is fixed in sof-icl-rt711-rt1308-rt715-hdmi.m4, but it is dynamic in machine driver. It will be the last SDW ID + 1. In this case, the IDs are
[    5.854750] sof_sdw sof_sdw: create dai link SDW2-Playback, id 0
[    5.854760] sof_sdw sof_sdw: create dai link SDW2-Capture, id 1
[    5.854763] sof_sdw sof_sdw: create dai link SDW0-Playback, id 2
[    5.854765] sof_sdw sof_sdw: create dai link SDW0-Capture, id 3
[    5.854766] sof_sdw sof_sdw: create dai link iDisp1, id 4
[    5.854767] sof_sdw sof_sdw: create dai link iDisp2, id 5
[    5.854768] sof_sdw sof_sdw: create dai link iDisp3, id 6
[    5.854769] sof_sdw sof_sdw: create dai link iDisp4, id 7
@libinyang is trying to work out a general formula that can fit all cases. My opinion is that we can just specify the HDMI id on CMakeList.txt.

we could add a quirk in the machine driver to skip the mic link as well. We've done it before. This is a very odd topology so better avoid getting migraines and use the simplest solution.

I am not a big fan of using CMakeList.txt because that's yet another file that we need to track, and we can't do any math in there, while we can update counters in M4 and .conf.

bardliao · 2022-02-25T02:23:47Z

As you can see, in sof-icl-rt711-rt1308-rt715, we only define 2 playback pipelines for speaker. We don't define capture pipelines for smart AMP. And in the driver, if it is using rt1308, it won't define capture link; if it is using rt1316,

@bardliao @libinyang What prevents us from enabling the capture for RT1308 in the machine driver? I do recall we worked on this with @shumingfan,

@plbossart capture is not supported by rt1308_sdw_dai[]

libinyang · 2022-03-01T02:54:04Z

@plbossart ping

plbossart · 2022-03-02T02:18:33Z

@plbossart ping

We can enable RT1308 feedback, in fact I did the work already in https://github.com/thesofproject/linux/pull/2514/commits
I would prefer if we align both smart amps rather than change the topology for no good reason.

libinyang · 2022-03-02T05:00:50Z

We can enable RT1308 feedback, in fact I did the work already in https://github.com/thesofproject/linux/pull/2514/commits I would prefer if we align both smart amps rather than change the topology for no good reason.

@plbossart You are right. My only concern is the productions with rt1308 in the market don't support smart AMP now. And what's more, rt1308 needs 4 channels to support smart AMP. It means we need 8 channel to support 2xrt1308. The old platforms may not have so much memory to run smart AMP. So we still cannot set all the platforms to support smart AMP. We need a long term solution to support smart AMP on rt1308 and rt1316. Do you think we can use this PR's workaround for short term?

shumingfan · 2022-03-02T05:29:01Z

@plbossart @libinyang @bardliao
It is helpful to merge left IV and right IV to one DAI link for rt1308-sdw feedback in https://github.com/thesofproject/linux/pull/2514/commits.
However, the rt1308-sdw driver still needs other settings to enable IV feedback.
I created the patch to enable IV feedback last time, but we encountered the problem @libinyang mentioned.
And there are not enough tests for IV feedback.
Could I add the capture feature later if the customer wants to run the AMP protection algorithm in the Linux platform?

bardliao · 2022-03-02T12:23:10Z

@plbossart Even if we enable rt1308 feedback, we still need to handle the no sdw mic case in topology. IMHO, that is a different topic.

plbossart · 2022-03-02T16:02:44Z

@plbossart @libinyang @bardliao It is helpful to merge left IV and right IV to one DAI link for rt1308-sdw feedback in https://github.com/thesofproject/linux/pull/2514/commits. However, the rt1308-sdw driver still needs other settings to enable IV feedback. I created the patch to enable IV feedback last time, but we encountered the problem @libinyang mentioned. And there are not enough tests for IV feedback. Could I add the capture feature later if the customer wants to run the AMP protection algorithm in the Linux platform?

You're reading too much into my comment @shumingfan @bardliao @libinyang

If we add the dailink in the machine driver, it does nothing. What matters is the follow-up topology change to actually enable a smart amp processing. And from that perspective, it's much better to push this change in the machine driver, so that the kernel support will be there ahead of time. It's very very hard to introduce a new feature that requires a topology/firmware change AND a kernel machine driver change. If we add a new feature in the topology and the machine driver does not create that dailink then it's a probe failure - a much worse situation.

In other words, adding this capture dailink today is harmless. it's just there, it simplifies are life wrt. indices in the topology, but we don't have to deal with the smart amp integration today.

libinyang · 2022-03-03T01:50:10Z

You're reading too much into my comment @shumingfan @bardliao @libinyang

If we add the dailink in the machine driver, it does nothing. What matters is the follow-up topology change to actually enable a smart amp processing. And from that perspective, it's much better to push this change in the machine driver, so that the kernel support will be there ahead of time. It's very very hard to introduce a new feature that requires a topology/firmware change AND a kernel machine driver change. If we add a new feature in the topology and the machine driver does not create that dailink then it's a probe failure - a much worse situation.

In other words, adding this capture dailink today is harmless. it's just there, it simplifies are life wrt. indices in the topology, but we don't have to deal with the smart amp integration today.

@plbossart Do you mean we will apply the PR thesofproject/linux#2514, and in the tplg, we will always assume that rt1316/rt1308 uses 2 BE IDs? In this case, we still need determine the tplg BE ID based on the platform configuration, with or without rt715. So which version patch do you prefer, the latest version (assign HDMI BE ID in CMailefile) or https://github.com/thesofproject/sof/compare/3dfb6f1c423f6c747d73c05928211cf3f06b9b8d..468ac6d31b9f8cd5da83a8afc9e512a74a685e85 (Calculate the HDMI BE ID in m4)?

plbossart · 2022-03-04T01:55:20Z

@libinyang @bardliao here's the suggested patch to remove the 'no-aggregation' support
remove-no-aggregation.diff.txt

I would recommend you check the binary topology before and after this patch to make sure we didn't break anything.
Thanks!

libinyang · 2022-03-04T06:21:16Z

@libinyang @bardliao here's the suggested patch to remove the 'no-aggregation' support remove-no-aggregation.diff.txt

I would recommend you check the binary topology before and after this patch to make sure we didn't break anything. Thanks!

@plbossart Thanks. I will test the tplg with the patch removing-no-aggregation. I will send out the test result next Monday.

libinyang · 2022-03-07T05:18:49Z

@libinyang @bardliao here's the suggested patch to remove the 'no-aggregation' support remove-no-aggregation.diff.txt

I would recommend you check the binary topology before and after this patch to make sure we didn't break anything. Thanks!

@plbossart The 'no-aggregation' patch need a little modification:

diff --git a/tools/topology/topology1/sof-icl-rt711-rt1308-rt715-hdmi.m4 b/tools/topology/topology1/sof-icl-rt711-rt1308-rt715-hdmi.m4
index 9c531c05f782..34afcfa618f7 100644
--- a/tools/topology/topology1/sof-icl-rt711-rt1308-rt715-hdmi.m4
+++ b/tools/topology/topology1/sof-icl-rt711-rt1308-rt715-hdmi.m4
@@ -110,14 +110,14 @@ PIPELINE_PCM_ADD(sof/pipe-volume-playback.m4,
 `
 # Low Latency playback pipeline 3 on PCM 2 using max 2 channels of s32le.
 # Schedule 48 frames per 1000us deadline with priority 0 on core 0
-PIPELINE_PCM_ADD(`sof/pipe-volume-demux-playback.m4'),
+PIPELINE_PCM_ADD(sof/pipe-volume-demux-playback.m4,
        3, 2, 2, s32le,
        1000, 0, 0,
        48000, 48000, 48000)

 # Low Latency playback pipeline 4 on PCM 40 using max 2 channels of s32le.
 # Schedule 48 frames per 1000us deadline with priority 0 on core 0
-PIPELINE_PCM_ADD(`sof/pipe-dai-endpoint.m4'),
+PIPELINE_PCM_ADD(sof/pipe-dai-endpoint.m4,
        4, 40, 2, s32le,
        1000, 0, 0,
        48000, 48000, 48000)

Otherwise, compile failed.
With the compile issue fixed, I tested run-all in short test mode on 5 platforms, which use different configurations of sof-icl-rt711-rt1308-rt715-hdmi.m4. 4 platform passes. 1 platform hangs, and the probe passed and PCMs are created successfully on the hang platform. So I don't think it's the 'no-aggregation' patch's issue.

libinyang · 2022-03-08T01:23:29Z

@libinyang @bardliao here's the suggested patch to remove the 'no-aggregation' support remove-no-aggregation.diff.txt
I would recommend you check the binary topology before and after this patch to make sure we didn't break anything. Thanks!

@plbossart The 'no-aggregation' patch need a little modification:
diff --git a/tools/topology/topology1/sof-icl-rt711-rt1308-rt715-hdmi.m4 b/tools/topology/topology1/sof-icl-rt711-rt1308-rt715-hdmi.m4
index 9c531c05f782..34afcfa618f7 100644
--- a/tools/topology/topology1/sof-icl-rt711-rt1308-rt715-hdmi.m4
+++ b/tools/topology/topology1/sof-icl-rt711-rt1308-rt715-hdmi.m4
@@ -110,14 +110,14 @@ PIPELINE_PCM_ADD(sof/pipe-volume-playback.m4,
 `
 # Low Latency playback pipeline 3 on PCM 2 using max 2 channels of s32le.
 # Schedule 48 frames per 1000us deadline with priority 0 on core 0
-PIPELINE_PCM_ADD(`sof/pipe-volume-demux-playback.m4'),
+PIPELINE_PCM_ADD(sof/pipe-volume-demux-playback.m4,
        3, 2, 2, s32le,
        1000, 0, 0,
        48000, 48000, 48000)

 # Low Latency playback pipeline 4 on PCM 40 using max 2 channels of s32le.
 # Schedule 48 frames per 1000us deadline with priority 0 on core 0
-PIPELINE_PCM_ADD(`sof/pipe-dai-endpoint.m4'),
+PIPELINE_PCM_ADD(sof/pipe-dai-endpoint.m4,
        4, 40, 2, s32le,
        1000, 0, 0,
        48000, 48000, 48000)
Otherwise, compile failed. With the compile issue fixed, I tested run-all in short test mode on 5 platforms, which use different configurations of sof-icl-rt711-rt1308-rt715-hdmi.m4. 4 platform passes. 1 platform hangs, and the probe passed and PCMs are created successfully on the hang platform. So I don't think it's the 'no-aggregation' patch's issue.

@plbossart Would you submit the 'no-aggregation' patch or I can help submit it together with my PR?

plbossart · 2022-03-08T01:46:28Z

@plbossart Would you submit the 'no-aggregation' patch or I can help submit it together with my PR?

please go ahead and include this patch in your PR @libinyang, thanks

NO_AGGREGATION is never used now. Let's remove it. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Signed-off-by: Libin Yang <libin.yang@intel.com>

The links are dynamically assigned. The descriptions should be changed correspondingly. Signed-off-by: Libin Yang <libin.yang@intel.com>

libinyang · 2022-03-11T02:28:30Z

ping @plbossart

plbossart · 2022-03-11T02:35:27Z

tools/topology/topology1/sof-icl-rt711-rt1308-rt715-hdmi.m4

+ifdef(`NO_LOCAL_MIC',
+  `ifdef(`NOAMP',
+       `ifdef(`NOJACK', `define(HDMI_BE_ID_BASE, `0')', `define(HDMI_BE_ID_BASE, `2')')',
+       `ifdef(`EXT_AMP_REF', `define(HDMI_BE_ID_BASE, `4')', `define(HDMI_BE_ID_BASE, `3')')'


that one I don't understand. Didn't we say that we would always reverse 2 links for the amplifiers, regardless of whether the reference is present or not?

@plbossart If we always reverse 2 links for the amplifiers, we need the patch thesofproject/linux#3491 in kernel . Otherwise, the driver won't reverse 2 BE link ID when it is 1308. But thesofproject/linux#3491 is not a good idea because it will impact on the existing platforms. So the driver will reserve 1 BE id for rt1308 and 2 BE ids for rt1316. This requires tplg set the HDMI BE ID based on whether there is EXT_AMP_REF or not.

plbossart · 2022-03-11T02:37:24Z

tools/topology/topology1/sof-icl-rt711-rt1308-rt715-hdmi.m4

+       `ifdef(`NOJACK', `define(HDMI_BE_ID_BASE, `0')', `define(HDMI_BE_ID_BASE, `2')')',
+       `ifdef(`EXT_AMP_REF', `define(HDMI_BE_ID_BASE, `4')', `define(HDMI_BE_ID_BASE, `3')')'
+   )',
+   `define(HDMI_BE_ID_BASE, `5')'


do we really have to do this three-level tests, or could we use the same technique as in the other topology file:

ifdef(`NO_JACK', ` define(JACK_OFFSET, `0') ', ` define(JACK_OFFSET, `2') ') # if there is an external RT1308 amplifier connected over SoundWire, # enable "EXT_AMP" option in the CMakefile. ifdef(`EXT_AMP', ` define(AMP_OFFSET, `1') ', ` define(AMP_OFFSET, `0') ' ) ifdef(`EXT_AMP_REF', ` define(AMP_REF_OFFSET, `1') ', ` define(AMP_REF_OFFSET, `0') ' ) define(HDMI_BE_ID_BASE, eval(JACK_OFFSET+AMP_OFFSET+AMP_REF_OFFSET+DMIC_OFFSET))

sof-tgl-rt711-rt1308 is something wrong. But it currently works because it doesn't meet the corner case. Let's image we don't have Jack and DMIC but have smart AMP on the board. Our driver code will always assigned the AMP BE ID from 2. So in this configuration, the driver will get the HDMI ID from 4, but in the sof-tgl-rt711-rt1308 tplg, HDMI ID will be from 2.

@plbossart Currently, we don't reserve a dai link for amp reference. But we do have a fixed dsi link id for sdw mic.
So the hdmi link will always be 5 if there is a sdw mic present, but it will be 4 if we use a amp without reference.

@libinyang How about

define(HDMI_BE_ID_BASE, `0') ifdef(`NO_JACK', `' ` define(HDMI_BE_ID_BASE, `2') ' ) ifdef(`NOAMP', `' ` define(HDMI_BE_ID_BASE, `3') ' ) ifdef(`EXT_AMP_REF', ` define(HDMI_BE_ID_BASE, `4') ', `' ) ifdef(`NO_LOCAL_MIC', `' ` define(HDMI_BE_ID_BASE, `5') ' )

We set HDMI_BE_ID_BASE = 0 initially, and check headset, amp, reference, mic one by one and overwrite HDMI_BE_ID_BASE when we find any sdw link.

@plbossart The PR test has been passed.

@libinyang you didn't reply to @bardliao 's suggestion

@libinyang you didn't reply to @bardliao 's suggestion

@plbossart @bardliao Oh, sorry. I thought I had replied @bardliao 's suggestion. But it seems I failed to send out my reply. The suggestion can't handle: 'NOAMP' is set but 'NO_LOCAL_MIC' is not set. In this case, there is local MIC, so HDMI BE ID should be 5, but Bard's suggestion will get 3 for HDMI BE ID.

@libinyang you didn't reply to @bardliao 's suggestion

@plbossart @bardliao Oh, sorry. I thought I had replied @bardliao 's suggestion. But it seems I failed to send out my reply. The suggestion can't handle: 'NOAMP' is set but 'NO_LOCAL_MIC' is not set. In this case, there is local MIC, so HDMI BE ID should be 5, but Bard's suggestion will get 3 for HDMI BE ID.

@libinyang Won't the HDMI BE ID be 5 in this case? HDMI_BE_ID_BASE will be overwrite to 5 if NO_LOCAL_MIC is not set, no?

@bardliao Oh, my fault. There's a little fault in your suggestion. We have to undefine HDMI_BE_ID_BASE before redefining it. Please see my new PR.

plbossart · 2022-03-11T02:38:38Z

ping @plbossart

sorry, I didn't see the update. Please use the button at the top right to request a review when you push an update, it makes sure reviewers will see it.

…support add sof-icl-rt711-rt1308-rt715-hdmi NO_LOCAL_MIC & NOAMP support Signed-off-by: Libin Yang <libin.yang@intel.com>

Add sof-adl-rt711-l2-rt1316-l01 support. This tplg doesn't include any local MICs. Signed-off-by: Libin Yang <libin.yang@intel.com>

libinyang · 2022-03-16T09:43:01Z

@plbossart @bardliao patch updated based on Bard's suggestion

plbossart

thanks @libinyang

libinyang · 2022-03-18T01:51:48Z

@plbossart @lrgirdwo Could you please help merge this PR? Thanks.

plbossart · 2022-03-18T15:22:06Z

Quickbuild is stuck @lgirdwood ?

lgirdwood · 2022-03-18T21:12:57Z

@kkarask @wszypelt fyi - looks like CI has been pending too long. Could someone check. Thanks !

wszypelt · 2022-03-21T11:51:23Z

@lgirdwood Unfortunately, there is a problem with the internal service,
they are already working on solving the problem.

libinyang requested review from lgirdwood, plbossart, mmaka1, lbetlej and dbaluta as code owners February 16, 2022 02:17

plbossart requested changes Feb 16, 2022

View reviewed changes

libinyang force-pushed the adl_nodmic branch from 5b18afc to 14ec290 Compare February 16, 2022 06:27

plbossart reviewed Feb 18, 2022

View reviewed changes

libinyang force-pushed the adl_nodmic branch 2 times, most recently from df5f4ff to 3dfb6f1 Compare February 21, 2022 02:23

libinyang force-pushed the adl_nodmic branch from 3dfb6f1 to 468ac6d Compare February 22, 2022 08:06

bardliao previously approved these changes Feb 22, 2022

View reviewed changes

libinyang added 2 commits March 8, 2022 10:48

topology1: sof-icl-rt711-rt1308-rt715-hdmi: remove NO_AGGREGATION

6a7dd27

NO_AGGREGATION is never used now. Let's remove it. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Signed-off-by: Libin Yang <libin.yang@intel.com>

topology1: sof-icl-rt711-rt1308-rt715-hdmi: correct the link description

03a328e

The links are dynamically assigned. The descriptions should be changed correspondingly. Signed-off-by: Libin Yang <libin.yang@intel.com>

libinyang force-pushed the adl_nodmic branch from 2e3a779 to 97845f1 Compare March 8, 2022 03:01

plbossart reviewed Mar 11, 2022

View reviewed changes

libinyang added 2 commits March 16, 2022 17:28

topology1: sof-icl-rt711-rt1308-rt715-hdmi: add NO_LOCAL_MIC & NOAMP …

2bd4770

…support add sof-icl-rt711-rt1308-rt715-hdmi NO_LOCAL_MIC & NOAMP support Signed-off-by: Libin Yang <libin.yang@intel.com>

topology1: CMakefile: add sof-adl-rt711-l2-rt1316-l01

375a469

Add sof-adl-rt711-l2-rt1316-l01 support. This tplg doesn't include any local MICs. Signed-off-by: Libin Yang <libin.yang@intel.com>

libinyang force-pushed the adl_nodmic branch from 97845f1 to 375a469 Compare March 16, 2022 09:41

bardliao approved these changes Mar 16, 2022

View reviewed changes

plbossart approved these changes Mar 16, 2022

View reviewed changes

lgirdwood merged commit 5ef9e49 into thesofproject:main Mar 21, 2022

topology1: add sof-icl-rt711-rt1308-rt715-hdmi NOMIC support #5367

topology1: add sof-icl-rt711-rt1308-rt715-hdmi NOMIC support #5367

Conversation

libinyang commented Feb 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

libinyang Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

libinyang commented Feb 21, 2022

libinyang commented Feb 21, 2022

libinyang commented Feb 21, 2022

bardliao commented Feb 22, 2022

libinyang commented Feb 22, 2022

bardliao commented Feb 22, 2022

libinyang commented Feb 22, 2022 • edited Loading

libinyang commented Feb 22, 2022

libinyang commented Feb 22, 2022

bardliao commented Feb 22, 2022

libinyang commented Feb 22, 2022

libinyang commented Feb 22, 2022

plbossart commented Feb 22, 2022 • edited Loading

libinyang commented Feb 23, 2022

plbossart commented Feb 23, 2022

bardliao commented Feb 23, 2022

plbossart commented Feb 23, 2022

bardliao commented Feb 25, 2022

libinyang commented Mar 1, 2022

plbossart commented Mar 2, 2022

libinyang commented Mar 2, 2022

shumingfan commented Mar 2, 2022

bardliao commented Mar 2, 2022

plbossart commented Mar 2, 2022

libinyang commented Mar 3, 2022 • edited Loading

plbossart commented Mar 4, 2022

libinyang commented Mar 4, 2022

libinyang commented Mar 7, 2022

libinyang commented Mar 8, 2022

plbossart commented Mar 8, 2022

libinyang commented Mar 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

libinyang Mar 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plbossart commented Mar 11, 2022

libinyang commented Mar 16, 2022

plbossart left a comment

Choose a reason for hiding this comment

libinyang commented Mar 18, 2022

plbossart commented Mar 18, 2022

lgirdwood commented Mar 18, 2022

wszypelt commented Mar 21, 2022

libinyang Feb 17, 2022 •

edited

Loading

libinyang commented Feb 22, 2022 •

edited

Loading

plbossart commented Feb 22, 2022 •

edited

Loading

libinyang commented Mar 3, 2022 •

edited

Loading

libinyang Mar 16, 2022 •

edited

Loading