Re: [PATCH v2 5/5] ALSA: xen-front: Implement ALSA virtual sound driver

From: Oleksandr Andrushchenko
Date: Tue Apr 17 2018 - 08:34:48 EST

On 04/17/2018 03:32 PM, Juergen Gross wrote:
On 17/04/18 14:26, Oleksandr Andrushchenko wrote:
On 04/17/2018 02:32 PM, Oleksandr Andrushchenko wrote:
On 04/16/2018 05:09 PM, Juergen Gross wrote:
On 16/04/18 08:24, Oleksandr Andrushchenko wrote:
From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>

Implement essential initialization of the sound driver:
ÂÂ - introduce required data structures
ÂÂ - handle driver registration
ÂÂ - handle sound card registration
ÂÂ - register sound driver on backend connection
ÂÂ - remove sound driver on backend disconnect

Initialize virtual sound card with streams according to the
Xen store configuration.

Implement ALSA driver operations including:
- manage frontend/backend shared buffers
- manage Xen bus event channel states

Implement requests from front to back for ALSA
PCM operations.
 - report ALSA period elapsed event: handle XENSND_EVT_CUR_POS
ÂÂÂ notifications from the backend when stream position advances
ÂÂÂ during playback/capture. The event carries a value of how
ÂÂÂ many octets were played/captured at the time of the event.
 - implement explicit stream parameter negotiation between
ÂÂÂ backend and frontend: handle XENSND_OP_HW_PARAM_QUERY request
ÂÂÂ to read/update configuration space for the parameter given:
ÂÂÂ request passes desired parameter interval and the response to
ÂÂÂ this request returns min/max interval for the parameter to be used.

Signed-off-by: Oleksandr Andrushchenko
<oleksandr_andrushchenko@xxxxxxxx>
---
 sound/xen/Makefile | 3 +-
 sound/xen/xen_snd_front.c | 193 ++++++++-
 sound/xen/xen_snd_front.h | 28 ++
 sound/xen/xen_snd_front_alsa.c | 830
++++++++++++++++++++++++++++++++++++++
 sound/xen/xen_snd_front_alsa.h | 23 ++
 sound/xen/xen_snd_front_evtchnl.c | 6 +-
 6 files changed, 1080 insertions(+), 3 deletions(-)
 create mode 100644 sound/xen/xen_snd_front_alsa.c
 create mode 100644 sound/xen/xen_snd_front_alsa.h

diff --git a/sound/xen/Makefile b/sound/xen/Makefile
index f028bc30af5d..1e6470ecc2f2 100644
--- a/sound/xen/Makefile
+++ b/sound/xen/Makefile
@@ -3,6 +3,7 @@
 snd_xen_front-objs := xen_snd_front.o \
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ xen_snd_front_cfg.o \
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ xen_snd_front_evtchnl.o \
-ÂÂÂÂÂÂÂÂÂÂÂÂÂ xen_snd_front_shbuf.o
+ÂÂÂÂÂÂÂÂÂÂÂÂÂ xen_snd_front_shbuf.o \
+ÂÂÂÂÂÂÂÂÂÂÂÂÂ xen_snd_front_alsa.o
  obj-$(CONFIG_SND_XEN_FRONTEND) += snd_xen_front.o
diff --git a/sound/xen/xen_snd_front.c b/sound/xen/xen_snd_front.c
index 0569c6c596a3..1fef253ea21a 100644
--- a/sound/xen/xen_snd_front.c
+++ b/sound/xen/xen_snd_front.c
@@ -19,10 +19,201 @@
 #include <xen/interface/io/sndif.h>
  #include "xen_snd_front.h"
+#include "xen_snd_front_alsa.h"
 #include "xen_snd_front_evtchnl.h"
