At Fri, 10 Jan 2014 12:29:08 +0000, Liam Girdwood wrote:
On Fri, 2014-01-10 at 13:01 +0100, Takashi Iwai wrote:
At Fri, 10 Jan 2014 19:59:42 +0800, Nenghua Cao wrote:
On 01/10/2014 07:47 PM, Liam Girdwood wrote:
On Fri, 2014-01-10 at 19:21 +0800, Nenghua Cao wrote:
On 01/10/2014 06:55 PM, Takashi Iwai wrote:
[Corrected mail addresses of both Mark and Liam]
Hi, Takashi: Thanks for correcting my mistake.
At Fri, 10 Jan 2014 13:36:35 +0800, Nenghua Cao wrote: > > From: Nenghua Cao nhcao@marvell.com > > It fixes the following case: > Two FEs connects the same BE; FE1 & BE path has been opened and hw_paramed. > At this momment, FE2 & BE path is being opened and hw_paramed. The BE > dai will do hw_param again even if it has done hw_param. It is not > reasonable. > FE1------------>BE > FE2-------------^
What happens if FE2 tries to set up an incompatible hw_params? (Through a quick glance, it won't work properly well, too, though...)
The intention in this case would be for the DSP FE driver to determine if it can perform format conversion or SRC to the running BE. If the DSP cant do the conversion then it should fail.
If FE2 uses an incompatible param, it will make FE1 doesn't work. Maybe FE2 works well. If FE2 uses the same param, BE hw_param function will be called twice (This is the most happening case). So we can't get benefits from it.
We shouldn't be calling the hw_params() on the BE when it's already configured in this case. So this seems like a bug. However :-
/* only allow hw_params() if no connected FEs are running */ if (!snd_soc_dpcm_can_be_params(fe, be, stream)) continue;
if ((be->dpcm[stream].state != SND_SOC_DPCM_STATE_OPEN) && (be->dpcm[stream].state != SND_SOC_DPCM_STATE_HW_PARAMS) && (be->dpcm[stream].state != SND_SOC_DPCM_STATE_HW_FREE)) continue;We do do a test to check if any connected FEs are running (i.e. triggered) prior to calling hw_params() on the BE. Can you confirm if the FE was running in your case ?
Hi, Liam: I am so glad to hear from you. In my case, FE1 has called hw_param, and before FE1 calls prepare/trigger function, the scheduler switches to do FE2 open, hw_param. So hw_param is called twice.
So basically the current implementation is racy about this.
This is a valid use case from the userspace perspective too. The BE in this case is a shared resource (whether userspace is aware or not) and I'd expect it to take the the last configured hw_params (in this case FE2 hw_params) before it is triggered.
Yes, it's how the current code works. But, what if FE1 didn't know that it's shared? (Actually how it can be informed explicitly?) FE1 will still try to feed data in wrong formats/rates/etc, won't it?
At best, it should return an error when an incompatible hw_params setup is done by FE2, IMO. The re-setup by FE1 should be available freely. So, BE needs to remember who has set it up, then allows only the further re-setup by that FE, for example.
Fwiw, a similar topic came up the conference this year wrt compressed streams. The question was about configuring the BE format and rate directly from userspace. This should be possible without too much effort since the BE is essentially a PCM. e.g. from userspace
configure FE1 hw_params
configure FE2 hw_params
optional - configure BE1 hw_params
If step 3 is not performed then the values from step2 are used.
I forgot about this discussion -- so how was the proposal to allow BE's hw_params? A new API, or a flag in hw_params?
OTOH, not calling hw_params twice is also buggy. hw_params may be called multiple times without hw_free for the same stream if user wants to re-setup/update the parameters. OSS emulation layer does it, for example.
This is supported under DPCM unless the BE is triggered, but will always take the last hw_params sent from userspace.
Yes, my comment above is only about the possible side effect by Nenghua's patch.
thanks,
Takashi
Liam
Takashi
Thanks
Liam
Takashi
> > Signed-off-by: Nenghua Cao nhcao@marvell.com > --- > sound/soc/soc-pcm.c | 1 - > 1 files changed, 0 insertions(+), 1 deletions(-) > > diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c > index 891b9a9..ec07e37 100644 > --- a/sound/soc/soc-pcm.c > +++ b/sound/soc/soc-pcm.c > @@ -1339,7 +1339,6 @@ static int dpcm_be_dai_hw_params(struct snd_soc_pcm_runtime *fe, int stream) > continue; > > if ((be->dpcm[stream].state != SND_SOC_DPCM_STATE_OPEN) && > - (be->dpcm[stream].state != SND_SOC_DPCM_STATE_HW_PARAMS) && > (be->dpcm[stream].state != SND_SOC_DPCM_STATE_HW_FREE)) > continue; > > -- > 1.7.0.4 > > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel >
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel