Anton Yakovlev
2021-Mar-01 15:30 UTC
[PATCH v6 9/9] ALSA: virtio: introduce device suspend/resume support
On 01.03.2021 14:38, Takashi Iwai wrote:> On Mon, 01 Mar 2021 11:03:04 +0100, > Anton Yakovlev wrote: >> >> On 28.02.2021 13:05, Takashi Iwai wrote: >>> On Sat, 27 Feb 2021 09:59:56 +0100, >>> Anton Yakovlev wrote: >>>> >>>> All running PCM substreams are stopped on device suspend and restarted >>>> on device resume. >>>> >>>> Signed-off-by: Anton Yakovlev <anton.yakovlev at opensynergy.com> >>>> --- >>>> sound/virtio/virtio_card.c | 56 +++++++++++++++++++++++++++++++++++ >>>> sound/virtio/virtio_pcm.c | 1 + >>>> sound/virtio/virtio_pcm_ops.c | 41 ++++++++++++++++++++----- >>>> 3 files changed, 90 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/sound/virtio/virtio_card.c b/sound/virtio/virtio_card.c >>>> index 59455a562018..c7ae8801991d 100644 >>>> --- a/sound/virtio/virtio_card.c >>>> +++ b/sound/virtio/virtio_card.c >>>> @@ -323,6 +323,58 @@ static void virtsnd_remove(struct virtio_device *vdev) >>>> kfree(snd->event_msgs); >>>> } >>>> >>>> +#ifdef CONFIG_PM_SLEEP >>>> +/** >>>> + * virtsnd_freeze() - Suspend device. >>>> + * @vdev: VirtIO parent device. >>>> + * >>>> + * Context: Any context. >>>> + * Return: 0 on success, -errno on failure. >>>> + */ >>>> +static int virtsnd_freeze(struct virtio_device *vdev) >>>> +{ >>>> + struct virtio_snd *snd = vdev->priv; >>>> + >>>> + virtsnd_ctl_msg_cancel_all(snd); >>>> + >>>> + vdev->config->del_vqs(vdev); >>>> + vdev->config->reset(vdev); >>>> + >>>> + kfree(snd->event_msgs); >>>> + >>>> + /* >>>> + * If the virtsnd_restore() fails before re-allocating events, then we >>>> + * get a dangling pointer here. >>>> + */ >>>> + snd->event_msgs = NULL; >>>> + >>>> + return 0; >>> >>> I suppose some cancel of inflight works is needed? >>> Ditto for the device removal, too. >> >> It's not necessary here, since the device is reset and all of this are >> happened automatically. > > Hrm, but the reset call itself might conflict with the inflight reset > work? I haven't see any work canceling or flushing, so...There maybe the following: 1. Some pending control requests -> these are cancelled in the virtsnd_ctl_msg_cancel_all() call. 2. PCM messages -> these must not be cancelled, since they will be requeued by driver on resume (starting with suspended position). 3. Some pending events from the device. These will be lost. Yeah, I think we can process all pending events before destroying virtqueue. Other that these, there are no other inflight works or so.>> But in the device remove it makes sense also to >> disable events before calling snd_card_free(), since the device is still >> able to send notifications at that moment. Thanks! >> >> >>>> --- a/sound/virtio/virtio_pcm.c >>>> +++ b/sound/virtio/virtio_pcm.c >>>> @@ -109,6 +109,7 @@ static int virtsnd_pcm_build_hw(struct virtio_pcm_substream *vss, >>>> SNDRV_PCM_INFO_BATCH | >>>> SNDRV_PCM_INFO_BLOCK_TRANSFER | >>>> SNDRV_PCM_INFO_INTERLEAVED | >>>> + SNDRV_PCM_INFO_RESUME | >>>> SNDRV_PCM_INFO_PAUSE; >>> >>> Actually you don't need to set SNDRV_PCM_INFO_RESUME. >>> This flag means that the driver supports the full resume procedure, >>> which isn't often the case; with this, the driver is supposed to >>> resume the stream exactly from the suspended position. >> >> If I understood you right, that's exactly how resume is implemented now >> in the driver. Although we fully restart substream on the device side, >> from an application point of view it is resumed exactly at the same >> position. >> >> >>> Most drivers don't set this but implement only the suspend-stop >>> action. Then the application (or the sound backend) will re-setup the >>> stream and restart accordingly. >> >> And an application must be aware of such possible situation? Since I >> have no doubt in alsa-lib, but I don't think that for example tinyalsa >> can handle this right. > > Tiny ALSA should work, too. Actually there are only few drivers that > have the full PCM resume. The majority of drivers are without the > resume support (including a large one like HD-audio).Then it's a great news! Since we can simplify code a lot.> And, with the resume implementation, I'm worried by the style like: > >>>> @@ -309,6 +318,21 @@ static int virtsnd_pcm_trigger(struct snd_pcm_substream *substream, int command) >>>> int rc; >>>> >>>> switch (command) { >>>> + case SNDRV_PCM_TRIGGER_RESUME: { >>>> + /* >>>> + * We restart the substream by executing the standard command >>>> + * sequence. >>>> + */ >>>> + rc = virtsnd_pcm_hw_params(substream, NULL); >>>> + if (rc) >>>> + return rc; >>>> + >>>> + rc = virtsnd_pcm_prepare(substream); >>>> + if (rc) >>>> + return rc; > > ... and this is rather what the core code should do, and it's exactly > the same procedure that would be done without RESUME flag. > > > Takashi >-- Anton Yakovlev Senior Software Engineer OpenSynergy GmbH Rotherstr. 20, 10245 Berlin