All of lore.kernel.org
 help / color / mirror / Atom feed
From: Takashi Iwai <tiwai@suse.de>
To: Dylan Reid <dgreid@chromium.org>
Cc: alsa-devel@alsa-project.org, Ian Minett <ian_minett@creativelabs.com>
Subject: Re: [PATCH 0/4] ALSA: Updates for the CA0132 codec
Date: Wed, 20 Mar 2013 18:39:32 +0100	[thread overview]
Message-ID: <s5hehfac6nf.wl%tiwai@suse.de> (raw)
In-Reply-To: <CAEUnVG59BDYMTnNjgr5nOaCE3doABJn4GhNMaKWdtqDtrXEpYQ@mail.gmail.com>

At Wed, 20 Mar 2013 10:21:59 -0700,
Dylan Reid wrote:
> 
> On Fri, Mar 15, 2013 at 1:23 PM, Takashi Iwai <tiwai@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@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@suse.de> wrote:
> >> >> > Hi Ian,
> >> >> >
> >> >> > At Fri, 8 Feb 2013 18:31:41 -0800,
> >> >> > Ian Minett wrote:
> >> >> >>
> >> >> >> From: Ian Minett <ian_minett@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@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@chromium.org>
> > Signed-off-by: Takashi Iwai <tiwai@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
> >
> 

      reply	other threads:[~2013-03-20 17:39 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-02-09  2:31 [PATCH 0/4] ALSA: Updates for the CA0132 codec Ian Minett
2013-02-09  2:31 ` [PATCH 1/4] ALSA: CA0132: Add SpeakerEQ feature firmware loading Ian Minett
2013-02-10 10:47   ` Takashi Iwai
2013-02-09  2:31 ` [PATCH 2/4] ALSA: CA0132: Improve the DSP transfer timeout calculations Ian Minett
2013-02-10 10:51   ` Takashi Iwai
2013-02-09  2:31 ` [PATCH 3/4] ALSA: CA0132: Add latency monitoring Ian Minett
2013-02-10 10:51   ` Takashi Iwai
2013-03-25 17:47     ` Dylan Reid
2013-04-02  9:30       ` Takashi Iwai
2013-04-02 20:24         ` Dylan Reid
2013-04-03  5:19           ` Takashi Iwai
2013-02-09  2:31 ` [PATCH 4/4] ALSA: CA0132: Update hp unsol event handler Ian Minett
2013-02-10 10:52   ` Takashi Iwai
2013-02-10 10:57 ` [PATCH 0/4] ALSA: Updates for the CA0132 codec Takashi Iwai
2013-03-15  0:34   ` Dylan Reid
2013-03-15  8:25     ` Takashi Iwai
2013-03-15 18:39       ` Dylan Reid
2013-03-15 20:23         ` Takashi Iwai
2013-03-20 17:21           ` Dylan Reid
2013-03-20 17:39             ` Takashi Iwai [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=s5hehfac6nf.wl%tiwai@suse.de \
    --to=tiwai@suse.de \
    --cc=alsa-devel@alsa-project.org \
    --cc=dgreid@chromium.org \
    --cc=ian_minett@creativelabs.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.