[PATCH] ASoC: dpcm: don't do hw_param when BE has done hw_param

[PATCH] ASoC: dpcm: don't do hw_param when BE has done hw_param

[Nenghua Cao]

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-------------^

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

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

[Takashi Iwai]

[Corrected mail addresses of both Mark and Liam]

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...)


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
> 

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

[Nenghua Cao]

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...)
> 
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.
> 
> 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
>>

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

[Liam Girdwood]

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 ?

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
> >>
> 

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

[Nenghua Cao]

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.

> 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
>>>>
>>
> 
> 
> 

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

[Takashi Iwai]

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.


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
> >>>>
> >>
> > 
> > 
> > 
> 

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

[Nenghua Cao]

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 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
>>>>>>
>>>>
>>>
>>>
>>>
>>

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

[Liam Girdwood]

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.

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

1) configure FE1 hw_params

2) configure FE2 hw_params

3) optional - configure BE1 hw_params

If step 3 is not performed then the values from step2 are used.

> 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.

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
> > >>>>
> > >>
> > > 
> > > 
> > > 
> > 

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

[Nenghua Cao]

On 01/10/2014 08:29 PM, 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.
> 
> 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
> 
> 1) configure FE1 hw_params
> 
> 2) configure FE2 hw_params
> 
> 3) optional - configure BE1 hw_params
> 
> If step 3 is not performed then the values from step2 are used.
> 
It sounds good. I only have one concern. Even if the FE1 and FE2 use the
same param, the hw_param may be called more times.

Nenghua
>> 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.
> 
> 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
>>>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>
> 
> 
> 

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

[Takashi Iwai]

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
> 

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

[Takashi Iwai]

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
> 
> 1) configure FE1 hw_params
> 
> 2) configure FE2 hw_params
> 
> 3) 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
> 

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

[Liam Girdwood]

On Fri, 2014-01-10 at 14:46 +0100, Takashi Iwai wrote:
> 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?)

It cant atm, although this is not a problem on Android. It can be fixed
when we export the graph to userspace though.

> FE1 will still try to feed data in wrong formats/rates/etc, won't it?
> 

No, FE1 wont change it's params in this state. It will be up to the DSP
driver to perform the conversion to then new formats or fail if the new
format cannot be supported.

> 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
> > 
> > 1) configure FE1 hw_params
> > 
> > 2) configure FE2 hw_params
> > 
> > 3) 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?

The intention was to use the existing alsa-lib/tinyalsa PCM hw_params
APIs. The BE would just export itself to usespace as a PCM (but without
the capability for direct playback/capture - just format, rate setting)

Thanks

Liam

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

[Takashi Iwai]

At Fri, 10 Jan 2014 18:43:09 +0000,
Liam Girdwood wrote:
> 
> On Fri, 2014-01-10 at 14:46 +0100, Takashi Iwai wrote:
> > 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?)
> 
> It cant atm, although this is not a problem on Android. It can be fixed
> when we export the graph to userspace though.

I see.

> > FE1 will still try to feed data in wrong formats/rates/etc, won't it?
> > 
> 
> No, FE1 wont change it's params in this state. It will be up to the DSP
> driver to perform the conversion to then new formats or fail if the new
> format cannot be supported.

Well, my concern is that FE1 is never notified about the hw_params
change done by FE2 in the current situation.  So, FE1 will feed the
data to BE as if the old hw_params is used.  The rest behavior is
certainly all up to hardware.

But, the point is that basically we already know that something is
wrong at the point BE2 setting up an incompatible hw_params; then it
should be notified properly to FE1, or the incompatible change must be
handled as an error.  This is the missing piece in the current
implementation.  The skip of redundant BE hw_params call can be
implemented as an optimization in this compatibility check, too.

> > 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
> > > 
> > > 1) configure FE1 hw_params
> > > 
> > > 2) configure FE2 hw_params
> > > 
> > > 3) 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?
> 
> The intention was to use the existing alsa-lib/tinyalsa PCM hw_params
> APIs. The BE would just export itself to usespace as a PCM (but without
> the capability for direct playback/capture - just format, rate setting)

Does it mean that, from kernel perspective, a BE creates a dedicated
(virtual) PCM device and expose it to user-space?  Or just through
special API?


thanks,

Takashi

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 706 bytes --]

On Sat, Jan 11, 2014 at 10:35:33AM +0100, Takashi Iwai wrote:

> But, the point is that basically we already know that something is
> wrong at the point BE2 setting up an incompatible hw_params; then it
> should be notified properly to FE1, or the incompatible change must be
> handled as an error.  This is the missing piece in the current
> implementation.  The skip of redundant BE hw_params call can be
> implemented as an optimization in this compatibility check, too.

Only in the case where they actually are incompatible though - if
there's DSP in place which can do suitable mixing it's not an issue.
At the minute the core is relying on the drivers handling any limits
just like with the CODECs.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Liam Girdwood]

On Sat, 2014-01-11 at 10:35 +0100, Takashi Iwai wrote:
> At Fri, 10 Jan 2014 18:43:09 +0000,
> Liam Girdwood wrote:
> > 
> > On Fri, 2014-01-10 at 14:46 +0100, Takashi Iwai wrote:
> > > At Fri, 10 Jan 2014 12:29:08 +0000,
> > > Liam Girdwood wrote:
> > > > 

> > 
> > The intention was to use the existing alsa-lib/tinyalsa PCM hw_params
> > APIs. The BE would just export itself to usespace as a PCM (but without
> > the capability for direct playback/capture - just format, rate setting)
> 
> Does it mean that, from kernel perspective, a BE creates a dedicated
> (virtual) PCM device and expose it to user-space?  Or just through
> special API?

I'm thinking a virtual PCM if you agree. 

We could keep the same userspace API for configuration OR we could
extend the API slightly to add some snd_pcm_virtual_() functions.
Extending the API would imply the virtual PCM only supports a subset of
PCM API calls (avoiding any confusion/mixing with regular PCM APIs).

Thanks

Liam    

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

[Takashi Iwai]

At Mon, 13 Jan 2014 10:48:51 +0000,
Liam Girdwood wrote:
> 
> On Sat, 2014-01-11 at 10:35 +0100, Takashi Iwai wrote:
> > At Fri, 10 Jan 2014 18:43:09 +0000,
> > Liam Girdwood wrote:
> > > 
> > > On Fri, 2014-01-10 at 14:46 +0100, Takashi Iwai wrote:
> > > > At Fri, 10 Jan 2014 12:29:08 +0000,
> > > > Liam Girdwood wrote:
> > > > > 
> 
> > > 
> > > The intention was to use the existing alsa-lib/tinyalsa PCM hw_params
> > > APIs. The BE would just export itself to usespace as a PCM (but without
> > > the capability for direct playback/capture - just format, rate setting)
> > 
> > Does it mean that, from kernel perspective, a BE creates a dedicated
> > (virtual) PCM device and expose it to user-space?  Or just through
> > special API?
> 
> I'm thinking a virtual PCM if you agree. 
> 
> We could keep the same userspace API for configuration OR we could
> extend the API slightly to add some snd_pcm_virtual_() functions.
> Extending the API would imply the virtual PCM only supports a subset of
> PCM API calls (avoiding any confusion/mixing with regular PCM APIs).

Yeah, I agree that a simple PCM device exposure would be more
straightforward.


thanks,

Takashi