Re: [alsa-devel] [PATCH 1/2] ca0106 playback 44100Hz support to SPDIF and playback format & rate constraints

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