All running PCM substreams are stopped on device suspend and restarted
on device resume.
Signed-off-by: Anton Yakovlev <[email protected]>
---
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;
+}
+
+/**
+ * virtsnd_restore() - Resume device.
+ * @vdev: VirtIO parent device.
+ *
+ * Context: Any context.
+ * Return: 0 on success, -errno on failure.
+ */
+static int virtsnd_restore(struct virtio_device *vdev)
+{
+ struct virtio_snd *snd = vdev->priv;
+ int rc;
+
+ rc = virtsnd_find_vqs(snd);
+ if (rc)
+ return rc;
+
+ virtio_device_ready(vdev);
+
+ virtsnd_enable_event_vq(snd);
+
+ return 0;
+}
+#endif /* CONFIG_PM_SLEEP */
+
static const struct virtio_device_id id_table[] = {
{ VIRTIO_ID_SOUND, VIRTIO_DEV_ANY_ID },
{ 0 },
@@ -335,6 +387,10 @@ static struct virtio_driver virtsnd_driver = {
.validate = virtsnd_validate,
.probe = virtsnd_probe,
.remove = virtsnd_remove,
+#ifdef CONFIG_PM_SLEEP
+ .freeze = virtsnd_freeze,
+ .restore = virtsnd_restore,
+#endif
};
static int __init init(void)
diff --git a/sound/virtio/virtio_pcm.c b/sound/virtio/virtio_pcm.c
index 3605151860f2..4a4a6583b002 100644
--- 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;
if (!info->channels_min || info->channels_min > info->channels_max) {
diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
index d02d79bd94f3..03a7e2666cfb 100644
--- a/sound/virtio/virtio_pcm_ops.c
+++ b/sound/virtio/virtio_pcm_ops.c
@@ -167,7 +167,8 @@ static int virtsnd_pcm_hw_params(struct snd_pcm_substream *substream,
int vrate = -1;
int rc;
- if (virtsnd_pcm_msg_pending_num(vss)) {
+ if (runtime->status->state != SNDRV_PCM_STATE_SUSPENDED &&
+ virtsnd_pcm_msg_pending_num(vss)) {
dev_err(&vdev->dev, "SID %u: invalid I/O queue state\n",
vss->sid);
return -EBADFD;
@@ -231,6 +232,10 @@ static int virtsnd_pcm_hw_params(struct snd_pcm_substream *substream,
if (rc)
return rc;
+ /* If messages have already been allocated before, do nothing. */
+ if (runtime->status->state == SNDRV_PCM_STATE_SUSPENDED)
+ return 0;
+
/* Free previously allocated messages (if any). */
virtsnd_pcm_msg_free(vss);
@@ -267,20 +272,24 @@ static int virtsnd_pcm_hw_free(struct snd_pcm_substream *substream)
*/
static int virtsnd_pcm_prepare(struct snd_pcm_substream *substream)
{
+ struct snd_pcm_runtime *runtime = substream->runtime;
struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
struct virtio_device *vdev = vss->snd->vdev;
struct virtio_snd_msg *msg;
- if (virtsnd_pcm_msg_pending_num(vss)) {
- dev_err(&vdev->dev, "SID %u: invalid I/O queue state\n",
- vss->sid);
- return -EBADFD;
+ if (runtime->status->state != SNDRV_PCM_STATE_SUSPENDED) {
+ if (virtsnd_pcm_msg_pending_num(vss)) {
+ dev_err(&vdev->dev, "SID %u: invalid I/O queue state\n",
+ vss->sid);
+ return -EBADFD;
+ }
+
+ vss->buffer_bytes = snd_pcm_lib_buffer_bytes(substream);
+ vss->hw_ptr = 0;
+ vss->msg_last_enqueued = -1;
}
- vss->buffer_bytes = snd_pcm_lib_buffer_bytes(substream);
- vss->hw_ptr = 0;
vss->xfer_xrun = false;
- vss->msg_last_enqueued = -1;
vss->msg_count = 0;
msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_PREPARE,
@@ -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;
+
+ fallthrough;
+ }
case SNDRV_PCM_TRIGGER_START:
case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: {
struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss);
@@ -335,6 +359,7 @@ static int virtsnd_pcm_trigger(struct snd_pcm_substream *substream, int command)
return virtsnd_ctl_msg_send_sync(snd, msg);
}
+ case SNDRV_PCM_TRIGGER_SUSPEND:
case SNDRV_PCM_TRIGGER_STOP:
case SNDRV_PCM_TRIGGER_PAUSE_PUSH: {
spin_lock_irqsave(&vss->lock, flags);
--
2.30.1
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 <[email protected]>
> ---
> 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.
> --- 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.
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.
> @@ -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 won't work at all unless nonatomic PCM ops.
thanks,
Takashi
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 <[email protected]>
>> ---
>> 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. 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.
>> @@ -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 won't work at all unless nonatomic PCM ops.
>
>
> thanks,
>
> Takashi
>
--
Anton Yakovlev
Senior Software Engineer
OpenSynergy GmbH
Rotherstr. 20, 10245 Berlin
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 <[email protected]>
> >> ---
> >> 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...
> 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).
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
On 28.02.2021 13:05, Takashi Iwai wrote:
> On Sat, 27 Feb 2021 09:59:56 +0100,
> Anton Yakovlev wrote:
>>
[snip]
>> --- 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.
>
> 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.
I tried to resume driver without SNDRV_PCM_INFO_RESUME, and alsa-lib
called only ops->prepare(). It makes sense for a typical hw, but we have
"clean" unconfigured device on resume. And we must set hw parameters as
a first step. It means, that code should be more or less the same. And
maybe it's better to set SNDRV_PCM_INFO_RESUME, since it allows us to
resume substream in any situation (regardless of application behavior).
I can refactor code to only send requests from trigger(RESUME) path and
not to call ops itself. It should make code more straitforward. What do
you say?
--
Anton Yakovlev
Senior Software Engineer
OpenSynergy GmbH
Rotherstr. 20, 10245 Berlin
On Tue, 02 Mar 2021 09:09:33 +0100,
Anton Yakovlev wrote:
>
> On 02.03.2021 07:48, Takashi Iwai wrote:
> > On Tue, 02 Mar 2021 07:29:20 +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:
> >>>>
> >>
> >> [snip]
> >>
> >>>> --- 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.
> >>>
> >>> 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.
> >>
> >> I tried to resume driver without SNDRV_PCM_INFO_RESUME, and alsa-lib
> >> called only ops->prepare(). It makes sense for a typical hw, but we have
> >> "clean" unconfigured device on resume. And we must set hw parameters as
> >> a first step. It means, that code should be more or less the same. And
> >> maybe it's better to set SNDRV_PCM_INFO_RESUME, since it allows us to
> >> resume substream in any situation (regardless of application behavior).
> >> I can refactor code to only send requests from trigger(RESUME) path and
> >> not to call ops itself. It should make code more straitforward. What do
> >> you say?
> >
> > How about calling hw_params(NULL) conditionally in the prepare?
>
> Then the question is that condition. When ops->prepare() is called, the
> substream is in SUSPENDED state or not? If not then we need to track
> this in some additional field (and it will make logic a little bit
> clumsy, since that field is needed to be carefully handled in other
> places).
Yes, you'd need to have a suspend/resume PM callback in the driver
that flips the internal flag to invalidate the hw_parmas, and in the
prepare callback, just call hw_params(NULL) if that flag is set.
> > Doing the full stack work in the trigger callback is bad from the API
> > design POV; in general the trigger callback is supposed to be as short
> > as possible.
>
> Yeah, but usually original subsystem design does not take into account
> para-virtualized devices, which usually have it's own slightly different
> reality. And we need to introduce some tricks.
The hardware drivers do a lot of more things in either suspend/resume
PM callbacks or prepare callback for re-setup of the hardware. We can
follow the similar pattern. Heavy-lifting works in the trigger
callbacks is really something to avoid.
Takashi
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 <[email protected]>
>>>> ---
>>>> 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
On Tue, 02 Mar 2021 07:29:20 +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:
> >>
>
> [snip]
>
> >> --- 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.
> >
> > 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.
>
> I tried to resume driver without SNDRV_PCM_INFO_RESUME, and alsa-lib
> called only ops->prepare(). It makes sense for a typical hw, but we have
> "clean" unconfigured device on resume. And we must set hw parameters as
> a first step. It means, that code should be more or less the same. And
> maybe it's better to set SNDRV_PCM_INFO_RESUME, since it allows us to
> resume substream in any situation (regardless of application behavior).
> I can refactor code to only send requests from trigger(RESUME) path and
> not to call ops itself. It should make code more straitforward. What do
> you say?
How about calling hw_params(NULL) conditionally in the prepare?
Doing the full stack work in the trigger callback is bad from the API
design POV; in general the trigger callback is supposed to be as short
as possible.
Takashi
On 02.03.2021 07:48, Takashi Iwai wrote:
> On Tue, 02 Mar 2021 07:29:20 +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:
>>>>
>>
>> [snip]
>>
>>>> --- 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.
>>>
>>> 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.
>>
>> I tried to resume driver without SNDRV_PCM_INFO_RESUME, and alsa-lib
>> called only ops->prepare(). It makes sense for a typical hw, but we have
>> "clean" unconfigured device on resume. And we must set hw parameters as
>> a first step. It means, that code should be more or less the same. And
>> maybe it's better to set SNDRV_PCM_INFO_RESUME, since it allows us to
>> resume substream in any situation (regardless of application behavior).
>> I can refactor code to only send requests from trigger(RESUME) path and
>> not to call ops itself. It should make code more straitforward. What do
>> you say?
>
> How about calling hw_params(NULL) conditionally in the prepare?
Then the question is that condition. When ops->prepare() is called, the
substream is in SUSPENDED state or not? If not then we need to track
this in some additional field (and it will make logic a little bit
clumsy, since that field is needed to be carefully handled in other
places).
> Doing the full stack work in the trigger callback is bad from the API
> design POV; in general the trigger callback is supposed to be as short
> as possible.
Yeah, but usually original subsystem design does not take into account
para-virtualized devices, which usually have it's own slightly different
reality. And we need to introduce some tricks.
>
> Takashi
>
--
Anton Yakovlev
Senior Software Engineer
OpenSynergy GmbH
Rotherstr. 20, 10245 Berlin
On 02.03.2021 10:11, Takashi Iwai wrote:
> On Tue, 02 Mar 2021 09:09:33 +0100,
> Anton Yakovlev wrote:
>>
>> On 02.03.2021 07:48, Takashi Iwai wrote:
>>> On Tue, 02 Mar 2021 07:29:20 +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:
>>>>>>
>>>>
>>>> [snip]
>>>>
>>>>>> --- 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.
>>>>>
>>>>> 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.
>>>>
>>>> I tried to resume driver without SNDRV_PCM_INFO_RESUME, and alsa-lib
>>>> called only ops->prepare(). It makes sense for a typical hw, but we have
>>>> "clean" unconfigured device on resume. And we must set hw parameters as
>>>> a first step. It means, that code should be more or less the same. And
>>>> maybe it's better to set SNDRV_PCM_INFO_RESUME, since it allows us to
>>>> resume substream in any situation (regardless of application behavior).
>>>> I can refactor code to only send requests from trigger(RESUME) path and
>>>> not to call ops itself. It should make code more straitforward. What do
>>>> you say?
>>>
>>> How about calling hw_params(NULL) conditionally in the prepare?
>>
>> Then the question is that condition. When ops->prepare() is called, the
>> substream is in SUSPENDED state or not? If not then we need to track
>> this in some additional field (and it will make logic a little bit
>> clumsy, since that field is needed to be carefully handled in other
>> places).
>
> Yes, you'd need to have a suspend/resume PM callback in the driver
> that flips the internal flag to invalidate the hw_parmas, and in the
> prepare callback, just call hw_params(NULL) if that flag is set.
>
>>> Doing the full stack work in the trigger callback is bad from the API
>>> design POV; in general the trigger callback is supposed to be as short
>>> as possible.
>>
>> Yeah, but usually original subsystem design does not take into account
>> para-virtualized devices, which usually have it's own slightly different
>> reality. And we need to introduce some tricks.
>
> The hardware drivers do a lot of more things in either suspend/resume
> PM callbacks or prepare callback for re-setup of the hardware. We can
> follow the similar pattern. Heavy-lifting works in the trigger
> callbacks is really something to avoid.
Ok, I redone this part and now the driver sets parameters for the device
in ops->prepare() if the substream was suspended. And everything works
fine. Thanks! I will send a new patch set soon.
> Takashi
>
--
Anton Yakovlev
Senior Software Engineer
OpenSynergy GmbH
Rotherstr. 20, 10245 Berlin