At Wed, 03 Sep 2008 03:11:07 +1000, Ben Stanley wrote:
Takashi,
Further to your previous comments [1], I have attempted to address the locking issues. I have re-designed the code to simplify the locking problems.
This is the patch against alsa-kmirror.
Signed-off-by: Ben Stanley Ben.Stanley@exemail.com.au
Thanks for the patch!
[1] http://thread.gmane.org/gmane.linux.alsa.devel/55527/focus=55539
pci/ca0106/ca0106.h | 13 ++- pci/ca0106/ca0106_main.c | 367 ++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 347 insertions(+), 33 deletions(-)
diff --git a/pci/ca0106/ca0106.h b/pci/ca0106/ca0106.h index 74175fc..bf502b5 100644 --- a/pci/ca0106/ca0106.h +++ b/pci/ca0106/ca0106.h @@ -1,7 +1,7 @@ /*
- Copyright (c) 2004 James Courtier-Dutton James@superbug.demon.co.uk
- Driver CA0106 chips. e.g. Sound Blaster Audigy LS and Live 24bit
- Version: 0.0.22
- Version: 0.0.23
- FEATURES currently supported:
- See ca0106_main.c for features.
@@ -49,6 +49,8 @@
- Implement support for Line-in capture on SB Live 24bit.
- 0.0.22
- Add support for mute control on SB Live 24bit (cards w/ SPI DAC)
- 0.0.23
- Add support for playback sampling rate and format constraints.
- This code was initally based on code from ALSA's emu10k1x.c which is:
@@ -644,6 +646,8 @@
#include "ca_midi.h"
+#define DRVNAME "snd-ca0106"
struct snd_ca0106;
struct snd_ca0106_channel { @@ -659,6 +663,7 @@ struct snd_ca0106_pcm { struct snd_pcm_substream *substream; int channel_id; unsigned short running;
- unsigned short hw_reserved;
};
struct snd_ca0106_details { @@ -684,6 +689,7 @@ struct snd_ca0106 { unsigned short model; /* subsystem id */
spinlock_t emu_lock;
- spinlock_t pcm_lock;
The spinlock is needed if the lock is accessed in the IRQ handler. For your patch, use mutex instead.
However, the problem is that the lock is missing in some important places, namely, *_open(), *_close() and snd_ca0106_interrupt(). If it's used in snd_ca0106_interrupt(), it has to be spinlock indeed.
struct snd_ac97 *ac97; struct snd_pcm *pcm; @@ -703,6 +709,11 @@ struct snd_ca0106 { struct snd_ca_midi midi2;
u16 spi_dac_reg[16];
- unsigned short count_pb_44100_chan;
- unsigned short count_pb_non_44100_chan;
- unsigned short count_pb_S16_chan;
- unsigned short count_pb_S32_chan;
Any reason to use short instead of char?
+static unsigned int all_spdif_playback_rates[] =
- {44100, 48000, 96000, 192000};
+static int hw_rule_playback_rate(struct snd_pcm_hw_params *params,
struct snd_pcm_hw_rule *rule)+{
- struct snd_ca0106 *chip = rule->private;
- int mask;
- if (snd_BUG_ON(!chip))
return -EINVAL;- spin_lock(&chip->pcm_lock);
- if (chip->spdif_enable) {
if (snd_BUG_ON(chip->count_pb_44100_chan &&chip->count_pb_non_44100_chan)) {spin_unlock(&chip->pcm_lock);return -EINVAL;}
Don't use too much snd_BUG_ON(). This would be better to be a normal check since we have anyway races in parameter constraints, and this may happen indeed.
if (snd_BUG_ON(chip->count_pb_44100_chan +chip->count_pb_non_44100_chan > 4)) {spin_unlock(&chip->pcm_lock);return -EINVAL;}
This check can be dropped. Whether it's more than 4 doesn't affect the behavior of this function.
/* Compute the mask applied to all_spdif_playback_rates */if (chip->count_pb_44100_chan)mask = 0x1;else if (chip->count_pb_non_44100_chan)mask = 0xE;elsemask = 0xF;- } else {
/* 44100Hz is not supported for DAC (FIXME Why?) */
Remove FIXME Why, as James suggested.
+static int hw_rule_playback_format(struct snd_pcm_hw_params *params,
struct snd_pcm_hw_rule *rule)+{
- struct snd_ca0106 *chip = rule->private;
- struct snd_mask fmt, *f = hw_param_mask(
params, SNDRV_PCM_HW_PARAM_FORMAT);
Better to put f = hw_params_mask() outside so that you don't need ugly line break here.
- int result;
- if (snd_BUG_ON(!chip))
return -EINVAL;- snd_mask_none(&fmt);
- spin_lock(&chip->pcm_lock);
- if (snd_BUG_ON(chip->count_pb_S16_chan &&
chip->count_pb_S32_chan)) {spin_unlock(&chip->pcm_lock);return -EINVAL;- }
Remove snd_BUG_ON().
- if (snd_BUG_ON(chip->count_pb_S16_chan +
chip->count_pb_S32_chan > 4)) {spin_unlock(&chip->pcm_lock);return -EINVAL;- }
This check is needless.
@@ -509,10 +602,24 @@ static int snd_ca0106_pcm_open_playback_channel(struct snd_pcm_substream *substr //printk("open:channel_id=%d, chip=%p, channel=%p\n",channel_id, chip, channel); //channel->interrupt = snd_ca0106_pcm_channel_interrupt; channel->epcm = epcm;
- if ((err = snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS)) < 0)
- err = snd_pcm_hw_constraint_integer(
runtime, SNDRV_PCM_HW_PARAM_PERIODS);
Usually runtime is in the same line of 'snd_pcm_hw_constraint_integer('
- if (err < 0) return err;
- if ((err = snd_pcm_hw_constraint_step(runtime, 0, SNDRV_PCM_HW_PARAM_PERIOD_BYTES, 64)) < 0)
- err = snd_pcm_hw_constraint_step(
runtime, 0, SNDRV_PCM_HW_PARAM_PERIOD_BYTES, 64);
Ditto.
@@ -647,6 +754,55 @@ static int snd_ca0106_pcm_hw_params_playback(struct snd_pcm_substream *substream /* hw_free callback */ static int snd_ca0106_pcm_hw_free_playback(struct snd_pcm_substream *substream) {
- struct snd_ca0106 *chip = snd_pcm_substream_chip(substream);
- struct snd_pcm_runtime *runtime = substream->runtime, *runtimei;
- struct snd_ca0106_pcm *epcm = runtime->private_data;
- struct snd_ca0106_channel *pchannel;
- int channel = epcm->channel_id, chi;
- spin_lock(&chip->pcm_lock);
- epcm->hw_reserved = 0;
- chip->count_pb_44100_chan = chip->count_pb_non_44100_chan = 0;
- chip->count_pb_S16_chan = chip->count_pb_S32_chan = 0;
- for (chi = 0; chi < 4; ++chi) {
if (chi == channel)continue;pchannel = &(chip->playback_channels[chi]);if (!pchannel->use)continue;if (snd_BUG_ON(!pchannel->epcm)) {spin_unlock(&chip->pcm_lock);return -EINVAL;}
snd_BUG_ON() is wrong here. Note that pchannel->epcm isn't protected at all by chip->pcm_lock in your patch because it's not used in *_open() and *_close() callbacks. Use the lock to protect the assignment of these.
if (!pchannel->epcm->hw_reserved)continue;if (snd_BUG_ON(!pchannel->epcm->substream)) {spin_unlock(&chip->pcm_lock);return -EINVAL;}if (snd_BUG_ON(!pchannel->epcm->substream->runtime)) {spin_unlock(&chip->pcm_lock);return -EINVAL;}
Ditto.
runtimei = pchannel->epcm->substream->runtime;chip->count_pb_44100_chan += runtimei->rate == 44100;chip->count_pb_non_44100_chan += runtimei->rate != 44100;chip->count_pb_S16_chan +=runtimei->format == SNDRV_PCM_FORMAT_S16_LE;chip->count_pb_S32_chan +=runtimei->format == SNDRV_PCM_FORMAT_S32_LE;- }
- snd_BUG_ON(chip->count_pb_44100_chan && chip->count_pb_non_44100_chan);
- snd_BUG_ON(chip->count_pb_44100_chan +
chip->count_pb_non_44100_chan > 4);- snd_BUG_ON(chip->count_pb_S16_chan && chip->count_pb_S32_chan);
- snd_BUG_ON(chip->count_pb_S16_chan + chip->count_pb_S32_chan > 4);
snd_BUG_ON() here is rather useless.
@@ -668,53 +824,196 @@ static int snd_ca0106_pcm_hw_free_capture(struct snd_pcm_substream *substream) static int snd_ca0106_pcm_prepare_playback(struct snd_pcm_substream *substream)
(snip)
- /* We are forced to build the settings for all the channels. */
- spin_lock(&emu->pcm_lock);
- emu->count_pb_44100_chan = emu->count_pb_non_44100_chan = 0;
- emu->count_pb_S16_chan = emu->count_pb_S32_chan = 0;
- for (chi = 0; chi < 4; ++chi) {
if (chi == channel)continue;pchannel = &(emu->playback_channels[chi]);if (!pchannel->use)continue;if (snd_BUG_ON(!pchannel->epcm)) {spin_unlock(&emu->pcm_lock);return -EINVAL;}if (!pchannel->epcm->hw_reserved)continue;if (snd_BUG_ON(!pchannel->epcm->substream)) {spin_unlock(&emu->pcm_lock);return -EINVAL;}if (snd_BUG_ON(!pchannel->epcm->substream->runtime)) {spin_unlock(&emu->pcm_lock);return -EINVAL;}runtimei = pchannel->epcm->substream->runtime;emu->count_pb_44100_chan += runtimei->rate == 44100;emu->count_pb_non_44100_chan += runtimei->rate != 44100;emu->count_pb_S16_chan +=runtimei->format == SNDRV_PCM_FORMAT_S16_LE;emu->count_pb_S32_chan +=runtimei->format == SNDRV_PCM_FORMAT_S32_LE;
This is as same as in *_hw_free(). Better to split this as another function and call it from both appropriately.
thanks,
Takashi