| Message ID | 20220913144319.1055302-4-Vsujithkumar.Reddy@amd.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ed2562c64b4f2cb434420f7d2818d0388250ac1a |
| Headers | show |
| Series | ADD SOF support for rembrandt platform | expand |
Il 13/09/22 16:43, V sujith kumar Reddy ha scritto: > Add I2S HS control instance to the sof core. > This will help the amd topology to use the I2S HS Dai. > > Signed-off-by: V sujith kumar Reddy <Vsujithkumar.Reddy@amd.com> Hello, Since this patch was merged, SoundOpenFirmware stopped working on MediaTek MT8195, as it fails on DAI component creation (firmware side)... check below... > --- > include/sound/sof/dai.h | 2 ++ > sound/soc/sof/ipc3-pcm.c | 9 +++++++++ > sound/soc/sof/ipc3-topology.c | 33 +++++++++++++++++++++++++++++++++ > sound/soc/sof/topology.c | 1 + > 4 files changed, 45 insertions(+) > > diff --git a/include/sound/sof/dai.h b/include/sound/sof/dai.h > index 21d98f31a9ca..83fd81c82e4c 100644 > --- a/include/sound/sof/dai.h > +++ b/include/sound/sof/dai.h > @@ -84,6 +84,7 @@ enum sof_ipc_dai_type { > SOF_DAI_AMD_BT, /**< AMD ACP BT*/ > SOF_DAI_AMD_SP, /**< AMD ACP SP */ > SOF_DAI_AMD_DMIC, /**< AMD ACP DMIC */ > + SOF_DAI_AMD_HS, /**< Amd HS */ > SOF_DAI_MEDIATEK_AFE, /**< Mediatek AFE */ Adding SOF_DAI_AMD_HS before SOF_DAI_MEDIATEK_AFE desynced this enumeration so the DAI type is now 11 and not 10 anymore, leading to a failure in firmware at IPC3 helper function `dai_get()`, as when `dai_find_type()` is called, the DAI type that the firmware expects doesn't match with the one that gets sent in the request message from the kernel. As a local test, I tried moving SOF_DAI_AMD_HS after SOF_DAI_MEDIATEK_AFE and this has restored full functionality on my MT8195 platform (Tomato Chromebook). If SOF is supposed to guarantee backwards compatibility (and I believe it is), this commit breaks that. I would be tempted to send a commit that moves SOF_DAI_AMD_HS to the end, but that would break the already compiled firmware for AMD platforms, so I am not sure how to proceed. So... how can we solve that? Any ideas? P.S.: Sharing some logs at the end of this email, just for completeness. Best regards, Angelo .. Relevant firmware and kernel trace/debug log lines .. Log from Xtensa DSP: [ 828266.737921] ( 3.125000) c0 ipc src/ipc/ipc3/handler.c:1579 INFO ipc: new cmd 0x30010000 [ 828273.404587] ( 6.666667) c0 component src/ipc/ipc3/helper.c:296 INFO comp new dai <c2b00d27-ffbc-4150-a51a-245c79c5e54b> type 2 id 4.22 [ 828284.342087] ( 10.937500) c0 dai src/audio/dai.c:177 ERROR dai_new(): dai_get() failed to create DAI. [ 828291.321253] ( 6.979167) c0 dai src/ipc/ipc3/helper.c:303 ERROR comp_new(): unable to create the new component [ 828295.383753] ( 4.062500) c0 ipc src/ipc/ipc3/helper.c:624 ERROR ipc_comp_new(): component cd = NULL [ 828299.654586] ( 4.270833) c0 ipc src/ipc/ipc3/handler.c:1248 ERROR ipc: pipe 4 comp 22 creation failed -22 Kernel log: [ 15.011677] sof-audio-of-mt8195 10803000.dsp: request_firmware mediatek/sof/sof-mt8195.ri successful ............... [ 15.021452] sof-audio-of-mt8195 10803000.dsp: Firmware info: version 2:0:0-df141 [ 15.039661] sof-audio-of-mt8195 10803000.dsp: Firmware: ABI 3:21:0 Kernel ABI 3:23:0 [ 15.039663] sof-audio-of-mt8195 10803000.dsp: found sof_ext_man header type 2 size 0x70 [ 15.039665] sof-audio-of-mt8195 10803000.dsp: Firmware info: used compiler XCC 12:0:8 <RI-2019.1-linux> used optimization flags -O2 ........... [ 15.107660] sof-audio-of-mt8195 10803000.dsp: Firmware: DBG_ABI 5:3:0 [ 15.292019] sof-audio-of-mt8195 10803000.dsp: booting DSP firmware [ 15.292025] sof-audio-of-mt8195 10803000.dsp: HIFIxDSP boot from base : 0x40000000 [ 15.297257] sof-audio-of-mt8195 10803000.dsp: ipc rx: 0x70000000: FW_READY [ 15.363305] sof-audio-of-mt8195 10803000.dsp: DSP is ready 0x70000000 offset 0x800000 [ 15.363319] sof-audio-of-mt8195 10803000.dsp: Firmware info: version 2:0:0-df141 [ 15.383651] sof-audio-of-mt8195 10803000.dsp: Firmware: ABI 3:21:0 Kernel ABI 3:23:0 ............ [ 16.336460] sof-audio-of-mt8195 10803000.dsp: loaded host PCM16P [ 16.336461] sof-audio-of-mt8195 10803000.dsp: config: periods snk 2 src 0 fmt 0 [ 16.336466] sof-audio-of-mt8195 10803000.dsp: ipc tx: 0x30100000: GLB_TPLG_MSG: PIPE_NEW [ 16.336603] sof-audio-of-mt8195 10803000.dsp: widget PIPELINE.4.AFE3.IN setup complete [ 16.336607] sof-audio-of-mt8195 10803000.dsp: ipc tx: 0x30010000: GLB_TPLG_MSG: COMP_NEW [ 16.336663] sof-audio-of-mt8195 10803000.dsp: ipc tx error for 0x30010000 (msg/reply size: 96/20): -22 [ 16.336665] sof-audio-of-mt8195 10803000.dsp: Failed to setup widget AFE3.IN [ 16.336670] sof-audio-of-mt8195 10803000.dsp: ipc tx: 0x30110000: GLB_TPLG_MSG: PIPE_FREE [ 16.336778] sof-audio-of-mt8195 10803000.dsp: widget PIPELINE.4.AFE3.IN freed [ 16.336887] sof-audio-of-mt8195 10803000.dsp: error: tplg component load failed -22 [ 16.336899] sof-audio-of-mt8195 10803000.dsp: error: failed to load DSP topology -22 [ 16.336900] sof-audio-of-mt8195 10803000.dsp: ASoC: error at snd_soc_component_probe on 10803000.dsp: -22 [ 16.336983] mt8195_mt6359 mt8195-sound: ASoC: failed to instantiate card -22 [ 16.340339] mt8195_mt6359: probe of mt8195-sound failed with error -22
>> diff --git a/include/sound/sof/dai.h b/include/sound/sof/dai.h >> index 21d98f31a9ca..83fd81c82e4c 100644 >> --- a/include/sound/sof/dai.h >> +++ b/include/sound/sof/dai.h >> @@ -84,6 +84,7 @@ enum sof_ipc_dai_type { >> SOF_DAI_AMD_BT, /**< AMD ACP BT*/ >> SOF_DAI_AMD_SP, /**< AMD ACP SP */ >> SOF_DAI_AMD_DMIC, /**< AMD ACP DMIC */ >> + SOF_DAI_AMD_HS, /**< Amd HS */ >> SOF_DAI_MEDIATEK_AFE, /**< Mediatek AFE */ > > Adding SOF_DAI_AMD_HS before SOF_DAI_MEDIATEK_AFE desynced this enumeration > so the DAI type is now 11 and not 10 anymore, leading to a failure in > firmware > at IPC3 helper function `dai_get()`, as when `dai_find_type()` is > called, the > DAI type that the firmware expects doesn't match with the one that gets > sent > in the request message from the kernel. > > As a local test, I tried moving SOF_DAI_AMD_HS after > SOF_DAI_MEDIATEK_AFE and > this has restored full functionality on my MT8195 platform (Tomato > Chromebook). > > If SOF is supposed to guarantee backwards compatibility (and I believe > it is), > this commit breaks that. > > I would be tempted to send a commit that moves SOF_DAI_AMD_HS to the > end, but > that would break the already compiled firmware for AMD platforms, so I > am not > sure how to proceed. D'oh. Yes this breaks backwards-compatibility and this is a clear mistake. I think your suggestion to add the AMD_HS at the end is the only practical solution indeed - this would need to be done for both kernel and SOF version of dai.h.
Il 16/11/22 16:04, Pierre-Louis Bossart ha scritto: > >>> diff --git a/include/sound/sof/dai.h b/include/sound/sof/dai.h >>> index 21d98f31a9ca..83fd81c82e4c 100644 >>> --- a/include/sound/sof/dai.h >>> +++ b/include/sound/sof/dai.h >>> @@ -84,6 +84,7 @@ enum sof_ipc_dai_type { >>> SOF_DAI_AMD_BT, /**< AMD ACP BT*/ >>> SOF_DAI_AMD_SP, /**< AMD ACP SP */ >>> SOF_DAI_AMD_DMIC, /**< AMD ACP DMIC */ >>> + SOF_DAI_AMD_HS, /**< Amd HS */ >>> SOF_DAI_MEDIATEK_AFE, /**< Mediatek AFE */ >> >> Adding SOF_DAI_AMD_HS before SOF_DAI_MEDIATEK_AFE desynced this enumeration >> so the DAI type is now 11 and not 10 anymore, leading to a failure in >> firmware >> at IPC3 helper function `dai_get()`, as when `dai_find_type()` is >> called, the >> DAI type that the firmware expects doesn't match with the one that gets >> sent >> in the request message from the kernel. >> >> As a local test, I tried moving SOF_DAI_AMD_HS after >> SOF_DAI_MEDIATEK_AFE and >> this has restored full functionality on my MT8195 platform (Tomato >> Chromebook). >> >> If SOF is supposed to guarantee backwards compatibility (and I believe >> it is), >> this commit breaks that. >> >> I would be tempted to send a commit that moves SOF_DAI_AMD_HS to the >> end, but >> that would break the already compiled firmware for AMD platforms, so I >> am not >> sure how to proceed. > > D'oh. Yes this breaks backwards-compatibility and this is a clear > mistake. I think your suggestion to add the AMD_HS at the end is the > only practical solution indeed - this would need to be done for both > kernel and SOF version of dai.h. > Okay, I will send a commit tomorrow :-) Thanks, Angelo
On 11/16/22 10:33, AngeloGioacchino Del Regno wrote: > Il 16/11/22 16:04, Pierre-Louis Bossart ha scritto: >> >>>> diff --git a/include/sound/sof/dai.h b/include/sound/sof/dai.h >>>> index 21d98f31a9ca..83fd81c82e4c 100644 >>>> --- a/include/sound/sof/dai.h >>>> +++ b/include/sound/sof/dai.h >>>> @@ -84,6 +84,7 @@ enum sof_ipc_dai_type { >>>> SOF_DAI_AMD_BT, /**< AMD ACP BT*/ >>>> SOF_DAI_AMD_SP, /**< AMD ACP SP */ >>>> SOF_DAI_AMD_DMIC, /**< AMD ACP DMIC */ >>>> + SOF_DAI_AMD_HS, /**< Amd HS */ >>>> SOF_DAI_MEDIATEK_AFE, /**< Mediatek AFE */ >>> >>> Adding SOF_DAI_AMD_HS before SOF_DAI_MEDIATEK_AFE desynced this >>> enumeration >>> so the DAI type is now 11 and not 10 anymore, leading to a failure in >>> firmware >>> at IPC3 helper function `dai_get()`, as when `dai_find_type()` is >>> called, the >>> DAI type that the firmware expects doesn't match with the one that gets >>> sent >>> in the request message from the kernel. >>> >>> As a local test, I tried moving SOF_DAI_AMD_HS after >>> SOF_DAI_MEDIATEK_AFE and >>> this has restored full functionality on my MT8195 platform (Tomato >>> Chromebook). >>> >>> If SOF is supposed to guarantee backwards compatibility (and I believe >>> it is), >>> this commit breaks that. >>> >>> I would be tempted to send a commit that moves SOF_DAI_AMD_HS to the >>> end, but >>> that would break the already compiled firmware for AMD platforms, so I >>> am not >>> sure how to proceed. >> >> D'oh. Yes this breaks backwards-compatibility and this is a clear >> mistake. I think your suggestion to add the AMD_HS at the end is the >> only practical solution indeed - this would need to be done for both >> kernel and SOF version of dai.h. >> > > Okay, I will send a commit tomorrow :-) I sent those two GitHub pull requests already: https://github.com/thesofproject/linux/pull/4017 https://github.com/thesofproject/sof/pull/6616
Il 16/11/22 17:55, Pierre-Louis Bossart ha scritto: > > > On 11/16/22 10:33, AngeloGioacchino Del Regno wrote: >> Il 16/11/22 16:04, Pierre-Louis Bossart ha scritto: >>> >>>>> diff --git a/include/sound/sof/dai.h b/include/sound/sof/dai.h >>>>> index 21d98f31a9ca..83fd81c82e4c 100644 >>>>> --- a/include/sound/sof/dai.h >>>>> +++ b/include/sound/sof/dai.h >>>>> @@ -84,6 +84,7 @@ enum sof_ipc_dai_type { >>>>> SOF_DAI_AMD_BT, /**< AMD ACP BT*/ >>>>> SOF_DAI_AMD_SP, /**< AMD ACP SP */ >>>>> SOF_DAI_AMD_DMIC, /**< AMD ACP DMIC */ >>>>> + SOF_DAI_AMD_HS, /**< Amd HS */ >>>>> SOF_DAI_MEDIATEK_AFE, /**< Mediatek AFE */ >>>> >>>> Adding SOF_DAI_AMD_HS before SOF_DAI_MEDIATEK_AFE desynced this >>>> enumeration >>>> so the DAI type is now 11 and not 10 anymore, leading to a failure in >>>> firmware >>>> at IPC3 helper function `dai_get()`, as when `dai_find_type()` is >>>> called, the >>>> DAI type that the firmware expects doesn't match with the one that gets >>>> sent >>>> in the request message from the kernel. >>>> >>>> As a local test, I tried moving SOF_DAI_AMD_HS after >>>> SOF_DAI_MEDIATEK_AFE and >>>> this has restored full functionality on my MT8195 platform (Tomato >>>> Chromebook). >>>> >>>> If SOF is supposed to guarantee backwards compatibility (and I believe >>>> it is), >>>> this commit breaks that. >>>> >>>> I would be tempted to send a commit that moves SOF_DAI_AMD_HS to the >>>> end, but >>>> that would break the already compiled firmware for AMD platforms, so I >>>> am not >>>> sure how to proceed. >>> >>> D'oh. Yes this breaks backwards-compatibility and this is a clear >>> mistake. I think your suggestion to add the AMD_HS at the end is the >>> only practical solution indeed - this would need to be done for both >>> kernel and SOF version of dai.h. >>> >> >> Okay, I will send a commit tomorrow :-) > > I sent those two GitHub pull requests already: > > https://github.com/thesofproject/linux/pull/4017 > https://github.com/thesofproject/sof/pull/6616 > Thanks for the fast action. Regards, Angelo
diff --git a/include/sound/sof/dai.h b/include/sound/sof/dai.h index 21d98f31a9ca..83fd81c82e4c 100644 --- a/include/sound/sof/dai.h +++ b/include/sound/sof/dai.h @@ -84,6 +84,7 @@ enum sof_ipc_dai_type { SOF_DAI_AMD_BT, /**< AMD ACP BT*/ SOF_DAI_AMD_SP, /**< AMD ACP SP */ SOF_DAI_AMD_DMIC, /**< AMD ACP DMIC */ + SOF_DAI_AMD_HS, /**< Amd HS */ SOF_DAI_MEDIATEK_AFE, /**< Mediatek AFE */ }; @@ -112,6 +113,7 @@ struct sof_ipc_dai_config { struct sof_ipc_dai_acp_params acpbt; struct sof_ipc_dai_acp_params acpsp; struct sof_ipc_dai_acpdmic_params acpdmic; + struct sof_ipc_dai_acp_params acphs; struct sof_ipc_dai_mtk_afe_params afe; }; } __packed; diff --git a/sound/soc/sof/ipc3-pcm.c b/sound/soc/sof/ipc3-pcm.c index 9c6a84bdeca7..dad57bef38f6 100644 --- a/sound/soc/sof/ipc3-pcm.c +++ b/sound/soc/sof/ipc3-pcm.c @@ -346,6 +346,15 @@ static int sof_ipc3_pcm_dai_link_fixup(struct snd_soc_pcm_runtime *rtd, dev_dbg(component->dev, "AMD_SP channels_min: %d channels_max: %d\n", channels->min, channels->max); break; + case SOF_DAI_AMD_HS: + rate->min = private->dai_config->acphs.fsync_rate; + rate->max = private->dai_config->acphs.fsync_rate; + channels->min = private->dai_config->acphs.tdm_slots; + channels->max = private->dai_config->acphs.tdm_slots; + + dev_dbg(component->dev, + "AMD_HS channel_max: %d rate_max: %d\n", channels->max, rate->max); + break; case SOF_DAI_AMD_DMIC: rate->min = private->dai_config->acpdmic.pdm_rate; rate->max = private->dai_config->acpdmic.pdm_rate; diff --git a/sound/soc/sof/ipc3-topology.c b/sound/soc/sof/ipc3-topology.c index 65923e7a5976..64f0bf60a11d 100644 --- a/sound/soc/sof/ipc3-topology.c +++ b/sound/soc/sof/ipc3-topology.c @@ -1217,6 +1217,36 @@ static int sof_link_acp_sp_load(struct snd_soc_component *scomp, struct snd_sof_ return 0; } +static int sof_link_acp_hs_load(struct snd_soc_component *scomp, struct snd_sof_dai_link *slink, + struct sof_ipc_dai_config *config, struct snd_sof_dai *dai) +{ + struct snd_soc_tplg_hw_config *hw_config = slink->hw_configs; + struct sof_dai_private_data *private = dai->private; + u32 size = sizeof(*config); + + /* Configures the DAI hardware format and inverted clocks */ + sof_dai_set_format(hw_config, config); + + /* init IPC */ + memset(&config->acphs, 0, sizeof(config->acphs)); + config->hdr.size = size; + + config->acphs.fsync_rate = le32_to_cpu(hw_config->fsync_rate); + config->acphs.tdm_slots = le32_to_cpu(hw_config->tdm_slots); + + dev_info(scomp->dev, "ACP_HS config ACP%d channel %d rate %d\n", + config->dai_index, config->acphs.tdm_slots, + config->acphs.fsync_rate); + + dai->number_configs = 1; + dai->current_config = 0; + private->dai_config = kmemdup(config, size, GFP_KERNEL); + if (!private->dai_config) + return -ENOMEM; + + return 0; +} + static int sof_link_afe_load(struct snd_soc_component *scomp, struct snd_sof_dai_link *slink, struct sof_ipc_dai_config *config, struct snd_sof_dai *dai) { @@ -1510,6 +1540,9 @@ static int sof_ipc3_widget_setup_comp_dai(struct snd_sof_widget *swidget) case SOF_DAI_AMD_SP: ret = sof_link_acp_sp_load(scomp, slink, config, dai); break; + case SOF_DAI_AMD_HS: + ret = sof_link_acp_hs_load(scomp, slink, config, dai); + break; case SOF_DAI_AMD_DMIC: ret = sof_link_acp_dmic_load(scomp, slink, config, dai); break; diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c index 9273a70fec25..f7b3302ec202 100644 --- a/sound/soc/sof/topology.c +++ b/sound/soc/sof/topology.c @@ -287,6 +287,7 @@ static const struct sof_dai_types sof_dais[] = { {"ACP", SOF_DAI_AMD_BT}, {"ACPSP", SOF_DAI_AMD_SP}, {"ACPDMIC", SOF_DAI_AMD_DMIC}, + {"ACPHS", SOF_DAI_AMD_HS}, {"AFE", SOF_DAI_MEDIATEK_AFE}, };
Add I2S HS control instance to the sof core. This will help the amd topology to use the I2S HS Dai. Signed-off-by: V sujith kumar Reddy <Vsujithkumar.Reddy@amd.com> --- include/sound/sof/dai.h | 2 ++ sound/soc/sof/ipc3-pcm.c | 9 +++++++++ sound/soc/sof/ipc3-topology.c | 33 +++++++++++++++++++++++++++++++++ sound/soc/sof/topology.c | 1 + 4 files changed, 45 insertions(+)