Re: [PATCH] virtio-sound: add latency_bytes definition

[Matias Ezequiel Vara Larsen <mvaralar@redhat.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