From: Matias Ezequiel Vara Larsen <mvaralar@redhat.com>
To: Anton Yakovlev <anton.yakovlev@opensynergy.com>
Cc: virtio-comment@lists.oasis-open.org
Subject: [virtio-comment] Re: [PATCH] virtio-sound: add latency_bytes definition
Date: Mon, 11 Dec 2023 11:39:40 +0100 [thread overview]
Message-ID: <ZXbm7KLkzszVVRY8@fedora> (raw)
In-Reply-To: <cc2648d8-503a-4a09-a9cd-0d7133cf39a3@opensynergy.com>
Hello Anton,
On Fri, Dec 08, 2023 at 11:45:18AM +0900, Anton Yakovlev wrote:
> Hi Matias,
>
> On 30.11.2023 19:46, Matias Ezequiel Vara Larsen wrote:
> > Hello Anton,
> >
> > On Thu, Nov 30, 2023 at 10:30:35AM +0900, Anton Yakovlev wrote:
> > > Hi Matias,
> > >
> > >
> > > On 07.11.2023 19:46, Matias Ezequiel Vara Larsen wrote:
> > > > Ping Anton.
> > > >
> > > > Thanks, Matias.
> > > >
> > > > On Thu, Oct 5, 2023 at 11:41 AM Matias Ezequiel Vara Larsen
> > > > <mvaralar@redhat.com> wrote:
> > > > >
> > > > > This commit aims at providing a better definition of latency_bytes. To
> > > > > do so, this commit defines latency_bytes as is defined in Crosvm.
> > > > >
> > > > > Signed-off-by: Matias Ezequiel Vara Larsen <mvaralar@redhat.com>
> > > > > ---
> > > > > device-types/sound/description.tex | 5 ++++-
> > > > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/device-types/sound/description.tex b/device-types/sound/description.tex
> > > > > index 54c9c8e..1349765 100644
> > > > > --- a/device-types/sound/description.tex
> > > > > +++ b/device-types/sound/description.tex
> > > > > @@ -694,7 +694,10 @@ \subsubsection{PCM I/O Messages}\label{sec:Device Types / Sound Device / Device
> > > > > \begin{description}
> > > > > \item[\field{status}] contains VIRTIO_SND_S_OK if an operation is successful,
> > > > > and VIRTIO_SND_S_IO_ERR otherwise.
> > > > > -\item[\field{latency_bytes}] indicates the current device latency.
> > > > > +\item[\field{latency_bytes}] indicates the current device latency. For
> > > > > +playback, this is the amount of bytes that must be consumed before
> > > > > +the current request can be played. For capture, this is the latency in terms of
> > > > > +bytes that the capture buffer was recorded.
> > > > > \end{description}
> > >
> > > Yes, this field definitely needs some clarification.
> > >
> > > If you think about it, from the point of view of the application in the guest,
> > > the size of the current latency is the current contents of the buffer. I.e.
> > > exactly what you added to the description, but without taking into account the
> > > value of this field.
> > >
> > > In the current Linux driver implementation, this "latency_bytes" value is
> > > saved to the "delay" field, which is then passed to user space. And this
> > > "delay", as I understand it, indicates an additional delay caused by the
> > > specifics of the hardware.
> >
> > Yes, I understood the same. For me, it was not clear how "delay" would
> > be used by user applications. In crosvm, for example, `latency_bytes` is
> > set to 0, but I'm not sure what would happen if it was set to something
> > else. When I played around with different values, I didn't notice any
> > difference in behavior.
>
> The value of this delay is reported to user space, and can also be taken into
> account when calculating timestamps for the substream. So the application
> can take this into account if necessary.
>
>
Got it. Thanks for the explanation.
> > > And it turns out that the latency_bytes field either should indicate such an
> > > additional delay (for example, caused by the specific implementation of the
> > > backend on the device side), or does not make sense, since the current latency
> > > value is already known a priori. What do you think?
> >
> > Do you mean that we could get rid of this field and add it in the future
> > in case we need it? Or, to add to the description the fact that
> > latency_bytes is a specific implementation value that adds additional
> > delay?
>
> I meant that we could add something like: "indicates the current value of the
> additional delay caused by the implementation of the device".
>
Thanks, I will add that in the PATCH.
>
> > Also I do not understand how the `latency_bytes` value and the
> > completion notification may interact. For example, a device could send
> > completion notification immediately after a request is received and just
> > set the correct value to the `latency_bytes` field to indicate how many
> > bytes would take to the current buffer to be consumed. I can't see any
> > issue with using the `latency_byte` value in that way.
>
> But what is the practical meaning of this?
>
> And, ideally, the completion of requests should coincide with period
> boundaries on the timeline. Otherwise, for example, there will be problems
> with synchronizing video and audio streams when timestamps based on the hw_ptr
> value are used.
>
>
Thanks for the clarification.
Matias
This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.
In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.
Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/
prev parent reply other threads:[~2023-12-11 10:39 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-05 9:41 [virtio-comment] [PATCH] virtio-sound: add latency_bytes definition Matias Ezequiel Vara Larsen
2023-11-07 10:46 ` [virtio-comment] " Matias Ezequiel Vara Larsen
2023-11-30 1:30 ` Anton Yakovlev
2023-11-30 10:46 ` Matias Ezequiel Vara Larsen
2023-12-08 2:45 ` Anton Yakovlev
2023-12-11 10:39 ` Matias Ezequiel Vara Larsen [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZXbm7KLkzszVVRY8@fedora \
--to=mvaralar@redhat.com \
--cc=anton.yakovlev@opensynergy.com \
--cc=virtio-comment@lists.oasis-open.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).