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

[Takashi Iwai]

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 at 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