2019-09-10 14:32:14

by Pierre-Louis Bossart

[permalink] [raw]
Subject: Re: [alsa-devel] [PATCH] ASoC: bdw-rt5677: channel constraint support


>> I also don't see any case where we support 4 channels in any broadwell
>> machine driver?
> It's the bdw-rt5650.c which only exists in chrome's 3.14 branch supporting Buddy
> project. They submitted the machine driver but not yet merged.
>
> https://patchwork.kernel.org/patch/11050985/
>
>>
>> So again can you point me to an issue or existing backport that requires the
>> patch below. Not trying to be obtuse but we should only change older
>> platforms when there is evidence that a change is needed.
> The story is Chrome has a tool called alsa_conformance_test which runs capture or
> playback against a PCM port with all possible configurations (channel, format, rate)
> then measure if the sample rate is correct. Since the channel max number reported
> is 4, it tests the 4-channel 48K capture and reports the actual sample rate is 24000
> instead of 48000. That's the reason we want to add a constraint in machine driver to
> avoid user space programs trying to do 4 channel recording since this machine does
> not support it in the beginning.

ok, that helps get context, thanks for the details.

I would have expected some error to be returned if there's a front-end
opened with 4 channels and the back-end only supports two. Adding the
constraint seems like a work-around to avoid dealing with the mismatch
between FE and BE. I don't understand DPCM enough to suggest an
alternative though. Ranjani, can you help on this one?

And even if we agree with this solution, it'd be nice to apply it for
the Broadwell machine driver for consistency.


2019-09-12 06:07:21

by Brent Lu

[permalink] [raw]
Subject: RE: [alsa-devel] [PATCH] ASoC: bdw-rt5677: channel constraint support

> >
> > The story is Chrome has a tool called alsa_conformance_test which runs
> > capture or playback against a PCM port with all possible
> > configurations (channel, format, rate) then measure if the sample rate
> > is correct. Since the channel max number reported is 4, it tests the
> > 4-channel 48K capture and reports the actual sample rate is 24000
> > instead of 48000. That's the reason we want to add a constraint in
> > machine driver to avoid user space programs trying to do 4 channel
> recording since this machine does not support it in the beginning.
>
> ok, that helps get context, thanks for the details.
>
> I would have expected some error to be returned if there's a front-end
> opened with 4 channels and the back-end only supports two. Adding the
> constraint seems like a work-around to avoid dealing with the mismatch
> between FE and BE. I don't understand DPCM enough to suggest an
> alternative though. Ranjani, can you help on this one?
>
> And even if we agree with this solution, it'd be nice to apply it for the
> Broadwell machine driver for consistency.

It's not only the mismatch but also the design limitation. According to the
information from google, the board (samus) only uses two microphone so
3 or 4 channel recording are not supported. That's the reason we leverage
the constraint from other machine driver (like kbl_da7219_max98357a.c)
to remove the 3 and 4 channel recording option.

The difference after the constraint is implemented is that the
snd_pcm_hw_params_set_channels() function will return error (Invalid
argument) when channel number is 3 or 4 so the application knows the
configuration is not supported.


Regards,
Brent

2019-09-12 13:09:25

by Pierre-Louis Bossart

[permalink] [raw]
Subject: Re: [alsa-devel] [PATCH] ASoC: bdw-rt5677: channel constraint support

On 9/12/19 1:00 AM, Lu, Brent wrote:
>>>
>>> The story is Chrome has a tool called alsa_conformance_test which runs
>>> capture or playback against a PCM port with all possible
>>> configurations (channel, format, rate) then measure if the sample rate
>>> is correct. Since the channel max number reported is 4, it tests the
>>> 4-channel 48K capture and reports the actual sample rate is 24000
>>> instead of 48000. That's the reason we want to add a constraint in
>>> machine driver to avoid user space programs trying to do 4 channel
>> recording since this machine does not support it in the beginning.
>>
>> ok, that helps get context, thanks for the details.
>>
>> I would have expected some error to be returned if there's a front-end
>> opened with 4 channels and the back-end only supports two. Adding the
>> constraint seems like a work-around to avoid dealing with the mismatch
>> between FE and BE. I don't understand DPCM enough to suggest an
>> alternative though. Ranjani, can you help on this one?
>>
>> And even if we agree with this solution, it'd be nice to apply it for the
>> Broadwell machine driver for consistency.
>
> It's not only the mismatch but also the design limitation. According to the
> information from google, the board (samus) only uses two microphone so
> 3 or 4 channel recording are not supported. That's the reason we leverage
> the constraint from other machine driver (like kbl_da7219_max98357a.c)
> to remove the 3 and 4 channel recording option.

The design limitation is already handled by the fact that the SSP
operates in 2ch mode, so it's a different case from KBL where indeed the
DMIC-based back-end can support 4 channels.

>
> The difference after the constraint is implemented is that the
> snd_pcm_hw_params_set_channels() function will return error (Invalid
> argument) when channel number is 3 or 4 so the application knows the
> configuration is not supported.

I get the error, I am just wondering if the fix is at the right
location. I'll look into it, give me until tomorrow.

2019-09-27 12:38:56

by Brent Lu

[permalink] [raw]
Subject: RE: [alsa-devel] [PATCH] ASoC: bdw-rt5677: channel constraint support

> >
> > It's not only the mismatch but also the design limitation. According
> > to the information from google, the board (samus) only uses two
> > microphone so
> > 3 or 4 channel recording are not supported. That's the reason we
> > leverage the constraint from other machine driver (like
> > kbl_da7219_max98357a.c) to remove the 3 and 4 channel recording option.
>
> The design limitation is already handled by the fact that the SSP operates in
> 2ch mode, so it's a different case from KBL where indeed the DMIC-based
> back-end can support 4 channels.
>
> >
> > The difference after the constraint is implemented is that the
> > snd_pcm_hw_params_set_channels() function will return error (Invalid
> > argument) when channel number is 3 or 4 so the application knows the
> > configuration is not supported.
>
> I get the error, I am just wondering if the fix is at the right location. I'll look
> into it, give me until tomorrow.

I think I got your point. I cherry-pick these commits back to v3.14 branch to fix
the FE/BE mismatch without adding constraint in machine driver. Thanks.

b073ed4e ASoC: soc-pcm: DPCM cares BE format
f4c277b8 ASoC: soc-pcm: DPCM cares BE channel constraint
4f2bd18b ASoC: dpcm: extend channel merging to the backend cpu dai
435ffb76 ASoC: dpcm: rework runtime stream merge
baacd8d1 ASoC: dpcm: add rate merge to the BE stream merge