Message-ID: <CACvf2oU4puqSuNj2k5REpmFJdvWcTWY+AaNEJtknLP4XMPdbsg@mail.gmail.com>
Date:   Thu, 17 Aug 2017 13:05:37 +0300
From:   Oleksandr Grytsov <al1img@...il.com>
To:     Oleksandr Andrushchenko <andr2000@...il.com>, clemens@...isch.de,
        o-takashi@...amocchi.jp
Cc:     alsa-devel@...a-project.org, xen-devel@...ts.xen.org,
        linux-kernel@...r.kernel.org, tiwai@...e.com,
        Oleksandr Andrushchenko <oleksandr_andrushchenko@...m.com>
Subject: Re: [Xen-devel] [PATCH RESEND1 00/12] ALSA: vsnd: Add Xen
 para-virtualized frontend driver

On Thu, Aug 10, 2017 at 11:10 AM, Oleksandr Andrushchenko
<andr2000@...il.com> wrote:
> Hi,
>
> thank you very much for valuable comments and your time!
>
>
> On 08/10/2017 06:14 AM, Takashi Sakamoto wrote:
>>
>> Hi,
>>
>> On Aug 7 2017 21:22, Oleksandr Andrushchenko wrote:
>>>
>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@...m.com>
>>>
>>> This patch series adds support for Xen [1] para-virtualized
>>> sound frontend driver. It implements the protocol from
>>> include/xen/interface/io/sndif.h with the following limitations:
>>> - mute/unmute is not supported
>>> - get/set volume is not supported
>>> Volume control is not supported for the reason that most of the
>>> use-cases (at the moment) are based on scenarious where
>>> unprivileged OS (e.g. Android, AGL etc) use software mixers.
>>>
>>> Both capture and playback are supported.
>>>
>>> Thank you,
>>> Oleksandr
>>>
>>> Resending because of rebase onto [2] + added missing patch
>>>
>>> [1] https://xenproject.org/
>>> [2]
>>> https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/log/?h=for-next
>>>
>>> Oleksandr Andrushchenko (12):
>>>    ALSA: vsnd: Introduce Xen para-virtualized sound frontend driver
>>>    ALSA: vsnd: Implement driver's probe/remove
>>>    ALSA: vsnd: Implement Xen bus state handling
>>>    ALSA: vsnd: Read sound driver configuration from Xen store
>>>    ALSA: vsnd: Implement Xen event channel handling
>>>    ALSA: vsnd: Implement handling of shared buffers
>>>    ALSA: vsnd: Introduce ALSA virtual sound driver
>>>    ALSA: vsnd: Initialize virtul sound card
>>>    ALSA: vsnd: Add timer for period interrupt emulation
>>>    ALSA: vsnd: Implement ALSA PCM operations
>>>    ALSA: vsnd: Implement communication with backend
>>>    ALSA: vsnd: Introduce Kconfig option to enable Xen PV sound
>>>
>>>   sound/drivers/Kconfig     |   12 +
>>>   sound/drivers/Makefile    |    2 +
>>>   sound/drivers/xen-front.c | 2107
>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 2121 insertions(+)
>>>   create mode 100644 sound/drivers/xen-front.c
>>
>>
>> For this patchset, I have the same concern which Clemens Ladisch
>> denoted[1]. If I can understand your explanation about queueing between
>> Dom0/DomU stuffs, the concern can be described in short words; this
>> driver works without any synchronization to data transmission by actual
>> sound hardwares.
>>
> Yes, both your concerns and understanding are correct
>>
>> In design of ALSA PCM core, drivers are expected to synchronize to
>> actual hardwares for semi-realtime data transmission. The
>> synchronization is done by two points:
>> 1) Interrupts to respond events from actual hardwares.
>> 2) Positions of actual data transmission in any serial sound interfaces
>>    of actual hardwares.
>>
>> These two points comes from typical designs of actual hardwares, thus
>> they doesn't come from unfair, unreasonable, intrusive demands from
>> software side.
>>
> This clear, thank you
>>
>> In design of typical stuffs on para-virtualization, Dom0 stuffs are hard
>> to give enough abstraction of sound hardwares in these two points for
>> DomU stuffs. Especially, it cannot abstract point 2) at all because the
>> value of position should be accurate against actual time frame, while
>> there's an overhead for DomU stuffs to read it. When DomU stuffs handles
>> the value, the value is enough past due to context switches between
>> Dom0/DomU. Therefore, this driver must rely on point 1) to synchronize
>> to actual sound hardwares.

In order to implement option 1) discussed (Interrupts to respond events from
actual hardwares) we did number of experiments to find out if it can be
implemented in the way it satisfies the requirements with respect to latency,
interrupt number and use-cases.

First of all the sound backend is a user-space application which uses either
ALSA or PulseAudio to play/capture audio depending on configuration.
Most of the use-cases we have are using PulseAudio as it allows to
implement more complex use cases then just plain ALSA.

We started to look at how can we get such an event so it can be used as
a period elapsed notification to the backend.

In case of ALSA we used poll mechanism to wait for events from ALSA:
we configured SW params to have period event, but the problem here is that
it is notified not only when period elapses, but also when ALSA is ready to
consume more data. There is no mechanism to distinguish between these
two events (please correct us if there is one). Anyways, even if ALSA provides
period event to user-space (again, backend is a user-space application)
latency will consist of: time from kernel to user-space, user-space Dom0 to
frontend driver DomU. Both are variable and depend on many factors,
so the latency is not deterministic.

(We were also thinking that we can implement a helper driver in Dom0 to have
a dedicated channel from ALSA to the backend to deliver period elapsed event,
so for instance, it can have some kind of a hook on snd_pcm_period_elapsed,
but it will not solve the use-case with PulseAudio discussed below.
Also it is unclear how to handle scenario when multiple DomU plays through
mixer with different frame rates, channels etc.).

