Home
Reading
Searching
Subscribe
Sponsors
Statistics
Posting
Contact
Spam
Lists
Links
About
Hosting
Filtering
Features Download
Marketing
Archives
FAQ
Blog
 
Gmane
From: Takashi Iwai <tiwai <at> suse.de>
Subject: Re: [PATCH 1/2] ca0106 playback 44100Hz support to SPDIF and playback format & rate constraints
Newsgroups: gmane.linux.alsa.devel
Date: Wednesday 3rd September 2008 09:49:54 UTC (over 9 years ago)
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 [email protected]

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
<[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 {
> @@ -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
 
CD: 3ms