+#include "xen_snd_front_shbuf.h"
+
+static struct xensnd_req *
+be_stream_prepare_req(struct xen_snd_front_evtchnl *evtchnl, u8
operation)
+{
+ÂÂÂ struct xensnd_req *req;
+
+ÂÂÂ req = RING_GET_REQUEST(&evtchnl->u.req.ring,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ evtchnl->u.req.ring.req_prod_pvt);
+ÂÂÂ req->operation = operation;
+ÂÂÂ req->id = evtchnl->evt_next_id++;
+ÂÂÂ evtchnl->evt_id = req->id;
+ÂÂÂ return req;
+}
+
+static int be_stream_do_io(struct xen_snd_front_evtchnl *evtchnl)
+{
+ÂÂÂ if (unlikely(evtchnl->state != EVTCHNL_STATE_CONNECTED))
+ÂÂÂÂÂÂÂ return -EIO;
+
+ÂÂÂ reinit_completion(&evtchnl->u.req.completion);
+ÂÂÂ xen_snd_front_evtchnl_flush(evtchnl);
+ÂÂÂ return 0;
+}
+
+static int be_stream_wait_io(struct xen_snd_front_evtchnl *evtchnl)
+{
+ÂÂÂ if (wait_for_completion_timeout(&evtchnl->u.req.completion,
+ÂÂÂÂÂÂÂÂÂÂÂ msecs_to_jiffies(VSND_WAIT_BACK_MS)) <= 0)
+ÂÂÂÂÂÂÂ return -ETIMEDOUT;
+
+ÂÂÂ return evtchnl->u.req.resp_status;
+}
+
+int xen_snd_front_stream_query_hw_param(struct
xen_snd_front_evtchnl *evtchnl,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct xensnd_query_hw_param *hw_param_req,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct xensnd_query_hw_param *hw_param_resp)
+{
+ÂÂÂ struct xen_snd_front_info *front_info = evtchnl->front_info;
+ÂÂÂ struct xensnd_req *req;
+ÂÂÂ unsigned long flags;
+ÂÂÂ int ret;
+
+ÂÂÂ mutex_lock(&evtchnl->u.req.req_io_lock);
+
+ÂÂÂ spin_lock_irqsave(&front_info->io_lock, flags);
+ÂÂÂ req = be_stream_prepare_req(evtchnl, XENSND_OP_HW_PARAM_QUERY);
+ÂÂÂ req->op.hw_param = *hw_param_req;
+
+ÂÂÂ ret = be_stream_do_io(evtchnl);
+ÂÂÂ spin_unlock_irqrestore(&front_info->io_lock, flags);
+
+ÂÂÂ if (ret == 0)
+ÂÂÂÂÂÂÂ ret = be_stream_wait_io(evtchnl);
+
+ÂÂÂ if (ret == 0)
+ÂÂÂÂÂÂÂ *hw_param_resp = evtchnl->u.req.resp.hw_param;
+
+ÂÂÂ mutex_unlock(&evtchnl->u.req.req_io_lock);
+ÂÂÂ return ret;
+}
+
+int xen_snd_front_stream_prepare(struct xen_snd_front_evtchnl
*evtchnl,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct xen_snd_front_shbuf *sh_buf,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ u8 format, unsigned int channels,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned int rate, u32 buffer_sz,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ u32 period_sz)
+{
+ÂÂÂ struct xen_snd_front_info *front_info = evtchnl->front_info;
+ÂÂÂ struct xensnd_req *req;
+ÂÂÂ unsigned long flags;
+ÂÂÂ int ret;
+
+ÂÂÂ mutex_lock(&evtchnl->u.req.req_io_lock);
+
+ÂÂÂ spin_lock_irqsave(&front_info->io_lock, flags);
+ÂÂÂ req = be_stream_prepare_req(evtchnl, XENSND_OP_OPEN);
+ÂÂÂ req->op.open.pcm_format = format;
+ÂÂÂ req->op.open.pcm_channels = channels;
+ÂÂÂ req->op.open.pcm_rate = rate;
+ÂÂÂ req->op.open.buffer_sz = buffer_sz;
+ÂÂÂ req->op.open.period_sz = period_sz;
+ÂÂÂ req->op.open.gref_directory =
xen_snd_front_shbuf_get_dir_start(sh_buf);
+
+ÂÂÂ ret = be_stream_do_io(evtchnl);
+ÂÂÂ spin_unlock_irqrestore(&front_info->io_lock, flags);
+
+ÂÂÂ if (ret == 0)
+ÂÂÂÂÂÂÂ ret = be_stream_wait_io(evtchnl);
+
+ÂÂÂ mutex_unlock(&evtchnl->u.req.req_io_lock);
+ÂÂÂ return ret;
+}
+
+int xen_snd_front_stream_close(struct xen_snd_front_evtchnl *evtchnl)
+{
+ÂÂÂ struct xen_snd_front_info *front_info = evtchnl->front_info;
+ÂÂÂ struct xensnd_req *req;
+ÂÂÂ unsigned long flags;
+ÂÂÂ int ret;
+
+ÂÂÂ mutex_lock(&evtchnl->u.req.req_io_lock);
+
+ÂÂÂ spin_lock_irqsave(&front_info->io_lock, flags);
+ÂÂÂ req = be_stream_prepare_req(evtchnl, XENSND_OP_CLOSE);
+
+ÂÂÂ ret = be_stream_do_io(evtchnl);
+ÂÂÂ spin_unlock_irqrestore(&front_info->io_lock, flags);
+
+ÂÂÂ if (ret == 0)
+ÂÂÂÂÂÂÂ ret = be_stream_wait_io(evtchnl);
+
+ÂÂÂ mutex_unlock(&evtchnl->u.req.req_io_lock);
+ÂÂÂ return ret;
+}
+
+int xen_snd_front_stream_write(struct xen_snd_front_evtchnl *evtchnl,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned long pos, unsigned long count)
+{
+ÂÂÂ struct xen_snd_front_info *front_info = evtchnl->front_info;
+ÂÂÂ struct xensnd_req *req;
+ÂÂÂ unsigned long flags;
+ÂÂÂ int ret;
+
+ÂÂÂ mutex_lock(&evtchnl->u.req.req_io_lock);
+
+ÂÂÂ spin_lock_irqsave(&front_info->io_lock, flags);
+ÂÂÂ req = be_stream_prepare_req(evtchnl, XENSND_OP_WRITE);
+ÂÂÂ req->op.rw.length = count;
+ÂÂÂ req->op.rw.offset = pos;
+
+ÂÂÂ ret = be_stream_do_io(evtchnl);
+ÂÂÂ spin_unlock_irqrestore(&front_info->io_lock, flags);
+
+ÂÂÂ if (ret == 0)
+ÂÂÂÂÂÂÂ ret = be_stream_wait_io(evtchnl);
+
+ÂÂÂ mutex_unlock(&evtchnl->u.req.req_io_lock);
+ÂÂÂ return ret;
+}
+
+int xen_snd_front_stream_read(struct xen_snd_front_evtchnl *evtchnl,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned long pos, unsigned long count)
+{
+ÂÂÂ struct xen_snd_front_info *front_info = evtchnl->front_info;
+ÂÂÂ struct xensnd_req *req;
+ÂÂÂ unsigned long flags;
+ÂÂÂ int ret;
+
+ÂÂÂ mutex_lock(&evtchnl->u.req.req_io_lock);
+
+ÂÂÂ spin_lock_irqsave(&front_info->io_lock, flags);
+ÂÂÂ req = be_stream_prepare_req(evtchnl, XENSND_OP_READ);
+ÂÂÂ req->op.rw.length = count;
+ÂÂÂ req->op.rw.offset = pos;
+
+ÂÂÂ ret = be_stream_do_io(evtchnl);
+ÂÂÂ spin_unlock_irqrestore(&front_info->io_lock, flags);
+
+ÂÂÂ if (ret == 0)
+ÂÂÂÂÂÂÂ ret = be_stream_wait_io(evtchnl);
+
+ÂÂÂ mutex_unlock(&evtchnl->u.req.req_io_lock);
+ÂÂÂ return ret;
+}
+
+int xen_snd_front_stream_trigger(struct xen_snd_front_evtchnl
*evtchnl,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ int type)
+{
+ÂÂÂ struct xen_snd_front_info *front_info = evtchnl->front_info;
+ÂÂÂ struct xensnd_req *req;
+ÂÂÂ unsigned long flags;
+ÂÂÂ int ret;
+
+ÂÂÂ mutex_lock(&evtchnl->u.req.req_io_lock);
+
+ÂÂÂ spin_lock_irqsave(&front_info->io_lock, flags);
+ÂÂÂ req = be_stream_prepare_req(evtchnl, XENSND_OP_TRIGGER);
+ÂÂÂ req->op.trigger.type = type;
+
+ÂÂÂ ret = be_stream_do_io(evtchnl);
+ÂÂÂ spin_unlock_irqrestore(&front_info->io_lock, flags);
+
+ÂÂÂ if (ret == 0)
+ÂÂÂÂÂÂÂ ret = be_stream_wait_io(evtchnl);
+
+ÂÂÂ mutex_unlock(&evtchnl->u.req.req_io_lock);
+ÂÂÂ return ret;
+}
  static void xen_snd_drv_fini(struct xen_snd_front_info *front_info)
 {
+ÂÂÂ xen_snd_front_alsa_fini(front_info);
ÂÂÂÂÂ xen_snd_front_evtchnl_free_all(front_info);
 }
 @@ -45,7 +236,7 @@ static int sndback_initwait(struct
xen_snd_front_info *front_info)
  static int sndback_connect(struct xen_snd_front_info *front_info)
 {
-ÂÂÂ return 0;
+ÂÂÂ return xen_snd_front_alsa_init(front_info);
 }
  static void sndback_disconnect(struct xen_snd_front_info
*front_info)
diff --git a/sound/xen/xen_snd_front.h b/sound/xen/xen_snd_front.h
index 9c2ffbb4e4b8..7adbdb4d2019 100644
--- a/sound/xen/xen_snd_front.h
+++ b/sound/xen/xen_snd_front.h
@@ -13,17 +13,45 @@
  #include "xen_snd_front_cfg.h"
 +struct card_info;
