[alsa-devel] [PATCH 1/5] ASoC codec: SSM2602: remove unsupported sample rates
From: Cliff Cai cliff.cai@analog.com
Signed-off-by: Cliff Cai cliff.cai@analog.com Signed-off-by: Mike Frysinger vapier@gentoo.org --- sound/soc/codecs/ssm2602.c | 8 +++----- 1 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/sound/soc/codecs/ssm2602.c b/sound/soc/codecs/ssm2602.c index 87f606c..d6af069 100644 --- a/sound/soc/codecs/ssm2602.c +++ b/sound/soc/codecs/ssm2602.c @@ -497,11 +497,9 @@ static int ssm2602_set_bias_level(struct snd_soc_codec *codec, return 0; }
-#define SSM2602_RATES (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_11025 |\ - SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_22050 |\ - SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 |\ - SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_88200 |\ - SNDRV_PCM_RATE_96000) +#define SSM2602_RATES (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_32000 |\ + SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000 |\ + SNDRV_PCM_RATE_88200 | SNDRV_PCM_RATE_96000)
#define SSM2602_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE |\ SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE)
From: Cliff Cai cliff.cai@analog.com
Fixes crash when shutting down.
Signed-off-by: Cliff Cai cliff.cai@analog.com Signed-off-by: Mike Frysinger vapier@gentoo.org --- sound/soc/codecs/ssm2602.c | 25 ++++++++++++++++--------- 1 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/sound/soc/codecs/ssm2602.c b/sound/soc/codecs/ssm2602.c index d6af069..1fc4c8e 100644 --- a/sound/soc/codecs/ssm2602.c +++ b/sound/soc/codecs/ssm2602.c @@ -336,15 +336,17 @@ static int ssm2602_startup(struct snd_pcm_substream *substream, master_runtime->sample_bits, master_runtime->rate);
- snd_pcm_hw_constraint_minmax(substream->runtime, - SNDRV_PCM_HW_PARAM_RATE, - master_runtime->rate, - master_runtime->rate); - - snd_pcm_hw_constraint_minmax(substream->runtime, - SNDRV_PCM_HW_PARAM_SAMPLE_BITS, - master_runtime->sample_bits, - master_runtime->sample_bits); + if (master_runtime->rate != 0) + snd_pcm_hw_constraint_minmax(substream->runtime, + SNDRV_PCM_HW_PARAM_RATE, + master_runtime->rate, + master_runtime->rate); + + if (master_runtime->sample_bits != 0) + snd_pcm_hw_constraint_minmax(substream->runtime, + SNDRV_PCM_HW_PARAM_SAMPLE_BITS, + master_runtime->sample_bits, + master_runtime->sample_bits);
ssm2602->slave_substream = substream; } else @@ -372,6 +374,11 @@ static void ssm2602_shutdown(struct snd_pcm_substream *substream, struct snd_soc_device *socdev = rtd->socdev; struct snd_soc_codec *codec = socdev->card->codec; struct ssm2602_priv *ssm2602 = codec->private_data; + + if (ssm2602->master_substream == substream) + ssm2602->master_substream = ssm2602->slave_substream; + + ssm2602->slave_substream = NULL; /* deactivate */ if (!codec->active) ssm2602_write(codec, SSM2602_ACTIVE, 0);
On Tue, Jun 02, 2009 at 12:18:54AM -0400, Mike Frysinger wrote:
snd_pcm_hw_constraint_minmax(substream->runtime,
SNDRV_PCM_HW_PARAM_RATE,
master_runtime->rate,
master_runtime->rate);
snd_pcm_hw_constraint_minmax(substream->runtime,
SNDRV_PCM_HW_PARAM_SAMPLE_BITS,
master_runtime->sample_bits,
master_runtime->sample_bits);
if (master_runtime->rate != 0)
snd_pcm_hw_constraint_minmax(substream->runtime,
SNDRV_PCM_HW_PARAM_RATE,
master_runtime->rate,
master_runtime->rate);
if (master_runtime->sample_bits != 0)
snd_pcm_hw_constraint_minmax(substream->runtime,
SNDRV_PCM_HW_PARAM_SAMPLE_BITS,
master_runtime->sample_bits,
master_runtime->sample_bits);
This change is entirely unrelated to your changelog and should have been submitted as a separate patch. It's preventing the use of the configuration from an unconfigured master which isn't what the patch is about). It's also going over the 80 column limit.
For the rate constraint you should switch to using the symmetric_rates flag in the DAI which implements the equivalent constraints with no per-driver code.
However, given that this fix has missed two kernel releases due to the very slow rate of responses to review comments I've applied it so users get the benefit of the bug fix. Things would go more smoothly if you could get new versions of patches submitted faster after the original review.
On Wed, Jun 3, 2009 at 06:52, Mark Brown wrote:
However, given that this fix has missed two kernel releases due to the very slow rate of responses to review comments I've applied it so users get the benefit of the bug fix.
not sure what you're referring to
Things would go more smoothly if you could get new versions of patches submitted faster after the original review.
the previous Blackfin kernel maintainer changed jobs so we've been transitioning. reality is, this doesnt happen over nite. -mike
On Wed, Jun 03, 2009 at 08:37:54AM -0400, Mike Frysinger wrote:
On Wed, Jun 3, 2009 at 06:52, Mark Brown wrote:
However, given that this fix has missed two kernel releases due to the very slow rate of responses to review comments I've applied it so users get the benefit of the bug fix.
not sure what you're referring to
This looks like a respin of a fix which was originally submitted back in 2.6.29 era or so. IIRC there's been two or three versions, each of which needed fixes.
Things would go more smoothly if you could get new versions of patches submitted faster after the original review.
the previous Blackfin kernel maintainer changed jobs so we've been transitioning. reality is, this doesnt happen over nite.
I appreciate that Brian leaving won't have helped here but this was an issue before then, it feels like there's some sort of general process issue going on. I'm not sure what it is - IIRC there tends to be about one set of Blackfin patches per kernel release.
It's not specifically causing me problems, it's more of a concern from the point of view of getting stuff to users.
On Wed, Jun 3, 2009 at 10:53, Mark Brown wrote:
On Wed, Jun 03, 2009 at 08:37:54AM -0400, Mike Frysinger wrote:
On Wed, Jun 3, 2009 at 06:52, Mark Brown wrote:
However, given that this fix has missed two kernel releases due to the very slow rate of responses to review comments I've applied it so users get the benefit of the bug fix.
not sure what you're referring to
This looks like a respin of a fix which was originally submitted back in 2.6.29 era or so. IIRC there's been two or three versions, each of which needed fixes.
it could be. i was going by the changes that werent in 2.6.30. i thought i looked for previous submissions, but i must have missed previous attempts. i'm not familiar with alsa stuff, so i'm not always sure when i need to bang on Cliff to get things fixed up (changes split / better comments / etc...).
Things would go more smoothly if you could get new versions of patches submitted faster after the original review.
the previous Blackfin kernel maintainer changed jobs so we've been transitioning. reality is, this doesnt happen over nite.
I appreciate that Brian leaving won't have helped here but this was an issue before then, it feels like there's some sort of general process issue going on. I'm not sure what it is - IIRC there tends to be about one set of Blackfin patches per kernel release.
It's not specifically causing me problems, it's more of a concern from the point of view of getting stuff to users.
if fixes miss a merge window, then that's an issue. but in general, users only see fixes per release, so sending things more quickly there wouldnt really help ? -mike
Mike Frysinger wrote:
On Wed, Jun 3, 2009 at 10:53, Mark Brown wrote:
On Wed, Jun 03, 2009 at 08:37:54AM -0400, Mike Frysinger wrote:
On Wed, Jun 3, 2009 at 06:52, Mark Brown wrote:
However, given that this fix has missed two kernel releases due to the very slow rate of responses to review comments I've applied it so users get the benefit of the bug fix.
not sure what you're referring to
This looks like a respin of a fix which was originally submitted back in 2.6.29 era or so. IIRC there's been two or three versions, each of which needed fixes.
it could be. i was going by the changes that werent in 2.6.30. i thought i looked for previous submissions, but i must have missed previous attempts. i'm not familiar with alsa stuff, so i'm not always sure when i need to bang on Cliff to get things fixed up (changes split / better comments / etc...).
Anyways, this commit has been trying to fix something already fixed since the first submission and if you look at its effect on the current tree you will see duplicate code.
On Thu, Jun 4, 2009 at 04:17, Karl Beldan wrote:
Mike Frysinger wrote:
On Wed, Jun 3, 2009 at 10:53, Mark Brown wrote:
On Wed, Jun 03, 2009 at 08:37:54AM -0400, Mike Frysinger wrote:
On Wed, Jun 3, 2009 at 06:52, Mark Brown wrote:
However, given that this fix has missed two kernel releases due to the very slow rate of responses to review comments I've applied it so users get the benefit of the bug fix.
not sure what you're referring to
This looks like a respin of a fix which was originally submitted back in 2.6.29 era or so. IIRC there's been two or three versions, each of which needed fixes.
it could be. i was going by the changes that werent in 2.6.30. i thought i looked for previous submissions, but i must have missed previous attempts. i'm not familiar with alsa stuff, so i'm not always sure when i need to bang on Cliff to get things fixed up (changes split / better comments / etc...).
Anyways, this commit has been trying to fix something already fixed since the first submission and if you look at its effect on the current tree you will see duplicate code.
you're going to have to be a bit more specific. i dont know alsa, so when i looked at the change history, i saw framework conversions. about the only thing that might be related is faab5a32. is that what you're talking about ? -mike
-----Original Message----- From: Mike Frysinger [mailto:vapier.adi@gmail.com] Sent: Thursday, June 04, 2009 4:23 PM To: Karl Beldan Cc: Mark Brown; uclinux-dist-devel@blackfin.uclinux.org; alsa-devel@alsa-project.org; Cai, Cliff Subject: Re: [alsa-devel] [Uclinux-dist-devel] [PATCH 2/5] ASoC codec: SSM2602: assign last substream to the master when shutting down
On Thu, Jun 4, 2009 at 04:17, Karl Beldan wrote:
Mike Frysinger wrote:
On Wed, Jun 3, 2009 at 10:53, Mark Brown wrote:
On Wed, Jun 03, 2009 at 08:37:54AM -0400, Mike Frysinger wrote:
On Wed, Jun 3, 2009 at 06:52, Mark Brown wrote:
However, given that this fix has missed two kernel
releases due to
the very slow rate of responses to review comments I've
applied it
so users get the benefit of the bug fix.
not sure what you're referring to
This looks like a respin of a fix which was originally submitted back in 2.6.29 era or so. IIRC there's been two or three
versions, each of
which needed fixes.
it could be. i was going by the changes that werent in 2.6.30. i thought i looked for previous submissions, but i must have missed previous attempts. i'm not familiar with alsa stuff, so i'm not always sure when i need to bang on Cliff to get things fixed up (changes split / better comments / etc...).
Anyways, this commit has been trying to fix something already fixed since the first submission and if you look at its effect on the current tree you will see duplicate code.
you're going to have to be a bit more specific. i dont know alsa, so when i looked at the change history, i saw framework conversions. about the only thing that might be related is faab5a32. is that what you're talking about ? -mike
The blackfin tree is quite different from the latest mainline tree now,I need to make fixes against the mainline tree And resend the patches
Cliff
On Thu, Jun 4, 2009 at 04:31, Cai, Cliff wrote:
The blackfin tree is quite different from the latest mainline tree now,I need to make fixes against the mainline tree And resend the patches
i wouldnt bother. 2.6.30 is going to be out shortly and i'll update our tree to that soon after. -mike
-----Original Message----- From: Mike Frysinger [mailto:vapier.adi@gmail.com] Sent: Thursday, June 04, 2009 4:46 PM To: Cai, Cliff Cc: Karl Beldan; Mark Brown; uclinux-dist-devel@blackfin.uclinux.org; alsa-devel@alsa-project.org Subject: Re: [alsa-devel] [Uclinux-dist-devel] [PATCH 2/5] ASoC codec: SSM2602: assign last substream to the master when shutting down
On Thu, Jun 4, 2009 at 04:31, Cai, Cliff wrote:
The blackfin tree is quite different from the latest mainline tree now,I need to make fixes against the mainline tree And resend the patches
i wouldnt bother. 2.6.30 is going to be out shortly and i'll update our tree to that soon after.
Then,should I re-make the patches against the mainline tree right now(I've cloned the mainline tree locally) or wait untill you've updated blackfin tree?
Cliff
Cai, Cliff wrote:
-----Original Message----- From: Mike Frysinger [mailto:vapier.adi@gmail.com] Sent: Thursday, June 04, 2009 4:46 PM To: Cai, Cliff Cc: Karl Beldan; Mark Brown; uclinux-dist-devel@blackfin.uclinux.org; alsa-devel@alsa-project.org Subject: Re: [alsa-devel] [Uclinux-dist-devel] [PATCH 2/5] ASoC codec: SSM2602: assign last substream to the master when shutting down
On Thu, Jun 4, 2009 at 04:31, Cai, Cliff wrote:
The blackfin tree is quite different from the latest mainline tree now,I need to make fixes against the mainline tree And resend the patches
i wouldnt bother. 2.6.30 is going to be out shortly and i'll update our tree to that soon after.
Then,should I re-make the patches against the mainline tree right now(I've cloned the mainline tree locally) or wait untill you've updated blackfin tree?
I suggest you test the blackfin tree once it has been updated, see if it got rid of the problems you encountered back then.
Mike Frysinger wrote:
On Thu, Jun 4, 2009 at 04:17, Karl Beldan wrote:
Mike Frysinger wrote:
On Wed, Jun 3, 2009 at 10:53, Mark Brown wrote:
On Wed, Jun 03, 2009 at 08:37:54AM -0400, Mike Frysinger wrote:
On Wed, Jun 3, 2009 at 06:52, Mark Brown wrote:
However, given that this fix has missed two kernel releases due to the very slow rate of responses to review comments I've applied it so users get the benefit of the bug fix.
not sure what you're referring to
This looks like a respin of a fix which was originally submitted back in 2.6.29 era or so. IIRC there's been two or three versions, each of which needed fixes.
it could be. i was going by the changes that werent in 2.6.30. i thought i looked for previous submissions, but i must have missed previous attempts. i'm not familiar with alsa stuff, so i'm not always sure when i need to bang on Cliff to get things fixed up (changes split / better comments / etc...).
Anyways, this commit has been trying to fix something already fixed since the first submission and if you look at its effect on the current tree you will see duplicate code.
you're going to have to be a bit more specific. i dont know alsa, so when i looked at the change history, i saw framework conversions. about the only thing that might be related is faab5a32. is that what you're talking about ?
Indeed. The commit duplicates some code, IMO this should be discussed before going further.
-----Original Message----- From: Karl Beldan [mailto:karl.beldan@gmail.com] Sent: Thursday, June 04, 2009 4:41 PM To: Mike Frysinger Cc: Mark Brown; uclinux-dist-devel@blackfin.uclinux.org; alsa-devel@alsa-project.org; Cai, Cliff Subject: Re: [alsa-devel] [Uclinux-dist-devel] [PATCH 2/5] ASoC codec: SSM2602: assign last substream to the master when shutting down
Mike Frysinger wrote:
On Thu, Jun 4, 2009 at 04:17, Karl Beldan wrote:
Mike Frysinger wrote:
On Wed, Jun 3, 2009 at 10:53, Mark Brown wrote:
On Wed, Jun 03, 2009 at 08:37:54AM -0400, Mike Frysinger wrote:
On Wed, Jun 3, 2009 at 06:52, Mark Brown wrote: > However, given that this fix has missed two kernel releases due > to the very slow rate of responses to review comments I've > applied it so users get the benefit of the bug fix. not sure what you're referring to
This looks like a respin of a fix which was originally submitted back in 2.6.29 era or so. IIRC there's been two or three
versions, each of
which needed fixes.
it could be. i was going by the changes that werent in 2.6.30. i thought i looked for previous submissions, but i must have missed previous attempts. i'm not familiar with alsa stuff, so i'm not always sure when i need to bang on Cliff to get things fixed up (changes split / better comments / etc...).
Anyways, this commit has been trying to fix something already fixed since the first submission and if you look at its effect on the current tree you will see duplicate code.
you're going to have to be a bit more specific. i dont know
alsa, so
when i looked at the change history, i saw framework conversions. about the only thing that might be related is faab5a32. is
that what
you're talking about ?
Indeed. The commit duplicates some code, IMO this should be discussed before going further.
Could you point it out,I don't know much about the changes of alsa in recent months.
Thanks a lot.
Cliff
On Thu, Jun 4, 2009 at 04:50, Cai, Cliff wrote:
Could you point it out,I don't know much about the changes of alsa in recent months.
he is talking about this commit: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdif...
which presumably makes the second change of your patch obsolete. which leaves the first change and i think Mark is still awaiting an explanation for it as it appears to be unrelated to the latter change. -mike
On Wed, Jun 03, 2009 at 09:26:34PM -0400, Mike Frysinger wrote:
it could be. i was going by the changes that werent in 2.6.30. i thought i looked for previous submissions, but i must have missed previous attempts. i'm not familiar with alsa stuff, so i'm not
The patch hadn't previously been sent in that form but it looks like it's attempting to fix a bug which had had some previous goes at fixing it that required updates.
always sure when i need to bang on Cliff to get things fixed up (changes split / better comments / etc...).
If you're not sure there's a case for better changelogs at least! :)
if fixes miss a merge window, then that's an issue. but in general, users only see fixes per release, so sending things more quickly there wouldnt really help ?
The issue is that things are missing merge windows - they're getting sent in but if there's any review comments a revised version of the patch isn't getting posted until the next batch of submissions. It's these revisions rather than the initial submissions that I'm concerned about.
On Thu, Jun 4, 2009 at 04:59, Mark Brown wrote:
On Wed, Jun 03, 2009 at 09:26:34PM -0400, Mike Frysinger wrote:
always sure when i need to bang on Cliff to get things fixed up (changes split / better comments / etc...).
If you're not sure there's a case for better changelogs at least! :)
meh, ive read some of the alsa changelogs and looked through the code and none of it really mattered. i'm just happy sound comes out of my machine at all (including mixing).
if fixes miss a merge window, then that's an issue. but in general, users only see fixes per release, so sending things more quickly there wouldnt really help ?
The issue is that things are missing merge windows - they're getting sent in but if there's any review comments a revised version of the patch isn't getting posted until the next batch of submissions. It's these revisions rather than the initial submissions that I'm concerned about.
well i can say i'm addressing that, but really nothing i say matters as this will only be proven over time. -mike
On Thu, Jun 4, 2009 at 10:59 AM, Mark Brownbroonie@opensource.wolfsonmicro.com wrote:
On Wed, Jun 03, 2009 at 09:26:34PM -0400, Mike Frysinger wrote:
it could be. i was going by the changes that werent in 2.6.30. i thought i looked for previous submissions, but i must have missed previous attempts. i'm not familiar with alsa stuff, so i'm not
The patch hadn't previously been sent in that form but it looks like it's attempting to fix a bug which had had some previous goes at fixing it that required updates.
always sure when i need to bang on Cliff to get things fixed up (changes split / better comments / etc...).
If you're not sure there's a case for better changelogs at least! :)
if fixes miss a merge window, then that's an issue. but in general, users only see fixes per release, so sending things more quickly there wouldnt really help ?
The issue is that things are missing merge windows - they're getting sent in but if there's any review comments a revised version of the patch isn't getting posted until the next batch of submissions. It's these revisions rather than the initial submissions that I'm concerned about.
This - f692fce - should not have reached upstream, how come ?
On Sat, Jun 13, 2009 at 05:37, Karl Beldan wrote:
On Thu, Jun 4, 2009 at 10:59 AM, Mark Brownbroonie@opensource.wolfsonmicro.com wrote:
On Wed, Jun 03, 2009 at 09:26:34PM -0400, Mike Frysinger wrote:
it could be. i was going by the changes that werent in 2.6.30. i thought i looked for previous submissions, but i must have missed previous attempts. i'm not familiar with alsa stuff, so i'm not
The patch hadn't previously been sent in that form but it looks like it's attempting to fix a bug which had had some previous goes at fixing it that required updates.
always sure when i need to bang on Cliff to get things fixed up (changes split / better comments / etc...).
If you're not sure there's a case for better changelogs at least! :)
if fixes miss a merge window, then that's an issue. but in general, users only see fixes per release, so sending things more quickly there wouldnt really help ?
The issue is that things are missing merge windows - they're getting sent in but if there's any review comments a revised version of the patch isn't getting posted until the next batch of submissions. It's these revisions rather than the initial submissions that I'm concerned about.
This - f692fce - should not have reached upstream, how come ?
i was going to send a revert for that and 2552a71 -mike
On Sat, Jun 13, 2009 at 11:37:33AM +0200, Karl Beldan wrote:
This - f692fce - should not have reached upstream, how come ?
The entire discussion of the patch occurred in response to my saying I'd applied it, with most of the discussion being a couple of days later after it'd have already gone into ALSA. Since nobody asked to have it reverted or indicated that there was any actual problem other than a repitition of code it wasn't reverted. For the most part repitition in ASoC would just be a no-op and therefore a code quality rather than behavioural issue. As I indicated when accepting the patch I've lowered my standards there for blackfin due to the past issues with iteration speed (which Mike has already said will be being addressed now).
I've now reverted the shutdown hunk of the patch where duplication will actually introduce a bug.
On Sat, Jun 13, 2009 at 06:44, Mark Brown wrote:
On Sat, Jun 13, 2009 at 11:37:33AM +0200, Karl Beldan wrote:
This - f692fce - should not have reached upstream, how come ?
The entire discussion of the patch occurred in response to my saying I'd applied it, with most of the discussion being a couple of days later after it'd have already gone into ALSA. Since nobody asked to have it reverted or indicated that there was any actual problem other than a repitition of code it wasn't reverted. For the most part repitition in ASoC would just be a no-op and therefore a code quality rather than behavioural issue. As I indicated when accepting the patch I've lowered my standards there for blackfin due to the past issues with iteration speed (which Mike has already said will be being addressed now).
I've now reverted the shutdown hunk of the patch where duplication will actually introduce a bug.
i think Cliff verified that the other part of the change was no longer needed in his testing, so reverting the whole patch is fine. we've done that in the Blackfin tree. -mike
On Sat, Jun 13, 2009 at 06:47:17AM -0400, Mike Frysinger wrote:
i think Cliff verified that the other part of the change was no longer needed in his testing, so reverting the whole patch is fine. we've done that in the Blackfin tree.
So, the first part of the patch was applying constraints. You definitely want to do that with your current implementation - the hw_params() function will completely ignore the parameters set for the second substream so unless you use constraints the second application will get a configuration it didn't ask for which isn't going to work so well.
On Sat, Jun 13, 2009 at 06:59, Mark Brown wrote:
On Sat, Jun 13, 2009 at 06:47:17AM -0400, Mike Frysinger wrote:
i think Cliff verified that the other part of the change was no longer needed in his testing, so reverting the whole patch is fine. we've done that in the Blackfin tree.
So, the first part of the patch was applying constraints. You definitely want to do that with your current implementation - the hw_params() function will completely ignore the parameters set for the second substream so unless you use constraints the second application will get a configuration it didn't ask for which isn't going to work so well.
the first part conditionalized the constraints. ignoring whitespace changes, this is the diff: +if (master_runtime->rate != 0) snd_pcm_hw_constraint_minmax(substream->runtime, ...... +if (master_runtime->sample_bits != 0) snd_pcm_hw_constraint_minmax(substream->runtime, ......
or maybe that is what you're saying and i dont know alsa -mike
On Sat, Jun 13, 2009 at 07:03:12AM -0400, Mike Frysinger wrote:
the first part conditionalized the constraints. ignoring whitespace changes, this is the diff: +if (master_runtime->rate != 0) snd_pcm_hw_constraint_minmax(substream->runtime, ...... +if (master_runtime->sample_bits != 0) snd_pcm_hw_constraint_minmax(substream->runtime, ......
or maybe that is what you're saying and i dont know alsa
That's not a bad change - there's a thin race condition if you simultaneously start up two applications where the primary substream could be there but not yet configured since stream startup is separate to the configuration of the stream. It's hard but not impossible to reproduce.
At Sat, 13 Jun 2009 12:06:07 +0100, Mark Brown wrote:
On Sat, Jun 13, 2009 at 07:03:12AM -0400, Mike Frysinger wrote:
the first part conditionalized the constraints. ignoring whitespace changes, this is the diff: +if (master_runtime->rate != 0) snd_pcm_hw_constraint_minmax(substream->runtime, ...... +if (master_runtime->sample_bits != 0) snd_pcm_hw_constraint_minmax(substream->runtime, ......
or maybe that is what you're saying and i dont know alsa
That's not a bad change - there's a thin race condition if you simultaneously start up two applications where the primary substream could be there but not yet configured since stream startup is separate to the configuration of the stream. It's hard but not impossible to reproduce.
An absolute race-free hw-constraint for multiple substreams is actually difficult to implement. The best practice would be to give an error and refuse the change when any inconsistency occurs, IMO.
Takashi
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Sunday, June 14, 2009 4:16 PM To: Mark Brown Cc: Mike Frysinger; Karl Beldan; uclinux-dist-devel@blackfin.uclinux.org; alsa-devel@alsa-project.org; Cai, Cliff Subject: Re: [alsa-devel] [Uclinux-dist-devel] [PATCH 2/5] ASoC codec: SSM2602: assign last substream to the master when shutting down
At Sat, 13 Jun 2009 12:06:07 +0100, Mark Brown wrote:
On Sat, Jun 13, 2009 at 07:03:12AM -0400, Mike Frysinger wrote:
the first part conditionalized the constraints. ignoring
whitespace
changes, this is the diff: +if (master_runtime->rate != 0) snd_pcm_hw_constraint_minmax(substream->runtime, ...... +if (master_runtime->sample_bits != 0) snd_pcm_hw_constraint_minmax(substream->runtime, ......
or maybe that is what you're saying and i dont know alsa
That's not a bad change - there's a thin race condition if you simultaneously start up two applications where the primary substream could be there but not yet configured since stream startup
is separate
to the configuration of the stream. It's hard but not impossible to reproduce.
An absolute race-free hw-constraint for multiple substreams is actually difficult to implement. The best practice would be to give an error and refuse the change when any inconsistency occurs, IMO.
Without this patch, Oss-based applications (such as "tone","vplay" etc.) won't work properly.
Cliff
From: Cliff Cai cliff.cai@analog.com
Treat the configured field as a boolean rather than using a counter where we only check one value so the process is more reliable.
Signed-off-by: Cliff Cai cliff.cai@analog.com Signed-off-by: Mike Frysinger vapier@gentoo.org --- sound/soc/blackfin/bf5xx-i2s.c | 18 ++++-------------- 1 files changed, 4 insertions(+), 14 deletions(-)
diff --git a/sound/soc/blackfin/bf5xx-i2s.c b/sound/soc/blackfin/bf5xx-i2s.c index 9648244..1c07696 100644 --- a/sound/soc/blackfin/bf5xx-i2s.c +++ b/sound/soc/blackfin/bf5xx-i2s.c @@ -49,7 +49,7 @@ struct bf5xx_i2s_port { u16 rcr1; u16 tcr2; u16 rcr2; - int counter; + int configured; };
static struct bf5xx_i2s_port bf5xx_i2s; @@ -132,16 +132,6 @@ static int bf5xx_i2s_set_dai_fmt(struct snd_soc_dai *cpu_dai, return ret; }
-static int bf5xx_i2s_startup(struct snd_pcm_substream *substream, - struct snd_soc_dai *dai) -{ - pr_debug("%s enter\n", __func__); - - /*this counter is used for counting how many pcm streams are opened*/ - bf5xx_i2s.counter++; - return 0; -} - static int bf5xx_i2s_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params, struct snd_soc_dai *dai) @@ -168,7 +158,7 @@ static int bf5xx_i2s_hw_params(struct snd_pcm_substream *substream, break; }
- if (bf5xx_i2s.counter == 1) { + if (!bf5xx_i2s.configured) { /* * TX and RX are not independent,they are enabled at the * same time, even if only one side is running. So, we @@ -190,6 +180,7 @@ static int bf5xx_i2s_hw_params(struct snd_pcm_substream *substream, pr_err("SPORT is busy!\n"); return -EBUSY; } + bf5xx_i2s.configured = 1; }
return 0; @@ -199,7 +190,7 @@ static void bf5xx_i2s_shutdown(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { pr_debug("%s enter\n", __func__); - bf5xx_i2s.counter--; + bf5xx_i2s.configured = 0; }
static int bf5xx_i2s_probe(struct platform_device *pdev, @@ -288,7 +279,6 @@ static int bf5xx_i2s_resume(struct platform_device *pdev, SNDRV_PCM_FMTBIT_S32_LE)
static struct snd_soc_dai_ops bf5xx_i2s_dai_ops = { - .startup = bf5xx_i2s_startup, .shutdown = bf5xx_i2s_shutdown, .hw_params = bf5xx_i2s_hw_params, .set_fmt = bf5xx_i2s_set_dai_fmt,
On Tue, Jun 02, 2009 at 12:18:55AM -0400, Mike Frysinger wrote:
@@ -199,7 +190,7 @@ static void bf5xx_i2s_shutdown(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { pr_debug("%s enter\n", __func__);
- bf5xx_i2s.counter--;
- bf5xx_i2s.configured = 0;
}
Are you sure this works properly with simultaneous record and playback? If one stops while the other is running then the configured flag will be cleared, meaning that the stopped stream can be restarted and used to reconfigure the DAI which appears to be what the code is trying to prevent.
This should be using constraints to stop applications trying to reconfigure active streams - for the sample rate you just need to set symmetric_rates in the DAI.
-----Original Message----- From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] Sent: Wednesday, June 03, 2009 6:29 PM To: Mike Frysinger Cc: alsa-devel@alsa-project.org; uclinux-dist-devel@blackfin.uclinux.org; Cai, Cliff Subject: Re: [alsa-devel] [PATCH 3/5] ASoC: Blackfin: tweak how weinitialize the SPORT
On Tue, Jun 02, 2009 at 12:18:55AM -0400, Mike Frysinger wrote:
@@ -199,7 +190,7 @@ static void bf5xx_i2s_shutdown(struct
snd_pcm_substream *substream,
struct snd_soc_dai *dai)
{ pr_debug("%s enter\n", __func__);
- bf5xx_i2s.counter--;
- bf5xx_i2s.configured = 0;
}
Are you sure this works properly with simultaneous record and playback? If one stops while the other is running then the configured flag will be cleared, meaning that the stopped stream can be restarted and used to reconfigure the DAI which appears to be what the code is trying to prevent.
This should be using constraints to stop applications trying to reconfigure active streams - for the sample rate you just need to set symmetric_rates in the DAI.
This is not a problem of rate but format,I think combine the two flags will solve all the problems. The reason I write this patch is that I found when some oss-based applications open soundcard ,the startup callbacks will be called several times. So using the counter to record opened pcm streams is not reliable.
Thanks
Cliff
On Thu, Jun 04, 2009 at 10:36:48AM +0800, Cai, Cliff wrote:
This is not a problem of rate but format,I think combine the two flags will solve all the problems.
Ah, yes - I was remembering the ssm2602 driver which does do rate constraints.
The reason I write this patch is that I found when some oss-based applications open soundcard ,the startup callbacks will be called several times.
All OSS applications do this, OSS has a separate ioctl for each configuration option so you get lots of configuration calls.
So using the counter to record opened pcm streams is not reliable.
It's only hw_params() that get called repeatedly.
I could be wrong but looking at the code I suspect you may just need to make the reconfiguration conditional on if it's a capture or playback stream and you'll be fine - at the minute you're configuring both RX and TX on every hw_params() but there are separate tcr2 and rcr2 registers that you write to. If you only wrote to the appropriate one then you wouldn't need to worry about interfering with another stream for that configuration. There is the wdsize parameter for the SPORT as well but a brief glance at the code for that suggests that there are actually separate RX and TX configurations there too.
If not then adding constraint code would help a lot.
-----Original Message----- From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] Sent: Thursday, June 04, 2009 6:51 PM To: Cai, Cliff Cc: Mike Frysinger; uclinux-dist-devel@blackfin.uclinux.org; alsa-devel@alsa-project.org Subject: Re: [alsa-devel] [PATCH 3/5] ASoC: Blackfin: tweak howweinitialize the SPORT
On Thu, Jun 04, 2009 at 10:36:48AM +0800, Cai, Cliff wrote:
This is not a problem of rate but format,I think combine the
two flags
will solve all the problems.
Ah, yes - I was remembering the ssm2602 driver which does do rate constraints.
The reason I write this patch is that I found when some oss-based applications open soundcard ,the startup callbacks will be called several times.
All OSS applications do this, OSS has a separate ioctl for each configuration option so you get lots of configuration calls.
So using the counter to record opened pcm streams is not reliable.
It's only hw_params() that get called repeatedly.
I could be wrong but looking at the code I suspect you may just need to make the reconfiguration conditional on if it's a capture or playback stream and you'll be fine - at the minute you're configuring both RX and TX on every hw_params() but there are separate tcr2 and rcr2 registers that you write to. If you only wrote to the appropriate one then you wouldn't need to worry about interfering with another stream for that configuration. There is the wdsize parameter for the SPORT as well but a brief glance at the code for that suggests that there are actually separate RX and TX configurations there too.
If not then adding constraint code would help a lot.
Yes,if RX and TX can be configured separately ,it would be much easier... But for full-duplex consideration,we now don't run RX and TX independently, I've tested the combining of "counter" and "configured" together to sovle the problem You mentioned by clearing "configured" when counter is 0.Once blackfin tree has been Updated to the head I will send you the patch.
Thanks a lot!
Cliff
From: Cliff Cai cliff.cai@analog.com
Do not let the SPORT be reconfigured until there are no more active streams. Then we can let the system reprogram the SPORT state.
Signed-off-by: Cliff Cai cliff.cai@analog.com Signed-off-by: Mike Frysinger vapier@gentoo.org --- sound/soc/blackfin/bf5xx-i2s.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/sound/soc/blackfin/bf5xx-i2s.c b/sound/soc/blackfin/bf5xx-i2s.c index 9648244..0e8af48 100644 --- a/sound/soc/blackfin/bf5xx-i2s.c +++ b/sound/soc/blackfin/bf5xx-i2s.c @@ -50,6 +50,7 @@ struct bf5xx_i2s_port { u16 tcr2; u16 rcr2; int counter; + int configured; };
static struct bf5xx_i2s_port bf5xx_i2s; @@ -168,7 +169,7 @@ static int bf5xx_i2s_hw_params(struct snd_pcm_substream *substream, break; }
- if (bf5xx_i2s.counter == 1) { + if (!bf5xx_i2s.configured) { /* * TX and RX are not independent,they are enabled at the * same time, even if only one side is running. So, we @@ -177,6 +178,7 @@ static int bf5xx_i2s_hw_params(struct snd_pcm_substream *substream, * * CPU DAI:slave mode. */ + bf5xx_i2s.configured = 1; ret = sport_config_rx(sport_handle, bf5xx_i2s.rcr1, bf5xx_i2s.rcr2, 0, 0); if (ret) { @@ -200,6 +202,9 @@ static void bf5xx_i2s_shutdown(struct snd_pcm_substream *substream, { pr_debug("%s enter\n", __func__); bf5xx_i2s.counter--; + /* No active stream, SPORT is allowed to be configured again. */ + if (!bf5xx_i2s.counter) + bf5xx_i2s.configured = 0; }
static int bf5xx_i2s_probe(struct platform_device *pdev,
On Sat, Jun 20, 2009 at 11:29:05AM -0400, Mike Frysinger wrote:
From: Cliff Cai cliff.cai@analog.com
Do not let the SPORT be reconfigured until there are no more active streams. Then we can let the system reprogram the SPORT state.
Applied.
Please remember to CC maintainers on patches, especially if not starting a new thread for them.
From: Cliff Cai cliff.cai@analog.com
Signed-off-by: Cliff Cai cliff.cai@analog.com Signed-off-by: Mike Frysinger vapier@gentoo.org Signed-off-by: Bryan Wu cooloney@kernel.org --- sound/soc/blackfin/bf5xx-sport.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/blackfin/bf5xx-sport.c b/sound/soc/blackfin/bf5xx-sport.c index b7953c8..469ce7f 100644 --- a/sound/soc/blackfin/bf5xx-sport.c +++ b/sound/soc/blackfin/bf5xx-sport.c @@ -190,7 +190,7 @@ static inline int sport_hook_rx_dummy(struct sport_device *sport) desc = get_dma_next_desc_ptr(sport->dma_rx_chan); /* Copy the descriptor which will be damaged to backup */ temp_desc = *desc; - desc->x_count = 0xa; + desc->x_count = sport->dummy_count / 2; desc->y_count = 0; desc->next_desc_addr = sport->dummy_rx_desc; local_irq_restore(flags); @@ -309,7 +309,7 @@ static inline int sport_hook_tx_dummy(struct sport_device *sport) desc = get_dma_next_desc_ptr(sport->dma_tx_chan); /* Store the descriptor which will be damaged */ temp_desc = *desc; - desc->x_count = 0xa; + desc->x_count = sport->dummy_count / 2; desc->y_count = 0; desc->next_desc_addr = sport->dummy_tx_desc; local_irq_restore(flags);
On Tue, Jun 02, 2009 at 12:18:56AM -0400, Mike Frysinger wrote:
From: Cliff Cai cliff.cai@analog.com
Signed-off-by: Cliff Cai cliff.cai@analog.com Signed-off-by: Mike Frysinger vapier@gentoo.org Signed-off-by: Bryan Wu cooloney@kernel.org
Applied, thanks.
From: Sonic Zhang sonic.zhang@analog.com
Signed-off-by: Sonic Zhang sonic.zhang@analog.com Signed-off-by: Mike Frysinger vapier@gentoo.org --- sound/soc/blackfin/bf5xx-ac97.c | 9 +++++++++ 1 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/sound/soc/blackfin/bf5xx-ac97.c b/sound/soc/blackfin/bf5xx-ac97.c index 8a935f2..b1ed423 100644 --- a/sound/soc/blackfin/bf5xx-ac97.c +++ b/sound/soc/blackfin/bf5xx-ac97.c @@ -31,6 +31,15 @@ #include "bf5xx-sport.h" #include "bf5xx-ac97.h"
+/* Anomaly notes: + * 05000250 - AD1980 is running in TDM mode and RFS/TFS are generated by SPORT + * contrtoller. But, RFSDIV and TFSDIV are always set to 16*16-1, + * while the max AC97 data size is 13*16. The DIV is always larger + * than data size. AD73311 and ad2602 are not running in TDM mode. + * AD1836 and AD73322 depend on external RFS/TFS only. So, this + * anomaly does not affect blackfin sound drivers. +*/ + static int *cmd_count; static int sport_num = CONFIG_SND_BF5XX_SPORT_NUM;
On Tue, Jun 02, 2009 at 12:18:57AM -0400, Mike Frysinger wrote:
From: Sonic Zhang sonic.zhang@analog.com
Signed-off-by: Sonic Zhang sonic.zhang@analog.com Signed-off-by: Mike Frysinger vapier@gentoo.org
Applied, thanks.
On Tue, Jun 02, 2009 at 12:18:53AM -0400, Mike Frysinger wrote:
From: Cliff Cai cliff.cai@analog.com
Signed-off-by: Cliff Cai cliff.cai@analog.com Signed-off-by: Mike Frysinger vapier@gentoo.org
Applied, thanks.
Mark Brown wrote:
On Tue, Jun 02, 2009 at 12:18:53AM -0400, Mike Frysinger wrote:
From: Cliff Cai cliff.cai@analog.com
Signed-off-by: Cliff Cai cliff.cai@analog.com Signed-off-by: Mike Frysinger vapier@gentoo.org
Applied, thanks.
These samples rates are supported by the codec, updating coeff_div would be a better move.
On Wed, Jun 03, 2009 at 02:11:36PM +0200, Karl Beldan wrote:
These samples rates are supported by the codec, updating coeff_div would be a better move.
It looks like the driver only supports a limited subset of the chip features as it is - there's also comments saying it supports asymmetric configurations but currently the driver only does symmetric record and playback setups.
Mark Brown wrote:
On Wed, Jun 03, 2009 at 02:11:36PM +0200, Karl Beldan wrote:
These samples rates are supported by the codec, updating coeff_div would be a better move.
It looks like the driver only supports a limited subset of the chip features as it is - there's also comments saying it supports asymmetric configurations but currently the driver only does symmetric record and playback setups.
True. The comment was from me. The codec supports these rates and the driver only needs little modification to support them (just update an array - coeff_div). Though this commit is better than nothing, updating coeff_div seems IMO the thing to do. This change being trivial, the commit feels like a feature cut.
participants (7)
-
Cai, Cliff
-
Karl Beldan
-
Mark Brown
-
Mark Brown
-
Mike Frysinger
-
Mike Frysinger
-
Takashi Iwai