ca0106 playback 44100Hz support to SPDIF and playback format & rate constraints

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 [email protected]

pci/ca0106/ca0106_main.c +8 -0

+		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)

pci/ca0106/ca0106_main.c +22 -2

+	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);
 }
 

pci/ca0106/ca0106_main.c +10 -30

+	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

pci/ca0106/ca0106_main.c +45 -0

+		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

pci/ca0106/ca0106_main.c +5 -2

+	/* 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");

(quoted diff) +1 -0

+		for (chi = 0; chi < 4; ++chi) {

Review

Quoted 1 line

(quoted diff) +1 -0

+			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.

(quoted diff) +1 -0

+	for (chi = 0; chi < 4; ++chi) {

Review

Quoted 1 line

(quoted diff) +1 -0

+		chp = &(chip->playback_channels[chi]);

This also needs a mutex as well.

(quoted diff) +1 -0

+	for (chi = 0; chi < 4; ++chi) {

Review

Quoted 1 line

(quoted diff) +1 -0

+		chp = &(emu->playback_channels[chi]);

Use a mutex in this function, too.

At Sat, 23 Aug 2008 00:38:41 +1000,

Quoted 31 lines · Ben Stanley

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.

Quoted 7 lines

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 [email protected]

Thanks for the patch!

Through a quick review, I find it almost fine, but slight concerns below.

Review

Quoted 3 lines

(quoted diff) +3 -0

+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.

Review

Quoted 3 lines

(quoted diff) +3 -0

+{
+	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).

Review

Quoted 1 line

(quoted diff) +1 -0

+	struct snd_ca0106_channel *chp = 0;

Ditto.

Review

Quoted 7 lines

(quoted diff) +7 -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]);

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.

Review

Quoted 13 lines

(quoted diff) +13 -0

+			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.

Review

Quoted 11 lines

(quoted diff) +11 -0

+			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...

Review

Quoted 7 lines

(quoted diff) +7 -0

+	} 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().

Review

Quoted 4 lines

(quoted diff) +4 -0

+	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.

Review

Quoted 5 lines

(quoted diff) +5 -0

+}
+
+static int hw_rule_playback_format(
+	struct snd_pcm_hw_params *params,
+	struct snd_pcm_hw_rule *rule)

Ditto.

Review

Quoted 4 lines

(quoted diff) +4 -0

+{
+	struct snd_ca0106 *chip = rule->private;
+	int chi, any_S16 = 0, any_S32 = 0;
+	struct snd_ca0106_channel *chp = 0;

No need to initialize.

Review

Quoted 10 lines

(quoted diff) +10 -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]);

This also needs a mutex as well.

Review

Quoted 13 lines

(quoted diff) +13 -0

+		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().

Quoted 8 lines

(quoted diff) +8 -0

+		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)

Review

Quoted 10 lines

(quoted diff) +10 -0

+		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().

Quoted 44 lines

(quoted diff) +22 -2

+	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);
 }
 

Review static int snd_ca0106_pcm_hw_free_capture(struct snd_pcm_substream *substream) · line 798

Quoted 5 lines

(quoted diff) +1 -1

@@ -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.

Review static int snd_ca0106_pcm_hw_free_capture(struct snd_pcm_substream *substream) · line 800

Quoted 3 lines

(quoted diff) +1 -1

 	struct snd_ca0106_pcm *epcm = runtime->private_data;
-	int channel = epcm->channel_id;
+	struct snd_ca0106_channel *chp = 0;

Ditto.

Quoted 51 lines

(quoted diff) +10 -30

+	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

Review static int snd_ca0106_pcm_hw_free_capture(struct snd_pcm_substream *substream) · line 831

Quoted 10 lines

(quoted diff) +10 -0

+		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).

Review static int snd_ca0106_pcm_hw_free_capture(struct snd_pcm_substream *substream) · line 838

Quoted 7 lines

(quoted diff) +7 -0

+	/*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.

Quoted 45 lines

(quoted diff) +45 -0

+		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

Review static int snd_ca0106_pcm_hw_free_capture(struct snd_pcm_substream *substream) · line 891

Quoted 10 lines

(quoted diff) +7 -2

+				": 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().

Quoted 18 lines

(quoted diff) +5 -2

+	/* 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");

Review static int __devinit snd_ca0106_create(int dev, struct snd_card *card, · line 1563

Quoted 9 lines

(quoted diff) +3 -1

@@ -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
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
From 1437172bbb9e2e3bcc0bb13e226fe43dda3a4fe2 Mon Sep 17 00:00:00 2001

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:

Quoted 5 lines

(quoted diff) +1 -0

+		for (chi = 0; chi < 4; ++chi) {

Review

Quoted 1 line

(quoted diff) +1 -0

+			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:

Quoted 4 lines

(quoted diff) +1 -0

+	for (chi = 0; chi < 4; ++chi) {

Review

Quoted 1 line

(quoted diff) +1 -0

+		chp = &(chip->playback_channels[chi]);

This also needs a mutex as well.

snd_ca0106_pcm_prepare_playback:

Quoted 4 lines

(quoted diff) +1 -0

+	for (chi = 0; chi < 4; ++chi) {

pci/ca0106/ca0106_main.c static int snd_ca0106_pcm_hw_free_capture(struct snd_pcm_substream *substream) · line 837

Quoted 1 line

pci/ca0106/ca0106_main.c +1 -0

+		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...

Review

Quoted 2 lines · Ben Stanley

(quoted diff) +2 -0

+		/* 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.

Quoted 7 lines · James Courtier-Dutton

Review

Quoted 2 lines · Ben Stanley

(quoted diff) +2 -0

+		/* 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?

Quoted 9 lines · Trent Piepho

Quoted 7 lines · James Courtier-Dutton

Review

Quoted 2 lines · Ben Stanley

(quoted diff) +2 -0

+		/* 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.

Quoted 2 lines

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.

At Wed, 03 Sep 2008 19:36:51 +0100,

Quoted 6 lines · James Courtier-Dutton

Quoted 4 lines · Trent Piepho

Quoted 3 lines · James Courtier-Dutton

Quoted 2 lines · Ben Stanley

(quoted diff) +2 -0

+         /* We only support 44100 to spdif, not to DAC.
+            (FIXME WHY?)*/

