* [PATCH 1/2] ca0106 playback 44100Hz support to SPDIF and playback format & rate constraints
@ 2008-08-22 14:38 Ben Stanley
2008-08-22 15:40 ` Takashi Iwai
0 siblings, 1 reply; 16+ messages in thread
From: Ben Stanley @ 2008-08-22 14:38 UTC (permalink / raw)
To: alsa-devel
[-- Attachment #1: Type: text/plain, Size: 15894 bytes --]
Dear list,
This patch provides the ability to play back digital audio via SPDIF at
a sampling rate of 44100Hz. It also implements rate constraints to
ensure that the hardware restrictions of the card are not violated. Note
that if 44100Hz is used, then all used channels must use that frequency.
For example, opening hw:0,0 at 48000 and then opening hw:0,1 at 44100
will fail due to constraints. However, you may successfully open both
hw:0,0 and hw:0,1 at 44100 (presume no other channels open). Further
details of the restriction may be found in the thread
http://thread.gmane.org/gmane.linux.alsa.devel/52149/focus=52345
This patch also implements playback format restrictions, such that only
one playback format may be in use at any time.
I tested driver playback using the attached script
(Test_ca0106_driver.sh). I get good audio output for all combinations of
sampling rates and formats except for 192kHz S16 (as noted at the end of
the script). Sometimes it works and sometimes it doesn't. The reason for
the failure is not apparent at this time. It could have something to do
with my receiver. I don't know.
The attached script also checks and validates that the rate and format
constraints operate as required.
I periodically have trouble with this driver not producing sound output.
This has been investigated and the cause is explained here
http://thread.gmane.org/gmane.linux.alsa.devel/55384/focus=55410
I intend to submit a new patch to address this in the future.
This patch has been in use for three months on my MythTV system (Ubuntu
8.04). I find it works reliably for me. I also use it with xine, aplay,
mplayer, mame, and childsplay.
I have checked with checkpatch.pl . Patch is against v1.0.18rc1.
Signed-off-by: Ben Stanley Ben.Stanley@exemail.com.au
This is the patch against alsa-kmirror.
---
pci/ca0106/ca0106.h | 7 +-
pci/ca0106/ca0106_main.c | 266 ++++++++++++++++++++++++++++++++++++++--------
2 files changed, 229 insertions(+), 44 deletions(-)
diff --git a/pci/ca0106/ca0106.h b/pci/ca0106/ca0106.h
index 74175fc..376fa9c 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 {
diff --git a/pci/ca0106/ca0106_main.c b/pci/ca0106/ca0106_main.c
index 2f8b28a..33f992b 100644
--- a/pci/ca0106/ca0106_main.c
+++ b/pci/ca0106/ca0106_main.c
@@ -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.25
+ * Version: 0.0.26
*
* FEATURES currently supported:
* Front, Rear and Center/LFE.
@@ -83,9 +83,14 @@
* Add support for mute control on SB Live 24bit (cards w/ SPI DAC)
* 0.0.25
* Powerdown SPI DAC channels when not in use
+ * 0.0.26 Ben Stanley
+ * Added support for output at 44100Hz rate (SPDIF only).
+ * Implemented constraints system for output rate and format.
*
* BUGS:
* Some stability problems when unloading the snd-ca0106 kernel module.
+ * Some programs fail to produce sound output (tested on SPDIF). See
+ * http://thread.gmane.org/gmane.linux.alsa.devel/55384/focus=55410
* --
*
* TODO:
@@ -145,6 +150,7 @@
#include <sound/core.h>
#include <sound/initval.h>
#include <sound/pcm.h>
+#include <sound/pcm_params.h>
#include <sound/ac97_codec.h>
#include <sound/info.h>
@@ -284,9 +290,9 @@ static struct snd_pcm_hardware snd_ca0106_playback_hw = {
SNDRV_PCM_INFO_MMAP_VALID |
SNDRV_PCM_INFO_SYNC_START,
.formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S32_LE,
- .rates = (SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_96000 |
- SNDRV_PCM_RATE_192000),
- .rate_min = 48000,
+ .rates = (SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000 |
+ SNDRV_PCM_RATE_96000 | SNDRV_PCM_RATE_192000),
+ .rate_min = 44100,
.rate_max = 192000,
.channels_min = 2, //1,
.channels_max = 2, //6,
@@ -318,6 +324,111 @@ static struct snd_pcm_hardware snd_ca0106_capture_hw = {
.fifo_size = 0,
};
+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 chi, any_44100 = 0, any_non_44100 = 0, mask = 0;
+ struct snd_ca0106_channel *chp = 0;
+ struct snd_pcm_runtime *runtime;
+ if (snd_BUG_ON(!chip))
+ return -EINVAL;
+
+ if (chip->spdif_enable) {
+ for (chi = 0; chi < 4; ++chi) {
+ chp = &(chip->playback_channels[chi]);
+ if (!chp->use)
+ continue;
+ if (snd_BUG_ON(!chp->epcm))
+ return -EINVAL;
+ if (!chp->epcm->hw_reserved)
+ continue;
+ if (snd_BUG_ON(!chp->epcm->substream))
+ return -EINVAL;
+ if (snd_BUG_ON(!chp->epcm->substream->runtime))
+ return -EINVAL;
+ runtime = chp->epcm->substream->runtime;
+ snd_printd("snd_hw_rule_playback_rate: ch=%d, "
+ "rate=%d.\n", chi, runtime->rate);
+ any_44100 += runtime->rate == 44100;
+ any_non_44100 += runtime->rate != 44100;
+ }
+ if (snd_BUG_ON(any_44100 && any_non_44100))
+ return -EINVAL;
+ if (any_44100)
+ mask = 0x1;
+ else if (any_non_44100)
+ mask = 0xE;
+ else
+ mask = 0xF;
+ } else {
+ /* 44100Hz is not supported for DAC (FIXME Why?) */
+ mask = 0xE;
+ }
+ snd_printd("snd_hw_rule_playback_rate: any_44100=%d, "
+ "any_non_44100=%d, mask=0x%X, spdif=%d\n",
+ any_44100, any_non_44100, mask, chip->spdif_enable);
+ return snd_interval_list(
+ hw_param_interval(params, SNDRV_PCM_HW_PARAM_RATE),
+ ARRAY_SIZE(all_spdif_playback_rates),
+ all_spdif_playback_rates, mask);
+}
+
+static int hw_rule_playback_format(
+ struct snd_pcm_hw_params *params,
+ struct snd_pcm_hw_rule *rule)
+{
+ struct snd_ca0106 *chip = rule->private;
+ int chi, any_S16 = 0, any_S32 = 0;
+ struct snd_ca0106_channel *chp = 0;
+ struct snd_pcm_runtime *runtime;
+ struct snd_mask fmt, *f = hw_param_mask(
+ params, SNDRV_PCM_HW_PARAM_FORMAT);
+ int result;
+ if (snd_BUG_ON(!chip))
+ return -EINVAL;
+ snd_mask_none(&fmt);
+
+ for (chi = 0; chi < 4; ++chi) {
+ chp = &(chip->playback_channels[chi]);
+ if (!chp->use)
+ continue;
+ if (snd_BUG_ON(!chp->epcm))
+ return -EINVAL;
+ if (!chp->epcm->hw_reserved)
+ continue;
+ if (snd_BUG_ON(!chp->epcm->substream))
+ return -EINVAL;
+ if (snd_BUG_ON(!chp->epcm->substream->runtime))
+ return -EINVAL;
+ runtime = chp->epcm->substream->runtime;
+ snd_printd("snd_hw_rule_playback_format: ch=%d, format=%d.\n",
+ chi, runtime->format);
+ any_S16 += runtime->format == SNDRV_PCM_FORMAT_S16_LE;
+ any_S32 += runtime->format == SNDRV_PCM_FORMAT_S32_LE;
+ }
+ if (snd_BUG_ON(any_S16 && any_S32))
+ return -EINVAL;
+ if (any_S16)
+ snd_mask_set(&fmt, SNDRV_PCM_FORMAT_S16_LE);
+ else if (any_S32)
+ snd_mask_set(&fmt, SNDRV_PCM_FORMAT_S32_LE);
+ else {
+ /* No format yet chosen, so both formats are available. */
+ snd_mask_set(&fmt, SNDRV_PCM_FORMAT_S16_LE);
+ snd_mask_set(&fmt, SNDRV_PCM_FORMAT_S32_LE);
+ }
+ result = snd_mask_refine(f, &fmt);
+ snd_printd("snd_hw_rule_playback_format: any_S16=%d, any_S32=%d, "
+ "refined_fmt=0x%X, avail_fmt=0x%X, changed=%d\n",
+ any_S16, any_S32, f->bits[0], fmt.bits[0], result);
+ return result;
+}
+
unsigned int snd_ca0106_ptr_read(struct snd_ca0106 * emu,
unsigned int reg,
unsigned int chn)
@@ -508,10 +619,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);
+ 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);
+ if (err < 0)
return err;
+ err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_RATE,
+ hw_rule_playback_rate, (void *)chip,
+ SNDRV_PCM_HW_PARAM_RATE, -1);
+ if (err < 0)
+ return err;
+ err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_FORMAT,
+ hw_rule_playback_format, (void *)chip,
+ SNDRV_PCM_HW_PARAM_FORMAT, -1);
+ if (err < 0)
+ return err;
snd_pcm_set_sync(substream);
if (chip->details->spi_dac && channel_id != PCM_FRONT_CHANNEL) {
@@ -646,6 +771,9 @@ 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_pcm_runtime *runtime = substream->runtime;
+ struct snd_ca0106_pcm *epcm = runtime->private_data;
+ epcm->hw_reserved = 0;
return snd_pcm_lib_free_pages(substream);
}
@@ -667,53 +795,103 @@ 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)
{
struct snd_ca0106 *emu = snd_pcm_substream_chip(substream);
- struct snd_pcm_runtime *runtime = substream->runtime;
+ struct snd_pcm_runtime *runtime = substream->runtime, *runtimei = 0;
struct snd_ca0106_pcm *epcm = runtime->private_data;
- int channel = epcm->channel_id;
+ struct snd_ca0106_channel *chp = 0;
+ int channel = epcm->channel_id, chi, any_44100 = 0, any_non_44100 = 0;
u32 *table_base = (u32 *)(emu->buffer.area+(8*16*channel));
u32 period_size_bytes = frames_to_bytes(runtime, runtime->period_size);
u32 hcfg_mask = HCFG_PLAYBACK_S32_LE;
u32 hcfg_set = 0x00000000;
u32 hcfg;
- u32 reg40_mask = 0x30000 << (channel<<1);
+ u32 reg40_mask = 0xFF0000;
u32 reg40_set = 0;
u32 reg40;
- /* FIXME: Depending on mixer selection of SPDIF out or not, select the spdif rate or the DAC rate. */
- u32 reg71_mask = 0x03030000 ; /* Global. Set SPDIF rate. We only support 44100 to spdif, not to DAC. */
+ u32 reg71_mask;
+ u32 reg71_shift;
u32 reg71_set = 0;
u32 reg71;
int i;
- //snd_printk("prepare:channel_number=%d, rate=%d, format=0x%x, channels=%d, buffer_size=%ld, period_size=%ld, periods=%u, frames_to_bytes=%d\n",channel, runtime->rate, runtime->format, runtime->channels, runtime->buffer_size, runtime->period_size, runtime->periods, frames_to_bytes(runtime, 1));
- //snd_printk("dma_addr=%x, dma_area=%p, table_base=%p\n",runtime->dma_addr, runtime->dma_area, table_base);
- //snd_printk("dma_addr=%x, dma_area=%p, dma_bytes(size)=%x\n",emu->buffer.addr, emu->buffer.area, emu->buffer.bytes);
- /* Rate can be set per channel. */
- /* reg40 control host to fifo */
- /* reg71 controls DAC rate. */
- switch (runtime->rate) {
- case 44100:
- reg40_set = 0x10000 << (channel<<1);
- reg71_set = 0x01010000;
- break;
- case 48000:
- reg40_set = 0;
- reg71_set = 0;
- break;
- case 96000:
- reg40_set = 0x20000 << (channel<<1);
- reg71_set = 0x02020000;
- break;
- case 192000:
- reg40_set = 0x30000 << (channel<<1);
- reg71_set = 0x03030000;
- break;
- default:
- reg40_set = 0;
- reg71_set = 0;
- break;
+ epcm->hw_reserved = 1;
+ /* FIXME CLEAN UP IF spdif_enable IS CHANGED WHILE CHANNELS ARE OPENED
+ * OR PREVENT THIS FROM HAPPENING. */
+ if (emu->spdif_enable)
+ reg71_shift = 24; /* SPDIF Output Rate */
+ else
+ reg71_shift = 16; /* I2S Output Rate */
+ reg71_mask = 0x3 << reg71_shift;
+
+ printk(KERN_DEBUG DRVNAME ": prepare_playback: "
+ "channel_number=%d, rate=%d, format=0x%x, channels=%d, "
+ "buffer_size=%ld,period_size=%ld, periods=%u, "
+ "frames_to_bytes=%d\n",
+ channel, runtime->rate, runtime->format, runtime->channels,
+ runtime->buffer_size, runtime->period_size, runtime->periods,
+ frames_to_bytes(runtime, 1));
+ /*printk("dma_addr=%x, dma_area=%p, table_base=%p\n",
+ runtime->dma_addr,runtime->dma_area, table_base);*/
+ /*printk("dma_addr=%x, dma_area=%p, dma_bytes(size)=%x\n",
+ emu->buffer.addr,emu->buffer.area, emu->buffer.bytes);*/
+ /* We are forced to build the settings for all the channels. */
+ for (chi = 0; chi < 4; ++chi) {
+ chp = &(emu->playback_channels[chi]);
+ if (!chp->use)
+ continue;
+ if (snd_BUG_ON(!chp->epcm))
+ return -EINVAL;
+ if (chi != channel && !chp->epcm->hw_reserved)
+ continue;
+ if (snd_BUG_ON(!chp->epcm->substream))
+ return -EINVAL;
+ if (snd_BUG_ON(!chp->epcm->substream->runtime))
+ return -EINVAL;
+ runtimei = chp->epcm->substream->runtime;
+ any_44100 += runtimei->rate == 44100;
+ any_non_44100 += runtimei->rate != 44100;
+ /* Rate can be set per channel. */
+ /* reg40 control host to fifo */
+ /* reg71 controls DAC rate. */
+ switch (runtimei->rate) {
+ case 44100:
+ /* We only support 44100 to spdif, not to DAC.
+ (FIXME WHY?)*/
+ if (emu->spdif_enable) {
+ /* When using 44100, *all* channels
+ must be set to that rate. */
+ reg40_set |= 0x550000;
+ reg71_set |= 0x1 << reg71_shift;
+ break;
+ } else {
+ printk(KERN_ERR DRVNAME
+ "prepare_playback: "
+ "44100Hz is invalid for DAC.\n");
+ }
+ case 48000:
+ /* reg40_set &= !(0x1 << (chi<<1)); */
+ /* reg71_set &= !(0x1 << reg71_shift); */
+ break;
+ case 96000:
+ reg40_set |= 0x20000 << (chi<<1);
+ reg71_set |= 0x2 << reg71_shift;
+ break;
+ case 192000:
+ reg40_set |= 0x30000 << (chi<<1);
+ reg71_set |= 0x3 << reg71_shift;
+ break;
+ default:
+ printk(KERN_ERR DRVNAME
+ ": prepare_playback: "
+ "Bad sampling frequency %d.\n",
+ runtimei->rate);
+ }
}
- /* Format is a global setting */
- /* FIXME: Only let the first channel accessed set this. */
+ printk(KERN_DEBUG DRVNAME ": prepare_playback: any_44100=%d, "
+ "any_non_44100=%d, spdif=%d.\n",
+ any_44100, any_non_44100, emu->spdif_enable);
+ /* Format is a global setting. */
+ /* Only the first channel accessed can set this
+ (enforced by constraints). */
switch (runtime->format) {
case SNDRV_PCM_FORMAT_S16_LE:
hcfg_set = 0;
@@ -1363,8 +1541,8 @@ static int __devinit snd_ca0106_create(int dev, struct snd_card *card,
pci_read_config_dword(pci, PCI_SUBSYSTEM_VENDOR_ID, &chip->serial);
pci_read_config_word(pci, PCI_SUBSYSTEM_ID, &chip->model);
#if 1
- printk(KERN_INFO "snd-ca0106: Model %04x Rev %08x Serial %08x\n", chip->model,
- pci->revision, chip->serial);
+ printk(KERN_INFO DRVNAME ": Model %04x Rev %08x Serial %08x\n",
+ chip->model, pci->revision, chip->serial);
#endif
strcpy(card->driver, "CA0106");
strcpy(card->shortname, "CA0106");
@@ -1378,7 +1556,9 @@ static int __devinit snd_ca0106_create(int dev, struct snd_card *card,
}
chip->details = c;
if (subsystem[dev]) {
- printk(KERN_INFO "snd-ca0106: Sound card name=%s, subsystem=0x%x. Forced to subsystem=0x%x\n",
+ printk(KERN_INFO DRVNAME
+ ": Sound card name=%s, subsystem=0x%x. "
+ "Forced to subsystem=0x%x\n",
c->name, chip->serial, subsystem[dev]);
}
--
1.5.4.3
[-- Attachment #2: Test_ca0106_driver.sh --]
[-- Type: application/x-shellscript, Size: 4891 bytes --]
[-- Attachment #3: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] ca0106 playback 44100Hz support to SPDIF and playback format & rate constraints
2008-08-22 14:38 [PATCH 1/2] ca0106 playback 44100Hz support to SPDIF and playback format & rate constraints Ben Stanley
@ 2008-08-22 15:40 ` Takashi Iwai
2008-08-23 14:37 ` Ben Stanley
0 siblings, 1 reply; 16+ messages in thread
From: Takashi Iwai @ 2008-08-22 15:40 UTC (permalink / raw)
To: Ben Stanley; +Cc: alsa-devel
At Sat, 23 Aug 2008 00:38:41 +1000,
Ben Stanley wrote:
>
> Dear list,
>
> This patch provides the ability to play back digital audio via SPDIF at
> a sampling rate of 44100Hz. It also implements rate constraints to
> ensure that the hardware restrictions of the card are not violated. Note
> that if 44100Hz is used, then all used channels must use that frequency.
> For example, opening hw:0,0 at 48000 and then opening hw:0,1 at 44100
> will fail due to constraints. However, you may successfully open both
> hw:0,0 and hw:0,1 at 44100 (presume no other channels open). Further
> details of the restriction may be found in the thread
> http://thread.gmane.org/gmane.linux.alsa.devel/52149/focus=52345
>
> This patch also implements playback format restrictions, such that only
> one playback format may be in use at any time.
>
> I tested driver playback using the attached script
> (Test_ca0106_driver.sh). I get good audio output for all combinations of
> sampling rates and formats except for 192kHz S16 (as noted at the end of
> the script). Sometimes it works and sometimes it doesn't. The reason for
> the failure is not apparent at this time. It could have something to do
> with my receiver. I don't know.
>
> The attached script also checks and validates that the rate and format
> constraints operate as required.
>
> I periodically have trouble with this driver not producing sound output.
> This has been investigated and the cause is explained here
> http://thread.gmane.org/gmane.linux.alsa.devel/55384/focus=55410
> I intend to submit a new patch to address this in the future.
>
> This patch has been in use for three months on my MythTV system (Ubuntu
> 8.04). I find it works reliably for me. I also use it with xine, aplay,
> mplayer, mame, and childsplay.
>
> I have checked with checkpatch.pl . Patch is against v1.0.18rc1.
>
> Signed-off-by: Ben Stanley Ben.Stanley@exemail.com.au
Thanks for the patch!
Through a quick review, I find it almost fine, but slight concerns below.
> +static int hw_rule_playback_rate(
> + struct snd_pcm_hw_params *params,
> + struct snd_pcm_hw_rule *rule)
Put arguments right after the open parenthesis.
It's more common style.
> +{
> + struct snd_ca0106 *chip = rule->private;
> + int chi, any_44100 = 0, any_non_44100 = 0, mask = 0;
No need to initialize here (at least for mask).
> + struct snd_ca0106_channel *chp = 0;
Ditto.
> + struct snd_pcm_runtime *runtime;
> + if (snd_BUG_ON(!chip))
> + return -EINVAL;
> +
> + if (chip->spdif_enable) {
> + for (chi = 0; chi < 4; ++chi) {
> + chp = &(chip->playback_channels[chi]);
I'm afraid it's racy. The chip->playback_channels[] can be changed at
any time. Try to put a mutex to protect this check.
> + if (!chp->use)
> + continue;
> + if (snd_BUG_ON(!chp->epcm))
> + return -EINVAL;
> + if (!chp->epcm->hw_reserved)
> + continue;
> + if (snd_BUG_ON(!chp->epcm->substream))
> + return -EINVAL;
> + if (snd_BUG_ON(!chp->epcm->substream->runtime))
> + return -EINVAL;
> + runtime = chp->epcm->substream->runtime;
> + snd_printd("snd_hw_rule_playback_rate: ch=%d, "
> + "rate=%d.\n", chi, runtime->rate);
Use snd_printdd() instead. CONFIG_SND_DEBUG=y on most cases, and this
would be too annoying.
> + any_44100 += runtime->rate == 44100;
> + any_non_44100 += runtime->rate != 44100;
> + }
> + if (snd_BUG_ON(any_44100 && any_non_44100))
> + return -EINVAL;
> + if (any_44100)
> + mask = 0x1;
> + else if (any_non_44100)
> + mask = 0xE;
> + else
> + mask = 0xF;
This mask value is a bit cryptic, but OK...
> + } else {
> + /* 44100Hz is not supported for DAC (FIXME Why?) */
> + mask = 0xE;
> + }
> + snd_printd("snd_hw_rule_playback_rate: any_44100=%d, "
> + "any_non_44100=%d, mask=0x%X, spdif=%d\n",
> + any_44100, any_non_44100, mask, chip->spdif_enable);
Use snd_printdd().
> + return snd_interval_list(
> + hw_param_interval(params, SNDRV_PCM_HW_PARAM_RATE),
> + ARRAY_SIZE(all_spdif_playback_rates),
> + all_spdif_playback_rates, mask);
Put arguments after the open parenthesis.
> +}
> +
> +static int hw_rule_playback_format(
> + struct snd_pcm_hw_params *params,
> + struct snd_pcm_hw_rule *rule)
Ditto.
> +{
> + struct snd_ca0106 *chip = rule->private;
> + int chi, any_S16 = 0, any_S32 = 0;
> + struct snd_ca0106_channel *chp = 0;
No need to initialize.
> + struct snd_pcm_runtime *runtime;
> + struct snd_mask fmt, *f = hw_param_mask(
> + params, SNDRV_PCM_HW_PARAM_FORMAT);
> + int result;
> + if (snd_BUG_ON(!chip))
> + return -EINVAL;
> + snd_mask_none(&fmt);
> +
> + for (chi = 0; chi < 4; ++chi) {
> + chp = &(chip->playback_channels[chi]);
This also needs a mutex as well.
> + if (!chp->use)
> + continue;
> + if (snd_BUG_ON(!chp->epcm))
> + return -EINVAL;
> + if (!chp->epcm->hw_reserved)
> + continue;
> + if (snd_BUG_ON(!chp->epcm->substream))
> + return -EINVAL;
> + if (snd_BUG_ON(!chp->epcm->substream->runtime))
> + return -EINVAL;
> + runtime = chp->epcm->substream->runtime;
> + snd_printd("snd_hw_rule_playback_format: ch=%d, format=%d.\n",
> + chi, runtime->format);
Use snd_printdd().
> + any_S16 += runtime->format == SNDRV_PCM_FORMAT_S16_LE;
> + any_S32 += runtime->format == SNDRV_PCM_FORMAT_S32_LE;
> + }
> + if (snd_BUG_ON(any_S16 && any_S32))
> + return -EINVAL;
> + if (any_S16)
> + snd_mask_set(&fmt, SNDRV_PCM_FORMAT_S16_LE);
> + else if (any_S32)
> + snd_mask_set(&fmt, SNDRV_PCM_FORMAT_S32_LE);
> + else {
> + /* No format yet chosen, so both formats are available. */
> + snd_mask_set(&fmt, SNDRV_PCM_FORMAT_S16_LE);
> + snd_mask_set(&fmt, SNDRV_PCM_FORMAT_S32_LE);
> + }
> + result = snd_mask_refine(f, &fmt);
> + snd_printd("snd_hw_rule_playback_format: any_S16=%d, any_S32=%d, "
> + "refined_fmt=0x%X, avail_fmt=0x%X, changed=%d\n",
> + any_S16, any_S32, f->bits[0], fmt.bits[0], result);
Use snd_printdd().
> + return result;
> +}
> +
> unsigned int snd_ca0106_ptr_read(struct snd_ca0106 * emu,
> unsigned int reg,
> unsigned int chn)
> @@ -508,10 +619,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);
> + 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);
> + if (err < 0)
> return err;
> + err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_RATE,
> + hw_rule_playback_rate, (void *)chip,
> + SNDRV_PCM_HW_PARAM_RATE, -1);
> + if (err < 0)
> + return err;
> + err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_FORMAT,
> + hw_rule_playback_format, (void *)chip,
> + SNDRV_PCM_HW_PARAM_FORMAT, -1);
> + if (err < 0)
> + return err;
> snd_pcm_set_sync(substream);
>
> if (chip->details->spi_dac && channel_id != PCM_FRONT_CHANNEL) {
> @@ -646,6 +771,9 @@ 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_pcm_runtime *runtime = substream->runtime;
> + struct snd_ca0106_pcm *epcm = runtime->private_data;
> + epcm->hw_reserved = 0;
> return snd_pcm_lib_free_pages(substream);
> }
>
> @@ -667,53 +795,103 @@ 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)
> {
> struct snd_ca0106 *emu = snd_pcm_substream_chip(substream);
> - struct snd_pcm_runtime *runtime = substream->runtime;
> + struct snd_pcm_runtime *runtime = substream->runtime, *runtimei = 0;
No need to initialize.
> struct snd_ca0106_pcm *epcm = runtime->private_data;
> - int channel = epcm->channel_id;
> + struct snd_ca0106_channel *chp = 0;
Ditto.
> + int channel = epcm->channel_id, chi, any_44100 = 0, any_non_44100 = 0;
> u32 *table_base = (u32 *)(emu->buffer.area+(8*16*channel));
> u32 period_size_bytes = frames_to_bytes(runtime, runtime->period_size);
> u32 hcfg_mask = HCFG_PLAYBACK_S32_LE;
> u32 hcfg_set = 0x00000000;
> u32 hcfg;
> - u32 reg40_mask = 0x30000 << (channel<<1);
> + u32 reg40_mask = 0xFF0000;
> u32 reg40_set = 0;
> u32 reg40;
> - /* FIXME: Depending on mixer selection of SPDIF out or not, select the spdif rate or the DAC rate. */
> - u32 reg71_mask = 0x03030000 ; /* Global. Set SPDIF rate. We only support 44100 to spdif, not to DAC. */
> + u32 reg71_mask;
> + u32 reg71_shift;
> u32 reg71_set = 0;
> u32 reg71;
> int i;
>
> - //snd_printk("prepare:channel_number=%d, rate=%d, format=0x%x, channels=%d, buffer_size=%ld, period_size=%ld, periods=%u, frames_to_bytes=%d\n",channel, runtime->rate, runtime->format, runtime->channels, runtime->buffer_size, runtime->period_size, runtime->periods, frames_to_bytes(runtime, 1));
> - //snd_printk("dma_addr=%x, dma_area=%p, table_base=%p\n",runtime->dma_addr, runtime->dma_area, table_base);
> - //snd_printk("dma_addr=%x, dma_area=%p, dma_bytes(size)=%x\n",emu->buffer.addr, emu->buffer.area, emu->buffer.bytes);
> - /* Rate can be set per channel. */
> - /* reg40 control host to fifo */
> - /* reg71 controls DAC rate. */
> - switch (runtime->rate) {
> - case 44100:
> - reg40_set = 0x10000 << (channel<<1);
> - reg71_set = 0x01010000;
> - break;
> - case 48000:
> - reg40_set = 0;
> - reg71_set = 0;
> - break;
> - case 96000:
> - reg40_set = 0x20000 << (channel<<1);
> - reg71_set = 0x02020000;
> - break;
> - case 192000:
> - reg40_set = 0x30000 << (channel<<1);
> - reg71_set = 0x03030000;
> - break;
> - default:
> - reg40_set = 0;
> - reg71_set = 0;
> - break;
> + epcm->hw_reserved = 1;
> + /* FIXME CLEAN UP IF spdif_enable IS CHANGED WHILE CHANNELS ARE OPENED
> + * OR PREVENT THIS FROM HAPPENING. */
> + if (emu->spdif_enable)
> + reg71_shift = 24; /* SPDIF Output Rate */
> + else
> + reg71_shift = 16; /* I2S Output Rate */
> + reg71_mask = 0x3 << reg71_shift;
> +
> + printk(KERN_DEBUG DRVNAME ": prepare_playback: "
> + "channel_number=%d, rate=%d, format=0x%x, channels=%d, "
> + "buffer_size=%ld,period_size=%ld, periods=%u, "
> + "frames_to_bytes=%d\n",
> + channel, runtime->rate, runtime->format, runtime->channels,
> + runtime->buffer_size, runtime->period_size, runtime->periods,
> + frames_to_bytes(runtime, 1));
Use snd_printdd() (if any).
> + /*printk("dma_addr=%x, dma_area=%p, table_base=%p\n",
> + runtime->dma_addr,runtime->dma_area, table_base);*/
> + /*printk("dma_addr=%x, dma_area=%p, dma_bytes(size)=%x\n",
> + emu->buffer.addr,emu->buffer.area, emu->buffer.bytes);*/
> + /* We are forced to build the settings for all the channels. */
> + for (chi = 0; chi < 4; ++chi) {
> + chp = &(emu->playback_channels[chi]);
Use a mutex in this function, too.
> + if (!chp->use)
> + continue;
> + if (snd_BUG_ON(!chp->epcm))
> + return -EINVAL;
> + if (chi != channel && !chp->epcm->hw_reserved)
> + continue;
> + if (snd_BUG_ON(!chp->epcm->substream))
> + return -EINVAL;
> + if (snd_BUG_ON(!chp->epcm->substream->runtime))
> + return -EINVAL;
> + runtimei = chp->epcm->substream->runtime;
> + any_44100 += runtimei->rate == 44100;
> + any_non_44100 += runtimei->rate != 44100;
> + /* Rate can be set per channel. */
> + /* reg40 control host to fifo */
> + /* reg71 controls DAC rate. */
> + switch (runtimei->rate) {
> + case 44100:
> + /* We only support 44100 to spdif, not to DAC.
> + (FIXME WHY?)*/
> + if (emu->spdif_enable) {
> + /* When using 44100, *all* channels
> + must be set to that rate. */
> + reg40_set |= 0x550000;
> + reg71_set |= 0x1 << reg71_shift;
> + break;
> + } else {
> + printk(KERN_ERR DRVNAME
> + "prepare_playback: "
> + "44100Hz is invalid for DAC.\n");
> + }
> + case 48000:
> + /* reg40_set &= !(0x1 << (chi<<1)); */
> + /* reg71_set &= !(0x1 << reg71_shift); */
> + break;
> + case 96000:
> + reg40_set |= 0x20000 << (chi<<1);
> + reg71_set |= 0x2 << reg71_shift;
> + break;
> + case 192000:
> + reg40_set |= 0x30000 << (chi<<1);
> + reg71_set |= 0x3 << reg71_shift;
> + break;
> + default:
> + printk(KERN_ERR DRVNAME
> + ": prepare_playback: "
> + "Bad sampling frequency %d.\n",
> + runtimei->rate);
> + }
> }
> - /* Format is a global setting */
> - /* FIXME: Only let the first channel accessed set this. */
> + printk(KERN_DEBUG DRVNAME ": prepare_playback: any_44100=%d, "
> + "any_non_44100=%d, spdif=%d.\n",
> + any_44100, any_non_44100, emu->spdif_enable);
Use snd_printdd().
> + /* Format is a global setting. */
> + /* Only the first channel accessed can set this
> + (enforced by constraints). */
> switch (runtime->format) {
> case SNDRV_PCM_FORMAT_S16_LE:
> hcfg_set = 0;
> @@ -1363,8 +1541,8 @@ static int __devinit snd_ca0106_create(int dev, struct snd_card *card,
> pci_read_config_dword(pci, PCI_SUBSYSTEM_VENDOR_ID, &chip->serial);
> pci_read_config_word(pci, PCI_SUBSYSTEM_ID, &chip->model);
> #if 1
> - printk(KERN_INFO "snd-ca0106: Model %04x Rev %08x Serial %08x\n", chip->model,
> - pci->revision, chip->serial);
> + printk(KERN_INFO DRVNAME ": Model %04x Rev %08x Serial %08x\n",
> + chip->model, pci->revision, chip->serial);
> #endif
> strcpy(card->driver, "CA0106");
> strcpy(card->shortname, "CA0106");
> @@ -1378,7 +1556,9 @@ static int __devinit snd_ca0106_create(int dev, struct snd_card *card,
> }
> chip->details = c;
> if (subsystem[dev]) {
> - printk(KERN_INFO "snd-ca0106: Sound card name=%s, subsystem=0x%x. Forced to subsystem=0x%x\n",
> + printk(KERN_INFO DRVNAME
> + ": Sound card name=%s, subsystem=0x%x. "
> + "Forced to subsystem=0x%x\n",
> c->name, chip->serial, subsystem[dev]);
> }
Could you fix these and repost?
Thanks!
Takashi
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] ca0106 playback 44100Hz support to SPDIF and playback format & rate constraints
2008-08-22 15:40 ` Takashi Iwai
@ 2008-08-23 14:37 ` Ben Stanley
2008-08-25 16:35 ` Takashi Iwai
0 siblings, 1 reply; 16+ messages in thread
From: Ben Stanley @ 2008-08-23 14:37 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel
[-- Attachment #1: Type: text/plain, Size: 18271 bytes --]
Takashi,
I have tried to address all of your concerns regarding formatting,
initialisers and printk/snd_printd/snd_printdd. I have a new patch
attached addressing these points.
However, you also raised three mutex issues, which I have not yet
addressed. I would like some more comments from you before I proceed.
hw_rule_playback_rate:
> + for (chi = 0; chi < 4; ++chi) {
> > + chp = &(chip->playback_channels[chi]);
>
> I'm afraid it's racy. The chip->playback_channels[] can be changed at
> any time. Try to put a mutex to protect this check.
hw_rule_playback_format:
> + for (chi = 0; chi < 4; ++chi) {
> > + chp = &(chip->playback_channels[chi]);
>
> This also needs a mutex as well.
snd_ca0106_pcm_prepare_playback:
> + for (chi = 0; chi < 4; ++chi) {
> > + chp = &(emu->playback_channels[chi]);
>
> Use a mutex in this function, too.
It seems you want to guard against interference to the members of
snd_ca0106_channel (and also the associated snd_ca0106_pcm). So far I
have detected that the following routines make use of these data
structures:
hw_rule_playback_rate (new)
hw_rule_playback_format (new)
snd_ca0106_pcm_open_playback_channel
snd_ca0106_pcm_close_playback
snd_ca0106_pcm_prepare_playback
snd_ca0106_interrupt
snd_ca0106_pcm_hw_free_playback
I don't yet claim that this list is exhaustive.
I'm not sure yet how I should do the mutex. Here is what I'm thinking
about:
1) introduce a new spinlock_t pcm_lock within snd_ca0106 to cover access
to *all* the pcm data (snd_ca0106_channel *4 + snd_ca0106_pcm * 4)
2) introduce a new spinlock_t pcm_lock within snd_ca0106_channel to
cover access to the snd_ca0106_channel *1 + snd_ca0106_pcm *1
Option 2 makes it difficult to make sure that hw_rule_playback_rate and
hw_rule_playback_format will work correctly when a channel is opened or
closed while the hw_rule_* call is in progress, so that suggests option
1) might be better.
Introducing the pcm_lock means that all the abovenamed functions will
have to hold the lock while doing their work.
(I'm not familiar with mutexes vs spinlocks in the kernel - point me to
a documentation url if you want me to switch.)
But, it seems to me that a race condition remains after all this. The
client code can do the hw_rule check on a channel, but another program
can prepare another channel using (using parameters which will change
the hw_rule result) before the first client gets back to prepare and
open its channel. In fact, I should change
snd_ca0106_pcm_prepare_playback so that it repeats the hardware check
(within a mutex) and returns -EINVAL if it fails. Unless there is
another mutex around that whole system to prevent this mess from
happening...
I presume that I should leave the existing emu_lock alone and leave it
to protect the register twiddling code exclusively.
I'll work on the mutex stuff after receiving feedback on the above.
Thanks for being encouraging about my first non-trivial patch here :-)
Ben Stanley.
On Fri, 2008-08-22 at 17:40 +0200, Takashi Iwai wrote:
> At Sat, 23 Aug 2008 00:38:41 +1000,
> Ben Stanley wrote:
> >
> > Dear list,
> >
> > This patch provides the ability to play back digital audio via SPDIF at
> > a sampling rate of 44100Hz. It also implements rate constraints to
> > ensure that the hardware restrictions of the card are not violated. Note
> > that if 44100Hz is used, then all used channels must use that frequency.
> > For example, opening hw:0,0 at 48000 and then opening hw:0,1 at 44100
> > will fail due to constraints. However, you may successfully open both
> > hw:0,0 and hw:0,1 at 44100 (presume no other channels open). Further
> > details of the restriction may be found in the thread
> > http://thread.gmane.org/gmane.linux.alsa.devel/52149/focus=52345
> >
> > This patch also implements playback format restrictions, such that only
> > one playback format may be in use at any time.
> >
> > I tested driver playback using the attached script
> > (Test_ca0106_driver.sh). I get good audio output for all combinations of
> > sampling rates and formats except for 192kHz S16 (as noted at the end of
> > the script). Sometimes it works and sometimes it doesn't. The reason for
> > the failure is not apparent at this time. It could have something to do
> > with my receiver. I don't know.
> >
> > The attached script also checks and validates that the rate and format
> > constraints operate as required.
> >
> > I periodically have trouble with this driver not producing sound output.
> > This has been investigated and the cause is explained here
> > http://thread.gmane.org/gmane.linux.alsa.devel/55384/focus=55410
> > I intend to submit a new patch to address this in the future.
> >
> > This patch has been in use for three months on my MythTV system (Ubuntu
> > 8.04). I find it works reliably for me. I also use it with xine, aplay,
> > mplayer, mame, and childsplay.
> >
> > I have checked with checkpatch.pl . Patch is against v1.0.18rc1.
> >
> > Signed-off-by: Ben Stanley Ben.Stanley@exemail.com.au
>
> Thanks for the patch!
>
> Through a quick review, I find it almost fine, but slight concerns below.
>
> > +static int hw_rule_playback_rate(
> > + struct snd_pcm_hw_params *params,
> > + struct snd_pcm_hw_rule *rule)
>
> Put arguments right after the open parenthesis.
> It's more common style.
>
> > +{
> > + struct snd_ca0106 *chip = rule->private;
> > + int chi, any_44100 = 0, any_non_44100 = 0, mask = 0;
>
> No need to initialize here (at least for mask).
>
> > + struct snd_ca0106_channel *chp = 0;
>
> Ditto.
>
> > + struct snd_pcm_runtime *runtime;
> > + if (snd_BUG_ON(!chip))
> > + return -EINVAL;
> > +
> > + if (chip->spdif_enable) {
> > + for (chi = 0; chi < 4; ++chi) {
> > + chp = &(chip->playback_channels[chi]);
>
> I'm afraid it's racy. The chip->playback_channels[] can be changed at
> any time. Try to put a mutex to protect this check.
>
> > + if (!chp->use)
> > + continue;
> > + if (snd_BUG_ON(!chp->epcm))
> > + return -EINVAL;
> > + if (!chp->epcm->hw_reserved)
> > + continue;
> > + if (snd_BUG_ON(!chp->epcm->substream))
> > + return -EINVAL;
> > + if (snd_BUG_ON(!chp->epcm->substream->runtime))
> > + return -EINVAL;
> > + runtime = chp->epcm->substream->runtime;
> > + snd_printd("snd_hw_rule_playback_rate: ch=%d, "
> > + "rate=%d.\n", chi, runtime->rate);
>
> Use snd_printdd() instead. CONFIG_SND_DEBUG=y on most cases, and this
> would be too annoying.
>
> > + any_44100 += runtime->rate == 44100;
> > + any_non_44100 += runtime->rate != 44100;
> > + }
> > + if (snd_BUG_ON(any_44100 && any_non_44100))
> > + return -EINVAL;
> > + if (any_44100)
> > + mask = 0x1;
> > + else if (any_non_44100)
> > + mask = 0xE;
> > + else
> > + mask = 0xF;
>
> This mask value is a bit cryptic, but OK...
>
> > + } else {
> > + /* 44100Hz is not supported for DAC (FIXME Why?) */
> > + mask = 0xE;
> > + }
> > + snd_printd("snd_hw_rule_playback_rate: any_44100=%d, "
> > + "any_non_44100=%d, mask=0x%X, spdif=%d\n",
> > + any_44100, any_non_44100, mask, chip->spdif_enable);
>
> Use snd_printdd().
>
> > + return snd_interval_list(
> > + hw_param_interval(params, SNDRV_PCM_HW_PARAM_RATE),
> > + ARRAY_SIZE(all_spdif_playback_rates),
> > + all_spdif_playback_rates, mask);
>
> Put arguments after the open parenthesis.
>
> > +}
> > +
> > +static int hw_rule_playback_format(
> > + struct snd_pcm_hw_params *params,
> > + struct snd_pcm_hw_rule *rule)
>
> Ditto.
>
> > +{
> > + struct snd_ca0106 *chip = rule->private;
> > + int chi, any_S16 = 0, any_S32 = 0;
> > + struct snd_ca0106_channel *chp = 0;
>
> No need to initialize.
>
> > + struct snd_pcm_runtime *runtime;
> > + struct snd_mask fmt, *f = hw_param_mask(
> > + params, SNDRV_PCM_HW_PARAM_FORMAT);
> > + int result;
> > + if (snd_BUG_ON(!chip))
> > + return -EINVAL;
> > + snd_mask_none(&fmt);
> > +
> > + for (chi = 0; chi < 4; ++chi) {
> > + chp = &(chip->playback_channels[chi]);
>
> This also needs a mutex as well.
>
> > + if (!chp->use)
> > + continue;
> > + if (snd_BUG_ON(!chp->epcm))
> > + return -EINVAL;
> > + if (!chp->epcm->hw_reserved)
> > + continue;
> > + if (snd_BUG_ON(!chp->epcm->substream))
> > + return -EINVAL;
> > + if (snd_BUG_ON(!chp->epcm->substream->runtime))
> > + return -EINVAL;
> > + runtime = chp->epcm->substream->runtime;
> > + snd_printd("snd_hw_rule_playback_format: ch=%d, format=%d.\n",
> > + chi, runtime->format);
>
> Use snd_printdd().
>
> > + any_S16 += runtime->format == SNDRV_PCM_FORMAT_S16_LE;
> > + any_S32 += runtime->format == SNDRV_PCM_FORMAT_S32_LE;
> > + }
> > + if (snd_BUG_ON(any_S16 && any_S32))
> > + return -EINVAL;
> > + if (any_S16)
> > + snd_mask_set(&fmt, SNDRV_PCM_FORMAT_S16_LE);
> > + else if (any_S32)
> > + snd_mask_set(&fmt, SNDRV_PCM_FORMAT_S32_LE);
> > + else {
> > + /* No format yet chosen, so both formats are available. */
> > + snd_mask_set(&fmt, SNDRV_PCM_FORMAT_S16_LE);
> > + snd_mask_set(&fmt, SNDRV_PCM_FORMAT_S32_LE);
> > + }
> > + result = snd_mask_refine(f, &fmt);
> > + snd_printd("snd_hw_rule_playback_format: any_S16=%d, any_S32=%d, "
> > + "refined_fmt=0x%X, avail_fmt=0x%X, changed=%d\n",
> > + any_S16, any_S32, f->bits[0], fmt.bits[0], result);
>
> Use snd_printdd().
>
>
> > + return result;
> > +}
> > +
> > unsigned int snd_ca0106_ptr_read(struct snd_ca0106 * emu,
> > unsigned int reg,
> > unsigned int chn)
> > @@ -508,10 +619,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);
> > + 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);
> > + if (err < 0)
> > return err;
> > + err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_RATE,
> > + hw_rule_playback_rate, (void *)chip,
> > + SNDRV_PCM_HW_PARAM_RATE, -1);
> > + if (err < 0)
> > + return err;
> > + err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_FORMAT,
> > + hw_rule_playback_format, (void *)chip,
> > + SNDRV_PCM_HW_PARAM_FORMAT, -1);
> > + if (err < 0)
> > + return err;
> > snd_pcm_set_sync(substream);
> >
> > if (chip->details->spi_dac && channel_id != PCM_FRONT_CHANNEL) {
> > @@ -646,6 +771,9 @@ 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_pcm_runtime *runtime = substream->runtime;
> > + struct snd_ca0106_pcm *epcm = runtime->private_data;
> > + epcm->hw_reserved = 0;
> > return snd_pcm_lib_free_pages(substream);
> > }
> >
> > @@ -667,53 +795,103 @@ 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)
> > {
> > struct snd_ca0106 *emu = snd_pcm_substream_chip(substream);
> > - struct snd_pcm_runtime *runtime = substream->runtime;
> > + struct snd_pcm_runtime *runtime = substream->runtime, *runtimei = 0;
>
> No need to initialize.
>
> > struct snd_ca0106_pcm *epcm = runtime->private_data;
> > - int channel = epcm->channel_id;
> > + struct snd_ca0106_channel *chp = 0;
>
> Ditto.
>
> > + int channel = epcm->channel_id, chi, any_44100 = 0, any_non_44100 = 0;
> > u32 *table_base = (u32 *)(emu->buffer.area+(8*16*channel));
> > u32 period_size_bytes = frames_to_bytes(runtime, runtime->period_size);
> > u32 hcfg_mask = HCFG_PLAYBACK_S32_LE;
> > u32 hcfg_set = 0x00000000;
> > u32 hcfg;
> > - u32 reg40_mask = 0x30000 << (channel<<1);
> > + u32 reg40_mask = 0xFF0000;
> > u32 reg40_set = 0;
> > u32 reg40;
> > - /* FIXME: Depending on mixer selection of SPDIF out or not, select the spdif rate or the DAC rate. */
> > - u32 reg71_mask = 0x03030000 ; /* Global. Set SPDIF rate. We only support 44100 to spdif, not to DAC. */
> > + u32 reg71_mask;
> > + u32 reg71_shift;
> > u32 reg71_set = 0;
> > u32 reg71;
> > int i;
> >
> > - //snd_printk("prepare:channel_number=%d, rate=%d, format=0x%x, channels=%d, buffer_size=%ld, period_size=%ld, periods=%u, frames_to_bytes=%d\n",channel, runtime->rate, runtime->format, runtime->channels, runtime->buffer_size, runtime->period_size, runtime->periods, frames_to_bytes(runtime, 1));
> > - //snd_printk("dma_addr=%x, dma_area=%p, table_base=%p\n",runtime->dma_addr, runtime->dma_area, table_base);
> > - //snd_printk("dma_addr=%x, dma_area=%p, dma_bytes(size)=%x\n",emu->buffer.addr, emu->buffer.area, emu->buffer.bytes);
> > - /* Rate can be set per channel. */
> > - /* reg40 control host to fifo */
> > - /* reg71 controls DAC rate. */
> > - switch (runtime->rate) {
> > - case 44100:
> > - reg40_set = 0x10000 << (channel<<1);
> > - reg71_set = 0x01010000;
> > - break;
> > - case 48000:
> > - reg40_set = 0;
> > - reg71_set = 0;
> > - break;
> > - case 96000:
> > - reg40_set = 0x20000 << (channel<<1);
> > - reg71_set = 0x02020000;
> > - break;
> > - case 192000:
> > - reg40_set = 0x30000 << (channel<<1);
> > - reg71_set = 0x03030000;
> > - break;
> > - default:
> > - reg40_set = 0;
> > - reg71_set = 0;
> > - break;
> > + epcm->hw_reserved = 1;
> > + /* FIXME CLEAN UP IF spdif_enable IS CHANGED WHILE CHANNELS ARE OPENED
> > + * OR PREVENT THIS FROM HAPPENING. */
> > + if (emu->spdif_enable)
> > + reg71_shift = 24; /* SPDIF Output Rate */
> > + else
> > + reg71_shift = 16; /* I2S Output Rate */
> > + reg71_mask = 0x3 << reg71_shift;
> > +
> > + printk(KERN_DEBUG DRVNAME ": prepare_playback: "
> > + "channel_number=%d, rate=%d, format=0x%x, channels=%d, "
> > + "buffer_size=%ld,period_size=%ld, periods=%u, "
> > + "frames_to_bytes=%d\n",
> > + channel, runtime->rate, runtime->format, runtime->channels,
> > + runtime->buffer_size, runtime->period_size, runtime->periods,
> > + frames_to_bytes(runtime, 1));
>
> Use snd_printdd() (if any).
>
> > + /*printk("dma_addr=%x, dma_area=%p, table_base=%p\n",
> > + runtime->dma_addr,runtime->dma_area, table_base);*/
> > + /*printk("dma_addr=%x, dma_area=%p, dma_bytes(size)=%x\n",
> > + emu->buffer.addr,emu->buffer.area, emu->buffer.bytes);*/
> > + /* We are forced to build the settings for all the channels. */
> > + for (chi = 0; chi < 4; ++chi) {
> > + chp = &(emu->playback_channels[chi]);
>
> Use a mutex in this function, too.
>
> > + if (!chp->use)
> > + continue;
> > + if (snd_BUG_ON(!chp->epcm))
> > + return -EINVAL;
> > + if (chi != channel && !chp->epcm->hw_reserved)
> > + continue;
> > + if (snd_BUG_ON(!chp->epcm->substream))
> > + return -EINVAL;
> > + if (snd_BUG_ON(!chp->epcm->substream->runtime))
> > + return -EINVAL;
> > + runtimei = chp->epcm->substream->runtime;
> > + any_44100 += runtimei->rate == 44100;
> > + any_non_44100 += runtimei->rate != 44100;
> > + /* Rate can be set per channel. */
> > + /* reg40 control host to fifo */
> > + /* reg71 controls DAC rate. */
> > + switch (runtimei->rate) {
> > + case 44100:
> > + /* We only support 44100 to spdif, not to DAC.
> > + (FIXME WHY?)*/
> > + if (emu->spdif_enable) {
> > + /* When using 44100, *all* channels
> > + must be set to that rate. */
> > + reg40_set |= 0x550000;
> > + reg71_set |= 0x1 << reg71_shift;
> > + break;
> > + } else {
> > + printk(KERN_ERR DRVNAME
> > + "prepare_playback: "
> > + "44100Hz is invalid for DAC.\n");
> > + }
> > + case 48000:
> > + /* reg40_set &= !(0x1 << (chi<<1)); */
> > + /* reg71_set &= !(0x1 << reg71_shift); */
> > + break;
> > + case 96000:
> > + reg40_set |= 0x20000 << (chi<<1);
> > + reg71_set |= 0x2 << reg71_shift;
> > + break;
> > + case 192000:
> > + reg40_set |= 0x30000 << (chi<<1);
> > + reg71_set |= 0x3 << reg71_shift;
> > + break;
> > + default:
> > + printk(KERN_ERR DRVNAME
> > + ": prepare_playback: "
> > + "Bad sampling frequency %d.\n",
> > + runtimei->rate);
> > + }
> > }
> > - /* Format is a global setting */
> > - /* FIXME: Only let the first channel accessed set this. */
> > + printk(KERN_DEBUG DRVNAME ": prepare_playback: any_44100=%d, "
> > + "any_non_44100=%d, spdif=%d.\n",
> > + any_44100, any_non_44100, emu->spdif_enable);
>
> Use snd_printdd().
>
> > + /* Format is a global setting. */
> > + /* Only the first channel accessed can set this
> > + (enforced by constraints). */
> > switch (runtime->format) {
> > case SNDRV_PCM_FORMAT_S16_LE:
> > hcfg_set = 0;
> > @@ -1363,8 +1541,8 @@ static int __devinit snd_ca0106_create(int dev, struct snd_card *card,
> > pci_read_config_dword(pci, PCI_SUBSYSTEM_VENDOR_ID, &chip->serial);
> > pci_read_config_word(pci, PCI_SUBSYSTEM_ID, &chip->model);
> > #if 1
> > - printk(KERN_INFO "snd-ca0106: Model %04x Rev %08x Serial %08x\n", chip->model,
> > - pci->revision, chip->serial);
> > + printk(KERN_INFO DRVNAME ": Model %04x Rev %08x Serial %08x\n",
> > + chip->model, pci->revision, chip->serial);
> > #endif
> > strcpy(card->driver, "CA0106");
> > strcpy(card->shortname, "CA0106");
> > @@ -1378,7 +1556,9 @@ static int __devinit snd_ca0106_create(int dev, struct snd_card *card,
> > }
> > chip->details = c;
> > if (subsystem[dev]) {
> > - printk(KERN_INFO "snd-ca0106: Sound card name=%s, subsystem=0x%x. Forced to subsystem=0x%x\n",
> > + printk(KERN_INFO DRVNAME
> > + ": Sound card name=%s, subsystem=0x%x. "
> > + "Forced to subsystem=0x%x\n",
> > c->name, chip->serial, subsystem[dev]);
> > }
>
>
> Could you fix these and repost?
>
>
> Thanks!
>
> Takashi
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
[-- Attachment #2: 0001-ca0106-playback-44100Hz-support-to-SPDIF-and-playbac.patch --]
[-- Type: application/mbox, Size: 16259 bytes --]
[-- Attachment #3: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] ca0106 playback 44100Hz support to SPDIF and playback format & rate constraints
2008-08-23 14:37 ` Ben Stanley
@ 2008-08-25 16:35 ` Takashi Iwai
2008-09-02 17:11 ` Ben Stanley
0 siblings, 1 reply; 16+ messages in thread
From: Takashi Iwai @ 2008-08-25 16:35 UTC (permalink / raw)
To: Ben Stanley; +Cc: alsa-devel
At Sun, 24 Aug 2008 00:37:04 +1000,
Ben Stanley wrote:
>
> Takashi,
>
> I have tried to address all of your concerns regarding formatting,
> initialisers and printk/snd_printd/snd_printdd. I have a new patch
> attached addressing these points.
Thanks!
> However, you also raised three mutex issues, which I have not yet
> addressed. I would like some more comments from you before I proceed.
>
> hw_rule_playback_rate:
> > + for (chi = 0; chi < 4; ++chi) {
> > > + chp = &(chip->playback_channels[chi]);
> >
> > I'm afraid it's racy. The chip->playback_channels[] can be changed at
> > any time. Try to put a mutex to protect this check.
>
> hw_rule_playback_format:
> > + for (chi = 0; chi < 4; ++chi) {
> > > + chp = &(chip->playback_channels[chi]);
> >
> > This also needs a mutex as well.
>
> snd_ca0106_pcm_prepare_playback:
> > + for (chi = 0; chi < 4; ++chi) {
> > > + chp = &(emu->playback_channels[chi]);
> >
> > Use a mutex in this function, too.
>
> It seems you want to guard against interference to the members of
> snd_ca0106_channel (and also the associated snd_ca0106_pcm). So far I
> have detected that the following routines make use of these data
> structures:
> hw_rule_playback_rate (new)
> hw_rule_playback_format (new)
> snd_ca0106_pcm_open_playback_channel
> snd_ca0106_pcm_close_playback
> snd_ca0106_pcm_prepare_playback
> snd_ca0106_interrupt
> snd_ca0106_pcm_hw_free_playback
> I don't yet claim that this list is exhaustive.
> I'm not sure yet how I should do the mutex. Here is what I'm thinking
> about:
> 1) introduce a new spinlock_t pcm_lock within snd_ca0106 to cover access
> to *all* the pcm data (snd_ca0106_channel *4 + snd_ca0106_pcm * 4)
> 2) introduce a new spinlock_t pcm_lock within snd_ca0106_channel to
> cover access to the snd_ca0106_channel *1 + snd_ca0106_pcm *1
>
> Option 2 makes it difficult to make sure that hw_rule_playback_rate and
> hw_rule_playback_format will work correctly when a channel is opened or
> closed while the hw_rule_* call is in progress, so that suggests option
> 1) might be better.
Right. The 1 is the right choice in this case.
> Introducing the pcm_lock means that all the abovenamed functions will
> have to hold the lock while doing their work.
>
> (I'm not familiar with mutexes vs spinlocks in the kernel - point me to
> a documentation url if you want me to switch.)
I forgot about that it's referred in the interrupt handler, too.
So it has to be a spinlock, indeed.
> But, it seems to me that a race condition remains after all this. The
> client code can do the hw_rule check on a channel, but another program
> can prepare another channel using (using parameters which will change
> the hw_rule result) before the first client gets back to prepare and
> open its channel. In fact, I should change
> snd_ca0106_pcm_prepare_playback so that it repeats the hardware check
> (within a mutex) and returns -EINVAL if it fails. Unless there is
> another mutex around that whole system to prevent this mess from
> happening...
It's a known issue that this constraint can't be race-free.
The only problem we need to fix right now is not to Oopsen -- to
protect the playback_channels[].
thanks,
Takashi
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] ca0106 playback 44100Hz support to SPDIF and playback format & rate constraints
2008-08-25 16:35 ` Takashi Iwai
@ 2008-09-02 17:11 ` Ben Stanley
2008-09-02 17:16 ` [PATCH 2/2] " Ben Stanley
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Ben Stanley @ 2008-09-02 17:11 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel
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
[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;
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;
};
int snd_ca0106_mixer(struct snd_ca0106 *emu);
diff --git a/pci/ca0106/ca0106_main.c b/pci/ca0106/ca0106_main.c
index a7d8966..806f34c 100644
--- a/pci/ca0106/ca0106_main.c
+++ b/pci/ca0106/ca0106_main.c
@@ -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.25
+ * Version: 0.0.26
*
* FEATURES currently supported:
* Front, Rear and Center/LFE.
@@ -83,9 +83,14 @@
* Add support for mute control on SB Live 24bit (cards w/ SPI DAC)
* 0.0.25
* Powerdown SPI DAC channels when not in use
+ * 0.0.26 Ben Stanley
+ * Added support for output at 44100Hz rate (SPDIF only).
+ * Implemented constraints system for output rate and format.
*
* BUGS:
* Some stability problems when unloading the snd-ca0106 kernel module.
+ * Some programs fail to produce sound output (tested on SPDIF). See
+ * http://thread.gmane.org/gmane.linux.alsa.devel/55384/focus=55410
* --
*
* TODO:
@@ -145,6 +150,7 @@
#include <sound/core.h>
#include <sound/initval.h>
#include <sound/pcm.h>
+#include <sound/pcm_params.h>
#include <sound/ac97_codec.h>
#include <sound/info.h>
@@ -285,9 +291,9 @@ static struct snd_pcm_hardware snd_ca0106_playback_hw = {
SNDRV_PCM_INFO_MMAP_VALID |
SNDRV_PCM_INFO_SYNC_START,
.formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S32_LE,
- .rates = (SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_96000 |
- SNDRV_PCM_RATE_192000),
- .rate_min = 48000,
+ .rates = (SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000 |
+ SNDRV_PCM_RATE_96000 | SNDRV_PCM_RATE_192000),
+ .rate_min = 44100,
.rate_max = 192000,
.channels_min = 2, //1,
.channels_max = 2, //6,
@@ -319,6 +325,93 @@ static struct snd_pcm_hardware snd_ca0106_capture_hw = {
.fifo_size = 0,
};
+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;
+ }
+ if (snd_BUG_ON(chip->count_pb_44100_chan +
+ chip->count_pb_non_44100_chan > 4)) {
+ spin_unlock(&chip->pcm_lock);
+ return -EINVAL;
+ }
+ /* 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;
+ else
+ mask = 0xF;
+ } else {
+ /* 44100Hz is not supported for DAC (FIXME Why?) */
+ mask = 0xE;
+ }
+ snd_printdd("snd_hw_rule_playback_rate: any_44100=%d, "
+ "any_non_44100=%d, mask=0x%X, spdif=%d\n",
+ chip->count_pb_44100_chan,
+ chip->count_pb_non_44100_chan,
+ mask, chip->spdif_enable);
+ spin_unlock(&chip->pcm_lock);
+ return snd_interval_list(hw_param_interval(params,
+ SNDRV_PCM_HW_PARAM_RATE),
+ ARRAY_SIZE(all_spdif_playback_rates),
+ all_spdif_playback_rates, mask);
+}
+
+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);
+ 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;
+ }
+ if (snd_BUG_ON(chip->count_pb_S16_chan +
+ chip->count_pb_S32_chan > 4)) {
+ spin_unlock(&chip->pcm_lock);
+ return -EINVAL;
+ }
+ if (chip->count_pb_S16_chan)
+ snd_mask_set(&fmt, SNDRV_PCM_FORMAT_S16_LE);
+ else if (chip->count_pb_S32_chan)
+ snd_mask_set(&fmt, SNDRV_PCM_FORMAT_S32_LE);
+ else {
+ /* No format yet chosen, so both formats are available. */
+ snd_mask_set(&fmt, SNDRV_PCM_FORMAT_S16_LE);
+ snd_mask_set(&fmt, SNDRV_PCM_FORMAT_S32_LE);
+ }
+ result = snd_mask_refine(f, &fmt);
+ snd_printdd("snd_hw_rule_playback_format: any_S16=%d, any_S32=%d, "
+ "refined_fmt=0x%X, avail_fmt=0x%X, changed=%d\n",
+ chip->count_pb_S16_chan,
+ chip->count_pb_S32_chan, f->bits[0], fmt.bits[0],
+ result);
+ spin_unlock(&chip->pcm_lock);
+ return result;
+}
+
unsigned int snd_ca0106_ptr_read(struct snd_ca0106 * emu,
unsigned int reg,
unsigned int chn)
@@ -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);
+ 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);
+ if (err < 0)
return err;
+ err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_RATE,
+ hw_rule_playback_rate, (void *)chip,
+ SNDRV_PCM_HW_PARAM_RATE, -1);
+ if (err < 0)
+ return err;
+ err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_FORMAT,
+ hw_rule_playback_format, (void *)chip,
+ SNDRV_PCM_HW_PARAM_FORMAT, -1);
+ if (err < 0)
+ return err;
snd_pcm_set_sync(substream);
if (chip->details->spi_dac && channel_id != PCM_FRONT_CHANNEL) {
@@ -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;
+ }
+ 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;
+ }
+ 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_printdd(KERN_DEBUG DRVNAME ": free_playback: any_44100=%d, "
+ "any_non_44100=%d, spdif=%d.\n",
+ chip->count_pb_44100_chan, chip->count_pb_non_44100_chan,
+ emu->spdif_enable);
+ spin_unlock(&chip->pcm_lock);
+
return snd_pcm_lib_free_pages(substream);
}
@@ -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)
{
struct snd_ca0106 *emu = snd_pcm_substream_chip(substream);
- struct snd_pcm_runtime *runtime = substream->runtime;
+ struct snd_pcm_runtime *runtime = substream->runtime, *runtimei;
struct snd_ca0106_pcm *epcm = runtime->private_data;
- int channel = epcm->channel_id;
+ struct snd_ca0106_channel *pchannel;
+ int channel = epcm->channel_id, chi;
+ unsigned short count_pb_44100_chan, count_pb_non_44100_chan;
+ unsigned short count_pb_S16_chan, count_pb_S32_chan;
u32 *table_base = (u32 *)(emu->buffer.area+(8*16*channel));
u32 period_size_bytes = frames_to_bytes(runtime, runtime->period_size);
u32 hcfg_mask = HCFG_PLAYBACK_S32_LE;
u32 hcfg_set = 0x00000000;
u32 hcfg;
- u32 reg40_mask = 0x30000 << (channel<<1);
+ u32 reg40_mask = 0xFF0000;
u32 reg40_set = 0;
u32 reg40;
- /* FIXME: Depending on mixer selection of SPDIF out or not, select the spdif rate or the DAC rate. */
- u32 reg71_mask = 0x03030000 ; /* Global. Set SPDIF rate. We only support 44100 to spdif, not to DAC. */
+ u32 reg71_mask;
+ u32 reg71_shift;
u32 reg71_set = 0;
u32 reg71;
int i;
- //snd_printk("prepare:channel_number=%d, rate=%d, format=0x%x, channels=%d, buffer_size=%ld, period_size=%ld, periods=%u, frames_to_bytes=%d\n",channel, runtime->rate, runtime->format, runtime->channels, runtime->buffer_size, runtime->period_size, runtime->periods, frames_to_bytes(runtime, 1));
- //snd_printk("dma_addr=%x, dma_area=%p, table_base=%p\n",runtime->dma_addr, runtime->dma_area, table_base);
- //snd_printk("dma_addr=%x, dma_area=%p, dma_bytes(size)=%x\n",emu->buffer.addr, emu->buffer.area, emu->buffer.bytes);
+ /* FIXME CLEAN UP IF spdif_enable IS CHANGED WHILE CHANNELS ARE OPENED
+ * OR PREVENT THIS FROM HAPPENING. */
+ if (emu->spdif_enable)
+ reg71_shift = 24; /* SPDIF Output Rate */
+ else
+ reg71_shift = 16; /* I2S Output Rate */
+ reg71_mask = 0x3 << reg71_shift;
+
+ /*printk(KERN_DEBUG DRVNAME ": prepare_playback: "
+ "channel_number=%d, rate=%d, format=0x%x, channels=%d, "
+ "buffer_size=%ld,period_size=%ld, periods=%u, "
+ "frames_to_bytes=%d\n",
+ channel, runtime->rate, runtime->format, runtime->channels,
+ runtime->buffer_size, runtime->period_size, runtime->periods,
+ frames_to_bytes(runtime, 1));*/
+ /*printk("dma_addr=%x, dma_area=%p, table_base=%p\n",
+ runtime->dma_addr,runtime->dma_area, table_base);*/
+ /*printk("dma_addr=%x, dma_area=%p, dma_bytes(size)=%x\n",
+ emu->buffer.addr,emu->buffer.area, emu->buffer.bytes);*/
+ /* 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;
+ /* Rate can be set per channel. */
+ /* reg40 control host to fifo */
+ /* reg71 controls DAC rate. */
+ switch (runtimei->rate) {
+ case 44100:
+ /* We only support 44100 to spdif, not to DAC.
+ (FIXME WHY?)*/
+ if (emu->spdif_enable) {
+ /* When using 44100, *all* channels
+ must be set to that rate. */
+ reg40_set |= 0x550000;
+ reg71_set |= 0x1 << reg71_shift;
+ break;
+ } else {
+ printk(KERN_ERR DRVNAME
+ "prepare_playback: "
+ "44100Hz is invalid for DAC.\n");
+ }
+ case 48000:
+ /* reg40_set &= !(0x1 << (chi<<1)); */
+ /* reg71_set &= !(0x1 << reg71_shift); */
+ break;
+ case 96000:
+ reg40_set |= 0x20000 << (chi<<1);
+ reg71_set |= 0x2 << reg71_shift;
+ break;
+ case 192000:
+ reg40_set |= 0x30000 << (chi<<1);
+ reg71_set |= 0x3 << reg71_shift;
+ break;
+ default:
+ printk(KERN_ERR DRVNAME
+ ": prepare_playback: "
+ "Bad sampling frequency %d.\n",
+ runtimei->rate);
+ }
+ }
+ snd_BUG_ON(emu->count_pb_44100_chan && emu->count_pb_non_44100_chan);
+ snd_BUG_ON(emu->count_pb_44100_chan +
+ emu->count_pb_non_44100_chan > 4);
+ snd_BUG_ON(emu->count_pb_S16_chan && emu->count_pb_S32_chan);
+ snd_BUG_ON(emu->count_pb_S16_chan + emu->count_pb_S32_chan > 4);
+
+ count_pb_44100_chan =
+ emu->count_pb_44100_chan + runtime->rate == 44100;
+ count_pb_non_44100_chan =
+ emu->count_pb_non_44100_chan + runtime->rate != 44100;
+ count_pb_S16_chan = emu->count_pb_S16_chan +
+ runtime->format == SNDRV_PCM_FORMAT_S16_LE;
+ count_pb_S32_chan = emu->count_pb_S32_chan +
+ runtime->format == SNDRV_PCM_FORMAT_S32_LE;
+ snd_BUG_ON(count_pb_44100_chan + count_pb_non_44100_chan > 4);
+ snd_BUG_ON(count_pb_S16_chan + count_pb_S32_chan > 4);
+
/* Rate can be set per channel. */
/* reg40 control host to fifo */
/* reg71 controls DAC rate. */
switch (runtime->rate) {
case 44100:
- reg40_set = 0x10000 << (channel<<1);
- reg71_set = 0x01010000;
- break;
- case 48000:
- reg40_set = 0;
- reg71_set = 0;
+ /* We only support 44100 to spdif, not to DAC.
+ (FIXME WHY?)*/
+ if (emu->spdif_enable) {
+ /* When using 44100, *all* channels
+ must be set to that rate. */
+ reg40_set |= 0x550000;
+ reg71_set |= 0x1 << reg71_shift;
+ break;
+ } else {
+ printk(KERN_ERR DRVNAME
+ "prepare_playback: "
+ "44100Hz is invalid for DAC.\n");
+ }
+ case 48000:
+ /* reg40_set &= !(0x1 << (channel<<1)); */
+ /* reg71_set &= !(0x1 << reg71_shift); */
break;
case 96000:
- reg40_set = 0x20000 << (channel<<1);
- reg71_set = 0x02020000;
+ reg40_set |= 0x20000 << (channel<<1);
+ reg71_set |= 0x2 << reg71_shift;
break;
case 192000:
- reg40_set = 0x30000 << (channel<<1);
- reg71_set = 0x03030000;
+ reg40_set |= 0x30000 << (channel<<1);
+ reg71_set |= 0x3 << reg71_shift;
break;
default:
- reg40_set = 0;
- reg71_set = 0;
- break;
+ spin_unlock(&emu->pcm_lock);
+ printk(KERN_ERR DRVNAME
+ ": prepare_playback: Bad sampling frequency %dHz.\n",
+ runtime->rate);
+ return -EINVAL;
}
- /* Format is a global setting */
- /* FIXME: Only let the first channel accessed set this. */
+ if (count_pb_44100_chan && count_pb_non_44100_chan) {
+ spin_unlock(&emu->pcm_lock);
+ printk(KERN_ERR DRVNAME
+ "prepare_playback: requested sampling rate %dHz"
+ " conflicts with other selected sampling rates.\n",
+ runtime->rate);
+ return -EINVAL;
+ }
+ if (count_pb_S16_chan && count_pb_S32_chan) {
+ spin_unlock(&emu->pcm_lock);
+ printk(KERN_ERR DRVNAME
+ "prepare_playback: requested sample format %d"
+ " conflicts with other selected sample formats.\n",
+ runtime->format);
+ return -EINVAL;
+ }
+ emu->count_pb_44100_chan = count_pb_44100_chan;
+ emu->count_pb_non_44100_chan = count_pb_non_44100_chan;
+ emu->count_pb_S16_chan = count_pb_S16_chan;
+ emu->count_pb_S32_chan = count_pb_S32_chan;
+ epcm->hw_reserved = 1;
+
+ snd_printdd(KERN_DEBUG DRVNAME ": prepare_playback: any_44100=%d, "
+ "any_non_44100=%d, spdif=%d.\n",
+ emu->count_pb_44100_chan, emu->count_pb_non_44100_chan,
+ emu->spdif_enable);
+ /* Format is a global setting. */
+ /* Only the first channel accessed can set this
+ (enforced by constraints). */
switch (runtime->format) {
case SNDRV_PCM_FORMAT_S16_LE:
hcfg_set = 0;
@@ -726,6 +1025,7 @@ static int snd_ca0106_pcm_prepare_playback(struct snd_pcm_substream *substream)
hcfg_set = 0;
break;
}
+ spin_unlock(&emu->pcm_lock);
hcfg = inl(emu->port + HCFG) ;
hcfg = (hcfg & ~hcfg_mask) | hcfg_set;
outl(hcfg, emu->port + HCFG);
@@ -1336,6 +1636,7 @@ static int __devinit snd_ca0106_create(int dev, struct snd_card *card,
chip->irq = -1;
spin_lock_init(&chip->emu_lock);
+ spin_lock_init(&chip->pcm_lock);
chip->port = pci_resource_start(pci, 0);
if ((chip->res_port = request_region(chip->port, 0x20,
@@ -1364,8 +1665,8 @@ static int __devinit snd_ca0106_create(int dev, struct snd_card *card,
pci_read_config_dword(pci, PCI_SUBSYSTEM_VENDOR_ID, &chip->serial);
pci_read_config_word(pci, PCI_SUBSYSTEM_ID, &chip->model);
#if 1
- printk(KERN_INFO "snd-ca0106: Model %04x Rev %08x Serial %08x\n", chip->model,
- pci->revision, chip->serial);
+ printk(KERN_INFO DRVNAME ": Model %04x Rev %08x Serial %08x\n",
+ chip->model, pci->revision, chip->serial);
#endif
strcpy(card->driver, "CA0106");
strcpy(card->shortname, "CA0106");
@@ -1379,7 +1680,9 @@ static int __devinit snd_ca0106_create(int dev, struct snd_card *card,
}
chip->details = c;
if (subsystem[dev]) {
- printk(KERN_INFO "snd-ca0106: Sound card name=%s, subsystem=0x%x. Forced to subsystem=0x%x\n",
+ printk(KERN_INFO DRVNAME
+ ": Sound card name=%s, subsystem=0x%x. "
+ "Forced to subsystem=0x%x\n",
c->name, chip->serial, subsystem[dev]);
}
--
1.5.4.3
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] ca0106 playback 44100Hz support to SPDIF and playback format & rate constraints
2008-09-02 17:11 ` Ben Stanley
@ 2008-09-02 17:16 ` Ben Stanley
2008-09-02 21:18 ` [PATCH 1/2] " James Courtier-Dutton
2008-09-03 9:49 ` Takashi Iwai
2 siblings, 0 replies; 16+ messages in thread
From: Ben Stanley @ 2008-09-02 17:16 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel
Signed-off-by: Ben Stanley Ben.Stanley@exemail.com.au
This is the patch against alsa-driver.
---
pci/ca0106/ca0106_main.patch | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/pci/ca0106/ca0106_main.patch b/pci/ca0106/ca0106_main.patch
index 03820b3..d661011 100644
--- a/pci/ca0106/ca0106_main.patch
+++ b/pci/ca0106/ca0106_main.patch
@@ -14,12 +14,12 @@
static struct snd_ca0106_details ca0106_chip_details[] = {
/* Sound Blaster X-Fi Extreme Audio. This does not have an AC97. 53SB079000000 */
-@@ -1352,7 +1353,7 @@
+@@ -1542,7 +1543,7 @@
pci_read_config_word(pci, PCI_SUBSYSTEM_ID, &chip->model);
#if 1
- printk(KERN_INFO "snd-ca0106: Model %04x Rev %08x Serial %08x\n", chip->model,
-- pci->revision, chip->serial);
-+ snd_pci_revision(pci), chip->serial);
+ printk(KERN_INFO DRVNAME ": Model %04x Rev %08x Serial %08x\n",
+- chip->model, pci->revision, chip->serial);
++ chip->model, snd_pci_revision(pci), chip->serial);
#endif
strcpy(card->driver, "CA0106");
strcpy(card->shortname, "CA0106");
--
1.5.4.3
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] ca0106 playback 44100Hz support to SPDIF and playback format & rate constraints
2008-09-02 17:11 ` Ben Stanley
2008-09-02 17:16 ` [PATCH 2/2] " Ben Stanley
@ 2008-09-02 21:18 ` James Courtier-Dutton
2008-09-03 16:13 ` Trent Piepho
2008-09-03 9:49 ` Takashi Iwai
2 siblings, 1 reply; 16+ messages in thread
From: James Courtier-Dutton @ 2008-09-02 21:18 UTC (permalink / raw)
To: Ben Stanley; +Cc: Takashi Iwai, alsa-devel
Ben Stanley wrote:
> + /* We only support 44100 to spdif, not to DAC.
> + (FIXME WHY?)*/
WHY? Because the DAC cannot be clocked at 44100, only 48000 and 96000
from the CA0106 chip.
The SPDIF can be clocked at 44100.
It is a hardware limitation.
Kind Regards
James
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] ca0106 playback 44100Hz support to SPDIF and playback format & rate constraints
2008-09-02 17:11 ` Ben Stanley
2008-09-02 17:16 ` [PATCH 2/2] " Ben Stanley
2008-09-02 21:18 ` [PATCH 1/2] " James Courtier-Dutton
@ 2008-09-03 9:49 ` Takashi Iwai
2008-09-03 15:15 ` Ben Stanley
2 siblings, 1 reply; 16+ messages in thread
From: Takashi Iwai @ 2008-09-03 9:49 UTC (permalink / raw)
To: Ben Stanley; +Cc: alsa-devel
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;
> + else
> + mask = 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] ca0106 playback 44100Hz support to SPDIF and playback format & rate constraints
2008-09-03 9:49 ` Takashi Iwai
@ 2008-09-03 15:15 ` Ben Stanley
2008-09-08 14:58 ` Takashi Iwai
0 siblings, 1 reply; 16+ messages in thread
From: Ben Stanley @ 2008-09-03 15:15 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel
Takashi,
Thankyou for your comments.
My modifications would be much simpler if I didn't have to re-build the
counts of how many channels are in each mode every time the prepare and
hw_free functions are called.
In the case of the prepare function, I could modify the existing counts
if I knew what the previous state (rate, format) was before the most
recent change.
In the case of the hw_free function, I think that the rate and format
information is invalid for the current channel by the time the hw_free
function is called.
If I knew these pieces of information, I could completely avoid
re-building the counts.
Regarding the locking, initially I only tried to protect my count
fields. I have now tried to work on the locking some more (see
incremental untested patch at end). Each time I learn some more...
thanks for your patience. Perhaps there is another driver that already
has to do this level of locking that you could point me to look at. The
other drivers I have looked at so far don't have to do this much
locking. (I gather that means they are designed to avoid the need to
lock?)
Regarding atomicity, the documentation states that
snd_ca0106_pcm_trigger_capture, snd_ca0106_pcm_pointer_playback,
snd_ca0106_pcm_pointer_capture are called with a spinlock held by the
middle layer. What lock is that? Do I need to hold my spinlock in
addition? Will that possibly cause deadlock if the spinlocks are grabbed
in different orders? So many questions...
Regarding snd_BUG_ON, I have used it like an assert, intending that it
should be turned off in normal production code. Is that not the case?
Further comments interleaved below.
On Wed, 2008-09-03 at 11:49 +0200, Takashi Iwai wrote:
> 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.
I tried to address this some more in the incremental patch at the end of
this email.
>
> >
> > 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?
I could change that.
>
> > +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.
This is just asserting the invariants, and it did find some problems
during development. This condition should not be triggered by the races
in parameter constraints, as I took special care to deal with that
problem in the prepare function. I would consider removal before
promotion to an 'error' check.
>
> > + 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.
Asserting invariants, same comments as above. If the total count of
channels in the 44100 and non-44100 sets is above the number of channels
on the card, there is a problem. However, this cannot happen with the
current design of the patch, so I am happy to remove it.
>
> > + /* 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;
> > + else
> > + mask = 0xF;
> > + } else {
> > + /* 44100Hz is not supported for DAC (FIXME Why?) */
>
> Remove FIXME Why, as James suggested.
Good, I'm glad James responded to clear that up :-)
>
> > +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.
Can do.
>
> > + 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.
During development these checks were useful. Now they can be removed.
>
> > @@ -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('
Can change.
>
> > + 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.
Can change.
>
> > @@ -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.
>
Hmmm... The open callback allocates a new snd_ca0106_pcm by kmalloc, but
the close callback does not kfree it. Is that a leak? Oh, I see that it
is kfreed by snd_ca0106_pcm_free_substream.
I can't find any other part of the driver that writes to the epcm field.
Furthermore, I observe that the use field is set to 1 before epcm field
is assigned. So perhaps the lock should cover this case.
> > + 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.
Yeah, I see you don't like my defensive programming. I'll work on
understanding these better so that I can be confident of removing the
checks.
>
> > + 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.
Can remove. Was useful during development.
>
> > @@ -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.
Yes, I thought about that after my late night of hacking...
Even better would be to figure out how to avoid re-building the counts,
as discussed at the top. But, this works and seems to be robust against
my nasty test script.
It is not quite the same between hw_free and prepare, as the prepare
function takes special care to check whether what is requested fulfils
the constraints. If it satisfies the constraints, it is configured. The
check and the configuration are performed atomically.
So it looks very similar between the two, but is subtly different.
Perhaps I can code both versions in one function.
Incremental untested patch trying to address the locking is shown below.
I'm sure you'll find things you don't like :-). Please ignore the other
issues outlined above for the moment - I will work on addressing your
other comments another night. Please review the locking.
Ben Stanley.
>From c1c72cf2584c493080ddc04f9b9c36c38bb1b2bd Mon Sep 17 00:00:00 2001
From: Ben Stanley <Ben.Stanley@exemail.com.au>
Date: Thu, 4 Sep 2008 01:00:26 +1000
Subject: [PATCH] Change locking to protect more stuff in accordance with Takashi's comments.
---
pci/ca0106/ca0106_main.c | 91 +++++++++++++++++++++++++++++++--------------
pci/ca0106/ca0106_mixer.c | 3 +
2 files changed, 66 insertions(+), 28 deletions(-)
diff --git a/pci/ca0106/ca0106_main.c b/pci/ca0106/ca0106_main.c
index 806f34c..794de68 100644
--- a/pci/ca0106/ca0106_main.c
+++ b/pci/ca0106/ca0106_main.c
@@ -333,19 +333,20 @@ static int hw_rule_playback_rate(struct snd_pcm_hw_params *params,
{
struct snd_ca0106 *chip = rule->private;
int mask;
+ unsigned long flags;
if (snd_BUG_ON(!chip))
return -EINVAL;
- spin_lock(&chip->pcm_lock);
+ spin_lock_irqsave(&chip->pcm_lock, flags);
if (chip->spdif_enable) {
if (snd_BUG_ON(chip->count_pb_44100_chan &&
chip->count_pb_non_44100_chan)) {
- spin_unlock(&chip->pcm_lock);
+ spin_unlock_irqrestore(&chip->pcm_lock, flags);
return -EINVAL;
}
if (snd_BUG_ON(chip->count_pb_44100_chan +
chip->count_pb_non_44100_chan > 4)) {
- spin_unlock(&chip->pcm_lock);
+ spin_unlock_irqrestore(&chip->pcm_lock, flags);
return -EINVAL;
}
/* Compute the mask applied to all_spdif_playback_rates */
@@ -364,7 +365,7 @@ static int hw_rule_playback_rate(struct snd_pcm_hw_params *params,
chip->count_pb_44100_chan,
chip->count_pb_non_44100_chan,
mask, chip->spdif_enable);
- spin_unlock(&chip->pcm_lock);
+ spin_unlock_irqrestore(&chip->pcm_lock, flags);
return snd_interval_list(hw_param_interval(params,
SNDRV_PCM_HW_PARAM_RATE),
ARRAY_SIZE(all_spdif_playback_rates),
@@ -377,20 +378,21 @@ static int hw_rule_playback_format(struct snd_pcm_hw_params *params,
struct snd_ca0106 *chip = rule->private;
struct snd_mask fmt, *f = hw_param_mask(
params, SNDRV_PCM_HW_PARAM_FORMAT);
+ unsigned long flags;
int result;
if (snd_BUG_ON(!chip))
return -EINVAL;
snd_mask_none(&fmt);
- spin_lock(&chip->pcm_lock);
+ spin_lock_irqsave(&chip->pcm_lock, flags);
if (snd_BUG_ON(chip->count_pb_S16_chan &&
chip->count_pb_S32_chan)) {
- spin_unlock(&chip->pcm_lock);
+ spin_unlock_irqrestore(&chip->pcm_lock, flags);
return -EINVAL;
}
if (snd_BUG_ON(chip->count_pb_S16_chan +
chip->count_pb_S32_chan > 4)) {
- spin_unlock(&chip->pcm_lock);
+ spin_unlock_irqrestore(&chip->pcm_lock, flags);
return -EINVAL;
}
if (chip->count_pb_S16_chan)
@@ -408,7 +410,7 @@ static int hw_rule_playback_format(struct snd_pcm_hw_params *params,
chip->count_pb_S16_chan,
chip->count_pb_S32_chan, f->bits[0], fmt.bits[0],
result);
- spin_unlock(&chip->pcm_lock);
+ spin_unlock_irqrestore(&chip->pcm_lock, flags);
return result;
}
@@ -581,6 +583,7 @@ static int snd_ca0106_pcm_open_playback_channel(struct snd_pcm_substream *substr
struct snd_ca0106_pcm *epcm;
struct snd_pcm_runtime *runtime = substream->runtime;
int err;
+ unsigned long flags;
epcm = kzalloc(sizeof(*epcm), GFP_KERNEL);
@@ -589,7 +592,8 @@ static int snd_ca0106_pcm_open_playback_channel(struct snd_pcm_substream *substr
epcm->emu = chip;
epcm->substream = substream;
epcm->channel_id=channel_id;
-
+
+ spin_lock_irqsave(&chip->pcm_lock, flags);
runtime->private_data = epcm;
runtime->private_free = snd_ca0106_pcm_free_substream;
@@ -605,22 +609,23 @@ static int snd_ca0106_pcm_open_playback_channel(struct snd_pcm_substream *substr
err = snd_pcm_hw_constraint_integer(
runtime, SNDRV_PCM_HW_PARAM_PERIODS);
if (err < 0)
- return err;
+ goto error;
err = snd_pcm_hw_constraint_step(
runtime, 0, SNDRV_PCM_HW_PARAM_PERIOD_BYTES, 64);
if (err < 0)
- return err;
+ goto error;
err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_RATE,
hw_rule_playback_rate, (void *)chip,
SNDRV_PCM_HW_PARAM_RATE, -1);
if (err < 0)
- return err;
+ goto error;
err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_FORMAT,
hw_rule_playback_format, (void *)chip,
SNDRV_PCM_HW_PARAM_FORMAT, -1);
if (err < 0)
- return err;
+ goto error;
snd_pcm_set_sync(substream);
+ spin_unlock_irqrestore(&chip->pcm_lock, flags);
if (chip->details->spi_dac && channel_id != PCM_FRONT_CHANNEL) {
const int reg = spi_dacd_reg[channel_id];
@@ -632,6 +637,9 @@ static int snd_ca0106_pcm_open_playback_channel(struct snd_pcm_substream *substr
return err;
}
return 0;
+error:
+ spin_unlock_irqrestore(&chip->pcm_lock, flags);
+ return err;
}
/* close callback */
@@ -639,7 +647,11 @@ static int snd_ca0106_pcm_close_playback(struct snd_pcm_substream *substream)
{
struct snd_ca0106 *chip = snd_pcm_substream_chip(substream);
struct snd_pcm_runtime *runtime = substream->runtime;
- struct snd_ca0106_pcm *epcm = runtime->private_data;
+ struct snd_ca0106_pcm *epcm;
+ unsigned long flags;
+
+ spin_lock_irqsave(&chip->pcm_lock, flags);
+ epcm = runtime->private_data;
chip->playback_channels[epcm->channel_id].use = 0;
if (chip->details->spi_dac && epcm->channel_id != PCM_FRONT_CHANNEL) {
@@ -650,6 +662,7 @@ static int snd_ca0106_pcm_close_playback(struct snd_pcm_substream *substream)
snd_ca0106_spi_write(chip, chip->spi_dac_reg[reg]);
}
/* FIXME: maybe zero others */
+ spin_unlock_irqrestore(&chip->pcm_lock, flags);
return 0;
}
@@ -681,6 +694,7 @@ static int snd_ca0106_pcm_open_capture_channel(struct snd_pcm_substream *substre
struct snd_ca0106_channel *channel = &(chip->capture_channels[channel_id]);
struct snd_ca0106_pcm *epcm;
struct snd_pcm_runtime *runtime = substream->runtime;
+ unsigned long flags;
int err;
epcm = kzalloc(sizeof(*epcm), GFP_KERNEL);
@@ -692,6 +706,7 @@ static int snd_ca0106_pcm_open_capture_channel(struct snd_pcm_substream *substre
epcm->substream = substream;
epcm->channel_id=channel_id;
+ spin_lock_irqsave(&chip->pcm_lock, flags);
runtime->private_data = epcm;
runtime->private_free = snd_ca0106_pcm_free_substream;
@@ -704,6 +719,7 @@ static int snd_ca0106_pcm_open_capture_channel(struct snd_pcm_substream *substre
//printk("open:channel_id=%d, chip=%p, channel=%p\n",channel_id, chip, channel);
//channel->interrupt = snd_ca0106_pcm_channel_interrupt;
channel->epcm = epcm;
+ spin_unlock_irqsave(&chip->pcm_lock, flags);
if ((err = snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS)) < 0)
return err;
//snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_PERIOD_SIZE, &hw_constraints_capture_period_sizes);
@@ -717,9 +733,14 @@ static int snd_ca0106_pcm_close_capture(struct snd_pcm_substream *substream)
{
struct snd_ca0106 *chip = snd_pcm_substream_chip(substream);
struct snd_pcm_runtime *runtime = substream->runtime;
- struct snd_ca0106_pcm *epcm = runtime->private_data;
+ struct snd_ca0106_pcm *epcm;
+ unsigned long flags;
+ spin_lock_irqsave(&chip->pcm_lock, flags);
+ epcm = runtime->private_data;
+
chip->capture_channels[epcm->channel_id].use = 0;
/* FIXME: maybe zero others */
+ spin_unlock_irqrestore(&chip->pcm_lock, flags);
return 0;
}
@@ -759,8 +780,9 @@ static int snd_ca0106_pcm_hw_free_playback(struct snd_pcm_substream *substream)
struct snd_ca0106_pcm *epcm = runtime->private_data;
struct snd_ca0106_channel *pchannel;
int channel = epcm->channel_id, chi;
+ unsigned long flags;
- spin_lock(&chip->pcm_lock);
+ spin_lock_irqsave(&chip->pcm_lock, flags);
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;
@@ -771,17 +793,17 @@ static int snd_ca0106_pcm_hw_free_playback(struct snd_pcm_substream *substream)
if (!pchannel->use)
continue;
if (snd_BUG_ON(!pchannel->epcm)) {
- spin_unlock(&chip->pcm_lock);
+ spin_unlock_irqrestore(&chip->pcm_lock, flags);
return -EINVAL;
}
if (!pchannel->epcm->hw_reserved)
continue;
if (snd_BUG_ON(!pchannel->epcm->substream)) {
- spin_unlock(&chip->pcm_lock);
+ spin_unlock_irqrestore(&chip->pcm_lock, flags);
return -EINVAL;
}
if (snd_BUG_ON(!pchannel->epcm->substream->runtime)) {
- spin_unlock(&chip->pcm_lock);
+ spin_unlock_irqrestore(&chip->pcm_lock, flags);
return -EINVAL;
}
runtimei = pchannel->epcm->substream->runtime;
@@ -801,7 +823,7 @@ static int snd_ca0106_pcm_hw_free_playback(struct snd_pcm_substream *substream)
"any_non_44100=%d, spdif=%d.\n",
chip->count_pb_44100_chan, chip->count_pb_non_44100_chan,
emu->spdif_enable);
- spin_unlock(&chip->pcm_lock);
+ spin_unlock_irqrestore(&chip->pcm_lock, flags);
return snd_pcm_lib_free_pages(substream);
}
@@ -843,7 +865,9 @@ static int snd_ca0106_pcm_prepare_playback(struct snd_pcm_substream *substream)
u32 reg71_set = 0;
u32 reg71;
int i;
+ unsigned long flags;
+ spin_lock_irqsave(&emu->pcm_lock, flags);
/* FIXME CLEAN UP IF spdif_enable IS CHANGED WHILE CHANNELS ARE OPENED
* OR PREVENT THIS FROM HAPPENING. */
if (emu->spdif_enable)
@@ -864,7 +888,6 @@ static int snd_ca0106_pcm_prepare_playback(struct snd_pcm_substream *substream)
/*printk("dma_addr=%x, dma_area=%p, dma_bytes(size)=%x\n",
emu->buffer.addr,emu->buffer.area, emu->buffer.bytes);*/
/* 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) {
@@ -874,17 +897,17 @@ static int snd_ca0106_pcm_prepare_playback(struct snd_pcm_substream *substream)
if (!pchannel->use)
continue;
if (snd_BUG_ON(!pchannel->epcm)) {
- spin_unlock(&emu->pcm_lock);
+ spin_unlock_irqrestore(&emu->pcm_lock, flags);
return -EINVAL;
}
if (!pchannel->epcm->hw_reserved)
continue;
if (snd_BUG_ON(!pchannel->epcm->substream)) {
- spin_unlock(&emu->pcm_lock);
+ spin_unlock_irqrestore(&emu->pcm_lock, flags);
return -EINVAL;
}
if (snd_BUG_ON(!pchannel->epcm->substream->runtime)) {
- spin_unlock(&emu->pcm_lock);
+ spin_unlock_irqrestore(&emu->pcm_lock, flags);
return -EINVAL;
}
runtimei = pchannel->epcm->substream->runtime;
@@ -979,14 +1002,14 @@ static int snd_ca0106_pcm_prepare_playback(struct snd_pcm_substream *substream)
reg71_set |= 0x3 << reg71_shift;
break;
default:
- spin_unlock(&emu->pcm_lock);
+ spin_unlock_irqrestore(&emu->pcm_lock, flags);
printk(KERN_ERR DRVNAME
": prepare_playback: Bad sampling frequency %dHz.\n",
runtime->rate);
return -EINVAL;
}
if (count_pb_44100_chan && count_pb_non_44100_chan) {
- spin_unlock(&emu->pcm_lock);
+ spin_unlock_irqrestore(&emu->pcm_lock, flags);
printk(KERN_ERR DRVNAME
"prepare_playback: requested sampling rate %dHz"
" conflicts with other selected sampling rates.\n",
@@ -994,7 +1017,7 @@ static int snd_ca0106_pcm_prepare_playback(struct snd_pcm_substream *substream)
return -EINVAL;
}
if (count_pb_S16_chan && count_pb_S32_chan) {
- spin_unlock(&emu->pcm_lock);
+ spin_unlock_irqrestore(&emu->pcm_lock, flags);
printk(KERN_ERR DRVNAME
"prepare_playback: requested sample format %d"
" conflicts with other selected sample formats.\n",
@@ -1025,7 +1048,7 @@ static int snd_ca0106_pcm_prepare_playback(struct snd_pcm_substream *substream)
hcfg_set = 0;
break;
}
- spin_unlock(&emu->pcm_lock);
+ spin_unlock_irqrestore(&emu->pcm_lock, flags);
hcfg = inl(emu->port + HCFG) ;
hcfg = (hcfg & ~hcfg_mask) | hcfg_set;
outl(hcfg, emu->port + HCFG);
@@ -1079,10 +1102,12 @@ static int snd_ca0106_pcm_prepare_capture(struct snd_pcm_substream *substream)
u32 reg71_mask = 0x0000c000 ; /* Global. Set ADC rate. */
u32 reg71_set = 0;
u32 reg71;
+ unsigned long flags;
//snd_printk("prepare:channel_number=%d, rate=%d, format=0x%x, channels=%d, buffer_size=%ld, period_size=%ld, periods=%u, frames_to_bytes=%d\n",channel, runtime->rate, runtime->format, runtime->channels, runtime->buffer_size, runtime->period_size, runtime->periods, frames_to_bytes(runtime, 1));
//snd_printk("dma_addr=%x, dma_area=%p, table_base=%p\n",runtime->dma_addr, runtime->dma_area, table_base);
//snd_printk("dma_addr=%x, dma_area=%p, dma_bytes(size)=%x\n",emu->buffer.addr, emu->buffer.area, emu->buffer.bytes);
+ spin_lock_irqsave(&emu->pcm_lock, flags);
/* reg71 controls ADC rate. */
switch (runtime->rate) {
case 44100:
@@ -1116,6 +1141,7 @@ static int snd_ca0106_pcm_prepare_capture(struct snd_pcm_substream *substream)
hcfg_set = 0;
break;
}
+ spin_unlock_irqrestore(&emu->pcm_lock, flags);
hcfg = inl(emu->port + HCFG) ;
hcfg = (hcfg & ~hcfg_mask) | hcfg_set;
outl(hcfg, emu->port + HCFG);
@@ -1149,6 +1175,7 @@ static int snd_ca0106_pcm_trigger_playback(struct snd_pcm_substream *substream,
u32 basic = 0;
u32 extended = 0;
int running=0;
+ unsigned long flags;
switch (cmd) {
case SNDRV_PCM_TRIGGER_START:
@@ -1159,6 +1186,7 @@ static int snd_ca0106_pcm_trigger_playback(struct snd_pcm_substream *substream,
running=0;
break;
}
+ spin_lock_irqsave(&emu->pcm_lock, flags);
snd_pcm_group_for_each_entry(s, substream) {
if (snd_pcm_substream_chip(s) != emu ||
s->stream != SNDRV_PCM_STREAM_PLAYBACK)
@@ -1187,6 +1215,7 @@ static int snd_ca0106_pcm_trigger_playback(struct snd_pcm_substream *substream,
result = -EINVAL;
break;
}
+ spin_unlock_irqrestore(&emu->pcm_lock, flags);
return result;
}
@@ -1199,7 +1228,9 @@ static int snd_ca0106_pcm_trigger_capture(struct snd_pcm_substream *substream,
struct snd_ca0106_pcm *epcm = runtime->private_data;
int channel = epcm->channel_id;
int result = 0;
+ unsigned long flags;
+ spin_lock_irqsave(&emu->pcm_lock, flags);
switch (cmd) {
case SNDRV_PCM_TRIGGER_START:
snd_ca0106_ptr_write(emu, EXTENDED_INT_MASK, 0, snd_ca0106_ptr_read(emu, EXTENDED_INT_MASK, 0) | (0x110000<<channel));
@@ -1215,6 +1246,7 @@ static int snd_ca0106_pcm_trigger_capture(struct snd_pcm_substream *substream,
result = -EINVAL;
break;
}
+ spin_unlock_irqrestore(&emu->pcm_lock, flags);
return result;
}
@@ -1451,6 +1483,7 @@ static irqreturn_t snd_ca0106_interrupt(int irq, void *dev_id)
int mask;
unsigned int stat76;
struct snd_ca0106_channel *pchannel;
+ unsigned long flags;
status = inl(chip->port + IPR);
if (! status)
@@ -1460,6 +1493,7 @@ static irqreturn_t snd_ca0106_interrupt(int irq, void *dev_id)
//snd_printk("interrupt status = 0x%08x, stat76=0x%08x\n", status, stat76);
//snd_printk("ptr=0x%08x\n",snd_ca0106_ptr_read(chip, PLAYBACK_POINTER, 0));
mask = 0x11; /* 0x1 for one half, 0x10 for the other half period. */
+ spin_lock_irqsave(&chip->pcm_lock, flags);
for(i = 0; i < 4; i++) {
pchannel = &(chip->playback_channels[i]);
if (stat76 & mask) {
@@ -1487,6 +1521,7 @@ static irqreturn_t snd_ca0106_interrupt(int irq, void *dev_id)
//printk(KERN_INFO "interrupt stat76[%d] = %08x, use=%d, channel=%d\n", i, stat76, pchannel->use, pchannel->number);
mask <<= 1;
}
+ spin_unlock_irqrestore(&chip->pcm_lock, flags);
snd_ca0106_ptr_write(chip, EXTENDED_INT, 0, stat76);
diff --git a/pci/ca0106/ca0106_mixer.c b/pci/ca0106/ca0106_mixer.c
index 3025ed1..6752a05 100644
--- a/pci/ca0106/ca0106_mixer.c
+++ b/pci/ca0106/ca0106_mixer.c
@@ -96,7 +96,9 @@ static int snd_ca0106_shared_spdif_put(struct snd_kcontrol *kcontrol,
unsigned int val;
int change = 0;
u32 mask;
+ unsigned long flags;
+ spin_lock_irqsave(&emu->pcm_lock, flags);
val = !!ucontrol->value.integer.value[0];
change = (emu->spdif_enable != val);
if (change) {
@@ -120,6 +122,7 @@ static int snd_ca0106_shared_spdif_put(struct snd_kcontrol *kcontrol,
outl(mask, emu->port + GPIO);
}
}
+ spin_unlock_irqrestore(&emu->pcm_lock, flags );
return change;
}
--
1.5.4.3
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] ca0106 playback 44100Hz support to SPDIF and playback format & rate constraints
2008-09-02 21:18 ` [PATCH 1/2] " James Courtier-Dutton
@ 2008-09-03 16:13 ` Trent Piepho
2008-09-03 18:36 ` James Courtier-Dutton
0 siblings, 1 reply; 16+ messages in thread
From: Trent Piepho @ 2008-09-03 16:13 UTC (permalink / raw)
To: James Courtier-Dutton; +Cc: Takashi Iwai, alsa-devel, Ben Stanley
On Tue, 2 Sep 2008, James Courtier-Dutton wrote:
> Ben Stanley wrote:
> > + /* We only support 44100 to spdif, not to DAC.
> > + (FIXME WHY?)*/
>
> WHY? Because the DAC cannot be clocked at 44100, only 48000 and 96000
> from the CA0106 chip.
> The SPDIF can be clocked at 44100.
> It is a hardware limitation.
The DAC supports 44100. Is the problem that the ca0106 can't provide a
44100 clock to the DAC?
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] ca0106 playback 44100Hz support to SPDIF and playback format & rate constraints
2008-09-03 16:13 ` Trent Piepho
@ 2008-09-03 18:36 ` James Courtier-Dutton
2008-09-04 6:34 ` Takashi Iwai
0 siblings, 1 reply; 16+ messages in thread
From: James Courtier-Dutton @ 2008-09-03 18:36 UTC (permalink / raw)
To: Trent Piepho; +Cc: Takashi Iwai, alsa-devel, Ben Stanley
Trent Piepho wrote:
> On Tue, 2 Sep 2008, James Courtier-Dutton wrote:
>> Ben Stanley wrote:
>>> + /* We only support 44100 to spdif, not to DAC.
>>> + (FIXME WHY?)*/
>> WHY? Because the DAC cannot be clocked at 44100, only 48000 and 96000
>> from the CA0106 chip.
>> The SPDIF can be clocked at 44100.
>> It is a hardware limitation.
>
> The DAC supports 44100. Is the problem that the ca0106 can't provide a
> 44100 clock to the DAC?
It does not really matter why, it does not work at 44100 and that is fact.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] ca0106 playback 44100Hz support to SPDIF and playback format & rate constraints
2008-09-03 18:36 ` James Courtier-Dutton
@ 2008-09-04 6:34 ` Takashi Iwai
2008-09-04 7:16 ` Vedran Miletić
0 siblings, 1 reply; 16+ messages in thread
From: Takashi Iwai @ 2008-09-04 6:34 UTC (permalink / raw)
To: James Courtier-Dutton; +Cc: alsa-devel, Trent Piepho, Ben Stanley
At Wed, 03 Sep 2008 19:36:51 +0100,
James Courtier-Dutton wrote:
>
> Trent Piepho wrote:
> > On Tue, 2 Sep 2008, James Courtier-Dutton wrote:
> >> Ben Stanley wrote:
> >>> + /* We only support 44100 to spdif, not to DAC.
> >>> + (FIXME WHY?)*/
> >> WHY? Because the DAC cannot be clocked at 44100, only 48000 and 96000
> >> from the CA0106 chip.
> >> The SPDIF can be clocked at 44100.
> >> It is a hardware limitation.
> >
> > The DAC supports 44100. Is the problem that the ca0106 can't provide a
> > 44100 clock to the DAC?
> It does not really matter why, it does not work at 44100 and that is fact.
It does matter. At least, the situation should be commented
correctly. Otherwise you'll have the same question and give the same
answer again and again.
Takashi
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] ca0106 playback 44100Hz support to SPDIF and playback format & rate constraints
2008-09-04 6:34 ` Takashi Iwai
@ 2008-09-04 7:16 ` Vedran Miletić
0 siblings, 0 replies; 16+ messages in thread
From: Vedran Miletić @ 2008-09-04 7:16 UTC (permalink / raw)
To: alsa-devel
Is it maybe that CA0106, as 10k1 and 10k2 can't work at 44100?
2008/9/4 Takashi Iwai <tiwai@suse.de>:
> At Wed, 03 Sep 2008 19:36:51 +0100,
> James Courtier-Dutton wrote:
>>
>> Trent Piepho wrote:
>> > On Tue, 2 Sep 2008, James Courtier-Dutton wrote:
>> >> Ben Stanley wrote:
>> >>> + /* We only support 44100 to spdif, not to DAC.
>> >>> + (FIXME WHY?)*/
>> >> WHY? Because the DAC cannot be clocked at 44100, only 48000 and 96000
>> >> from the CA0106 chip.
>> >> The SPDIF can be clocked at 44100.
>> >> It is a hardware limitation.
>> >
>> > The DAC supports 44100. Is the problem that the ca0106 can't provide a
>> > 44100 clock to the DAC?
>> It does not really matter why, it does not work at 44100 and that is fact.
>
> It does matter. At least, the situation should be commented
> correctly. Otherwise you'll have the same question and give the same
> answer again and again.
>
>
> Takashi
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
--
Vedran Miletić
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] ca0106 playback 44100Hz support to SPDIF and playback format & rate constraints
2008-09-03 15:15 ` Ben Stanley
@ 2008-09-08 14:58 ` Takashi Iwai
2008-09-09 21:13 ` Ben Stanley
0 siblings, 1 reply; 16+ messages in thread
From: Takashi Iwai @ 2008-09-08 14:58 UTC (permalink / raw)
To: Ben Stanley; +Cc: alsa-devel
At Thu, 04 Sep 2008 01:15:47 +1000,
Ben Stanley wrote:
>
> Takashi,
>
> Thankyou for your comments.
>
> My modifications would be much simpler if I didn't have to re-build the
> counts of how many channels are in each mode every time the prepare and
> hw_free functions are called.
>
> In the case of the prepare function, I could modify the existing counts
> if I knew what the previous state (rate, format) was before the most
> recent change.
>
> In the case of the hw_free function, I think that the rate and format
> information is invalid for the current channel by the time the hw_free
> function is called.
>
> If I knew these pieces of information, I could completely avoid
> re-building the counts.
>
> Regarding the locking, initially I only tried to protect my count
> fields. I have now tried to work on the locking some more (see
> incremental untested patch at end). Each time I learn some more...
> thanks for your patience. Perhaps there is another driver that already
> has to do this level of locking that you could point me to look at. The
> other drivers I have looked at so far don't have to do this much
> locking. (I gather that means they are designed to avoid the need to
> lock?)
There must be some, but each driver has different things :)
> Regarding atomicity, the documentation states that
> snd_ca0106_pcm_trigger_capture, snd_ca0106_pcm_pointer_playback,
> snd_ca0106_pcm_pointer_capture are called with a spinlock held by the
> middle layer. What lock is that?
It's PCM substream lock.
> Do I need to hold my spinlock in
> addition? Will that possibly cause deadlock if the spinlocks are grabbed
> in different orders? So many questions...
Yes, you need another type of lock because PCM substream lock is
specific to each substream, and you need a global lock over all
substreams.
> Regarding snd_BUG_ON, I have used it like an assert, intending that it
> should be turned off in normal production code. Is that not the case?
The problem is that some checks should be done even for the production
system, not only for debugging. At least, exclusivity of
44.1kHz/48kHZ rates should be checked always, because the hardware
constraint can be racy.
> > > + 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.
>
> This is just asserting the invariants, and it did find some problems
> during development. This condition should not be triggered by the races
> in parameter constraints,
... but this could happen because of races, I'm afraid.
> > > @@ -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.
> >
>
> Hmmm... The open callback allocates a new snd_ca0106_pcm by kmalloc, but
> the close callback does not kfree it. Is that a leak? Oh, I see that it
> is kfreed by snd_ca0106_pcm_free_substream.
>
> I can't find any other part of the driver that writes to the epcm field.
> Furthermore, I observe that the use field is set to 1 before epcm field
> is assigned. So perhaps the lock should cover this case.
That's a part of the problem. It can run between use = 1 and epcm set.
> It is not quite the same between hw_free and prepare, as the prepare
> function takes special care to check whether what is requested fulfils
> the constraints. If it satisfies the constraints, it is configured. The
> check and the configuration are performed atomically.
>
> So it looks very similar between the two, but is subtly different.
> Perhaps I can code both versions in one function.
>
> Incremental untested patch trying to address the locking is shown below.
> I'm sure you'll find things you don't like :-). Please ignore the other
> issues outlined above for the moment - I will work on addressing your
> other comments another night. Please review the locking.
I'll check the patch later (now busy during attending conference...)
Thanks!
Takashi
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] ca0106 playback 44100Hz support to SPDIF and playback format & rate constraints
2008-09-08 14:58 ` Takashi Iwai
@ 2008-09-09 21:13 ` Ben Stanley
2008-09-10 12:11 ` Takashi Iwai
0 siblings, 1 reply; 16+ messages in thread
From: Ben Stanley @ 2008-09-09 21:13 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel
Takashi,
Just a quick note:
The current version of my patch causes kernel lockups.
I have not yet figured out how to debug this.
I will do some research, possibly compile a debug kernel with the magic
sysreq key enabled or something. I need to figure out where it is
locking up. Any hints appreciated.
For now I have reverted to an earlier version of the patch before I
included the spinlocks, and I now get no lockups. The machine is in
daily use running MythTV. However, only one of the four outputs is in
regular use. (My card only has one connection on the back, anyway.)
But perhaps you could tell by examining my locking code whether I have
broken some cardinal rule.
I already found a bug in an error path in prepare where I forgot to
unlock.
Thanks,
Ben Stanley.
On Mon, 2008-09-08 at 16:58 +0200, Takashi Iwai wrote:
> At Thu, 04 Sep 2008 01:15:47 +1000,
> Ben Stanley wrote:
> >
> > Takashi,
> >
> > Thankyou for your comments.
> >
> > My modifications would be much simpler if I didn't have to re-build the
> > counts of how many channels are in each mode every time the prepare and
> > hw_free functions are called.
> >
> > In the case of the prepare function, I could modify the existing counts
> > if I knew what the previous state (rate, format) was before the most
> > recent change.
> >
> > In the case of the hw_free function, I think that the rate and format
> > information is invalid for the current channel by the time the hw_free
> > function is called.
> >
> > If I knew these pieces of information, I could completely avoid
> > re-building the counts.
> >
> > Regarding the locking, initially I only tried to protect my count
> > fields. I have now tried to work on the locking some more (see
> > incremental untested patch at end). Each time I learn some more...
> > thanks for your patience. Perhaps there is another driver that already
> > has to do this level of locking that you could point me to look at. The
> > other drivers I have looked at so far don't have to do this much
> > locking. (I gather that means they are designed to avoid the need to
> > lock?)
>
> There must be some, but each driver has different things :)
>
> > Regarding atomicity, the documentation states that
> > snd_ca0106_pcm_trigger_capture, snd_ca0106_pcm_pointer_playback,
> > snd_ca0106_pcm_pointer_capture are called with a spinlock held by the
> > middle layer. What lock is that?
>
> It's PCM substream lock.
>
> > Do I need to hold my spinlock in
> > addition? Will that possibly cause deadlock if the spinlocks are grabbed
> > in different orders? So many questions...
>
> Yes, you need another type of lock because PCM substream lock is
> specific to each substream, and you need a global lock over all
> substreams.
>
> > Regarding snd_BUG_ON, I have used it like an assert, intending that it
> > should be turned off in normal production code. Is that not the case?
>
> The problem is that some checks should be done even for the production
> system, not only for debugging. At least, exclusivity of
> 44.1kHz/48kHZ rates should be checked always, because the hardware
> constraint can be racy.
>
> > > > + 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.
> >
> > This is just asserting the invariants, and it did find some problems
> > during development. This condition should not be triggered by the races
> > in parameter constraints,
>
> ... but this could happen because of races, I'm afraid.
>
> > > > @@ -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.
> > >
> >
> > Hmmm... The open callback allocates a new snd_ca0106_pcm by kmalloc, but
> > the close callback does not kfree it. Is that a leak? Oh, I see that it
> > is kfreed by snd_ca0106_pcm_free_substream.
> >
> > I can't find any other part of the driver that writes to the epcm field.
> > Furthermore, I observe that the use field is set to 1 before epcm field
> > is assigned. So perhaps the lock should cover this case.
>
> That's a part of the problem. It can run between use = 1 and epcm set.
>
> > It is not quite the same between hw_free and prepare, as the prepare
> > function takes special care to check whether what is requested fulfils
> > the constraints. If it satisfies the constraints, it is configured. The
> > check and the configuration are performed atomically.
> >
> > So it looks very similar between the two, but is subtly different.
> > Perhaps I can code both versions in one function.
> >
> > Incremental untested patch trying to address the locking is shown below.
> > I'm sure you'll find things you don't like :-). Please ignore the other
> > issues outlined above for the moment - I will work on addressing your
> > other comments another night. Please review the locking.
>
> I'll check the patch later (now busy during attending conference...)
>
>
> Thanks!
>
> Takashi
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] ca0106 playback 44100Hz support to SPDIF and playback format & rate constraints
2008-09-09 21:13 ` Ben Stanley
@ 2008-09-10 12:11 ` Takashi Iwai
0 siblings, 0 replies; 16+ messages in thread
From: Takashi Iwai @ 2008-09-10 12:11 UTC (permalink / raw)
To: Ben Stanley; +Cc: alsa-devel
At Wed, 10 Sep 2008 07:13:17 +1000,
Ben Stanley wrote:
>
> Takashi,
>
> Just a quick note:
>
> The current version of my patch causes kernel lockups.
>
> I have not yet figured out how to debug this.
>
> I will do some research, possibly compile a debug kernel with the magic
> sysreq key enabled or something. I need to figure out where it is
> locking up. Any hints appreciated.
>
> For now I have reverted to an earlier version of the patch before I
> included the spinlocks, and I now get no lockups. The machine is in
> daily use running MythTV. However, only one of the four outputs is in
> regular use. (My card only has one connection on the back, anyway.)
>
> But perhaps you could tell by examining my locking code whether I have
> broken some cardinal rule.
>
> I already found a bug in an error path in prepare where I forgot to
> unlock.
Thanks for information. And, don't worry, I didn't applied your
previous one yet :)
(I'm in a conference and cannot work intensively for ALSA right now.)
But will check if you send a newer patch.
thanks,
Takashi
> Thanks,
> Ben Stanley.
>
> On Mon, 2008-09-08 at 16:58 +0200, Takashi Iwai wrote:
> > At Thu, 04 Sep 2008 01:15:47 +1000,
> > Ben Stanley wrote:
> > >
> > > Takashi,
> > >
> > > Thankyou for your comments.
> > >
> > > My modifications would be much simpler if I didn't have to re-build the
> > > counts of how many channels are in each mode every time the prepare and
> > > hw_free functions are called.
> > >
> > > In the case of the prepare function, I could modify the existing counts
> > > if I knew what the previous state (rate, format) was before the most
> > > recent change.
> > >
> > > In the case of the hw_free function, I think that the rate and format
> > > information is invalid for the current channel by the time the hw_free
> > > function is called.
> > >
> > > If I knew these pieces of information, I could completely avoid
> > > re-building the counts.
> > >
> > > Regarding the locking, initially I only tried to protect my count
> > > fields. I have now tried to work on the locking some more (see
> > > incremental untested patch at end). Each time I learn some more...
> > > thanks for your patience. Perhaps there is another driver that already
> > > has to do this level of locking that you could point me to look at. The
> > > other drivers I have looked at so far don't have to do this much
> > > locking. (I gather that means they are designed to avoid the need to
> > > lock?)
> >
> > There must be some, but each driver has different things :)
> >
> > > Regarding atomicity, the documentation states that
> > > snd_ca0106_pcm_trigger_capture, snd_ca0106_pcm_pointer_playback,
> > > snd_ca0106_pcm_pointer_capture are called with a spinlock held by the
> > > middle layer. What lock is that?
> >
> > It's PCM substream lock.
> >
> > > Do I need to hold my spinlock in
> > > addition? Will that possibly cause deadlock if the spinlocks are grabbed
> > > in different orders? So many questions...
> >
> > Yes, you need another type of lock because PCM substream lock is
> > specific to each substream, and you need a global lock over all
> > substreams.
> >
> > > Regarding snd_BUG_ON, I have used it like an assert, intending that it
> > > should be turned off in normal production code. Is that not the case?
> >
> > The problem is that some checks should be done even for the production
> > system, not only for debugging. At least, exclusivity of
> > 44.1kHz/48kHZ rates should be checked always, because the hardware
> > constraint can be racy.
> >
> > > > > + 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.
> > >
> > > This is just asserting the invariants, and it did find some problems
> > > during development. This condition should not be triggered by the races
> > > in parameter constraints,
> >
> > ... but this could happen because of races, I'm afraid.
> >
> > > > > @@ -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.
> > > >
> > >
> > > Hmmm... The open callback allocates a new snd_ca0106_pcm by kmalloc, but
> > > the close callback does not kfree it. Is that a leak? Oh, I see that it
> > > is kfreed by snd_ca0106_pcm_free_substream.
> > >
> > > I can't find any other part of the driver that writes to the epcm field.
> > > Furthermore, I observe that the use field is set to 1 before epcm field
> > > is assigned. So perhaps the lock should cover this case.
> >
> > That's a part of the problem. It can run between use = 1 and epcm set.
> >
> > > It is not quite the same between hw_free and prepare, as the prepare
> > > function takes special care to check whether what is requested fulfils
> > > the constraints. If it satisfies the constraints, it is configured. The
> > > check and the configuration are performed atomically.
> > >
> > > So it looks very similar between the two, but is subtly different.
> > > Perhaps I can code both versions in one function.
> > >
> > > Incremental untested patch trying to address the locking is shown below.
> > > I'm sure you'll find things you don't like :-). Please ignore the other
> > > issues outlined above for the moment - I will work on addressing your
> > > other comments another night. Please review the locking.
> >
> > I'll check the patch later (now busy during attending conference...)
> >
> >
> > Thanks!
> >
> > Takashi
>
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2008-09-10 12:11 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-08-22 14:38 [PATCH 1/2] ca0106 playback 44100Hz support to SPDIF and playback format & rate constraints Ben Stanley
2008-08-22 15:40 ` Takashi Iwai
2008-08-23 14:37 ` Ben Stanley
2008-08-25 16:35 ` Takashi Iwai
2008-09-02 17:11 ` Ben Stanley
2008-09-02 17:16 ` [PATCH 2/2] " Ben Stanley
2008-09-02 21:18 ` [PATCH 1/2] " James Courtier-Dutton
2008-09-03 16:13 ` Trent Piepho
2008-09-03 18:36 ` James Courtier-Dutton
2008-09-04 6:34 ` Takashi Iwai
2008-09-04 7:16 ` Vedran Miletić
2008-09-03 9:49 ` Takashi Iwai
2008-09-03 15:15 ` Ben Stanley
2008-09-08 14:58 ` Takashi Iwai
2008-09-09 21:13 ` Ben Stanley
2008-09-10 12:11 ` Takashi Iwai
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.