| Message ID | 20191117085308.23915-8-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: pcm: API cleanups and extensions | expand |
On 11/17/19 2:53 AM, Takashi Iwai wrote: > Many PCI and other drivers performs snd_pcm_period_elapsed() simply in > its interrupt handler, so the sync_stop operation is just to call > synchronize_irq(). Instead of putting this call multiple times, > introduce the common card->sync_irq field. When this field is set, > PCM core performs synchronize_irq() for sync-stop operation. Each > driver just needs to copy its local IRQ number to card->sync_irq, and > that's all we need. Maybe a red-herring or complete non-sense, but I wonder if this is going to get in the way of Ranjani's multi-client work, where we could have multiple cards created but with a single IRQ handled by the parent PCI device? Ranjani, you may want to double-check this and chime in, thanks! > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > --- > include/sound/core.h | 1 + > sound/core/init.c | 1 + > sound/core/pcm_native.c | 2 ++ > 3 files changed, 4 insertions(+) > > diff --git a/include/sound/core.h b/include/sound/core.h > index ee238f100f73..af3dce956c17 100644 > --- a/include/sound/core.h > +++ b/include/sound/core.h > @@ -117,6 +117,7 @@ struct snd_card { > struct device card_dev; /* cardX object for sysfs */ > const struct attribute_group *dev_groups[4]; /* assigned sysfs attr */ > bool registered; /* card_dev is registered? */ > + int sync_irq; /* assigned irq, used for PCM sync */ > wait_queue_head_t remove_sleep; > > #ifdef CONFIG_PM > diff --git a/sound/core/init.c b/sound/core/init.c > index db99b7fad6ad..faa9f03c01ca 100644 > --- a/sound/core/init.c > +++ b/sound/core/init.c > @@ -215,6 +215,7 @@ int snd_card_new(struct device *parent, int idx, const char *xid, > init_waitqueue_head(&card->power_sleep); > #endif > init_waitqueue_head(&card->remove_sleep); > + card->sync_irq = -1; > > device_initialize(&card->card_dev); > card->card_dev.parent = parent; > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c > index 163d621ff238..1fe581167b7b 100644 > --- a/sound/core/pcm_native.c > +++ b/sound/core/pcm_native.c > @@ -574,6 +574,8 @@ static void snd_pcm_sync_stop(struct snd_pcm_substream *substream) > substream->runtime->stop_operating = false; > if (substream->ops->sync_stop) > substream->ops->sync_stop(substream); > + else if (substream->pcm->card->sync_irq > 0) > + synchronize_irq(substream->pcm->card->sync_irq); > } > } > >
On Mon, 18 Nov 2019 17:38:49 +0100, Pierre-Louis Bossart wrote: > > > > On 11/17/19 2:53 AM, Takashi Iwai wrote: > > Many PCI and other drivers performs snd_pcm_period_elapsed() simply in > > its interrupt handler, so the sync_stop operation is just to call > > synchronize_irq(). Instead of putting this call multiple times, > > introduce the common card->sync_irq field. When this field is set, > > PCM core performs synchronize_irq() for sync-stop operation. Each > > driver just needs to copy its local IRQ number to card->sync_irq, and > > that's all we need. > > Maybe a red-herring or complete non-sense, but I wonder if this is > going to get in the way of Ranjani's multi-client work, where we could > have multiple cards created but with a single IRQ handled by the > parent PCI device? > > Ranjani, you may want to double-check this and chime in, thanks! The synchronize_irq() is fairly safe to call multiple times, and I don't think any problem by invoking it for multi-clients sharing the same IRQ. For example, Digigram miXart driver creates multiple card objects from a single PCI entry, and I already thought of that possibility; they set the same card->sync_irq value to all card objects, which eventually will call synchronize_irq() multiple times. From the performance POV, this shouldn't be a big problem, because the place calling this is only at hw_params, prepare and hw_free, neither are hot-path. thanks, Takashi
On Mon, Nov 18, 2019 at 10:54 AM Takashi Iwai <tiwai@suse.de> wrote: > On Mon, 18 Nov 2019 17:38:49 +0100, > Pierre-Louis Bossart wrote: > > > > > > > > On 11/17/19 2:53 AM, Takashi Iwai wrote: > > > Many PCI and other drivers performs snd_pcm_period_elapsed() simply in > > > its interrupt handler, so the sync_stop operation is just to call > > > synchronize_irq(). Instead of putting this call multiple times, > > > introduce the common card->sync_irq field. When this field is set, > > > PCM core performs synchronize_irq() for sync-stop operation. Each > > > driver just needs to copy its local IRQ number to card->sync_irq, and > > > that's all we need. > > > > Maybe a red-herring or complete non-sense, but I wonder if this is > > going to get in the way of Ranjani's multi-client work, where we could > > have multiple cards created but with a single IRQ handled by the > > parent PCI device? > > > > Ranjani, you may want to double-check this and chime in, thanks! > > The synchronize_irq() is fairly safe to call multiple times, and I > don't think any problem by invoking it for multi-clients sharing the > same IRQ. For example, Digigram miXart driver creates multiple card > objects from a single PCI entry, and I already thought of that > possibility; they set the same card->sync_irq value to all card > objects, which eventually will call synchronize_irq() multiple times. > From the performance POV, this shouldn't be a big problem, because > the place calling this is only at hw_params, prepare and hw_free, > neither are hot-path. > Thanks for the clarification, Takashi. But just wondering how would one pass on the sync_irq when the snd_card is created? Typically in the case of the Intel platforms, the card->dev points to the platform device for the machine driver that registers the card and the PCI device is the parent of the machine drv platform device. Thanks, Ranjani
On Mon, 18 Nov 2019 20:20:41 +0100, Sridharan, Ranjani wrote: > > On Mon, Nov 18, 2019 at 10:54 AM Takashi Iwai <tiwai@suse.de> wrote: > > On Mon, 18 Nov 2019 17:38:49 +0100, > Pierre-Louis Bossart wrote: > > > > > > > > On 11/17/19 2:53 AM, Takashi Iwai wrote: > > > Many PCI and other drivers performs snd_pcm_period_elapsed() simply in > > > its interrupt handler, so the sync_stop operation is just to call > > > synchronize_irq(). Instead of putting this call multiple times, > > > introduce the common card->sync_irq field. When this field is set, > > > PCM core performs synchronize_irq() for sync-stop operation. Each > > > driver just needs to copy its local IRQ number to card->sync_irq, and > > > that's all we need. > > > > Maybe a red-herring or complete non-sense, but I wonder if this is > > going to get in the way of Ranjani's multi-client work, where we could > > have multiple cards created but with a single IRQ handled by the > > parent PCI device? > > > > Ranjani, you may want to double-check this and chime in, thanks! > > The synchronize_irq() is fairly safe to call multiple times, and I > don't think any problem by invoking it for multi-clients sharing the > same IRQ. For example, Digigram miXart driver creates multiple card > objects from a single PCI entry, and I already thought of that > possibility; they set the same card->sync_irq value to all card > objects, which eventually will call synchronize_irq() multiple times. > From the performance POV, this shouldn't be a big problem, because > the place calling this is only at hw_params, prepare and hw_free, > neither are hot-path. > > Thanks for the clarification, Takashi. But just wondering how would one pass > on the sync_irq when the snd_card is created? Typically in the case of the > Intel platforms, the card->dev points to the platform device for the machine > driver that registers the card and the PCI device is the parent of the machine > drv platform device. It's completely up to the driver implementation :) You can implement the own sync_stop ops if that's easier, too. In general, the driver can set up card->sync_irq when the request_irq() or its variant is called. Takashi
> > > > > Thanks for the clarification, Takashi. But just wondering how would one > pass > > on the sync_irq when the snd_card is created? Typically in the case of > the > > Intel platforms, the card->dev points to the platform device for the > machine > > driver that registers the card and the PCI device is the parent of the > machine > > drv platform device. > > It's completely up to the driver implementation :) > You can implement the own sync_stop ops if that's easier, too. > I think this would make sense in the case of the SOF driver and we'd probably need to just call synchronize_irq() in the sync_stop() operation. With this change, we can probably remove the workaround we have to address the issue we were facing during snd_pcm_period_elapsed(). I can give this a try. We might need to run some stress tests to make sure it doesn't break anything. Thanks, Ranjani
On Mon, 18 Nov 2019 20:55:19 +0100, Sridharan, Ranjani wrote: > > > Thanks for the clarification, Takashi. But just wondering how would one > pass > > on the sync_irq when the snd_card is created? Typically in the case of > the > > Intel platforms, the card->dev points to the platform device for the > machine > > driver that registers the card and the PCI device is the parent of the > machine > > drv platform device. > > It's completely up to the driver implementation :) > You can implement the own sync_stop ops if that's easier, too. > > I think this would make sense in the case of the SOF driver and we'd probably > need to just call synchronize_irq() in the sync_stop() operation. With this > change, we can probably remove the workaround we have to address the issue we > were facing during snd_pcm_period_elapsed(). > > I can give this a try. We might need to run some stress tests to make sure it > doesn't break anything. If this helps for SOF, it'd be great. I have converted only non-ASoC drivers regarding the sync-stop stuff, so it won't conflict my upcoming changes :) FWIW, the rest diff (post topic/pcm-managed) looks like: .../gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c | 1 - drivers/media/pci/cobalt/cobalt-alsa-pcm.c | 8 ----- drivers/media/pci/cx18/cx18-alsa-pcm.c | 13 -------- drivers/media/pci/cx23885/cx23885-alsa.c | 1 - drivers/media/pci/cx25821/cx25821-alsa.c | 1 - drivers/media/pci/cx88/cx88-alsa.c | 1 - drivers/media/pci/ivtv/ivtv-alsa-pcm.c | 13 -------- drivers/media/pci/saa7134/saa7134-alsa.c | 1 - drivers/media/pci/solo6x10/solo6x10-g723.c | 1 - drivers/media/pci/tw686x/tw686x-audio.c | 1 - drivers/media/usb/cx231xx/cx231xx-audio.c | 1 - drivers/media/usb/em28xx/em28xx-audio.c | 1 - drivers/media/usb/go7007/snd-go7007.c | 1 - drivers/media/usb/tm6000/tm6000-alsa.c | 1 - drivers/media/usb/usbtv/usbtv-audio.c | 1 - drivers/staging/most/sound/sound.c | 1 - .../vc04_services/bcm2835-audio/bcm2835-pcm.c | 2 -- drivers/usb/gadget/function/u_audio.c | 1 - include/sound/pcm.h | 2 -- sound/aoa/soundbus/i2sbus/pcm.c | 2 -- sound/arm/aaci.c | 2 -- sound/arm/pxa2xx-ac97.c | 1 - sound/atmel/ac97c.c | 2 -- sound/core/pcm_local.h | 2 ++ sound/core/pcm_memory.c | 16 ++------- sound/drivers/aloop.c | 1 - sound/drivers/dummy.c | 2 -- sound/drivers/ml403-ac97cr.c | 2 -- sound/drivers/pcsp/pcsp_lib.c | 1 - sound/drivers/vx/vx_pcm.c | 2 -- sound/firewire/bebob/bebob_pcm.c | 2 -- sound/firewire/dice/dice-pcm.c | 2 -- sound/firewire/digi00x/digi00x-pcm.c | 2 -- sound/firewire/fireface/ff-pcm.c | 2 -- sound/firewire/fireworks/fireworks_pcm.c | 2 -- sound/firewire/isight.c | 1 - sound/firewire/motu/motu-pcm.c | 2 -- sound/firewire/oxfw/oxfw-pcm.c | 2 -- sound/firewire/tascam/tascam-pcm.c | 2 -- sound/isa/ad1816a/ad1816a_lib.c | 3 +- sound/isa/es1688/es1688_lib.c | 9 +---- sound/isa/es18xx.c | 3 +- sound/isa/gus/gus_main.c | 1 + sound/isa/gus/gus_pcm.c | 2 -- sound/isa/gus/gusmax.c | 3 +- sound/isa/gus/interwave.c | 1 + sound/isa/msnd/msnd.c | 2 -- sound/isa/msnd/msnd_pinnacle.c | 1 + sound/isa/opl3sa2.c | 1 + sound/isa/opti9xx/opti92x-ad1848.c | 1 + sound/isa/sb/emu8000_pcm.c | 1 - sound/isa/sb/sb16_main.c | 2 -- sound/isa/sb/sb8_main.c | 2 -- sound/isa/sb/sb_common.c | 1 + sound/isa/wavefront/wavefront.c | 1 + sound/isa/wss/wss_lib.c | 3 +- sound/mips/hal2.c | 2 -- sound/mips/sgio2audio.c | 3 -- sound/parisc/harmony.c | 2 -- sound/pci/ad1889.c | 4 +-- sound/pci/ali5451/ali5451.c | 7 +--- sound/pci/als300.c | 4 +-- sound/pci/als4000.c | 2 -- sound/pci/asihpi/asihpi.c | 17 ---------- sound/pci/atiixp.c | 5 +-- sound/pci/atiixp_modem.c | 4 +-- sound/pci/au88x0/au88x0.c | 1 + sound/pci/au88x0/au88x0_pcm.c | 1 - sound/pci/aw2/aw2-alsa.c | 3 +- sound/pci/azt3328.c | 5 +-- sound/pci/bt87x.c | 3 +- sound/pci/ca0106/ca0106_main.c | 9 +---- sound/pci/cmipci.c | 6 +--- sound/pci/cs4281.c | 7 +--- sound/pci/cs46xx/cs46xx_lib.c | 11 +----- sound/pci/cs5535audio/cs5535audio.c | 2 +- sound/pci/cs5535audio/cs5535audio_pcm.c | 2 -- sound/pci/ctxfi/cthw20k1.c | 4 +-- sound/pci/ctxfi/cthw20k2.c | 1 + sound/pci/ctxfi/ctpcm.c | 2 -- sound/pci/echoaudio/echoaudio.c | 7 ++-- sound/pci/emu10k1/emu10k1_main.c | 1 + sound/pci/emu10k1/emu10k1x.c | 3 +- sound/pci/emu10k1/emupcm.c | 6 ---- sound/pci/emu10k1/p16v.c | 2 -- sound/pci/ens1370.c | 7 +--- sound/pci/es1938.c | 5 +-- sound/pci/es1968.c | 4 --- sound/pci/fm801.c | 3 +- sound/pci/hda/hda_controller.c | 1 - sound/pci/hda/hda_intel.c | 4 ++- sound/pci/hda/hda_tegra.c | 4 +-- sound/pci/ice1712/ice1712.c | 7 +--- sound/pci/ice1712/ice1724.c | 7 +--- sound/pci/intel8x0.c | 16 ++------- sound/pci/intel8x0m.c | 5 +-- sound/pci/korg1212/korg1212.c | 1 + sound/pci/lola/lola.c | 2 +- sound/pci/lola/lola_pcm.c | 1 - sound/pci/lx6464es/lx6464es.c | 3 +- sound/pci/maestro3.c | 3 +- sound/pci/mixart/mixart.c | 3 +- sound/pci/nm256/nm256.c | 4 +-- sound/pci/oxygen/oxygen_lib.c | 2 +- sound/pci/oxygen/oxygen_pcm.c | 6 ---- sound/pci/pcxhr/pcxhr.c | 2 +- sound/pci/riptide/riptide.c | 3 +- sound/pci/rme32.c | 9 +---- sound/pci/rme96.c | 5 +-- sound/pci/rme9652/hdsp.c | 1 + sound/pci/rme9652/hdspm.c | 1 + sound/pci/rme9652/rme9652.c | 1 + sound/pci/sis7019.c | 3 +- sound/pci/sonicvibes.c | 3 +- sound/pci/trident/trident_main.c | 32 +----------------- sound/pci/via82xx.c | 8 +---- sound/pci/via82xx_modem.c | 5 +-- sound/pci/vx222/vx222.c | 1 + sound/pci/ymfpci/ymfpci_main.c | 6 +--- sound/pcmcia/pdaudiocf/pdaudiocf.c | 1 + sound/pcmcia/pdaudiocf/pdaudiocf_pcm.c | 1 - sound/pcmcia/vx/vxpocket.c | 1 + sound/ppc/pmac.c | 2 -- sound/ppc/snd_ps3.c | 1 - sound/sh/aica.c | 1 - sound/sh/sh_dac_audio.c | 1 - sound/sparc/amd7930.c | 2 -- sound/sparc/cs4231.c | 2 -- sound/sparc/dbri.c | 1 - sound/spi/at73c213.c | 1 - sound/usb/6fire/pcm.c | 1 - sound/usb/caiaq/audio.c | 1 - sound/usb/hiface/pcm.c | 1 - sound/usb/line6/capture.c | 1 - sound/usb/line6/playback.c | 1 - sound/usb/misc/ua101.c | 2 -- sound/usb/pcm.c | 39 ++++++++++++++-------- sound/usb/usx2y/usbusx2yaudio.c | 1 - sound/usb/usx2y/usx2yhwdeppcm.c | 1 - sound/x86/intel_hdmi_audio.c | 1 - sound/xen/xen_snd_front_alsa.c | 2 -- 141 files changed, 104 insertions(+), 394 deletions(-) Takashi
On Mon, 2019-11-18 at 21:40 +0100, Takashi Iwai wrote: > On Mon, 18 Nov 2019 20:55:19 +0100, > Sridharan, Ranjani wrote: > > > > > Thanks for the clarification, Takashi. But just wondering how > > would one > > pass > > > on the sync_irq when the snd_card is created? Typically in > > the case of > > the > > > Intel platforms, the card->dev points to the platform device > > for the > > machine > > > driver that registers the card and the PCI device is the > > parent of the > > machine > > > drv platform device. > > > > It's completely up to the driver implementation :) > > You can implement the own sync_stop ops if that's easier, too. > > > > I think this would make sense in the case of the SOF driver and > > we'd probably > > need to just call synchronize_irq() in the sync_stop() operation. > > With this > > change, we can probably remove the workaround we have to address > > the issue we > > were facing during snd_pcm_period_elapsed(). > > > > I can give this a try. We might need to run some stress tests to > > make sure it > > doesn't break anything. > > If this helps for SOF, it'd be great. I have converted only non-ASoC > drivers regarding the sync-stop stuff, so it won't conflict my > upcoming changes :) > Hi Takashi, I just realized that In the SOF driver, we only set the component driver ops. The pcm ops are set when creating the new pcm. So, should I also add the sync_stop op in the component driver and set the pcm sync_stop op to point to the component sync_stop op? Just wanted to confirm if I am on the right track. Thanks, Ranjani
On Tue, 19 Nov 2019 00:47:53 +0100, Ranjani Sridharan wrote: > > On Mon, 2019-11-18 at 21:40 +0100, Takashi Iwai wrote: > > On Mon, 18 Nov 2019 20:55:19 +0100, > > Sridharan, Ranjani wrote: > > > > > > > Thanks for the clarification, Takashi. But just wondering how > > > would one > > > pass > > > > on the sync_irq when the snd_card is created? Typically in > > > the case of > > > the > > > > Intel platforms, the card->dev points to the platform device > > > for the > > > machine > > > > driver that registers the card and the PCI device is the > > > parent of the > > > machine > > > > drv platform device. > > > > > > It's completely up to the driver implementation :) > > > You can implement the own sync_stop ops if that's easier, too. > > > > > > I think this would make sense in the case of the SOF driver and > > > we'd probably > > > need to just call synchronize_irq() in the sync_stop() operation. > > > With this > > > change, we can probably remove the workaround we have to address > > > the issue we > > > were facing during snd_pcm_period_elapsed(). > > > > > > I can give this a try. We might need to run some stress tests to > > > make sure it > > > doesn't break anything. > > > > If this helps for SOF, it'd be great. I have converted only non-ASoC > > drivers regarding the sync-stop stuff, so it won't conflict my > > upcoming changes :) > > > Hi Takashi, > > I just realized that In the SOF driver, we only set the component > driver ops. The pcm ops are set when creating the new pcm. So, should I > also add the sync_stop op in the component driver and set the pcm > sync_stop op to point to the component sync_stop op? Just wanted to > confirm if I am on the right track. Yes, I didn't touch this yet, but that's the way to go I suppose. One caveat is that this ops is optional and needs NULL as default, hence you'd need to set only when defined, like copy_user, page or mmap ops, at least. thanks, Takashi
> > > > Hi Takashi, > > > > I just realized that In the SOF driver, we only set the component > > driver ops. The pcm ops are set when creating the new pcm. So, > > should I > > also add the sync_stop op in the component driver and set the pcm > > sync_stop op to point to the component sync_stop op? Just wanted to > > confirm if I am on the right track. > > Yes, I didn't touch this yet, but that's the way to go I suppose. > One caveat is that this ops is optional and needs NULL as default, > hence you'd need to set only when defined, like copy_user, page or > mmap ops, at least. Hi Takashi, This is what I tried in the SOF driver: https://github.com/thesofproject/linux/pull/1513/commits And it seems to cause the system to hang when I stop the stream and I have no meaningful logs to pinpoint to the problem. Could you please have a look at the 4 commits that I have added to your series and let me know what I could be missing? Thanks, Ranjani > > > thanks, > > Takashi > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Tue, 19 Nov 2019 08:40:25 +0100, Ranjani Sridharan wrote: > > > > > > > Hi Takashi, > > > > > > I just realized that In the SOF driver, we only set the component > > > driver ops. The pcm ops are set when creating the new pcm. So, > > > should I > > > also add the sync_stop op in the component driver and set the pcm > > > sync_stop op to point to the component sync_stop op? Just wanted to > > > confirm if I am on the right track. > > > > Yes, I didn't touch this yet, but that's the way to go I suppose. > > One caveat is that this ops is optional and needs NULL as default, > > hence you'd need to set only when defined, like copy_user, page or > > mmap ops, at least. > Hi Takashi, > > This is what I tried in the SOF driver: > https://github.com/thesofproject/linux/pull/1513/commits > > And it seems to cause the system to hang when I stop the stream and I > have no meaningful logs to pinpoint to the problem. Could you please > have a look at the 4 commits that I have added to your series and let > me know what I could be missing? I couldn't find anything obvious. Could you try without changing snd_sof_pcm_period_elapsed(), i.e. only adding the stuff and calling sync_stop, in order to see whether the additional stuff broke anything? thanks, Takashi
On Tue, 19 Nov 2019 09:24:33 +0100, Takashi Iwai wrote: > > On Tue, 19 Nov 2019 08:40:25 +0100, > Ranjani Sridharan wrote: > > > > > > > > > > Hi Takashi, > > > > > > > > I just realized that In the SOF driver, we only set the component > > > > driver ops. The pcm ops are set when creating the new pcm. So, > > > > should I > > > > also add the sync_stop op in the component driver and set the pcm > > > > sync_stop op to point to the component sync_stop op? Just wanted to > > > > confirm if I am on the right track. > > > > > > Yes, I didn't touch this yet, but that's the way to go I suppose. > > > One caveat is that this ops is optional and needs NULL as default, > > > hence you'd need to set only when defined, like copy_user, page or > > > mmap ops, at least. > > Hi Takashi, > > > > This is what I tried in the SOF driver: > > https://github.com/thesofproject/linux/pull/1513/commits > > > > And it seems to cause the system to hang when I stop the stream and I > > have no meaningful logs to pinpoint to the problem. Could you please > > have a look at the 4 commits that I have added to your series and let > > me know what I could be missing? > > I couldn't find anything obvious. Could you try without changing > snd_sof_pcm_period_elapsed(), i.e. only adding the stuff and calling > sync_stop, in order to see whether the additional stuff broke > anything? ... and looking at the code again, I don't think the stop_sync ops would help in your case. The PCM ops is called *after* trigger-STOP is issued, while your case requires the serialization of trigger call itself. But I'm still wondering whether the current implementation is safe, too. Basically the scheduled work may be executed immediately after queuing, so if it's about the timing issue, it's not solid solution. Pushing to work helps if it's about the locking issue, though. Takashi
On Tue, 2019-11-19 at 09:24 +0100, Takashi Iwai wrote: > On Tue, 19 Nov 2019 08:40:25 +0100, > Ranjani Sridharan wrote: > > > > > > > > > > Hi Takashi, > > > > > > > > I just realized that In the SOF driver, we only set the > > > > component > > > > driver ops. The pcm ops are set when creating the new pcm. So, > > > > should I > > > > also add the sync_stop op in the component driver and set the > > > > pcm > > > > sync_stop op to point to the component sync_stop op? Just > > > > wanted to > > > > confirm if I am on the right track. > > > > > > Yes, I didn't touch this yet, but that's the way to go I suppose. > > > One caveat is that this ops is optional and needs NULL as > > > default, > > > hence you'd need to set only when defined, like copy_user, page > > > or > > > mmap ops, at least. > > > > Hi Takashi, > > > > This is what I tried in the SOF driver: > > https://github.com/thesofproject/linux/pull/1513/commits > > > > And it seems to cause the system to hang when I stop the stream and > > I > > have no meaningful logs to pinpoint to the problem. Could you > > please > > have a look at the 4 commits that I have added to your series and > > let > > me know what I could be missing? > > I couldn't find anything obvious. Could you try without changing > snd_sof_pcm_period_elapsed(), i.e. only adding the stuff and calling > sync_stop, in order to see whether the additional stuff broke > anything? It is indeed the removal of snd_sof_pcm_period_elapsed() that makes the device hang when the stream is stoppped. But that's a bit surprising given that all I tried was using the snd_pcm_period_elapsed() directly instead of scheduling the delayed work to call it. Thanks, Ranjani
On Tue, 19 Nov 2019 17:36:46 +0100, Ranjani Sridharan wrote: > > On Tue, 2019-11-19 at 09:24 +0100, Takashi Iwai wrote: > > On Tue, 19 Nov 2019 08:40:25 +0100, > > Ranjani Sridharan wrote: > > > > > > > > > > > > > Hi Takashi, > > > > > > > > > > I just realized that In the SOF driver, we only set the > > > > > component > > > > > driver ops. The pcm ops are set when creating the new pcm. So, > > > > > should I > > > > > also add the sync_stop op in the component driver and set the > > > > > pcm > > > > > sync_stop op to point to the component sync_stop op? Just > > > > > wanted to > > > > > confirm if I am on the right track. > > > > > > > > Yes, I didn't touch this yet, but that's the way to go I suppose. > > > > One caveat is that this ops is optional and needs NULL as > > > > default, > > > > hence you'd need to set only when defined, like copy_user, page > > > > or > > > > mmap ops, at least. > > > > > > Hi Takashi, > > > > > > This is what I tried in the SOF driver: > > > https://github.com/thesofproject/linux/pull/1513/commits > > > > > > And it seems to cause the system to hang when I stop the stream and > > > I > > > have no meaningful logs to pinpoint to the problem. Could you > > > please > > > have a look at the 4 commits that I have added to your series and > > > let > > > me know what I could be missing? > > > > I couldn't find anything obvious. Could you try without changing > > snd_sof_pcm_period_elapsed(), i.e. only adding the stuff and calling > > sync_stop, in order to see whether the additional stuff broke > > anything? > It is indeed the removal of snd_sof_pcm_period_elapsed() that makes the > device hang when the stream is stoppped. But that's a bit surprising > given that all I tried was using the snd_pcm_period_elapsed() directly > instead of scheduling the delayed work to call it. If I read the code correctly, this can't work irrelevantly from the sync_stop stuff. The call of period_elapsed is from hda_dsp_stream_check() which is performed in bus->reg_lock spinlock in hda_dsp_stream_threaded_handler(). Meanwhile, the XRUN trigger goes to hda_dsp_pcm_trigger() that follows hda_dsp_stream_trigger(), and this function expects the sleepable context due to snd_sof_dsp_read_poll_timeout() call. So something like below works? Takashi --- a/sound/soc/sof/intel/hda-stream.c +++ b/sound/soc/sof/intel/hda-stream.c @@ -592,8 +592,11 @@ static bool hda_dsp_stream_check(struct hdac_bus *bus, u32 status) continue; /* Inform ALSA only in case not do that with IPC */ - if (sof_hda->no_ipc_position) - snd_sof_pcm_period_elapsed(s->substream); + if (sof_hda->no_ipc_position) { + spin_unlock_irq(&bus->reg_lock); + snd_pcm_period_elapsed(s->substream); + spin_lock_irq(&bus->reg_lock); + } } }
On Tue, Nov 19, 2019 at 1:28 PM Takashi Iwai <tiwai@suse.de> wrote: > On Tue, 19 Nov 2019 17:36:46 +0100, > Ranjani Sridharan wrote: > > > > On Tue, 2019-11-19 at 09:24 +0100, Takashi Iwai wrote: > > > On Tue, 19 Nov 2019 08:40:25 +0100, > > > Ranjani Sridharan wrote: > > > > > > > > > > > > > > > > Hi Takashi, > > > > > > > > > > > > I just realized that In the SOF driver, we only set the > > > > > > component > > > > > > driver ops. The pcm ops are set when creating the new pcm. So, > > > > > > should I > > > > > > also add the sync_stop op in the component driver and set the > > > > > > pcm > > > > > > sync_stop op to point to the component sync_stop op? Just > > > > > > wanted to > > > > > > confirm if I am on the right track. > > > > > > > > > > Yes, I didn't touch this yet, but that's the way to go I suppose. > > > > > One caveat is that this ops is optional and needs NULL as > > > > > default, > > > > > hence you'd need to set only when defined, like copy_user, page > > > > > or > > > > > mmap ops, at least. > > > > > > > > Hi Takashi, > > > > > > > > This is what I tried in the SOF driver: > > > > https://github.com/thesofproject/linux/pull/1513/commits > > > > > > > > And it seems to cause the system to hang when I stop the stream and > > > > I > > > > have no meaningful logs to pinpoint to the problem. Could you > > > > please > > > > have a look at the 4 commits that I have added to your series and > > > > let > > > > me know what I could be missing? > > > > > > I couldn't find anything obvious. Could you try without changing > > > snd_sof_pcm_period_elapsed(), i.e. only adding the stuff and calling > > > sync_stop, in order to see whether the additional stuff broke > > > anything? > > It is indeed the removal of snd_sof_pcm_period_elapsed() that makes the > > device hang when the stream is stoppped. But that's a bit surprising > > given that all I tried was using the snd_pcm_period_elapsed() directly > > instead of scheduling the delayed work to call it. > > If I read the code correctly, this can't work irrelevantly from the > sync_stop stuff. The call of period_elapsed is from > hda_dsp_stream_check() which is performed in bus->reg_lock spinlock in > hda_dsp_stream_threaded_handler(). Meanwhile, the XRUN trigger goes > to hda_dsp_pcm_trigger() that follows hda_dsp_stream_trigger(), and > this function expects the sleepable context due to > snd_sof_dsp_read_poll_timeout() call. > > So something like below works? > > > Takashi > > --- a/sound/soc/sof/intel/hda-stream.c > +++ b/sound/soc/sof/intel/hda-stream.c > @@ -592,8 +592,11 @@ static bool hda_dsp_stream_check(struct hdac_bus > *bus, u32 status) > continue; > > /* Inform ALSA only in case not do that with IPC */ > - if (sof_hda->no_ipc_position) > - snd_sof_pcm_period_elapsed(s->substream); > + if (sof_hda->no_ipc_position) { > + spin_unlock_irq(&bus->reg_lock); > + snd_pcm_period_elapsed(s->substream); > + spin_lock_irq(&bus->reg_lock); > Thanks, Takashi. Yes, I realized it this morning as well that it is due to the reg_lock. It does work with this change now. I will run some stress tests with this change and get back with the results. Thanks, Ranjani
> > > > >> > > I couldn't find anything obvious. Could you try without changing >> > > snd_sof_pcm_period_elapsed(), i.e. only adding the stuff and calling >> > > sync_stop, in order to see whether the additional stuff broke >> > > anything? >> > It is indeed the removal of snd_sof_pcm_period_elapsed() that makes the >> > device hang when the stream is stoppped. But that's a bit surprising >> > given that all I tried was using the snd_pcm_period_elapsed() directly >> > instead of scheduling the delayed work to call it. >> >> If I read the code correctly, this can't work irrelevantly from the >> sync_stop stuff. The call of period_elapsed is from >> hda_dsp_stream_check() which is performed in bus->reg_lock spinlock in >> hda_dsp_stream_threaded_handler(). Meanwhile, the XRUN trigger goes >> to hda_dsp_pcm_trigger() that follows hda_dsp_stream_trigger(), and >> this function expects the sleepable context due to >> snd_sof_dsp_read_poll_timeout() call. >> >> So something like below works? >> >> >> Takashi >> >> --- a/sound/soc/sof/intel/hda-stream.c >> +++ b/sound/soc/sof/intel/hda-stream.c >> @@ -592,8 +592,11 @@ static bool hda_dsp_stream_check(struct hdac_bus >> *bus, u32 status) >> continue; >> >> /* Inform ALSA only in case not do that with IPC >> */ >> - if (sof_hda->no_ipc_position) >> - snd_sof_pcm_period_elapsed(s->substream); >> + if (sof_hda->no_ipc_position) { >> + spin_unlock_irq(&bus->reg_lock); >> + snd_pcm_period_elapsed(s->substream); >> + spin_lock_irq(&bus->reg_lock); >> > Thanks, Takashi. Yes, I realized it this morning as well that it is due to > the reg_lock. It does work with this change now. I will run some stress > tests with this change and get back with the results. > Hi Takashi, Sorry the stress tests took a while. As we discussed earlier, adding the sync_stop() op didnt quite help the SOF driver in removing the delayed work for snd_pcm_period_elapsed(). Thanks, Ranjani > > Thanks, > Ranjani >
On Thu, 21 Nov 2019 20:22:03 +0100, Sridharan, Ranjani wrote: > > > > > > > I couldn't find anything obvious. Could you try without changing > > > snd_sof_pcm_period_elapsed(), i.e. only adding the stuff and > calling > > > sync_stop, in order to see whether the additional stuff broke > > > anything? > > It is indeed the removal of snd_sof_pcm_period_elapsed() that makes > the > > device hang when the stream is stoppped. But that's a bit surprising > > given that all I tried was using the snd_pcm_period_elapsed() > directly > > instead of scheduling the delayed work to call it. > > If I read the code correctly, this can't work irrelevantly from the > sync_stop stuff. The call of period_elapsed is from > hda_dsp_stream_check() which is performed in bus->reg_lock spinlock in > hda_dsp_stream_threaded_handler(). Meanwhile, the XRUN trigger goes > to hda_dsp_pcm_trigger() that follows hda_dsp_stream_trigger(), and > this function expects the sleepable context due to > snd_sof_dsp_read_poll_timeout() call. > > So something like below works? > > Takashi > > --- a/sound/soc/sof/intel/hda-stream.c > +++ b/sound/soc/sof/intel/hda-stream.c > @@ -592,8 +592,11 @@ static bool hda_dsp_stream_check(struct hdac_bus > *bus, u32 status) > continue; > > /* Inform ALSA only in case not do that with > IPC */ > - if (sof_hda->no_ipc_position) > - snd_sof_pcm_period_elapsed(s-> > substream); > + if (sof_hda->no_ipc_position) { > + spin_unlock_irq(&bus->reg_lock); > + snd_pcm_period_elapsed(s->substream); > + spin_lock_irq(&bus->reg_lock); > > Thanks, Takashi. Yes, I realized it this morning as well that it is due to > the reg_lock. It does work with this change now. I will run some stress > tests with this change and get back with the results. > > Hi Takashi, > > Sorry the stress tests took a while. > As we discussed earlier, adding the sync_stop() op didnt quite help the SOF > driver in removing the delayed work for snd_pcm_period_elapsed(). Yeah, that's understandable. If the stop operation itself needs some serialization, sync_stop() won't influence at all. However, now after these discussions, I have some concerns in the current code: - The async work started by schedule_work() may be executed (literally) immediately. So if the timing or the serialization matters, it doesn't guarantee at all. The same level of concurrency can happen at any time. - The period_elapsed work might be pending at prepare or other operation; the async work means also that it doesn't guarantee its execution in time, and it might be delayed much, and the PCM core might go to prepare or other state even before the work is executed. The second point can be fixed easily now with sync_stop. You can just put flush_work() in sync_stop in addition to synchronize_irq(). But the first point is still unclear. More exactly, which operation does it conflict? Does it the playback drain? Then it might take very long (up to seconds) to block the next operation? thanks, Takashi
> > > > > > Hi Takashi, > > > > Sorry the stress tests took a while. > > As we discussed earlier, adding the sync_stop() op didnt quite help the > SOF > > driver in removing the delayed work for snd_pcm_period_elapsed(). > > Yeah, that's understandable. If the stop operation itself needs some > serialization, sync_stop() won't influence at all. > > However, now after these discussions, I have some concerns in the > current code: > > - The async work started by schedule_work() may be executed > (literally) immediately. So if the timing or the serialization > matters, it doesn't guarantee at all. The same level of concurrency > can happen at any time. > > - The period_elapsed work might be pending at prepare or other > operation; > the async work means also that it doesn't guarantee its execution in > time, and it might be delayed much, and the PCM core might go to > prepare or other state even before the work is executed. > > The second point can be fixed easily now with sync_stop. You can just > put flush_work() in sync_stop in addition to synchronize_irq(). > > But the first point is still unclear. More exactly, which operation > does it conflict? Does it the playback drain? Then it might take > very long (up to seconds) to block the next operation? > Hi Takashi, As I understand the original intention for adding the period_elapsed_work() was that snd_pcm_period_elapsed() could cause a STOP trigger while the current IPC interrupt is still being handled. In this case, the STOP trigger generates an IPC to the DSP but the host never misses the IPC response from the DSP because it is still handling the previous interrupt. Adding Keyon who added this change to add more and clarify your concerns. Thanks, Ranjani > > > thanks, > > Takashi >
On Thu, 21 Nov 2019 21:46:17 +0100, Sridharan, Ranjani wrote: > > > > > Hi Takashi, > > > > Sorry the stress tests took a while. > > As we discussed earlier, adding the sync_stop() op didnt quite help the > SOF > > driver in removing the delayed work for snd_pcm_period_elapsed(). > > Yeah, that's understandable. If the stop operation itself needs some > serialization, sync_stop() won't influence at all. > > However, now after these discussions, I have some concerns in the > current code: > > - The async work started by schedule_work() may be executed > (literally) immediately. So if the timing or the serialization > matters, it doesn't guarantee at all. The same level of concurrency > can happen at any time. > > - The period_elapsed work might be pending at prepare or other > operation; > the async work means also that it doesn't guarantee its execution in > time, and it might be delayed much, and the PCM core might go to > prepare or other state even before the work is executed. > > The second point can be fixed easily now with sync_stop. You can just > put flush_work() in sync_stop in addition to synchronize_irq(). > > But the first point is still unclear. More exactly, which operation > does it conflict? Does it the playback drain? Then it might take > very long (up to seconds) to block the next operation? > > Hi Takashi, > > As I understand the original intention for adding the period_elapsed_work() > was that snd_pcm_period_elapsed() could cause a STOP trigger while the > current IPC interrupt is still being handled. > In this case, the STOP trigger generates an IPC to the DSP but the host never > misses the IPC response from the DSP because it is still handling the previous > interrupt. OK, that makes sense. So the issue is that the trigger stop itself requires the ack via the interrupt and it can't be caught because it's being called from the irq handler itself. In that case, though, another solution would be to make the trigger- stop an async work (but conditionally) while processing the normal period_elapsed in the irq handler. That is, set some flag before calling snd_pcm_period_elapsed(), and in the trigger-stop, check the flag. If the flag is set, schedule the work and return. And, you'll sync this async work with sync_stop(). In that way, the period handling is processed without any delay more lightly. thanks, Takashi > Adding Keyon who added this change to add more and clarify your concerns. > > Thanks, > Ranjani > > thanks, > > Takashi > >
On Thu, Nov 21, 2019 at 1:13 PM Takashi Iwai <tiwai@suse.de> wrote: > On Thu, 21 Nov 2019 21:46:17 +0100, > Sridharan, Ranjani wrote: > > > > > > > > Hi Takashi, > > > > > > Sorry the stress tests took a while. > > > As we discussed earlier, adding the sync_stop() op didnt quite > help the > > SOF > > > driver in removing the delayed work for snd_pcm_period_elapsed(). > > > > Yeah, that's understandable. If the stop operation itself needs some > > serialization, sync_stop() won't influence at all. > > > > However, now after these discussions, I have some concerns in the > > current code: > > > > - The async work started by schedule_work() may be executed > > (literally) immediately. So if the timing or the serialization > > matters, it doesn't guarantee at all. The same level of > concurrency > > can happen at any time. > > > > - The period_elapsed work might be pending at prepare or other > > operation; > > the async work means also that it doesn't guarantee its execution > in > > time, and it might be delayed much, and the PCM core might go to > > prepare or other state even before the work is executed. > > > > The second point can be fixed easily now with sync_stop. You can > just > > put flush_work() in sync_stop in addition to synchronize_irq(). > > > > But the first point is still unclear. More exactly, which operation > > does it conflict? Does it the playback drain? Then it might take > > very long (up to seconds) to block the next operation? > > > > Hi Takashi, > > > > As I understand the original intention for adding the > period_elapsed_work() > > was that snd_pcm_period_elapsed() could cause a STOP trigger while the > > current IPC interrupt is still being handled. > > In this case, the STOP trigger generates an IPC to the DSP but the host > never > > misses the IPC response from the DSP because it is still handling the > previous > > interrupt. > > OK, that makes sense. So the issue is that the trigger stop itself > requires the ack via the interrupt and it can't be caught because it's > being called from the irq handler itself. > > In that case, though, another solution would be to make the trigger- > stop an async work (but conditionally) while processing the normal > period_elapsed in the irq handler. That is, set some flag before > calling snd_pcm_period_elapsed(), and in the trigger-stop, check the > flag. If the flag is set, schedule the work and return. And, you'll > sync this async work with sync_stop(). In that way, the period > handling is processed without any delay more lightly. > OK, that makes sense. Thanks for the suggestion. Regarding your previous comment about adding flush_work() to the sync_stop() op, would that still be required? Thanks, Ranjani > > > thanks, > > Takashi > > > Adding Keyon who added this change to add more and clarify your concerns. > > > > Thanks, > > Ranjani > > > > thanks, > > > > Takashi > > > > >
On Thu, 21 Nov 2019 22:17:41 +0100, Sridharan, Ranjani wrote: > > On Thu, Nov 21, 2019 at 1:13 PM Takashi Iwai <tiwai@suse.de> wrote: > > On Thu, 21 Nov 2019 21:46:17 +0100, > Sridharan, Ranjani wrote: > > > > > > > > Hi Takashi, > > > > > > Sorry the stress tests took a while. > > > As we discussed earlier, adding the sync_stop() op didnt quite > help the > > SOF > > > driver in removing the delayed work for snd_pcm_period_elapsed(). > > > > Yeah, that's understandable. If the stop operation itself needs > some > > serialization, sync_stop() won't influence at all. > > > > However, now after these discussions, I have some concerns in the > > current code: > > > > - The async work started by schedule_work() may be executed > > (literally) immediately. So if the timing or the serialization > > matters, it doesn't guarantee at all. The same level of > concurrency > > can happen at any time. > > > > - The period_elapsed work might be pending at prepare or other > > operation; > > the async work means also that it doesn't guarantee its execution > in > > time, and it might be delayed much, and the PCM core might go to > > prepare or other state even before the work is executed. > > > > The second point can be fixed easily now with sync_stop. You can > just > > put flush_work() in sync_stop in addition to synchronize_irq(). > > > > But the first point is still unclear. More exactly, which operation > > does it conflict? Does it the playback drain? Then it might take > > very long (up to seconds) to block the next operation? > > > > Hi Takashi, > > > > As I understand the original intention for adding the > period_elapsed_work() > > was that snd_pcm_period_elapsed() could cause a STOP trigger while the > > current IPC interrupt is still being handled. > > In this case, the STOP trigger generates an IPC to the DSP but the host > never > > misses the IPC response from the DSP because it is still handling the > previous > > interrupt. > > OK, that makes sense. So the issue is that the trigger stop itself > requires the ack via the interrupt and it can't be caught because it's > being called from the irq handler itself. > > In that case, though, another solution would be to make the trigger- > stop an async work (but conditionally) while processing the normal > period_elapsed in the irq handler. That is, set some flag before > calling snd_pcm_period_elapsed(), and in the trigger-stop, check the > flag. If the flag is set, schedule the work and return. And, you'll > sync this async work with sync_stop(). In that way, the period > handling is processed without any delay more lightly. > > OK, that makes sense. Thanks for the suggestion. > Regarding your previous comment about adding flush_work() to the sync_stop() > op, would that still be required? Yes, that's needed no matter which way is used; the pending work must be synced at some point. Takashi
> > > > > OK, that makes sense. Thanks for the suggestion. > > Regarding your previous comment about adding flush_work() to the > sync_stop() > > op, would that still be required? > > Yes, that's needed no matter which way is used; the pending work must > be synced at some point. > Thanks, Takashi. I will follow up with the suggested SOF changes after your patches have been applied. Thanks, Ranjani
>-----Original Message----- >From: Takashi Iwai <tiwai@suse.de> >Sent: Thursday, November 21, 2019 1:13 PM >To: Sridharan, Ranjani <ranjani.sridharan@intel.com> >Cc: Jie, Yang <yang.jie@intel.com>; Ranjani Sridharan ><ranjani.sridharan@linux.intel.com>; Linux-ALSA <alsa-devel@alsa-project.org>; >Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> >Subject: Re: [alsa-devel] [PATCH 7/8] ALSA: pcm: Add card sync_irq field > >On Thu, 21 Nov 2019 21:46:17 +0100, >Sridharan, Ranjani wrote: >> >> > >> > Hi Takashi, >> > >> > Sorry the stress tests took a while. >> > As we discussed earlier, adding the sync_stop() op didnt quite help the >> SOF >> > driver in removing the delayed work for >> snd_pcm_period_elapsed(). >> >> Yeah, that's understandable. If the stop operation itself needs some >> serialization, sync_stop() won't influence at all. >> >> However, now after these discussions, I have some concerns in the >> current code: >> >> - The async work started by schedule_work() may be executed >> (literally) immediately. So if the timing or the serialization >> matters, it doesn't guarantee at all. The same level of concurrency >> can happen at any time. >> >> - The period_elapsed work might be pending at prepare or other >> operation; >> the async work means also that it doesn't guarantee its execution in >> time, and it might be delayed much, and the PCM core might go to >> prepare or other state even before the work is executed. >> >> The second point can be fixed easily now with sync_stop. You can just >> put flush_work() in sync_stop in addition to synchronize_irq(). >> >> But the first point is still unclear. More exactly, which operation >> does it conflict? Does it the playback drain? Then it might take >> very long (up to seconds) to block the next operation? >> >> Hi Takashi, >> >> As I understand the original intention for adding the >> period_elapsed_work() was that snd_pcm_period_elapsed() could cause a >> STOP trigger while the current IPC interrupt is still being handled. >> In this case, the STOP trigger generates an IPC to the DSP but the >> host never misses the IPC response from the DSP because it is still >> handling the previous interrupt. > >OK, that makes sense. So the issue is that the trigger stop itself requires the ack >via the interrupt and it can't be caught because it's being called from the irq >handler itself. > >In that case, though, another solution would be to make the trigger- stop an >async work (but conditionally) while processing the normal period_elapsed in the >irq handler. That is, set some flag before calling snd_pcm_period_elapsed(), and >in the trigger-stop, check the flag. If the flag is set, schedule the work and return. >And, you'll sync this async work with sync_stop(). In that way, the period handling >is processed without any delay more lightly. Yes, this was actually the method @Uimonen, Jaska suggested on April, since we have sync_stop() to flush the potential un-finished async trigger_stop task now, it's time to switch to use this solution now. Thanks, ~Keyon > > >thanks, > >Takashi > >> Adding Keyon who added this change to add more and clarify your concerns. >> >> Thanks, >> Ranjani >> >> thanks, >> >> Takashi >> >>
diff --git a/include/sound/core.h b/include/sound/core.h index ee238f100f73..af3dce956c17 100644 --- a/include/sound/core.h +++ b/include/sound/core.h @@ -117,6 +117,7 @@ struct snd_card { struct device card_dev; /* cardX object for sysfs */ const struct attribute_group *dev_groups[4]; /* assigned sysfs attr */ bool registered; /* card_dev is registered? */ + int sync_irq; /* assigned irq, used for PCM sync */ wait_queue_head_t remove_sleep; #ifdef CONFIG_PM diff --git a/sound/core/init.c b/sound/core/init.c index db99b7fad6ad..faa9f03c01ca 100644 --- a/sound/core/init.c +++ b/sound/core/init.c @@ -215,6 +215,7 @@ int snd_card_new(struct device *parent, int idx, const char *xid, init_waitqueue_head(&card->power_sleep); #endif init_waitqueue_head(&card->remove_sleep); + card->sync_irq = -1; device_initialize(&card->card_dev); card->card_dev.parent = parent; diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 163d621ff238..1fe581167b7b 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -574,6 +574,8 @@ static void snd_pcm_sync_stop(struct snd_pcm_substream *substream) substream->runtime->stop_operating = false; if (substream->ops->sync_stop) substream->ops->sync_stop(substream); + else if (substream->pcm->card->sync_irq > 0) + synchronize_irq(substream->pcm->card->sync_irq); } }
Many PCI and other drivers performs snd_pcm_period_elapsed() simply in its interrupt handler, so the sync_stop operation is just to call synchronize_irq(). Instead of putting this call multiple times, introduce the common card->sync_irq field. When this field is set, PCM core performs synchronize_irq() for sync-stop operation. Each driver just needs to copy its local IRQ number to card->sync_irq, and that's all we need. Signed-off-by: Takashi Iwai <tiwai@suse.de> --- include/sound/core.h | 1 + sound/core/init.c | 1 + sound/core/pcm_native.c | 2 ++ 3 files changed, 4 insertions(+)