[alsa-devel] [PATCH 0/4] ALSA: Updates for the CA0132 codec
Dylan Reid
dgreid at chromium.org
Wed Mar 20 18:21:59 CET 2013
On Fri, Mar 15, 2013 at 1:23 PM, Takashi Iwai <tiwai at suse.de> wrote:
> At Fri, 15 Mar 2013 11:39:50 -0700,
> Dylan Reid wrote:
>>
>> On Fri, Mar 15, 2013 at 1:25 AM, Takashi Iwai <tiwai at suse.de> wrote:
>> > At Thu, 14 Mar 2013 17:34:34 -0700,
>> > Dylan Reid wrote:
>> >>
>> >> On Sun, Feb 10, 2013 at 2:57 AM, Takashi Iwai <tiwai at suse.de> wrote:
>> >> > Hi Ian,
>> >> >
>> >> > At Fri, 8 Feb 2013 18:31:41 -0800,
>> >> > Ian Minett wrote:
>> >> >>
>> >> >> From: Ian Minett <ian_minett at creativelabs.com>
>> >> >>
>> >> >> Hi,
>> >> >>
>> >> >> This series contains the following updates to CA0132:
>> >> >>
>> >> >> 1: Add firmware loading for the SpeakerEQ DSP effect.
>> >> >> 2: Improve DSP transfer timeout calculations.
>> >> >> 3: Add stream and effect latency monitoring routines
>> >> >> 4: Update hp unsolicited event handler to manage queued work.
>> >> >
>> >> > Thanks, I took the one now but leave others. See my reply to each.
>> >> >
>> >> >>
>> >> >> These were based on the latest sound.git, but please let me know if
>> >> >> they should be against sound-unstable tree instead.
>> >> >
>> >> > sound.git tree for-next branch would be the place to look at.
>> >> > If I start a new branch for ca0132, I'll let you know.
>> >> >
>> >> >
>> >> > But, before going further: as I already mailed you, I could get a test
>> >> > board and started testing. The result wasn't good. At the first time
>> >> > the driver loads, the DSP loading fails always. When the driver is
>> >> > reloaded after that, it's working. So, something is still pretty
>> >> > fragile.
>> >>
>> >> I tried this today and I can't get it to work. It is failing in
>> >> snd_hda_lock_devices called from azx_load_dsp_prepare. The DSP is
>> >> loaded from ca0132_init which is called as the result of open/resume.
>> >> Both the check for open control files and attached substreams are
>> >> failing. How can that be avoided?
>> >
>> > snd_hda_lock_devices() is obviously not suitable for the power-saving
>> > or PM resume. OK, this must be fixed indeed...
>> >
>> > How about the patch below? (Note that it's just compile tested.)
>>
>> Thanks Takashi. This works for the init case, but it deadlocks in the
>> case the load originates from open:
>> azx_pcm_open -> calls snd_hda_power_up_d3wait -> ... -> ca0132_init ->
>> azx_load_dsp_prepare
>>
>> both open and load_dsp_prepare grab open_mutex.
>
> Right. The revised patch is below.
With the two changes I noted inline this works. I tested it at
startup, open/close and suspend/resume.
Thanks for the speedy fixes!
>
>
> Takashi
>
> ---
> From: Takashi Iwai <tiwai at suse.de>
> Subject: [PATCH v2] ALSA: hda - Fix abuse of snd_hda_lock_devices() for DSP loader
>
> The current DSP loader code abuses snd_hda_lock_devices() for ensuring
> the DSP loader not conflicting with the other normal operations. But
> this trick obviously doesn't work for the PM resume since the streams
> are kept opened there where snd_hda_lock_devices() returns -EBUSY.
> That means we need another lock mechanism instead of abuse.
>
> This patch provides the new lock state to azx_dev. Theoretically it's
> possible that the DSP loader conflicts with the stream that has been
> already assigned for another PCM. If it's running, the DSP loader
> should simply fail. If not -- it's the case for PM resume --, we
> should assign this stream temporarily to the DSP loader, and take it
> back to the PCM after finishing DSP loading. If the PCM is operated
> during the DSP loading, it should get an error, too.
>
> Reported-by: Dylan Reid <dgreid at chromium.org>
> Signed-off-by: Takashi Iwai <tiwai at suse.de>
> ---
> sound/pci/hda/hda_intel.c | 132 ++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 109 insertions(+), 23 deletions(-)
>
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index 4cea6bb6..880bdc7 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -415,6 +415,8 @@ struct azx_dev {
> unsigned int opened :1;
> unsigned int running :1;
> unsigned int irq_pending :1;
> + unsigned int prepared:1;
> + unsigned int locked:1;
> /*
> * For VIA:
> * A flag to ensure DMA position is 0
> @@ -426,8 +428,25 @@ struct azx_dev {
>
> struct timecounter azx_tc;
> struct cyclecounter azx_cc;
> +
> +#ifdef CONFIG_SND_HDA_DSP_LOADER
> + struct mutex dsp_mutex;
> +#endif
> };
>
> +/* DSP lock helpers */
> +#ifdef CONFIG_SND_HDA_DSP_LOADER
> +#define dsp_lock_init(dev) mutex_init(&(dev)->dsp_mutex)
> +#define dsp_lock(dev) mutex_lock(&(dev)->dsp_mutex)
> +#define dsp_unlock(dev) mutex_unlock(&(dev)->dsp_mutex)
> +#define dsp_is_locked(dev) (dev)->locked
> +#else
> +#define dsp_lock_init(dev) do {} while (0)
> +#define dsp_lock(dev) do {} while (0)
> +#define dsp_unlock(dev) do {} while (0)
> +#define dsp_is_locked(dev) 0
> +#endif
> +
> /* CORB/RIRB */
> struct azx_rb {
> u32 *buf; /* CORB/RIRB buffer
> @@ -527,6 +546,10 @@ struct azx {
>
> /* card list (for power_save trigger) */
> struct list_head list;
> +
> +#ifdef CONFIG_SND_HDA_DSP_LOADER
> + struct azx_dev saved_azx_dev;
> +#endif
> };
>
> #define CREATE_TRACE_POINTS
> @@ -1793,15 +1816,25 @@ azx_assign_device(struct azx *chip, struct snd_pcm_substream *substream)
> dev = chip->capture_index_offset;
> nums = chip->capture_streams;
> }
> - for (i = 0; i < nums; i++, dev++)
> - if (!chip->azx_dev[dev].opened) {
> - res = &chip->azx_dev[dev];
> - if (res->assigned_key == key)
> - break;
> + for (i = 0; i < nums; i++, dev++) {
> + struct azx_dev *azx_dev = &chip->azx_dev[dev];
> + dsp_lock(azx_dev);
> + if (!azx_dev->opened && !dsp_is_locked(azx_dev)) {
> + res = azx_dev;
> + if (res->assigned_key == key) {
> + res->opened = 1;
> + res->assigned_key = key;
> + dsp_unlock(azx_dev);
> + return res;
> + }
> }
> + dsp_unlock(azx_dev);
> + }
> if (res) {
> + dsp_lock(res);
> res->opened = 1;
> res->assigned_key = key;
> + dsp_unlock(res);
> }
> return res;
> }
> @@ -2009,6 +2042,12 @@ static int azx_pcm_hw_params(struct snd_pcm_substream *substream,
> struct azx_dev *azx_dev = get_azx_dev(substream);
> int ret;
>
> + dsp_lock(azx_dev);
> + if (dsp_is_locked(azx_dev)) {
> + ret = -EBUSY;
> + goto unlock;
> + }
> +
> mark_runtime_wc(chip, azx_dev, substream, false);
> azx_dev->bufsize = 0;
> azx_dev->period_bytes = 0;
> @@ -2016,8 +2055,10 @@ static int azx_pcm_hw_params(struct snd_pcm_substream *substream,
> ret = snd_pcm_lib_malloc_pages(substream,
> params_buffer_bytes(hw_params));
> if (ret < 0)
> - return ret;
> + goto unlock;
> mark_runtime_wc(chip, azx_dev, substream, true);
> + unlock:
> + dsp_unlock(azx_dev);
> return ret;
> }
>
> @@ -2029,16 +2070,21 @@ static int azx_pcm_hw_free(struct snd_pcm_substream *substream)
> struct hda_pcm_stream *hinfo = apcm->hinfo[substream->stream];
>
> /* reset BDL address */
> - azx_sd_writel(azx_dev, SD_BDLPL, 0);
> - azx_sd_writel(azx_dev, SD_BDLPU, 0);
> - azx_sd_writel(azx_dev, SD_CTL, 0);
> - azx_dev->bufsize = 0;
> - azx_dev->period_bytes = 0;
> - azx_dev->format_val = 0;
> + dsp_lock(azx_dev);
> + if (!dsp_is_locked(azx_dev)) {
> + azx_sd_writel(azx_dev, SD_BDLPL, 0);
> + azx_sd_writel(azx_dev, SD_BDLPU, 0);
> + azx_sd_writel(azx_dev, SD_CTL, 0);
> + azx_dev->bufsize = 0;
> + azx_dev->period_bytes = 0;
> + azx_dev->format_val = 0;
> + }
>
> snd_hda_codec_cleanup(apcm->codec, hinfo, substream);
>
> mark_runtime_wc(chip, azx_dev, substream, false);
> + azx_dev->prepared = 0;
> + dsp_unlock(azx_dev);
> return snd_pcm_lib_free_pages(substream);
> }
>
> @@ -2055,6 +2101,12 @@ static int azx_pcm_prepare(struct snd_pcm_substream *substream)
> snd_hda_spdif_out_of_nid(apcm->codec, hinfo->nid);
> unsigned short ctls = spdif ? spdif->ctls : 0;
>
> + dsp_lock(azx_dev);
> + if (dsp_is_locked(azx_dev)) {
> + err = -EBUSY;
> + goto unlock;
> + }
> +
> azx_stream_reset(chip, azx_dev);
> format_val = snd_hda_calc_stream_format(runtime->rate,
> runtime->channels,
> @@ -2065,7 +2117,8 @@ static int azx_pcm_prepare(struct snd_pcm_substream *substream)
> snd_printk(KERN_ERR SFX
> "%s: invalid format_val, rate=%d, ch=%d, format=%d\n",
> pci_name(chip->pci), runtime->rate, runtime->channels, runtime->format);
> - return -EINVAL;
> + err = -EINVAL;
> + goto unlock;
> }
>
> bufsize = snd_pcm_lib_buffer_bytes(substream);
> @@ -2084,7 +2137,7 @@ static int azx_pcm_prepare(struct snd_pcm_substream *substream)
> azx_dev->no_period_wakeup = runtime->no_period_wakeup;
> err = azx_setup_periods(chip, substream, azx_dev);
> if (err < 0)
> - return err;
> + goto unlock;
> }
>
> /* wallclk has 24Mhz clock source */
> @@ -2101,8 +2154,14 @@ static int azx_pcm_prepare(struct snd_pcm_substream *substream)
> if ((chip->driver_caps & AZX_DCAPS_CTX_WORKAROUND) &&
> stream_tag > chip->capture_streams)
> stream_tag -= chip->capture_streams;
> - return snd_hda_codec_prepare(apcm->codec, hinfo, stream_tag,
> + err = snd_hda_codec_prepare(apcm->codec, hinfo, stream_tag,
> azx_dev->format_val, substream);
> +
> + unlock:
> + if (!err)
> + azx_dev->prepared = 1;
> + dsp_unlock(azx_dev);
> + return err;
> }
>
> static int azx_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
> @@ -2117,6 +2176,9 @@ static int azx_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
> azx_dev = get_azx_dev(substream);
> trace_azx_pcm_trigger(chip, azx_dev, cmd);
>
> + if (!dsp_is_locked(azx_dev) || !azx_dev->prepared)
I inverted this check. I think the intent here is that either the dsp
load is active or a substream is prepared. If that's true this should
return when both are false.
> + return -EPIPE;
> +
> switch (cmd) {
> case SNDRV_PCM_TRIGGER_START:
> rstart = 1;
> @@ -2621,17 +2683,27 @@ static int azx_load_dsp_prepare(struct hda_bus *bus, unsigned int format,
> struct azx_dev *azx_dev;
> int err;
>
> - if (snd_hda_lock_devices(bus))
> - return -EBUSY;
> + azx_dev = azx_get_dsp_loader_dev(chip);
> +
> + dsp_lock(azx_dev);
> + spin_lock_irq(&chip->reg_lock);
> + if (azx_dev->running || azx_dev->locked) {
> + spin_unlock_irq(&chip->reg_lock);
> + err = -EBUSY;
> + goto unlock;
> + }
> + azx_dev->prepared = 0;
> + chip->saved_azx_dev = *azx_dev;
> + azx_dev->locked = 1;
> + spin_unlock_irq(&chip->reg_lock);
>
> err = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV_SG,
> snd_dma_pci_data(chip->pci),
> byte_size, bufp);
> if (err < 0)
> - goto unlock;
> + goto err_alloc;
>
> mark_pages_wc(chip, bufp, true);
> - azx_dev = azx_get_dsp_loader_dev(chip);
> azx_dev->bufsize = byte_size;
> azx_dev->period_bytes = byte_size;
> azx_dev->format_val = format;
> @@ -2649,13 +2721,20 @@ static int azx_load_dsp_prepare(struct hda_bus *bus, unsigned int format,
> goto error;
>
> azx_setup_controller(chip, azx_dev);
I added dsp_unlock(azx_dev); here.
> + mutex_unlock(&chip->open_mutex);
> return azx_dev->stream_tag;
>
> error:
> mark_pages_wc(chip, bufp, false);
> snd_dma_free_pages(bufp);
> -unlock:
> - snd_hda_unlock_devices(bus);
> + err_alloc:
> + spin_lock_irq(&chip->reg_lock);
> + if (azx_dev->opened)
> + *azx_dev = chip->saved_azx_dev;
> + azx_dev->locked = 0;
> + spin_unlock_irq(&chip->reg_lock);
> + unlock:
> + dsp_unlock(azx_dev);
> return err;
> }
>
> @@ -2677,9 +2756,10 @@ static void azx_load_dsp_cleanup(struct hda_bus *bus,
> struct azx *chip = bus->private_data;
> struct azx_dev *azx_dev = azx_get_dsp_loader_dev(chip);
>
> - if (!dmab->area)
> + if (!dmab->area || !azx_dev->locked)
> return;
>
> + dsp_lock(azx_dev);
> /* reset BDL address */
> azx_sd_writel(azx_dev, SD_BDLPL, 0);
> azx_sd_writel(azx_dev, SD_BDLPU, 0);
> @@ -2692,7 +2772,12 @@ static void azx_load_dsp_cleanup(struct hda_bus *bus,
> snd_dma_free_pages(dmab);
> dmab->area = NULL;
>
> - snd_hda_unlock_devices(bus);
> + spin_lock_irq(&chip->reg_lock);
> + if (azx_dev->opened)
> + *azx_dev = chip->saved_azx_dev;
> + azx_dev->locked = 0;
> + spin_unlock_irq(&chip->reg_lock);
> + dsp_unlock(azx_dev);
> }
> #endif /* CONFIG_SND_HDA_DSP_LOADER */
>
> @@ -3481,6 +3566,7 @@ static int azx_first_init(struct azx *chip)
> }
>
> for (i = 0; i < chip->num_streams; i++) {
> + dsp_lock_init(&chip->azx_dev[i]);
> /* allocate memory for the BDL for each stream */
> err = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV,
> snd_dma_pci_data(chip->pci),
> --
> 1.8.2
>
More information about the Alsa-devel
mailing list