[7/8] ALSA: pcm: Add card sync_irq field
diff mbox series

Message ID 20191117085308.23915-8-tiwai@suse.de
State New
Headers show
Series
  • ALSA: pcm: API cleanups and extensions
Related show

Commit Message

Takashi Iwai Nov. 17, 2019, 8:53 a.m. UTC
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(+)

Comments

Pierre-Louis Bossart Nov. 18, 2019, 4:38 p.m. UTC | #1
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);
>   	}
>   }
>   
>
Takashi Iwai Nov. 18, 2019, 6:52 p.m. UTC | #2
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
Sridharan, Ranjani Nov. 18, 2019, 7:20 p.m. UTC | #3
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
Takashi Iwai Nov. 18, 2019, 7:49 p.m. UTC | #4
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
Sridharan, Ranjani Nov. 18, 2019, 7:55 p.m. UTC | #5
>
> >
> > 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
Takashi Iwai Nov. 18, 2019, 8:40 p.m. UTC | #6
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
Ranjani Sridharan Nov. 18, 2019, 11:47 p.m. UTC | #7
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
Takashi Iwai Nov. 19, 2019, 6:44 a.m. UTC | #8
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
Ranjani Sridharan Nov. 19, 2019, 7:40 a.m. UTC | #9
> > 
> > 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
Takashi Iwai Nov. 19, 2019, 8:24 a.m. UTC | #10
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
Takashi Iwai Nov. 19, 2019, 9:39 a.m. UTC | #11
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
Ranjani Sridharan Nov. 19, 2019, 4:36 p.m. UTC | #12
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
Takashi Iwai Nov. 19, 2019, 9:27 p.m. UTC | #13
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);
+			}
 		}
 	}
Sridharan, Ranjani Nov. 19, 2019, 9:43 p.m. UTC | #14
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
Sridharan, Ranjani Nov. 21, 2019, 7:22 p.m. UTC | #15
>
> > >
>> > > 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
>
Takashi Iwai Nov. 21, 2019, 8:34 p.m. UTC | #16
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
Sridharan, Ranjani Nov. 21, 2019, 8:46 p.m. UTC | #17
>
>
> >
> > 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
>
Takashi Iwai Nov. 21, 2019, 9:13 p.m. UTC | #18
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
> 
>
Sridharan, Ranjani Nov. 21, 2019, 9:17 p.m. UTC | #19
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
> >
> >
>
Takashi Iwai Nov. 21, 2019, 9:28 p.m. UTC | #20
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
Sridharan, Ranjani Nov. 21, 2019, 9:45 p.m. UTC | #21
>
> >
> > 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
Jie, Yang Nov. 22, 2019, 4:08 a.m. UTC | #22
>-----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
>>
>>

Patch
diff mbox series

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);
 	}
 }