topology2: add reference capture for MAX98357A speaker amp #6880
Conversation
yongzhi1
requested review from
dbaluta,
lbetlej,
lgirdwood,
mmaka1 and
plbossart
as code owners
|
This is for MAX98357A speaker amp which does not have IV feedback capability, I am not sure about any side effect of lbm_mode, other than that any comments/suggestion? |
|
@yongzhi1 Using LBM makes sense to me. |
yongzhi1
changed the title
Ack, tested working with arecord based on sof-dev kernel. |
Not sure why we need to fake IV feedback here with loop back ? Can you provide context. Fwiw, The code looks fine so if not suitable for this use case could be used for testing. |
|
In addition to @lgirdwood the open is "Why do we need an echo reference"? |
My understanding is that we did the same for max98360a, we now get the loopback data at I2S instead of MUXDEMUX as shown in sof-adl-max98360a-rt5682.tplg:
|
nope, this is using the loopback with the demux. The SSP0 is only used because we need a DPCM backend, this is the reason why we had issues with the updated kernel where the .dpcm_capture field was not set. thesofproject/linux#4083 |
Hi, @plbossart, yes, totally agree with above, think @bardliao also followed same approach to use copier-as-demux initially #6657. |
Hi, @plbossart , to use ACE in the DSP, I’d be tempted to follow #6601, but it's a little over-kill to have "three copier.module for a simple demux functionality.". Hi, @sathya-nujella , what's your opinion on above "Why do we need" question? |
Yes. In playback path Post Processing/non-linear processing module would be enabled. Hence the requirement is to give this as echo ref PCM to CRAS/user space. |
|
@plbossart This PR is trying to get reference with loop back mode which should have the same result as getting reference from a demux like sof-adl-max98360a-rt5682.tplg does and will be simpler. |
yongzhi1
force-pushed
the
ref-cap-for-dumb-amp
branch
from
c062233 to
7daa73b
Compare
|
V2: Added copier module in the pipeline to use as demux.
|
Hi, @lgirdwood, MAX98357A is a dumb amp that does not support feedback data in PCM, so it's "fake" (not true IV). |
Thanks @yongzhi1, got you, that was confusing me. @plbossart is this the OK for you ? |
plbossart
left a comment
plbossart
left a comment
There was a problem hiding this comment.
not getting why there's both LBM and demux (with the copier).
There was a problem hiding this comment.
why are we using LBM mode if there's a demux?
There was a problem hiding this comment.
Hi, @plbossart , the demux is put on capture pipeline to connect with ACE module in the future.
There was a problem hiding this comment.
can you share the picture of the new topology, will be easier to see. Thanks.
There was a problem hiding this comment.
Hi, @plbossart
Here is the picture with copier.module.8.2 added:
#6880 (comment)
There was a problem hiding this comment.
Are you saying that copier.module.8.2 would have at some point additional sinks connected? I am struggling between what you want now and what will be done later.
There was a problem hiding this comment.
Hi, @plbossart , yes, that's my current thoughts, something like this (sorry for the ugly flow chart):
graph TD;
SSP.IN-->copier.module.8.2;
copier.module.8.2 --> echo-ref-PCM;
copier.module.8.2 --> AEC;
subgraph ide1 [future]
PDM --> AEC;
AEC --> DMIC48K;
end
There was a problem hiding this comment.
can we expand the comment to say reference capture please?
There was a problem hiding this comment.
why do you need to set this?
There was a problem hiding this comment.
Here I just followed the example of similar place like:
https://github.com/thesofproject/sof/blob/main/tools/topology/topology2/cavs-nocodec.conf#L289
It's more out of convention than necessity.
There was a problem hiding this comment.
lets not set it blindly if it is not needed,
There was a problem hiding this comment.
pcm.2 please. Lets keep it incremental as it is really only an instance ID.
There was a problem hiding this comment.
Ack, here I followed #6783 (comment) to use 27 for PCM, yes, the ID can be 2.
There was a problem hiding this comment.
@yongzhi1 the 27 is set below in the ID attribute. What I mean was the instance ID here doesnt need to match it
There was a problem hiding this comment.
can we please wait till #6986 merged and update this to use instance ID to instantiate?
There was a problem hiding this comment.
Can you make this string "Echo reference" a variable defined in the Define {} block so you can use it here and in the pcm caps object below?
There was a problem hiding this comment.
this route can go into the pipeline definition passthrough-copier-be.conf
There was a problem hiding this comment.
Let me think about it, thanks for your review!!
yongzhi1
force-pushed
the
ref-cap-for-dumb-amp
branch
from
7daa73b to
d0e0e6c
Compare
|
V3 update:
|
There was a problem hiding this comment.
why 200? please use incremental values so 1 in this case.
There was a problem hiding this comment.
Ack, the 200 serves as a warning that this instance was referenced elsewhere, do not change one place without changing another .. . will use incremental values for this and other places (route index etc.)
There was a problem hiding this comment.
@yongzhi1 its the other way around, only included files need to set large instance IDs atm. top-level files should not. But in any case once my alsautils PR is merged, we'll not need to worry about this
There was a problem hiding this comment.
can we call it dai-copier-buffer.conf? all copiers are capable of format conversion. So its not just passthrough anymore
There was a problem hiding this comment.
I have a PR submitted to alsautils alsa-project/alsa-utils#188 to avoid such collisions in the alsatplg compiler instead of having to do this manually.
There was a problem hiding this comment.
Can we not use "dumb" in the name of the file please. Can you call this simply speaker-echo-ref.conf? What makes this exclusive to not-so-smart amps? When needed, the same file could be included in the case of smart amps too right?
There was a problem hiding this comment.
I agree the file name is rather dumb, for amps with IV data, we do not need the lbm_mode. Will update the file name.
tools/topology/topology2/platform/intel/i2s-dumb-amp-echo-ref.conf
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
similarly for direction here
There was a problem hiding this comment.
Ack, will update in V4.
yongzhi1
force-pushed
the
ref-cap-for-dumb-amp
branch
from
d0e0e6c to
28d05bb
Compare
|
V4 update:
|
There was a problem hiding this comment.
there's only 1 audio format
There was a problem hiding this comment.
Ack, cut & paste error :)
I saw it was fixed by 48afcef
There was a problem hiding this comment.
are these period counts really useful?
There was a problem hiding this comment.
I must admit I do not know.
There was a problem hiding this comment.
lets remove it. im pretty sure these are not used in ipc4
There was a problem hiding this comment.
@yongzhi1 lets not do this. My alsauilts PR will not allow object overrides. You need to set the direction and lbm mode to duplex is INCLUDE_ECHO_REF is true in the top-level file.
There was a problem hiding this comment.
The lbm_mode is the only valid value for SSP quirks, I will think about your suggestion.
!valid_values [ "lbm_mode" ]
There was a problem hiding this comment.
@yongzhi1 what you need is something like this. You will need the latest upstream alsa-utils for this to work.
diff --git a/tools/topology/topology2/cavs-rt5682.conf b/tools/topology/topology2/cavs-rt5682.conf
index b04957d99..e13bc362c 100644
--- a/tools/topology/topology2/cavs-rt5682.conf
+++ b/tools/topology/topology2/cavs-rt5682.conf
@@ -116,7 +116,15 @@ Object.Dai {
SSP."1" {
id $SPK_ID
dai_index $SPEAKER_SSP_DAI_INDEX
+IncludeByKey.INCLUDE_ECHO_REF {
+"false" {
direction "playback"
+ }
+"true" {
+ direction "duplex"
+ quirks "lbm_mode"
+ }
+}
name $SPEAKER_CODEC_NAME
default_hw_conf_id 0
sample_bits 32
There was a problem hiding this comment.
Ack, fortunately I waited a while before attempting the changes, the new capability arrived to rescue at the right moment:
alsa-project/alsa-utils@1350900
yongzhi1
force-pushed
the
ref-cap-for-dumb-amp
branch
from
28d05bb to
1dd9a32
Compare
There was a problem hiding this comment.
BTW @yongzhi1 if you want to give it a try. you can start your object instances from 1 for the pcm and routes in this conf file with this PR alsa-project/alsa-utils#188. The compiler will take care of avoiding clashes with the top-level file objects
There was a problem hiding this comment.
Lets do this as part of the topology2 feature cleanup. @jsarha fyi.
|
V5 update
|
Use lbm_mode to loopback playback data for PCM27. Add copier module in DAI BE for demux function. Signed-off-by: Yong Zhi <yong.zhi@intel.com>
yongzhi1
force-pushed
the
ref-cap-for-dumb-amp
branch
from
1dd9a32 to
805ab3e
Compare
|
Pushed for SOFCI TEST. |
|
SOFCI TEST |
|
@yongzhi1 @ranj063 @lgirdwood I think we have a problem. This doesn't build and for some reason, pr-tools-build is not running topology2 check. FYI @marc-hb @greg-intel @fredoh9 @keqiaozhang |
|
so we need very latest alsa-utils for this, where ever it needs to be updated... |
|
Yes, the tplg2 coverage seems ok after all, so this was a false alarm. One does need to have a recent alsa-utils to have the build work, but the CI checks seem good with regard to this. |
@kv2019i the CI docker image has been updated with the latest alsa-utils. The build did pass right? |
|
Yes this builds ONLY if you update your docker image maintained by @fredoh9 |
URL? This one looks OK https://sof-ci.01.org/sofpr/PR6880/build3554/build/tools.txt |
9 participants
Use lbm_mode to loopback playback data for PCM27.
Signed-off-by: Yong Zhi yong.zhi@intel.com