[alsa-devel] [PATCH 0/4] ALSA: Updates for the CA0132 codec
Takashi Iwai
tiwai at suse.de
Wed Mar 20 18:39:32 CET 2013
At Wed, 20 Mar 2013 10:21:59 -0700,
Dylan Reid wrote:
>
> 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!
Thanks, I fixed the patch and committed now.
Takashi
>
> >
> >
> > 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