| Message ID | 20200729154639.1983854-1-jbrunet@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: core: restore dpcm flags semantics | expand |
On 7/29/20 10:46 AM, Jerome Brunet wrote: > commit b73287f0b0745 ('ASoC: soc-pcm: dpcm: fix playback/capture checks') > changed dpcm_playback and dpcm_capture semantic by throwing an error if > these flags are not aligned with DAIs capabilities on the link. > > The former semantic did not force the flags and DAI caps to be aligned. > The flag previously allowed card drivers to disable a stream direction on > a link (whether or not such feature is deemed useful). > > With change ('ASoC: core: use less strict tests for dailink capabilities') > an error is thrown if the flags and and the DAI caps are not aligned. Those > parameters were not meant to aligned initially. No technical reason was > given about why cards should now be considered "broken" in such condition > is not met, or why it should be considered to be an improvement to enforce > that. > > Forcing the flags to be aligned with DAI caps just make the information > the flag carry redundant with DAI caps, breaking a few cards along the way. > > This change drops the added error conditions and restore the initial flag > semantics. or rather lack thereof. I am ok to move dev_err to dev_warn and remove the return -EINVAL, but I maintain that we have to reach a point where configurations make sense before we can clean them up. If we implicitly push issues under the rug by not even being aware of them we'll never make progress. > > Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > --- > > Hi Mark, > > Because b73287f0b0745 ('ASoC: soc-pcm: dpcm: fix playback/capture checks') > introduced more than one problem, the change > "ASoC: core: use less strict tests for dailink capabilities" [0] is still > necessary but the change of semantic remains a problem with it. > > This patch applies on top of it. > > sound/soc/soc-pcm.c | 14 -------------- > 1 file changed, 14 deletions(-) > > diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c > index 00ac1cbf6f88..2e205b738eae 100644 > --- a/sound/soc/soc-pcm.c > +++ b/sound/soc/soc-pcm.c > @@ -2749,13 +2749,6 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num) > break; > } > } > - > - if (!playback) { > - dev_err(rtd->card->dev, > - "No CPU DAIs support playback for stream %s\n", > - rtd->dai_link->stream_name); > - return -EINVAL; > - } > } > if (rtd->dai_link->dpcm_capture) { > stream = SNDRV_PCM_STREAM_CAPTURE; > @@ -2766,13 +2759,6 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num) > break; > } > } > - > - if (!capture) { > - dev_err(rtd->card->dev, > - "No CPU DAIs support capture for stream %s\n", > - rtd->dai_link->stream_name); > - return -EINVAL; > - } > } > } else { > /* Adapt stream for codec2codec links */ >
On Wed 29 Jul 2020 at 17:56, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote: > On 7/29/20 10:46 AM, Jerome Brunet wrote: >> commit b73287f0b0745 ('ASoC: soc-pcm: dpcm: fix playback/capture checks') >> changed dpcm_playback and dpcm_capture semantic by throwing an error if >> these flags are not aligned with DAIs capabilities on the link. >> >> The former semantic did not force the flags and DAI caps to be aligned. >> The flag previously allowed card drivers to disable a stream direction on >> a link (whether or not such feature is deemed useful). >> >> With change ('ASoC: core: use less strict tests for dailink capabilities') >> an error is thrown if the flags and and the DAI caps are not aligned. Those >> parameters were not meant to aligned initially. No technical reason was >> given about why cards should now be considered "broken" in such condition >> is not met, or why it should be considered to be an improvement to enforce >> that. >> >> Forcing the flags to be aligned with DAI caps just make the information >> the flag carry redundant with DAI caps, breaking a few cards along the way. >> >> This change drops the added error conditions and restore the initial flag >> semantics. > > or rather lack thereof. Again, why ? All there is so far is your personal preference. no facts. * What we had gave capabilities to the link, independent of the DAI components. ASoC just computes the intersection of all that to determine which direction needs to be enabled. Seems rather simple and straight forward. * It worked for every user of DPCM so a far. Your changes: * Causes regression * Makes information redundant. The code used to build the flag in snd_soc_dai_link_set_capabilities() and check it soc_new_pcm() is more or less the same. It just adds complexity and waste cycles. I don't see the upside to it. > > I am ok to move dev_err to dev_warn and remove the return -EINVAL, but I > maintain that we have to reach a point where configurations make sense > before we can clean them up. If we implicitly push issues under the rug by > not even being aware of them we'll never make progress. But why should we bother the user with that ? How is throwing this error/warning an improvement ? What does not make sense in the configuration ? What are we pushing under the rug exactly ? I'm willing to go your way, even help you out, but you need to: * explain concretely why changing the semantics improve the overall situation, concretely ? * update all the users of DPCM. Causing regression is not OK. Carrying redundant information makes things complex and error prone. If you really want to update this, here is another proposition: * Removing snd_soc_dai_link_set_capabilities() * Removing both flags completely * Let ASoC figure out what is needed based on the components present. > >> >> Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> >> --- >> >> Hi Mark, >> >> Because b73287f0b0745 ('ASoC: soc-pcm: dpcm: fix playback/capture checks') >> introduced more than one problem, the change >> "ASoC: core: use less strict tests for dailink capabilities" [0] is still >> necessary but the change of semantic remains a problem with it. >> >> This patch applies on top of it. >> >> sound/soc/soc-pcm.c | 14 -------------- >> 1 file changed, 14 deletions(-) >> >> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c >> index 00ac1cbf6f88..2e205b738eae 100644 >> --- a/sound/soc/soc-pcm.c >> +++ b/sound/soc/soc-pcm.c >> @@ -2749,13 +2749,6 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num) >> break; >> } >> } >> - >> - if (!playback) { >> - dev_err(rtd->card->dev, >> - "No CPU DAIs support playback for stream %s\n", >> - rtd->dai_link->stream_name); >> - return -EINVAL; >> - } >> } >> if (rtd->dai_link->dpcm_capture) { >> stream = SNDRV_PCM_STREAM_CAPTURE; >> @@ -2766,13 +2759,6 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num) >> break; >> } >> } >> - >> - if (!capture) { >> - dev_err(rtd->card->dev, >> - "No CPU DAIs support capture for stream %s\n", >> - rtd->dai_link->stream_name); >> - return -EINVAL; >> - } >> } >> } else { >> /* Adapt stream for codec2codec links */ >>
On 7/30/20 4:04 AM, Jerome Brunet wrote: > > On Wed 29 Jul 2020 at 17:56, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote: > >> On 7/29/20 10:46 AM, Jerome Brunet wrote: >>> commit b73287f0b0745 ('ASoC: soc-pcm: dpcm: fix playback/capture checks') >>> changed dpcm_playback and dpcm_capture semantic by throwing an error if >>> these flags are not aligned with DAIs capabilities on the link. >>> >>> The former semantic did not force the flags and DAI caps to be aligned. >>> The flag previously allowed card drivers to disable a stream direction on >>> a link (whether or not such feature is deemed useful). >>> >>> With change ('ASoC: core: use less strict tests for dailink capabilities') >>> an error is thrown if the flags and and the DAI caps are not aligned. Those >>> parameters were not meant to aligned initially. No technical reason was >>> given about why cards should now be considered "broken" in such condition >>> is not met, or why it should be considered to be an improvement to enforce >>> that. >>> >>> Forcing the flags to be aligned with DAI caps just make the information >>> the flag carry redundant with DAI caps, breaking a few cards along the way. >>> >>> This change drops the added error conditions and restore the initial flag >>> semantics. >> >> or rather lack thereof. > > Again, why ? All there is so far is your personal preference. no facts. What would be the meaning/purpose of a dailink with .dpcm_capture set, with only dais that support playback only? What would be the meaning/purpose of a dailink with .capture_only set, but with a dai that supports playback? What happens if none of these flags are set? What happens when all these flags are set? No one seems to know, so my suggestion is to align first on consistent configurations, then see what can be removed. > * What we had gave capabilities to the link, independent of the DAI > components. ASoC just computes the intersection of all that to > determine which direction needs to be enabled. Seems rather simple > and straight forward. that's what my last patch did, and when there is no intersection it complains. Please clarify what you expect when there is no overlap between dai and dailink capabilities. Keep in mind that we have a mix of hard-codec configuration and DT-created ones, your case is not the general one. > * It worked for every user of DPCM so a far. Not completely true, when Morimoto-san added snd_soc_dai_stream_valid() it exposed tons of cases where the information on direction was not provided in a reliable at the DAI level. I will assert that we are still finding out cases with broken DAI configurations, and as a result we will also find broken dailink configurations. Your picture of DPCM as a perfectly functional system that I broke is a distortion of reality. The reality is that we have to work in steps, first make sure all DAIs are properly described, then work on the dailinks and optimize at a later point. we will need warnings to find out what the problem cases are, and move slowly.
On Thu, Jul 30, 2020 at 11:04:53AM +0200, Jerome Brunet wrote: > Carrying redundant information makes things complex and error prone. > If you really want to update this, here is another proposition: > * Removing snd_soc_dai_link_set_capabilities() > * Removing both flags completely > * Let ASoC figure out what is needed based on the components present. My understanding is that that was broadly where we were headed with this stuff - snd_soc_dai_link_set_capabilities() is trying to figure things out from the components already, it's storing the flags as a cache but could be modified so we use it every time we need a value. > > > > >> > >> Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > >> --- > >> > >> Hi Mark, > >> > >> Because b73287f0b0745 ('ASoC: soc-pcm: dpcm: fix playback/capture checks') > >> introduced more than one problem, the change > >> "ASoC: core: use less strict tests for dailink capabilities" [0] is still > >> necessary but the change of semantic remains a problem with it. > >> > >> This patch applies on top of it. > >> > >> sound/soc/soc-pcm.c | 14 -------------- > >> 1 file changed, 14 deletions(-) > >> > >> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c > >> index 00ac1cbf6f88..2e205b738eae 100644 > >> --- a/sound/soc/soc-pcm.c > >> +++ b/sound/soc/soc-pcm.c > >> @@ -2749,13 +2749,6 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num) > >> break; > >> } > >> } > >> - > >> - if (!playback) { > >> - dev_err(rtd->card->dev, > >> - "No CPU DAIs support playback for stream %s\n", > >> - rtd->dai_link->stream_name); > >> - return -EINVAL; > >> - } > >> } > >> if (rtd->dai_link->dpcm_capture) { > >> stream = SNDRV_PCM_STREAM_CAPTURE; > >> @@ -2766,13 +2759,6 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num) > >> break; > >> } > >> } > >> - > >> - if (!capture) { > >> - dev_err(rtd->card->dev, > >> - "No CPU DAIs support capture for stream %s\n", > >> - rtd->dai_link->stream_name); > >> - return -EINVAL; > >> - } > >> } > >> } else { > >> /* Adapt stream for codec2codec links */ > >> >
On Thu, Jul 30, 2020 at 11:06:23AM -0500, Pierre-Louis Bossart wrote: > On 7/30/20 4:04 AM, Jerome Brunet wrote: > > On Wed 29 Jul 2020 at 17:56, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote: > > > On 7/29/20 10:46 AM, Jerome Brunet wrote: > > > > The flag previously allowed card drivers to disable a stream direction on > > > > a link (whether or not such feature is deemed useful). Right, and I can see a use case for this if someone has a board that for some reason didn't physically connect one of the directions for some reason - perhaps they were running out of pins or something. It's not clear if anyone's actually doing that though. > > > > Forcing the flags to be aligned with DAI caps just make the information > > > > the flag carry redundant with DAI caps, breaking a few cards along the way. > > > > This change drops the added error conditions and restore the initial flag > > > > semantics. I'm not 100% clear, have we actually found cases where the flags are used or is this something found through inspection and review? > > * It worked for every user of DPCM so a far. > Not completely true, when Morimoto-san added snd_soc_dai_stream_valid() it > exposed tons of cases where the information on direction was not provided in > a reliable at the DAI level. I will assert that we are still finding out > cases with broken DAI configurations, and as a result we will also find > broken dailink configurations. Your picture of DPCM as a perfectly > functional system that I broke is a distortion of reality. > The reality is that we have to work in steps, first make sure all DAIs are > properly described, then work on the dailinks and optimize at a later point. > we will need warnings to find out what the problem cases are, and move > slowly. This was all triggered by Morimoto-san's changes like you say. DPCM has quite a lot of problems in general, here IIRC the issues were that we had multiple different ways of doing similar things which it wasn't quite clear if people were even using. The intention with the warnings was to remove them one way or another, they're mainly intended to flush out actual active usage of the flags as opposed to redundant usage of them which could be confused/broken. This could definitely have been clearer in the changelogs though.
On Thu 30 Jul 2020 at 18:06, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote: > On 7/30/20 4:04 AM, Jerome Brunet wrote: >> >> On Wed 29 Jul 2020 at 17:56, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote: >> >>> On 7/29/20 10:46 AM, Jerome Brunet wrote: >>>> commit b73287f0b0745 ('ASoC: soc-pcm: dpcm: fix playback/capture checks') >>>> changed dpcm_playback and dpcm_capture semantic by throwing an error if >>>> these flags are not aligned with DAIs capabilities on the link. >>>> >>>> The former semantic did not force the flags and DAI caps to be aligned. >>>> The flag previously allowed card drivers to disable a stream direction on >>>> a link (whether or not such feature is deemed useful). >>>> >>>> With change ('ASoC: core: use less strict tests for dailink capabilities') >>>> an error is thrown if the flags and and the DAI caps are not aligned. Those >>>> parameters were not meant to aligned initially. No technical reason was >>>> given about why cards should now be considered "broken" in such condition >>>> is not met, or why it should be considered to be an improvement to enforce >>>> that. >>>> >>>> Forcing the flags to be aligned with DAI caps just make the information >>>> the flag carry redundant with DAI caps, breaking a few cards along the way. >>>> >>>> This change drops the added error conditions and restore the initial flag >>>> semantics. >>> >>> or rather lack thereof. >> >> Again, why ? All there is so far is your personal preference. no facts. > > What would be the meaning/purpose of a dailink with .dpcm_capture set, with > only dais that support playback only? > > What would be the meaning/purpose of a dailink with .capture_only set, but > with a dai that supports playback? You get to throw an error in those case > > What happens if none of these flags are set? I think I already suggested to throw an error in the initial review of your patch > > What happens when all these flags are set? I don't see the problem here > > No one seems to know, so my suggestion is to align first on consistent > configurations, then see what can be removed. > >> * What we had gave capabilities to the link, independent of the DAI >> components. ASoC just computes the intersection of all that to >> determine which direction needs to be enabled. Seems rather simple >> and straight forward. > > that's what my last patch did, and when there is no intersection it > complains. Please clarify what you expect when there is no overlap between > dai and dailink capabilities. Keep in mind that we have a mix of hard-codec > configuration and DT-created ones, your case is not the general one. > >> * It worked for every user of DPCM so a far. > > Not completely true, when Morimoto-san added snd_soc_dai_stream_valid() it > exposed tons of cases where the information on direction was not provided > in a reliable at the DAI level. I will assert that we are still finding out > cases with broken DAI configurations, and as a result we will also find > broken dailink configurations. Your picture of DPCM as a perfectly > functional system that I broke is a distortion of reality. If it was not working, it was certainly not clear in the changelog. What's clear is the regression it caused > > The reality is that we have to work in steps, first make sure all DAIs are > properly described, then work on the dailinks and optimize at a later > point. we will need warnings to find out what the problem cases are, and > move slowly. Sure, have it your way
On Thu 30 Jul 2020 at 20:52, Mark Brown <broonie@kernel.org> wrote: > On Thu, Jul 30, 2020 at 11:06:23AM -0500, Pierre-Louis Bossart wrote: >> On 7/30/20 4:04 AM, Jerome Brunet wrote: >> > On Wed 29 Jul 2020 at 17:56, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote: >> > > On 7/29/20 10:46 AM, Jerome Brunet wrote: > >> > > > The flag previously allowed card drivers to disable a stream direction on >> > > > a link (whether or not such feature is deemed useful). > > Right, and I can see a use case for this if someone has a board that > for some reason didn't physically connect one of the directions for some > reason - perhaps they were running out of pins or something. It's not > clear if anyone's actually doing that though. > >> > > > Forcing the flags to be aligned with DAI caps just make the information >> > > > the flag carry redundant with DAI caps, breaking a few cards along the way. > >> > > > This change drops the added error conditions and restore the initial flag >> > > > semantics. > > I'm not 100% clear, have we actually found cases where the flags are > used or is this something found through inspection and review? One last thing I'd like to understand. Is this behavior of throwing an error going to applied to the non-DPCM case as well ? so at least thing are consistent between both cases ? IOW: * An error is now throw if dpcm_capture is set on the link and the CPU DAI support playback_only * on non-DPCM links, will an error be thrown as well if playback_only is not set and the CPU on the link happen to not support capture ? > >> > * It worked for every user of DPCM so a far. > >> Not completely true, when Morimoto-san added snd_soc_dai_stream_valid() it >> exposed tons of cases where the information on direction was not provided in >> a reliable at the DAI level. I will assert that we are still finding out >> cases with broken DAI configurations, and as a result we will also find >> broken dailink configurations. Your picture of DPCM as a perfectly >> functional system that I broke is a distortion of reality. > >> The reality is that we have to work in steps, first make sure all DAIs are >> properly described, then work on the dailinks and optimize at a later point. >> we will need warnings to find out what the problem cases are, and move >> slowly. > > This was all triggered by Morimoto-san's changes like you say. DPCM has > quite a lot of problems in general, here IIRC the issues were that we > had multiple different ways of doing similar things which it wasn't > quite clear if people were even using. The intention with the warnings > was to remove them one way or another, they're mainly intended to flush > out actual active usage of the flags as opposed to redundant usage of > them which could be confused/broken. > > This could definitely have been clearer in the changelogs though.
On Fri, Jul 31, 2020 at 02:16:43PM +0200, Jerome Brunet wrote: > One last thing I'd like to understand. Is this behavior of throwing an > error going to applied to the non-DPCM case as well ? so at least thing > are consistent between both cases ? > IOW: > * An error is now throw if dpcm_capture is set on the link and the CPU > DAI support playback_only We should definitely complain about that. > * on non-DPCM links, will an error be thrown as well if playback_only > is not set and the CPU on the link happen to not support capture ? I think we should move towards not needing to do that for DPCM.
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 00ac1cbf6f88..2e205b738eae 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -2749,13 +2749,6 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num) break; } } - - if (!playback) { - dev_err(rtd->card->dev, - "No CPU DAIs support playback for stream %s\n", - rtd->dai_link->stream_name); - return -EINVAL; - } } if (rtd->dai_link->dpcm_capture) { stream = SNDRV_PCM_STREAM_CAPTURE; @@ -2766,13 +2759,6 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num) break; } } - - if (!capture) { - dev_err(rtd->card->dev, - "No CPU DAIs support capture for stream %s\n", - rtd->dai_link->stream_name); - return -EINVAL; - } } } else { /* Adapt stream for codec2codec links */
commit b73287f0b0745 ('ASoC: soc-pcm: dpcm: fix playback/capture checks') changed dpcm_playback and dpcm_capture semantic by throwing an error if these flags are not aligned with DAIs capabilities on the link. The former semantic did not force the flags and DAI caps to be aligned. The flag previously allowed card drivers to disable a stream direction on a link (whether or not such feature is deemed useful). With change ('ASoC: core: use less strict tests for dailink capabilities') an error is thrown if the flags and and the DAI caps are not aligned. Those parameters were not meant to aligned initially. No technical reason was given about why cards should now be considered "broken" in such condition is not met, or why it should be considered to be an improvement to enforce that. Forcing the flags to be aligned with DAI caps just make the information the flag carry redundant with DAI caps, breaking a few cards along the way. This change drops the added error conditions and restore the initial flag semantics. Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> --- Hi Mark, Because b73287f0b0745 ('ASoC: soc-pcm: dpcm: fix playback/capture checks') introduced more than one problem, the change "ASoC: core: use less strict tests for dailink capabilities" [0] is still necessary but the change of semantic remains a problem with it. This patch applies on top of it. sound/soc/soc-pcm.c | 14 -------------- 1 file changed, 14 deletions(-)