In case of PulseAudio it is even worse. PulseAudio can’t provide any period
related information (again, please let us know if this is not true). From our
understanding, event if get such an event from PulseAudio it will be software
emulated in user-space: for example, when PulseAudio uses a single ALSA
sink and mixes two streams with different frame rates, it will generate at least
for one of the streams a software period elapsed event.

So, from the above we think that period elapsed event derived in the described
ways may not improve latency and will complicate the system. So, for that
reason we are thinking of the option 2) (Positions of actual data transmission
in any serial sound interfaces of actual hardwares.)

In both ALSA and PulseAudio cases we can get timestamp information
(current sample timestamp). It can be converted to frames or bytes or
whatever that has been processed by the HW. The backend can get
timestamp periodically (with polling period configured with respect
to framerates being played) and pass it to the frontend. Due to context
switch and other factors this information will be outdated as well, but it seems
to be the best sync approach the backend can provide.

So, from the above, both options for synchronization cannot guarantee that
we will indeed be synchronous or latency is deterministic, but may improve
things comparing to the kernel timer on frontend’s side.

All, could you please tell us your opinion on the above and suggest what
could be the right way to go?

> Which will also introduce some latency too: time needed to deliver and
> handle interrupt from Dom0 to DomU
>>
>> Typically, drivers configure hardwares to
>> generate interrupts per period of PCM buffer. This means that this
>> driver should notify to Dom0 about the value of period size requested
>> by applications.
>>
>> In 'include/xen/interface/io/sndif.h', there's no functionalities I
>> described the above:
>> 1. notifications from DomU to Dom0 about the size of period for
>>    interrupts from actual hardwares. Or no way from Dom0 to DomU about
>>    the configured size of the period.
>
> Ok, then on "open" command I will pass from DomU to Dom0 an additional
> parameter, period size. Then Dom0 will respond with actual period size
> for DomU to use. So, this way period size will be negotiated.
> Does the above look ok to you?
>>
>> 2. notifications of the interrupts from actual hardwares to DomU.
>>
> Ok, I will introduce an event from Dom0 to DomU to signal period elapsed.
>
> Taking into account the fact that period size may be as small as,
> say, 1ms, do you think we can/need to mangle period size in 1) on Dom0 side
> to be reasonable, so we do not flood with interrupts/events from Dom0 to
> DomU?
> Do you see any "formula" to determine that reasonable/acceptable
> period limit, so both Dom0 and DomU are happy?
>
>> For the reasons, your driver used kernel's timer interface to generate
>> 'pseudo' interrupts for the purpose. However, it depends on Dom0's
>> abstraction different from sound hardwares and Linux kernel's
>> abstraction for timer functionality. In this case, gap between 'actual'
>> interrupts from hardware and the 'pseudo' interrupts from a combination
>> of several components brings unexpected result on several situations.
>>
> You are right
>>
>> I think this is defects of 'sndif' interface in Xen side. I think it
>> better for you to work in Xen community to improve the above interface
>> at first, then work for Linux stuffs.
>>
> Please see above for planned changes to the protocol
>>
>>
>> Additionally, in next time, please remind of several points below:
>>  * When a first patch adds an initial code for drivers, it should
>>    include entries for Makefile and Kconfig, so that the driver can be
>>    built even if it's still in an initial shape.
>
> Will do
>>
>> Each patch should be
>>    self-contained and should be in a shape so that developers easily run
>>    bisecting. In other words, your first patch[2] includes modification
>>   for Makefile and Kconfig in your last patch[3].
>
> Will do
>>
>>  * When any read-only symbols is added,  it should have 'const'
>>    qualifier so that the symbol places to .rodata section of ELF
>>    binaries. For example, in your code, 'alsa_sndif_formats' is such an
>>    symbol. In recent Linux development, some developers work for
>>    constifying such symbols. Please remind of their continuous works in
>>    upstream[4].
>
> Will do
>>
>>  * You can split your driver to several files. In
>>    'include/xen/interface/io/sndif.h', Dom0 produces functionalities for
>>    DomU to control gain/volume/mute and in future your driver may get
>>    more codes. If split to several files make it readable, it should be
>>    done.
>
> Will do. If I split, do you think it would be better to move the driver
> from sound/drivers to sound/xen folder, so all those files do not mix
> with the rest?
>>
>>  * In my taste, a prefix of the subject line should be 'xen-front',
>>   instead of 'vsnd'. It comes from name of your driver.
>>
> Will do
>>
>> [1] [alsa-devel] [PATCH 08/11] ALSA: vsnd: Add timer for period interrupt
>> emulation
>>
>> http://mailman.alsa-project.org/pipermail/alsa-devel/2017-August/123617.html
>> [2] [PATCH RESEND1 01/12] ALSA: vsnd: Introduce Xen para-virtualized sound
>> frontend driver
>>
>> http://mailman.alsa-project.org/pipermail/alsa-devel/2017-August/123654.html
>> [3] [alsa-devel] [PATCH RESEND1 12/12] ALSA: vsnd: Introduce Kconfig
>> option to enable Xen PV sound
>>
>> http://mailman.alsa-project.org/pipermail/alsa-devel/2017-August/123662.html
>> [4] You can see many posts for this; e.g. [alsa-devel] [PATCH 0/7]
>> constify ALSA usb_device_id.
>>
>> http://mailman.alsa-project.org/pipermail/alsa-devel/2017-August/123564.html
>>
>> Regards
>>
>> Takashi Sakamoto
>
> Thank you,
> Oleksandr
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@...ts.xen.org
> https://lists.xen.org/xen-devel



-- 
Best Regards,
Oleksandr Grytsov.

Powered by blists - more mailing lists