+struct xen_snd_front_evtchnl;
 struct xen_snd_front_evtchnl_pair;
+struct xen_snd_front_shbuf;
+struct xensnd_query_hw_param;
  struct xen_snd_front_info {
ÂÂÂÂÂ struct xenbus_device *xb_dev;
 + struct card_info *card_info;
+
ÂÂÂÂÂ /* serializer for backend IO: request/response */
ÂÂÂÂÂ spinlock_t io_lock;
+
ÂÂÂÂÂ int num_evt_pairs;
ÂÂÂÂÂ struct xen_snd_front_evtchnl_pair *evt_pairs;
 Â struct xen_front_cfg_card cfg;
 };
 +int xen_snd_front_stream_query_hw_param(struct
xen_snd_front_evtchnl *evtchnl,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct xensnd_query_hw_param *hw_param_req,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct xensnd_query_hw_param *hw_param_resp);
+
+int xen_snd_front_stream_prepare(struct xen_snd_front_evtchnl
*evtchnl,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct xen_snd_front_shbuf *sh_buf,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ u8 format, unsigned int channels,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned int rate, u32 buffer_sz,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ u32 period_sz);
+
+int xen_snd_front_stream_close(struct xen_snd_front_evtchnl *evtchnl);
+
+int xen_snd_front_stream_write(struct xen_snd_front_evtchnl *evtchnl,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned long pos, unsigned long count);
+
+int xen_snd_front_stream_read(struct xen_snd_front_evtchnl *evtchnl,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned long pos, unsigned long count);
+
+int xen_snd_front_stream_trigger(struct xen_snd_front_evtchnl
*evtchnl,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ int type);
+
 #endif /* __XEN_SND_FRONT_H */
