Re: [alsa-devel] [PATCH] ASoC: dpcm: don't do hw_param when BE has done hw_param

[Takashi Iwai <tiwai@suse.de>]

At Fri, 10 Jan 2014 20:22:59 +0800,
Nenghua Cao wrote:
> 
> On 01/10/2014 08:01 PM, 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.
> > 
> > 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.
> >
> Hi, Takashi
> 
>   On current alsa framework, the hw_param can be called multiple time.
> Here, FE1 also can call do it.

But it won't any longer if your patch is applied, no?


Takashi

> But here we should add constraint to
> avoid another FE call it due to FE1 has choose it priority.
> 
> > 
> > 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
>