Uh oh!
There was an error while loading. Please reload this page.
get spcm from FE dai link during BE dai link prepare#834
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -169,19 +169,28 @@ static int hda_link_pcm_prepare(struct snd_pcm_substream *substream, | ||
| struct snd_soc_dai *dai) | ||
| { | ||
| struct snd_soc_pcm_runtime *rtd = substream->private_data; | ||
| struct snd_soc_dpcm *dpcm; | ||
| struct snd_sof_dev *sdev = | ||
| snd_soc_component_get_drvdata(dai->component); | ||
| struct snd_sof_pcm *spcm; | ||
| int stream = substream->stream; | ||
| for_each_dpcm_fe(rtd, stream, dpcm) { | ||
| spcm = snd_sof_find_spcm_dai(sdev, dpcm->fe); | ||
| if (!spcm) | ||
| return -EINVAL; | ||
There was a problem hiding this comment. @ranj063 I'm just wondering in which case spcm will be NULL. I believe the initial intent of prepare() is to set link DMA hw_params when system returns from S3. If spcm can be NULL, does it mean that link DMA register is not restored 100%? So my concern is whether it will impact on the S3 stress test? Or can we make sure that every time return from S3, spcm is always valid? There was a problem hiding this comment. Or use continue, not return? Any suggestion? @plbossart@xiulipan CollaboratorAuthor There was a problem hiding this comment. @libinyang if there is a FE connected to a BE dai, there will always be a valid spcm. But I can't use snd_sof_find_spcm_dai() without checking. ANd I dont think -ENODEV is the correct return value. -EINVAL is more apprppriate. There was a problem hiding this comment. @ranj063 If the prepare() is called, the BE we are using should alwasy connect to a FE. So we don't need to have a check?
CollaboratorAuthor There was a problem hiding this comment. @libinyang sorry but if snd_sof_find_spcm_dai() can return NULL, the return value has to be checked. There was a problem hiding this comment. @ranj063 What I concern is if "return -EINVAL“ is really executed in some case, does it mean the link DMA register will not be restored when resume from S3. CollaboratorAuthor There was a problem hiding this comment. @libinyang yes, if it is invalid, prepare will fail and the link dam will not resume properly. But that should be expected isnt it? I dont understand your point. There was a problem hiding this comment. Link DMA not being restored properly means resume from S3 when playback will have issue just like we met before. CollaboratorAuthor There was a problem hiding this comment. @libinyang you are making this review unnecessarily complicated. If there is an error, we cannot continue. A program does not guarantee it will always succeed. But it should be capture all possible errors and take actions appropriately. If there is no spcm for a FE dai, it is an error and we should quit at this point. There was a problem hiding this comment. @ranj063 After a second thought, maybe I did think too complicated. Mostly spcm should not be NULL. If it's NULL, it really means there is something wrong. So we will not garantee it works properly. And we currently only connect to one FE for a BE. So 'continue' and 'return error' directly should be the same. I'm OK with this patch now. If spcm is NULL, just let it be wrong and report it. There was a problem hiding this comment. I'm not sure EINVAL is a good errno or not. Maybe ENODEV is better? As you know, Linus is very strict with the errno. | ||
| /* | ||
| * setup hw_params only if resuming from system suspend. | ||
| * The hw_params_upon_resume flag is set for suspended | ||
| * streams during system suspend. | ||
| */ | ||
| if (spcm->hw_params_upon_resume[stream]) | ||
There was a problem hiding this comment. Please comment when hw_params_upon_resume is be set. It will help us to understand the whole flow. | ||
| goto hw_params; | ||
| } | ||
| return 0; | ||
| hw_params: | ||
| dev_dbg(sdev->dev, "hda: prepare stream %d dir %d\n", | ||
| spcm->pcm.pcm_id, substream->stream); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if that BE is currently not connected to a FE due to mixer settings?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@plbossart i saw your argument in Rander's PR about this too but I cant quite wrap my head around it. Are you talking about hostless pcm's?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, a front-end and a back-end connected by a switch which is off. Same as on Baytrail with the 'no BE error' without the mixers properly set-up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Google "Baytrail Audio Port: ASoC: no backend DAIs enabled for Baytrail Audio Port"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@plbossart dpcm is certainly not my expertise. But let me take a stab at arguing that this case will be OK:
soc_dpcm_runtime_update() is called by DAPM mixer/mux changes. So when the switch is off, the runtime update will disconnect all BE dai links that were previously connected.
I dont have any proof of this working as we dont have a pipeline with a mux to test it out. So this is purely what I understood from the dpcm flow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems we did not need to handle the dpcm cases here. We will only find some PCM that is connecting to the BE, then get the status from spcm, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xiulipan Yes, the purpose, I think, is to find whether the link dma used by the substream needs restore registers or not. However, spcm is used for fe. I think this is why the dpcm is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could have 2 FEs that are connected to a BE in a mutually exclusive manner, or get mixed. So when a path is established, which FE is used completely depends on the configuration of the switches. If you have a mixer it's also unclear to me which FE you'd select.
We really need to build a topology with 2 BEs and 2 FEs, and a set of switches/mixer components to deal with the 2x2 connection. That's the only way to progress with those DPCM questions.