diff --git a/sound/xen/xen_snd_front_alsa.c
b/sound/xen/xen_snd_front_alsa.c
new file mode 100644
index 000000000000..f524b172750e
--- /dev/null
+++ b/sound/xen/xen_snd_front_alsa.c
@@ -0,0 +1,830 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+
+/*
+ * Xen para-virtual sound device
+ *
+ * Copyright (C) 2016-2018 EPAM Systems Inc.
+ *
+ * Author: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
+ */
+
+#include <linux/platform_device.h>
+
+#include <sound/core.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+
+#include <xen/xenbus.h>
+
+#include "xen_snd_front.h"
+#include "xen_snd_front_alsa.h"
+#include "xen_snd_front_cfg.h"
+#include "xen_snd_front_evtchnl.h"
+#include "xen_snd_front_shbuf.h"
+
+struct pcm_stream_info {
Not sure how this is generally handled in the sound drivers, but when
reviewing the code using those structures I repeatedly tried to find
their definitions in the sound headers instead of here. Same applies to
the alsa_* names.

I'd prefer names which don't poison the name space.
I'll try to do something about naming
One question still remains wrt alsa_* names: if this is for structures
I have (alsa_sndif_sample_format/alsa_sndif_hw_param), then
those already have sndif in their name which clearly says these are
Xen related ones (sndif is a Xen protocol).
If you also don't like the alsa_* function names then those are all
static and defined in this same file, so see no confusion here.

I have changed other non-obvious struct names:

-struct pcm_stream_info {
+struct xen_snd_front_pcm_stream_info {

-struct pcm_instance_info {
+struct xen_snd_front_pcm_instance_info {

-struct card_info {
+struct xen_snd_front_card_info {

Does the above work for you?
Yes, this seems to be okay.
Good, thank you
Thanks,

Juergen