Quoted 8 lines

Quoted 7 lines

Quoted 4 lines

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
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/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 [email protected]

[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(-)

pci/ca0106/ca0106.h +6 -1

index 74175fc..bf502b5 100644
@@ -1,7 +1,7 @@
 /*
  *  Copyright (c) 2004 James Courtier-Dutton <[email protected]>
  *  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 {

pci/ca0106/ca0106.h +8 -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;

pci/ca0106/ca0106.h +10 -0

@@ -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;

pci/ca0106/ca0106.h +0 -0

@@ -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)

pci/ca0106/ca0106.h +21 -0

+	/* 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)) {

At Wed, 03 Sep 2008 03:11:07 +1000,

Quoted 10 lines · Ben Stanley

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 [email protected]

Thanks for the patch!

Quoted 48 lines

[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(-)

pci/ca0106/ca0106.h +6 -1

index 74175fc..bf502b5 100644
@@ -1,7 +1,7 @@
 /*
  *  Copyright (c) 2004 James Courtier-Dutton <[email protected]>
  *  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 {

pci/ca0106/ca0106.h struct snd_ca0106 { · line 692

Quoted 4 lines

pci/ca0106/ca0106.h +1 -0

@@ -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.

Quoted 7 lines

pci/ca0106/ca0106.h +0 -0

 	struct snd_ac97 *ac97;
 	struct snd_pcm *pcm;

@@ -703,6 +709,11 @@ struct snd_ca0106 {
 	struct snd_ca_midi midi2;
 

pci/ca0106/ca0106.h struct snd_ca0106 { · line 716

Quoted 6 lines

pci/ca0106/ca0106.h +5 -0

 	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?

Quoted 15 lines

pci/ca0106/ca0106.h +14 -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 &&

pci/ca0106/ca0106.h

Quoted 4 lines

pci/ca0106/ca0106.h +4 -0

+			 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.

pci/ca0106/ca0106.h

Quoted 6 lines

pci/ca0106/ca0106.h +5 -0

+		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.

pci/ca0106/ca0106.h

Quoted 10 lines

pci/ca0106/ca0106.h +9 -0

+		/* 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.

pci/ca0106/ca0106.h

Quoted 7 lines

pci/ca0106/ca0106.h +6 -0

+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.

pci/ca0106/ca0106.h

Quoted 12 lines

pci/ca0106/ca0106.h +11 -0

+	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().

pci/ca0106/ca0106.h

Quoted 5 lines

pci/ca0106/ca0106.h +5 -0

+	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.

pci/ca0106/ca0106.h static int snd_ca0106_pcm_open_playback_channel(struct snd_pcm_substream *substr · line 606

Quoted 8 lines

pci/ca0106/ca0106.h +2 -1

@@ -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('

pci/ca0106/ca0106.h static int snd_ca0106_pcm_open_playback_channel(struct snd_pcm_substream *substr · line 610

Quoted 6 lines

pci/ca0106/ca0106.h +3 -1

+	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.

Quoted 22 lines

pci/ca0106/ca0106.h +17 -0

@@ -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)) {

pci/ca0106/ca0106.h static int snd_ca0106_pcm_hw_params_playback(struct snd_pcm_substream *substream · line 776

Quoted 3 lines

pci/ca0106/ca0106.h +3 -0

+			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.

Quoted 4 lines

pci/ca0106/ca0106.h +4 -0

+		if (!pchannel->epcm->hw_reserved)
+			continue;
+		if (snd_BUG_ON(!pchannel->epcm->substream)) {
+			spin_unlock(&chip->pcm_lock);

pci/ca0106/ca0106.h static int snd_ca0106_pcm_hw_params_playback(struct snd_pcm_substream *substream · line 786

Quoted 6 lines

pci/ca0106/ca0106.h +6 -0

+			return -EINVAL;
+		}
+		if (snd_BUG_ON(!pchannel->epcm->substream->runtime)) {
+			spin_unlock(&chip->pcm_lock);
+			return -EINVAL;
+		}

Ditto.

Quoted 4 lines

pci/ca0106/ca0106.h +3 -0

+		runtimei = pchannel->epcm->substream->runtime;
+		chip->count_pb_44100_chan += runtimei->rate == 44100;
+		chip->count_pb_non_44100_chan += runtimei->rate != 44100;

pci/ca0106/ca0106.h static int snd_ca0106_pcm_hw_params_playback(struct snd_pcm_substream *substream · line 799

Quoted 10 lines

pci/ca0106/ca0106.h +10 -0

+		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.

Quoted 3 lines

pci/ca0106/ca0106.h +0 -0

@@ -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)

Quoted 27 lines

pci/ca0106/ca0106.h +27 -0

+	/* 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;

pci/ca0106/ca0106.h

Quoted 4 lines

pci/ca0106/ca0106.h +4 -0

+		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.

From c1c72cf2584c493080ddc04f9b9c36c38bb1b2bd Mon Sep 17 00:00:00 2001

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?

Quoted 11 lines

Review

Quoted 7 lines

(quoted diff) +7 -0

+	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,

Quoted 21 lines

Quoted 21 lines

(quoted diff) +17 -0

@@ -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)) {

Quoted 9 lines

Review static int snd_ca0106_pcm_hw_params_playback(struct snd_pcm_substream *substream · line 776

Quoted 3 lines

(quoted diff) +3 -0

+			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.

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.

At Thu, 04 Sep 2008 01:15:47 +1000,

Quoted 20 lines · Ben Stanley

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.

Quoted 8 lines

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 :)

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 12 lines

Quoted 11 lines

Review

Quoted 7 lines

(quoted diff) +7 -0

+	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.

Quoted 3 lines

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.

Quoted 31 lines

Quoted 21 lines

Quoted 21 lines

(quoted diff) +17 -0

@@ -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)) {

Quoted 9 lines

Review static int snd_ca0106_pcm_hw_params_playback(struct snd_pcm_substream *substream · line 776

Quoted 3 lines

(quoted diff) +3 -0

+			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.

Quoted 7 lines

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.

Quoted 5 lines

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.

Quoted 7 lines

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

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.

Quoted 134 lines · Takashi Iwai

At Thu, 04 Sep 2008 01:15:47 +1000,

Quoted 20 lines · Ben Stanley

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.

Quoted 8 lines

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 :)

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 12 lines

Quoted 11 lines

Review

Quoted 7 lines

(quoted diff) +7 -0

+	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.

Quoted 3 lines

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.

Quoted 31 lines

Quoted 21 lines

Quoted 21 lines

(quoted diff) +17 -0

@@ -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)) {

Quoted 9 lines

Review static int snd_ca0106_pcm_hw_params_playback(struct snd_pcm_substream *substream · line 776

Quoted 3 lines

(quoted diff) +3 -0

+			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.

Quoted 7 lines

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.

Quoted 5 lines

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.

Quoted 7 lines

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