| Message ID | 20230629075910.21982-1-trevor.wu@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: mediatek: mt8188: add constraints for PCM | expand |
On Thu, Jun 29, 2023 at 03:59:10PM +0800, Trevor Wu wrote: > For certain projects, only a limited number of parameters are necessary. > Therefore, it may be beneficial to add constraints that restrict the > capacity of the PCM. This commit message isn't entirely clear. The effect of the commit is to restrict the configurations supported when using a nau8825 but it's not clear what a nau8825 has to do with this or why we're doing this in general. What exactly do you mean when saying that "only a limited number of parameters are necessary" and what makes this the case?
On Thu, 2023-06-29 at 16:06 +0100, Mark Brown wrote: > On Thu, Jun 29, 2023 at 03:59:10PM +0800, Trevor Wu wrote: > > > For certain projects, only a limited number of parameters are > > necessary. > > Therefore, it may be beneficial to add constraints that restrict > > the > > capacity of the PCM. > > This commit message isn't entirely clear. The effect of the commit > is > to restrict the configurations supported when using a nau8825 but > it's > not clear what a nau8825 has to do with this or why we're doing this > in > general. What exactly do you mean when saying that "only a limited > number of parameters are necessary" and what makes this the case? Hi Mark, For instance, some userspace frameworks only support specific sampling rates such as 48kHz on Chromebook, making other parameters unnecessary. By restricting the configuration, unexpected usage can be prevented and the alsa_conformance_test process which checks all parameters provided by an ALSA driver can be sped up. Although machine drivers can be reused for multiple projects, not all projects have the same criterion. Therefore, I will add the criterion only for mt8188_nau8825 which is used for Chromebook board. If there are similar criteria for other projects, they can be adopted in the same way. Perhaps a new constraint, such as DPCM_FE_CONSTRAINT1, could be introduced for different parameter combinations. Would it be more beneficial to establish the criteria as a general rule for this machine driver, while limiting the use of the machine driver solely to the Chromebook project? Or do you just suggest that I add more details in the commit messages? Thanks, Trevor
On Fri, Jun 30, 2023 at 05:29:23AM +0000, Trevor Wu (吳文良) wrote: > On Thu, 2023-06-29 at 16:06 +0100, Mark Brown wrote: > > This commit message isn't entirely clear. The effect of the commit > > is > > to restrict the configurations supported when using a nau8825 but > > it's > > not clear what a nau8825 has to do with this or why we're doing this > > in > > general. What exactly do you mean when saying that "only a limited > > number of parameters are necessary" and what makes this the case? > For instance, some userspace frameworks only support specific sampling > rates such as 48kHz on Chromebook, making other parameters unnecessary. > By restricting the configuration, unexpected usage can be prevented and > the alsa_conformance_test process which checks all parameters provided > by an ALSA driver can be sped up. That's a userspace policy decision, we shouldn't be enforcing this in the kernel - even for Chromebooks people can install other OSs on them which may make different decisions, and it's always possible that the ChromeOS people might change their mind later. If they're only interested in testing 48kHz and it's slowing things down unreasonably to test more then they should just only test 48kHz rather than changing the driver to work around it. > Would it be more beneficial to establish the criteria as a general rule > for this machine driver, while limiting the use of the machine driver > solely to the Chromebook project? Or do you just suggest that I add > more details in the commit messages? I think we just shouldn't do this, it's policy for ChromeOS rather than something that's actually needed. If we were doing this it would need a much clearer commit message and we should be restricting things to Chromebooks only.
Il 30/06/23 13:00, Mark Brown ha scritto: > On Fri, Jun 30, 2023 at 05:29:23AM +0000, Trevor Wu (吳文良) wrote: >> On Thu, 2023-06-29 at 16:06 +0100, Mark Brown wrote: > >>> This commit message isn't entirely clear. The effect of the commit >>> is >>> to restrict the configurations supported when using a nau8825 but >>> it's >>> not clear what a nau8825 has to do with this or why we're doing this >>> in >>> general. What exactly do you mean when saying that "only a limited >>> number of parameters are necessary" and what makes this the case? > >> For instance, some userspace frameworks only support specific sampling >> rates such as 48kHz on Chromebook, making other parameters unnecessary. >> By restricting the configuration, unexpected usage can be prevented and >> the alsa_conformance_test process which checks all parameters provided >> by an ALSA driver can be sped up. > > That's a userspace policy decision, we shouldn't be enforcing this in > the kernel - even for Chromebooks people can install other OSs on them > which may make different decisions, and it's always possible that the > ChromeOS people might change their mind later. If they're only > interested in testing 48kHz and it's slowing things down unreasonably > to test more then they should just only test 48kHz rather than changing > the driver to work around it. > >> Would it be more beneficial to establish the criteria as a general rule >> for this machine driver, while limiting the use of the machine driver >> solely to the Chromebook project? Or do you just suggest that I add >> more details in the commit messages? > > I think we just shouldn't do this, it's policy for ChromeOS rather than > something that's actually needed. If we were doing this it would need a > much clearer commit message and we should be restricting things to > Chromebooks only. I agree with Mark. Except for me it's not a *should not* but a *shall not*. Such other configurations are supported by the hardware and it is the driver's duty to support all of them - otherwise I deem the driver to be *incomplete*. It's then the userspace's duty to properly use the sound APIs and request the right sampling rate for specific usecases. Chromebooks aren't special at all in this regard. Regards, Angelo
On Mon, 2023-07-03 at 14:27 +0200, AngeloGioacchino Del Regno wrote: > Il 30/06/23 13:00, Mark Brown ha scritto: > > On Fri, Jun 30, 2023 at 05:29:23AM +0000, Trevor Wu (吳文良) wrote: > >> On Thu, 2023-06-29 at 16:06 +0100, Mark Brown wrote: > > > >>> This commit message isn't entirely clear. The effect of the > commit > >>> is > >>> to restrict the configurations supported when using a nau8825 but > >>> it's > >>> not clear what a nau8825 has to do with this or why we're doing > this > >>> in > >>> general. What exactly do you mean when saying that "only a > limited > >>> number of parameters are necessary" and what makes this the case? > > > >> For instance, some userspace frameworks only support specific > sampling > >> rates such as 48kHz on Chromebook, making other parameters > unnecessary. > >> By restricting the configuration, unexpected usage can be > prevented and > >> the alsa_conformance_test process which checks all parameters > provided > >> by an ALSA driver can be sped up. > > > > That's a userspace policy decision, we shouldn't be enforcing this > in > > the kernel - even for Chromebooks people can install other OSs on > them > > which may make different decisions, and it's always possible that > the > > ChromeOS people might change their mind later. If they're only > > interested in testing 48kHz and it's slowing things down > unreasonably > > to test more then they should just only test 48kHz rather than > changing > > the driver to work around it. > > > >> Would it be more beneficial to establish the criteria as a general > rule > >> for this machine driver, while limiting the use of the machine > driver > >> solely to the Chromebook project? Or do you just suggest that I > add > >> more details in the commit messages? > > > > I think we just shouldn't do this, it's policy for ChromeOS rather > than > > something that's actually needed. If we were doing this it would > need a > > much clearer commit message and we should be restricting things to > > Chromebooks only. > > I agree with Mark. Except for me it's not a *should not* but a *shall > not*. > > Such other configurations are supported by the hardware and it is the > driver's > duty to support all of them - otherwise I deem the driver to be > *incomplete*. > It's then the userspace's duty to properly use the sound APIs and > request the > right sampling rate for specific usecases. > > Chromebooks aren't special at all in this regard. > > Thanks for your suggestion. After the internal discussion, we decided to drop the patch. On the other hand, I will send another patch using dpcm_merged_xxx to report the actual capability of hardware. Thanks, Trevor
diff --git a/sound/soc/mediatek/mt8188/mt8188-mt6359.c b/sound/soc/mediatek/mt8188/mt8188-mt6359.c index ac69c23e0da1..0e4e1b8c99e2 100644 --- a/sound/soc/mediatek/mt8188/mt8188-mt6359.c +++ b/sound/soc/mediatek/mt8188/mt8188-mt6359.c @@ -30,6 +30,7 @@ #define TEST_MISO_DONE_2 BIT(29) #define NAU8825_HS_PRESENT BIT(0) +#define DPCM_FE_CONSTRAINT BIT(16) /* * Maxim MAX98390 @@ -452,6 +453,143 @@ enum { DAI_LINK_UL_SRC_BE, }; +static int mt8188_playback_startup(struct snd_pcm_substream *substream) +{ + static const unsigned int rates[] = { + 48000 + }; + static const unsigned int channels[] = { + 2 + }; + static const struct snd_pcm_hw_constraint_list constraints_rates = { + .count = ARRAY_SIZE(rates), + .list = rates, + .mask = 0, + }; + static const struct snd_pcm_hw_constraint_list constraints_channels = { + .count = ARRAY_SIZE(channels), + .list = channels, + .mask = 0, + }; + + struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); + struct snd_pcm_runtime *runtime = substream->runtime; + struct mt8188_mt6359_priv *priv = snd_soc_card_get_drvdata(rtd->card); + struct mt8188_card_data *data = (struct mt8188_card_data *)priv->private_data; + int ret; + + if (!(data->quirk & DPCM_FE_CONSTRAINT)) + return 0; + + ret = snd_pcm_hw_constraint_list(runtime, 0, + SNDRV_PCM_HW_PARAM_RATE, + &constraints_rates); + if (ret < 0) { + dev_err(rtd->dev, "hw_constraint_list rate failed\n"); + return ret; + } + + ret = snd_pcm_hw_constraint_list(runtime, 0, + SNDRV_PCM_HW_PARAM_CHANNELS, + &constraints_channels); + if (ret < 0) { + dev_err(rtd->dev, "hw_constraint_list channel failed\n"); + return ret; + } + + return 0; +} + +static const struct snd_soc_ops mt8188_playback_ops = { + .startup = mt8188_playback_startup, +}; + +static int mt8188_capture_startup(struct snd_pcm_substream *substream) +{ + static const unsigned int rates[] = { + 48000 + }; + static const struct snd_pcm_hw_constraint_list constraints_rates = { + .count = ARRAY_SIZE(rates), + .list = rates, + .mask = 0, + }; + + struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); + struct snd_pcm_runtime *runtime = substream->runtime; + struct mt8188_mt6359_priv *priv = snd_soc_card_get_drvdata(rtd->card); + struct mt8188_card_data *data = (struct mt8188_card_data *)priv->private_data; + int ret; + + if (!(data->quirk & DPCM_FE_CONSTRAINT)) + return 0; + + ret = snd_pcm_hw_constraint_list(runtime, 0, + SNDRV_PCM_HW_PARAM_RATE, + &constraints_rates); + if (ret < 0) { + dev_err(rtd->dev, "hw_constraint_list rate failed\n"); + return ret; + } + + return 0; +} + +static const struct snd_soc_ops mt8188_capture_ops = { + .startup = mt8188_capture_startup, +}; + +static int mt8188_hdmi_dptx_startup(struct snd_pcm_substream *substream) +{ + static const unsigned int rates[] = { + 48000 + }; + static const unsigned int channels[] = { + 2, 4, 6, 8 + }; + static const struct snd_pcm_hw_constraint_list constraints_rates = { + .count = ARRAY_SIZE(rates), + .list = rates, + .mask = 0, + }; + static const struct snd_pcm_hw_constraint_list constraints_channels = { + .count = ARRAY_SIZE(channels), + .list = channels, + .mask = 0, + }; + + struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); + struct snd_pcm_runtime *runtime = substream->runtime; + struct mt8188_mt6359_priv *priv = snd_soc_card_get_drvdata(rtd->card); + struct mt8188_card_data *data = (struct mt8188_card_data *)priv->private_data; + int ret; + + if (!(data->quirk & DPCM_FE_CONSTRAINT)) + return 0; + + ret = snd_pcm_hw_constraint_list(runtime, 0, + SNDRV_PCM_HW_PARAM_RATE, + &constraints_rates); + if (ret < 0) { + dev_err(rtd->dev, "hw_constraint_list rate failed\n"); + return ret; + } + + ret = snd_pcm_hw_constraint_list(runtime, 0, + SNDRV_PCM_HW_PARAM_CHANNELS, + &constraints_channels); + if (ret < 0) { + dev_err(rtd->dev, "hw_constraint_list channel failed\n"); + return ret; + } + + return 0; +} + +static const struct snd_soc_ops mt8188_hdmi_dptx_playback_ops = { + .startup = mt8188_hdmi_dptx_startup, +}; + static int mt8188_dptx_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params) { @@ -723,6 +861,7 @@ static struct snd_soc_dai_link mt8188_mt6359_dai_links[] = { }, .dynamic = 1, .dpcm_playback = 1, + .ops = &mt8188_playback_ops, SND_SOC_DAILINK_REG(playback2), }, [DAI_LINK_DL3_FE] = { @@ -734,6 +873,7 @@ static struct snd_soc_dai_link mt8188_mt6359_dai_links[] = { }, .dynamic = 1, .dpcm_playback = 1, + .ops = &mt8188_playback_ops, SND_SOC_DAILINK_REG(playback3), }, [DAI_LINK_DL6_FE] = { @@ -778,6 +918,7 @@ static struct snd_soc_dai_link mt8188_mt6359_dai_links[] = { }, .dynamic = 1, .dpcm_playback = 1, + .ops = &mt8188_hdmi_dptx_playback_ops, SND_SOC_DAILINK_REG(playback10), }, [DAI_LINK_DL11_FE] = { @@ -833,6 +974,7 @@ static struct snd_soc_dai_link mt8188_mt6359_dai_links[] = { }, .dynamic = 1, .dpcm_capture = 1, + .ops = &mt8188_capture_ops, SND_SOC_DAILINK_REG(capture4), }, [DAI_LINK_UL5_FE] = { @@ -844,6 +986,7 @@ static struct snd_soc_dai_link mt8188_mt6359_dai_links[] = { }, .dynamic = 1, .dpcm_capture = 1, + .ops = &mt8188_capture_ops, SND_SOC_DAILINK_REG(capture5), }, [DAI_LINK_UL6_FE] = { @@ -1122,7 +1265,7 @@ static struct mt8188_card_data mt8188_evb_card = { static struct mt8188_card_data mt8188_nau8825_card = { .name = "mt8188_nau8825", - .quirk = NAU8825_HS_PRESENT, + .quirk = NAU8825_HS_PRESENT | DPCM_FE_CONSTRAINT, }; static const struct of_device_id mt8188_mt6359_dt_match[] = {
For certain projects, only a limited number of parameters are necessary. Therefore, it may be beneficial to add constraints that restrict the capacity of the PCM. Signed-off-by: Trevor Wu <trevor.wu@mediatek.com> --- sound/soc/mediatek/mt8188/mt8188-mt6359.c | 145 +++++++++++++++++++++- 1 file changed, 144 insertions(+), 1 deletion(-)