[RFC PATCH v2 0/3] UAC2 Gadget: feedback endpoint support

[RFC PATCH v2 0/3] UAC2 Gadget: feedback endpoint support

[Jerome Brunet]

Current UAC2 gadget implements capture/sync paths as two USB ISO ASYNC
endpoints (IN and OUT).

This violates USB spec which says that ISO ASYNC OUT endpoint should
have feedback companion endpoint.  See USB2.0 spec "5.12.4.1
Synchronization Type": asynchronous sink provides explicit feedback
(isochronous pipe).  Interesting that for ISO ASYNC *IN* endpoint
respective feedback isn't required since source provides implicit
feedforward (data stream).

While it's not an issue if UAC2 Gadget is connected to Linux host (Linux
ignores missing feedback endpoint), with other hosts like Windows or
MacOS the UAC2 Gadget isn't enumerated due to missing feedback endpoint.

This patch series adds feedback endpoint support to UAC2 function, new
control to UAC2 mixer which can be used by userspace tools (like
alsaloop from alsa-utils) for updating feedback frequency reported to
the host.  This is useful for usecases when UAC2 Gadget's audio samples
are played to another codec or audio card with its own internal
freerunning clock so host can be notified that more/less samples are
required.

The alsaloop tool requires some (relatively small) modifications in
order to start support driving feedback frequency through UAC2 mixer
control.  That change will be sent as a separate patch to ALSA
community.

Also added ability to switch ISO ASYNC OUT endpoint into adaptive
endpoint which doesn't require feedback endpoint (as per USB spec).

v1 [0] -> v2:
* Instead of HW rate used by gadget (which the controlling application
  might not be directly dealing with), the kcontrol used to provide the
  feedback value is expressed in PPM. This way the correction is
  agnostic of the rate used by the gadget while retaining enough
  precision to allow a smooth adaptation of the clock on the playback
  side, if necessary.
* The minimum value for the correction is aligned with
  sound/usb/endpoint.c at -25%.
* The maximum correction must be chosen more carefully as it impact the
  bandwidth required by the output EP. The minimum is one extra frame
  per packet. This is ~500PPM at 192KHz. This seemed more than enough
  while testing.
  Of course, there is a trade off between saving bandwidth and enabling
  devices with a significant (abnormal) clock deviation. The default
  extra reserved bandwidth is set at 500PPM but it is configurable
  through configfs, like the rest of the gadget.

I have kept Ruslan assigned as the author of series. He did the vast
majority of the work and deserve the credit. I have merely tweaked in
his original patchset.

This patchset has been tested on an arm64 host at HIGH_SPEED, with
various rates and channel configuration. I have not tested with
FULL_SPEED or SUPER_SPEED.

See here [1] an example of regulation with a patched version of
alsaloop.

[0]: https://lore.kernel.org/r/1604794711-8661-1-git-send-email-ruslan.bilovol@gmail.com
[1]: https://pastebin.com/0hiK7LAe

Ruslan Bilovol (3):
  usb: gadget: f_uac2/u_audio: add feedback endpoint support
  usb: gadget: f_uac2: add adaptive sync support for capture
  usb: gadget: u_audio: add real feedback implementation

 .../ABI/testing/configfs-usb-gadget-uac2      |   2 +
 Documentation/usb/gadget-testing.rst          |   2 +
 drivers/usb/gadget/function/f_uac2.c          | 144 +++++++++++-
 drivers/usb/gadget/function/u_audio.c         | 214 +++++++++++++++++-
 drivers/usb/gadget/function/u_audio.h         |  12 +
 drivers/usb/gadget/function/u_uac2.h          |   4 +
 6 files changed, 372 insertions(+), 6 deletions(-)

-- 
2.31.1

[RFC PATCH v2 1/3] usb: gadget: f_uac2/u_audio: add feedback endpoint support

[Jerome Brunet]

From: Ruslan Bilovol <ruslan.bilovol@gmail.com>

As per USB and UAC2 specs, asynchronous audio sink endpoint
requires explicit synchronization mechanism (Isochronous
Feedback Endpoint)

Implement feedback companion endpoint for ISO OUT endpoint

This patch adds all required infrastructure and USB requests
handling for feedback endpoint. Syncrhonization itself is
still dummy (feedback ep always reports 'nomimal frequency'
 e.g. no adjustement is needed). This satisfies hosts that
require feedback endpoint (like Win10) and poll it periodically

Actual synchronization mechanism should be implemented
separately

Signed-off-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/usb/gadget/function/f_uac2.c  |  49 ++++++++++-
 drivers/usb/gadget/function/u_audio.c | 119 +++++++++++++++++++++++++-
 drivers/usb/gadget/function/u_audio.h |   3 +
 3 files changed, 168 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c
index 7aa4c8bc5a1a..5d63244ba319 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -240,7 +240,7 @@ static struct usb_interface_descriptor std_as_out_if1_desc = {
 	.bDescriptorType = USB_DT_INTERFACE,
 
 	.bAlternateSetting = 1,
-	.bNumEndpoints = 1,
+	.bNumEndpoints = 2,
 	.bInterfaceClass = USB_CLASS_AUDIO,
 	.bInterfaceSubClass = USB_SUBCLASS_AUDIOSTREAMING,
 	.bInterfaceProtocol = UAC_VERSION_2,
@@ -317,6 +317,37 @@ static struct uac2_iso_endpoint_descriptor as_iso_out_desc = {
 	.wLockDelay = 0,
 };
 
+/* STD AS ISO IN Feedback Endpoint */
+static struct usb_endpoint_descriptor fs_epin_fback_desc = {
+	.bLength = USB_DT_ENDPOINT_SIZE,
+	.bDescriptorType = USB_DT_ENDPOINT,
+
+	.bEndpointAddress = USB_DIR_IN,
+	.bmAttributes = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_USAGE_FEEDBACK,
+	.wMaxPacketSize = cpu_to_le16(3),
+	.bInterval = 1,
+};
+
+static struct usb_endpoint_descriptor hs_epin_fback_desc = {
+	.bLength = USB_DT_ENDPOINT_SIZE,
+	.bDescriptorType = USB_DT_ENDPOINT,
+
+	.bmAttributes = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_USAGE_FEEDBACK,
+	.wMaxPacketSize = cpu_to_le16(4),
+	.bInterval = 4,
+};
+
+static struct usb_endpoint_descriptor ss_epin_fback_desc = {
+	.bLength = USB_DT_ENDPOINT_SIZE,
+	.bDescriptorType = USB_DT_ENDPOINT,
+
+	.bEndpointAddress = USB_DIR_IN,
+	.bmAttributes = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_USAGE_FEEDBACK,
+	.wMaxPacketSize = cpu_to_le16(4),
+	.bInterval = 4,
+};
+
+
 /* Audio Streaming IN Interface - Alt0 */
 static struct usb_interface_descriptor std_as_in_if0_desc = {
 	.bLength = sizeof std_as_in_if0_desc,
@@ -431,6 +462,7 @@ static struct usb_descriptor_header *fs_audio_desc[] = {
 	(struct usb_descriptor_header *)&as_out_fmt1_desc,
 	(struct usb_descriptor_header *)&fs_epout_desc,
 	(struct usb_descriptor_header *)&as_iso_out_desc,
+	(struct usb_descriptor_header *)&fs_epin_fback_desc,
 
 	(struct usb_descriptor_header *)&std_as_in_if0_desc,
 	(struct usb_descriptor_header *)&std_as_in_if1_desc,
@@ -461,6 +493,7 @@ static struct usb_descriptor_header *hs_audio_desc[] = {
 	(struct usb_descriptor_header *)&as_out_fmt1_desc,
 	(struct usb_descriptor_header *)&hs_epout_desc,
 	(struct usb_descriptor_header *)&as_iso_out_desc,
+	(struct usb_descriptor_header *)&hs_epin_fback_desc,
 
 	(struct usb_descriptor_header *)&std_as_in_if0_desc,
 	(struct usb_descriptor_header *)&std_as_in_if1_desc,
@@ -492,6 +525,7 @@ static struct usb_descriptor_header *ss_audio_desc[] = {
 	(struct usb_descriptor_header *)&ss_epout_desc,
 	(struct usb_descriptor_header *)&ss_epout_desc_comp,
 	(struct usb_descriptor_header *)&as_iso_out_desc,
+	(struct usb_descriptor_header *)&ss_epin_fback_desc,
 
 	(struct usb_descriptor_header *)&std_as_in_if0_desc,
 	(struct usb_descriptor_header *)&std_as_in_if1_desc,
@@ -568,22 +602,26 @@ static void setup_headers(struct f_uac2_opts *opts,
 	struct usb_ss_ep_comp_descriptor *epin_desc_comp = NULL;
 	struct usb_endpoint_descriptor *epout_desc;
 	struct usb_endpoint_descriptor *epin_desc;
+	struct usb_endpoint_descriptor *epin_fback_desc;
 	int i;
 
 	switch (speed) {
 	case USB_SPEED_FULL:
 		epout_desc = &fs_epout_desc;
 		epin_desc = &fs_epin_desc;
+		epin_fback_desc = &fs_epin_fback_desc;
 		break;
 	case USB_SPEED_HIGH:
 		epout_desc = &hs_epout_desc;
 		epin_desc = &hs_epin_desc;
+		epin_fback_desc = &hs_epin_fback_desc;
 		break;
 	default:
 		epout_desc = &ss_epout_desc;
 		epin_desc = &ss_epin_desc;
 		epout_desc_comp = &ss_epout_desc_comp;
 		epin_desc_comp = &ss_epin_desc_comp;
+		epin_fback_desc = &ss_epin_fback_desc;
 	}
 
 	i = 0;
@@ -611,6 +649,7 @@ static void setup_headers(struct f_uac2_opts *opts,
 			headers[i++] = USBDHDR(epout_desc_comp);
 
 		headers[i++] = USBDHDR(&as_iso_out_desc);
+		headers[i++] = USBDHDR(epin_fback_desc);
 	}
 	if (EPIN_EN(opts)) {
 		headers[i++] = USBDHDR(&std_as_in_if0_desc);
@@ -844,6 +883,12 @@ afunc_bind(struct usb_configuration *cfg, struct usb_function *fn)
 			dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
 			return -ENODEV;
 		}
+		agdev->in_ep_fback = usb_ep_autoconfig(gadget,
+						       &fs_epin_fback_desc);
+		if (!agdev->in_ep_fback) {
+			dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
+			return -ENODEV;
+		}
 	}
 
 	if (EPIN_EN(uac2_opts)) {
@@ -867,8 +912,10 @@ afunc_bind(struct usb_configuration *cfg, struct usb_function *fn)
 				le16_to_cpu(ss_epout_desc.wMaxPacketSize));
 
 	hs_epout_desc.bEndpointAddress = fs_epout_desc.bEndpointAddress;
+	hs_epin_fback_desc.bEndpointAddress = fs_epin_fback_desc.bEndpointAddress;
 	hs_epin_desc.bEndpointAddress = fs_epin_desc.bEndpointAddress;
 	ss_epout_desc.bEndpointAddress = fs_epout_desc.bEndpointAddress;
+	ss_epin_fback_desc.bEndpointAddress = fs_epin_fback_desc.bEndpointAddress;
 	ss_epin_desc.bEndpointAddress = fs_epin_desc.bEndpointAddress;
 
 	setup_descriptor(uac2_opts);
diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c
index 5fbceee897a3..f637ebec80b0 100644
--- a/drivers/usb/gadget/function/u_audio.c
+++ b/drivers/usb/gadget/function/u_audio.c
@@ -38,6 +38,10 @@ struct uac_rtd_params {
 	unsigned int max_psize;	/* MaxPacketSize of endpoint */
 
 	struct usb_request **reqs;
+
+	struct usb_request *req_fback; /* Feedback endpoint request */
+	unsigned int ffback; /* Real frequency reported by feedback endpoint */
+	bool fb_ep_enabled; /* if the ep is enabled */
 };
 
 struct snd_uac_chip {
@@ -70,6 +74,32 @@ static const struct snd_pcm_hardware uac_pcm_hardware = {
 	.periods_min = MIN_PERIODS,
 };
 
+static void u_audio_set_fback_frequency(enum usb_device_speed speed,
+					unsigned int freq, void *buf)
+{
+	u32 ff = 0;
+
+	if (speed == USB_SPEED_FULL) {
+		/*
+		 * Full-speed feedback endpoints report frequency
+		 * in samples/microframe
+		 * Format is encoded in Q10.10 left-justified in the 24 bits,
+		 * so that it has a Q10.14 format.
+		 */
+		ff = DIV_ROUND_UP((freq << 14), 1000);
+	} else {
+		/*
+		 * High-speed feedback endpoints report frequency
+		 * in samples/microframe.
+		 * Format is encoded in Q12.13 fitted into four bytes so that
+		 * the binary point is located between the second and the third
+		 * byte fromat (that is Q16.16)
+		 */
+		ff = DIV_ROUND_UP((freq << 13), 1000);
+	}
+	*(__le32 *)buf = cpu_to_le32(ff);
+}
+
 static void u_audio_iso_complete(struct usb_ep *ep, struct usb_request *req)
 {
 	unsigned int pending;
@@ -173,6 +203,34 @@ static void u_audio_iso_complete(struct usb_ep *ep, struct usb_request *req)
 		dev_err(uac->card->dev, "%d Error!\n", __LINE__);
 }
 
+static void u_audio_iso_fback_complete(struct usb_ep *ep,
+				       struct usb_request *req)
+{
+	struct uac_rtd_params *prm = req->context;
+	struct snd_uac_chip *uac = prm->uac;
+	struct g_audio *audio_dev = uac->audio_dev;
+	int status = req->status;
+	unsigned long flags;
+
+	/* i/f shutting down */
+	if (!prm->fb_ep_enabled || req->status == -ESHUTDOWN)
+		return;
+
+	/*
+	 * We can't really do much about bad xfers.
+	 * Afterall, the ISOCH xfers could fail legitimately.
+	 */
+	if (status)
+		pr_debug("%s: iso_complete status(%d) %d/%d\n",
+			__func__, status, req->actual, req->length);
+
+	u_audio_set_fback_frequency(audio_dev->gadget->speed,
+				    prm->ffback, req->buf);
+
+	if (usb_ep_queue(ep, req, GFP_ATOMIC))
+		dev_err(uac->card->dev, "%d Error!\n", __LINE__);
+}
+
 static int uac_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 {
 	struct snd_uac_chip *uac = snd_pcm_substream_chip(substream);
@@ -335,14 +393,33 @@ static inline void free_ep(struct uac_rtd_params *prm, struct usb_ep *ep)
 		dev_err(uac->card->dev, "%s:%d Error!\n", __func__, __LINE__);
 }
 
+static inline void free_ep_fback(struct uac_rtd_params *prm, struct usb_ep *ep)
+{
+	struct snd_uac_chip *uac = prm->uac;
+
+	if (!prm->fb_ep_enabled)
+		return;
+
+	prm->fb_ep_enabled = false;
+
+	if (prm->req_fback) {
+		usb_ep_dequeue(ep, prm->req_fback);
+		kfree(prm->req_fback->buf);
+		usb_ep_free_request(ep, prm->req_fback);
+		prm->req_fback = NULL;
+	}
+
+	if (usb_ep_disable(ep))
+		dev_err(uac->card->dev, "%s:%d Error!\n", __func__, __LINE__);
+}
 
 int u_audio_start_capture(struct g_audio *audio_dev)
 {
 	struct snd_uac_chip *uac = audio_dev->uac;
 	struct usb_gadget *gadget = audio_dev->gadget;
 	struct device *dev = &gadget->dev;
-	struct usb_request *req;
-	struct usb_ep *ep;
+	struct usb_request *req, *req_fback;
+	struct usb_ep *ep, *ep_fback;
 	struct uac_rtd_params *prm;
 	struct uac_params *params = &audio_dev->params;
 	int req_len, i;
@@ -374,6 +451,42 @@ int u_audio_start_capture(struct g_audio *audio_dev)
 			dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
 	}
 
+	ep_fback = audio_dev->in_ep_fback;
+	if (!ep_fback)
+		return 0;
+
+	/* Setup feedback endpoint */
+	config_ep_by_speed(gadget, &audio_dev->func, ep_fback);
+	prm->fb_ep_enabled = true;
+	usb_ep_enable(ep_fback);
+	req_len = ep_fback->maxpacket;
+
+	req_fback = usb_ep_alloc_request(ep_fback, GFP_ATOMIC);
+	if (req_fback == NULL)
+		return -ENOMEM;
+
+	prm->req_fback = req_fback;
+	req_fback->zero = 0;
+	req_fback->context = prm;
+	req_fback->length = req_len;
+	req_fback->complete = u_audio_iso_fback_complete;
+
+	req_fback->buf = kzalloc(req_len, GFP_ATOMIC);
+	if (!req_fback->buf)
+		return -ENOMEM;
+
+	/*
+	 * Configure the feedback endpoint's reported frequency.
+	 * Always start with original frequency since its deviation can't
+	 * be meauserd at start of playback
+	 */
+	prm->ffback = params->c_srate;
+	u_audio_set_fback_frequency(audio_dev->gadget->speed,
+				    prm->ffback, req_fback->buf);
+
+	if (usb_ep_queue(ep_fback, req_fback, GFP_ATOMIC))
+		dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(u_audio_start_capture);
@@ -382,6 +495,8 @@ void u_audio_stop_capture(struct g_audio *audio_dev)
 {
 	struct snd_uac_chip *uac = audio_dev->uac;
 
+	if (audio_dev->in_ep_fback)
+		free_ep_fback(&uac->c_prm, audio_dev->in_ep_fback);
 	free_ep(&uac->c_prm, audio_dev->out_ep);
 }
 EXPORT_SYMBOL_GPL(u_audio_stop_capture);
diff --git a/drivers/usb/gadget/function/u_audio.h b/drivers/usb/gadget/function/u_audio.h
index 5ea6b86f1fda..53e6baf55cbf 100644
--- a/drivers/usb/gadget/function/u_audio.h
+++ b/drivers/usb/gadget/function/u_audio.h
@@ -30,7 +30,10 @@ struct g_audio {
 	struct usb_gadget *gadget;
 
 	struct usb_ep *in_ep;
+
 	struct usb_ep *out_ep;
+	/* feedback IN endpoint corresponding to out_ep */
+	struct usb_ep *in_ep_fback;
 
 	/* Max packet size for all in_ep possible speeds */
 	unsigned int in_ep_maxpsize;
-- 
2.31.1

[RFC PATCH v2 2/3] usb: gadget: f_uac2: add adaptive sync support for capture

[Jerome Brunet]

From: Ruslan Bilovol <ruslan.bilovol@gmail.com>

Current f_uac2 USB OUT (aka 'capture') synchronization
implements 'ASYNC' scenario which means USB Gadget has
it's own freerunning clock and can update Host about
real clock frequency through feedback endpoint so Host
can align number of samples sent to the USB gadget to
prevent overruns/underruns

In case if Gadget can has no it's internal clock and
can consume audio samples at any rate (for example,
on the Gadget side someone records audio directly to
a file, or audio samples are played through an
external DAC as soon as they arrive), UAC2 spec
suggests 'ADAPTIVE' synchronization type.

Change UAC2 driver to make it configurable through
additional 'c_sync' configfs file.

Default remains 'asynchronous' with possibility to
switch it to 'adaptive'

Signed-off-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 .../ABI/testing/configfs-usb-gadget-uac2      |   1 +
 Documentation/usb/gadget-testing.rst          |   1 +
 drivers/usb/gadget/function/f_uac2.c          | 100 ++++++++++++++++--
 drivers/usb/gadget/function/u_uac2.h          |   2 +
 4 files changed, 95 insertions(+), 9 deletions(-)

diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uac2 b/Documentation/ABI/testing/configfs-usb-gadget-uac2
index d4356c8b8cd6..e7e59d7fb12f 100644
--- a/Documentation/ABI/testing/configfs-usb-gadget-uac2
+++ b/Documentation/ABI/testing/configfs-usb-gadget-uac2
@@ -8,6 +8,7 @@ Description:
 		c_chmask   capture channel mask
 		c_srate    capture sampling rate
 		c_ssize    capture sample size (bytes)
+		c_sync     capture synchronization type (async/adaptive)
 		p_chmask   playback channel mask
 		p_srate    playback sampling rate
 		p_ssize    playback sample size (bytes)
diff --git a/Documentation/usb/gadget-testing.rst b/Documentation/usb/gadget-testing.rst
index 2085e7b24eeb..f5a12667bd41 100644
--- a/Documentation/usb/gadget-testing.rst
+++ b/Documentation/usb/gadget-testing.rst
@@ -728,6 +728,7 @@ The uac2 function provides these attributes in its function directory:
 	c_chmask	capture channel mask
 	c_srate		capture sampling rate
 	c_ssize		capture sample size (bytes)
+	c_sync		capture synchronization type (async/adaptive)
 	p_chmask	playback channel mask
 	p_srate		playback sampling rate
 	p_ssize		playback sample size (bytes)
diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c
index 5d63244ba319..321e6c05ba93 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -44,6 +44,7 @@
 
 #define EPIN_EN(_opts) ((_opts)->p_chmask != 0)
 #define EPOUT_EN(_opts) ((_opts)->c_chmask != 0)
+#define EPOUT_FBACK_IN_EN(_opts) ((_opts)->c_sync == USB_ENDPOINT_SYNC_ASYNC)
 
 struct f_uac2 {
 	struct g_audio g_audio;
@@ -240,7 +241,7 @@ static struct usb_interface_descriptor std_as_out_if1_desc = {
 	.bDescriptorType = USB_DT_INTERFACE,
 
 	.bAlternateSetting = 1,
-	.bNumEndpoints = 2,
+	.bNumEndpoints = 1,
 	.bInterfaceClass = USB_CLASS_AUDIO,
 	.bInterfaceSubClass = USB_SUBCLASS_AUDIOSTREAMING,
 	.bInterfaceProtocol = UAC_VERSION_2,
@@ -273,7 +274,7 @@ static struct usb_endpoint_descriptor fs_epout_desc = {
 	.bDescriptorType = USB_DT_ENDPOINT,
 
 	.bEndpointAddress = USB_DIR_OUT,
-	.bmAttributes = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ASYNC,
+	/* .bmAttributes = DYNAMIC */
 	/* .wMaxPacketSize = DYNAMIC */
 	.bInterval = 1,
 };
@@ -282,7 +283,7 @@ static struct usb_endpoint_descriptor hs_epout_desc = {
 	.bLength = USB_DT_ENDPOINT_SIZE,
 	.bDescriptorType = USB_DT_ENDPOINT,
 
-	.bmAttributes = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ASYNC,
+	/* .bmAttributes = DYNAMIC */
 	/* .wMaxPacketSize = DYNAMIC */
 	.bInterval = 4,
 };
@@ -292,7 +293,7 @@ static struct usb_endpoint_descriptor ss_epout_desc = {
 	.bDescriptorType = USB_DT_ENDPOINT,
 
 	.bEndpointAddress = USB_DIR_OUT,
-	.bmAttributes = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ASYNC,
+	/* .bmAttributes = DYNAMIC */
 	/* .wMaxPacketSize = DYNAMIC */
 	.bInterval = 4,
 };
@@ -649,7 +650,9 @@ static void setup_headers(struct f_uac2_opts *opts,
 			headers[i++] = USBDHDR(epout_desc_comp);
 
 		headers[i++] = USBDHDR(&as_iso_out_desc);
-		headers[i++] = USBDHDR(epin_fback_desc);
+
+		if (EPOUT_FBACK_IN_EN(opts))
+			headers[i++] = USBDHDR(epin_fback_desc);
 	}
 	if (EPIN_EN(opts)) {
 		headers[i++] = USBDHDR(&std_as_in_if0_desc);
@@ -820,6 +823,23 @@ afunc_bind(struct usb_configuration *cfg, struct usb_function *fn)
 		std_as_out_if1_desc.bInterfaceNumber = ret;
 		uac2->as_out_intf = ret;
 		uac2->as_out_alt = 0;
+
+		if (EPOUT_FBACK_IN_EN(uac2_opts)) {
+			fs_epout_desc.bmAttributes =
+			  USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ASYNC;
+			hs_epout_desc.bmAttributes =
+			  USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ASYNC;
+			ss_epout_desc.bmAttributes =
+			  USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ASYNC;
+			std_as_out_if1_desc.bNumEndpoints++;
+		} else {
+			fs_epout_desc.bmAttributes =
+			  USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ADAPTIVE;
+			hs_epout_desc.bmAttributes =
+			  USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ADAPTIVE;
+			ss_epout_desc.bmAttributes =
+			  USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ADAPTIVE;
+		}
 	}
 
 	if (EPIN_EN(uac2_opts)) {
@@ -883,11 +903,14 @@ afunc_bind(struct usb_configuration *cfg, struct usb_function *fn)
 			dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
 			return -ENODEV;
 		}
-		agdev->in_ep_fback = usb_ep_autoconfig(gadget,
+		if (EPOUT_FBACK_IN_EN(uac2_opts)) {
+			agdev->in_ep_fback = usb_ep_autoconfig(gadget,
 						       &fs_epin_fback_desc);
-		if (!agdev->in_ep_fback) {
-			dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
-			return -ENODEV;
+			if (!agdev->in_ep_fback) {
+				dev_err(dev, "%s:%d Error!\n",
+					__func__, __LINE__);
+				return -ENODEV;
+			}
 		}
 	}
 
@@ -1242,11 +1265,68 @@ end:									\
 									\
 CONFIGFS_ATTR(f_uac2_opts_, name)
 
+#define UAC2_ATTRIBUTE_SYNC(name)					\
+static ssize_t f_uac2_opts_##name##_show(struct config_item *item,	\
+					 char *page)			\
+{									\
+	struct f_uac2_opts *opts = to_f_uac2_opts(item);		\
+	int result;							\
+	char *str;							\
+									\
+	mutex_lock(&opts->lock);					\
+	switch (opts->name) {						\
+	case USB_ENDPOINT_SYNC_ASYNC:					\
+		str = "async";						\
+		break;							\
+	case USB_ENDPOINT_SYNC_ADAPTIVE:				\
+		str = "adaptive";					\
+		break;							\
+	default:							\
+		str = "unknown";					\
+		break;							\
+	}								\
+	result = sprintf(page, "%s\n", str);				\
+	mutex_unlock(&opts->lock);					\
+									\
+	return result;							\
+}									\
+									\
+static ssize_t f_uac2_opts_##name##_store(struct config_item *item,	\
+					  const char *page, size_t len)	\
+{									\
+	struct f_uac2_opts *opts = to_f_uac2_opts(item);		\
+	int ret = 0;							\
+									\
+	mutex_lock(&opts->lock);					\
+	if (opts->refcnt) {						\
+		ret = -EBUSY;						\
+		goto end;						\
+	}								\
+									\
+	if (!strncmp(page, "async", 5))					\
+		opts->name = USB_ENDPOINT_SYNC_ASYNC;			\
+	else if (!strncmp(page, "adaptive", 8))				\
+		opts->name = USB_ENDPOINT_SYNC_ADAPTIVE;		\
+	else {								\
+		ret = -EINVAL;						\
+		goto end;						\
+	}								\
+									\
+	ret = len;							\
+									\
+end:									\
+	mutex_unlock(&opts->lock);					\
+	return ret;							\
+}									\
+									\
+CONFIGFS_ATTR(f_uac2_opts_, name)
+
 UAC2_ATTRIBUTE(p_chmask);
 UAC2_ATTRIBUTE(p_srate);
 UAC2_ATTRIBUTE(p_ssize);
 UAC2_ATTRIBUTE(c_chmask);
 UAC2_ATTRIBUTE(c_srate);
+UAC2_ATTRIBUTE_SYNC(c_sync);
 UAC2_ATTRIBUTE(c_ssize);
 UAC2_ATTRIBUTE(req_number);
 
@@ -1257,6 +1337,7 @@ static struct configfs_attribute *f_uac2_attrs[] = {
 	&f_uac2_opts_attr_c_chmask,
 	&f_uac2_opts_attr_c_srate,
 	&f_uac2_opts_attr_c_ssize,
+	&f_uac2_opts_attr_c_sync,
 	&f_uac2_opts_attr_req_number,
 	NULL,
 };
@@ -1295,6 +1376,7 @@ static struct usb_function_instance *afunc_alloc_inst(void)
 	opts->c_chmask = UAC2_DEF_CCHMASK;
 	opts->c_srate = UAC2_DEF_CSRATE;
 	opts->c_ssize = UAC2_DEF_CSSIZE;
+	opts->c_sync = UAC2_DEF_CSYNC;
 	opts->req_number = UAC2_DEF_REQ_NUM;
 	return &opts->func_inst;
 }
diff --git a/drivers/usb/gadget/function/u_uac2.h b/drivers/usb/gadget/function/u_uac2.h
index b5035711172d..13589c3c805c 100644
--- a/drivers/usb/gadget/function/u_uac2.h
+++ b/drivers/usb/gadget/function/u_uac2.h
@@ -21,6 +21,7 @@
 #define UAC2_DEF_CCHMASK 0x3
 #define UAC2_DEF_CSRATE 64000
 #define UAC2_DEF_CSSIZE 2
+#define UAC2_DEF_CSYNC		USB_ENDPOINT_SYNC_ASYNC
 #define UAC2_DEF_REQ_NUM 2
 
 struct f_uac2_opts {
@@ -31,6 +32,7 @@ struct f_uac2_opts {
 	int				c_chmask;
 	int				c_srate;
 	int				c_ssize;
+	int				c_sync;
 	int				req_number;
 	bool				bound;
 
-- 
2.31.1

[RFC PATCH v2 3/3] usb: gadget: u_audio: add real feedback implementation

[Jerome Brunet]

From: Ruslan Bilovol <ruslan.bilovol@gmail.com>

This adds interface between userspace and feedback endpoint to report real
feedback frequency to the Host.

Current implementation adds new userspace interface ALSA mixer control
"Capture Pitch 1000000" (similar to aloop driver's "PCM Rate Shift 100000"
mixer control)

Value in PPM is chosen to have correction value agnostic of the actual HW
rate, which the application is not necessarily dealing with, while still
retaining a good enough precision to allow smooth clock correction on the
playback side, if necessary.

Similar to sound/usb/endpoint.c, a slow down is allowed up to 25%. This
has no impact on the required bandwidth. Speedup correction has an impact
on the bandwidth reserved for the isochronous endpoint. The default
allowed speedup is 500ppm. This seems to be more than enough but, if
necessary, this is configurable through a module parameter. The reserved
bandwidth is rounded up to the next packet size.

Usage of this new control is easy to implement in existing userspace tools
like alsaloop from alsa-utils.

Signed-off-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 .../ABI/testing/configfs-usb-gadget-uac2      |   1 +
 Documentation/usb/gadget-testing.rst          |   1 +
 drivers/usb/gadget/function/f_uac2.c          |   9 +-
 drivers/usb/gadget/function/u_audio.c         | 111 ++++++++++++++++--
 drivers/usb/gadget/function/u_audio.h         |   9 ++
 drivers/usb/gadget/function/u_uac2.h          |   2 +
 6 files changed, 124 insertions(+), 9 deletions(-)

diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uac2 b/Documentation/ABI/testing/configfs-usb-gadget-uac2
index e7e59d7fb12f..26fb8e9b4e61 100644
--- a/Documentation/ABI/testing/configfs-usb-gadget-uac2
+++ b/Documentation/ABI/testing/configfs-usb-gadget-uac2
@@ -9,6 +9,7 @@ Description:
 		c_srate    capture sampling rate
 		c_ssize    capture sample size (bytes)
 		c_sync     capture synchronization type (async/adaptive)
+		fb_max     maximum extra bandwidth in async mode
 		p_chmask   playback channel mask
 		p_srate    playback sampling rate
 		p_ssize    playback sample size (bytes)
diff --git a/Documentation/usb/gadget-testing.rst b/Documentation/usb/gadget-testing.rst
index f5a12667bd41..9d6276f82774 100644
--- a/Documentation/usb/gadget-testing.rst
+++ b/Documentation/usb/gadget-testing.rst
@@ -729,6 +729,7 @@ The uac2 function provides these attributes in its function directory:
 	c_srate		capture sampling rate
 	c_ssize		capture sample size (bytes)
 	c_sync		capture synchronization type (async/adaptive)
+	fb_max          maximum extra bandwidth in async mode
 	p_chmask	playback channel mask
 	p_srate		playback sampling rate
 	p_ssize		playback sample size (bytes)
diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c
index 321e6c05ba93..ae29ff2b2b68 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -584,8 +584,11 @@ static int set_ep_max_packet_size(const struct f_uac2_opts *uac2_opts,
 		ssize = uac2_opts->c_ssize;
 	}
 
+	if (!is_playback && (uac2_opts->c_sync == USB_ENDPOINT_SYNC_ASYNC))
+		srate = srate * (1000 + uac2_opts->fb_max) / 1000;
+
 	max_size_bw = num_channels(chmask) * ssize *
-		((srate / (factor / (1 << (ep_desc->bInterval - 1)))) + 1);
+		DIV_ROUND_UP(srate, factor / (1 << (ep_desc->bInterval - 1)));
 	ep_desc->wMaxPacketSize = cpu_to_le16(min_t(u16, max_size_bw,
 						    max_size_ep));
 
@@ -957,6 +960,7 @@ afunc_bind(struct usb_configuration *cfg, struct usb_function *fn)
 	agdev->params.c_srate = uac2_opts->c_srate;
 	agdev->params.c_ssize = uac2_opts->c_ssize;
 	agdev->params.req_number = uac2_opts->req_number;
+	agdev->params.fb_max = uac2_opts->fb_max;
 	ret = g_audio_setup(agdev, "UAC2 PCM", "UAC2_Gadget");
 	if (ret)
 		goto err_free_descs;
@@ -1329,6 +1333,7 @@ UAC2_ATTRIBUTE(c_srate);
 UAC2_ATTRIBUTE_SYNC(c_sync);
 UAC2_ATTRIBUTE(c_ssize);
 UAC2_ATTRIBUTE(req_number);
+UAC2_ATTRIBUTE(fb_max);
 
 static struct configfs_attribute *f_uac2_attrs[] = {
 	&f_uac2_opts_attr_p_chmask,
@@ -1339,6 +1344,7 @@ static struct configfs_attribute *f_uac2_attrs[] = {
 	&f_uac2_opts_attr_c_ssize,
 	&f_uac2_opts_attr_c_sync,
 	&f_uac2_opts_attr_req_number,
+	&f_uac2_opts_attr_fb_max,
 	NULL,
 };
 
@@ -1378,6 +1384,7 @@ static struct usb_function_instance *afunc_alloc_inst(void)
 	opts->c_ssize = UAC2_DEF_CSSIZE;
 	opts->c_sync = UAC2_DEF_CSYNC;
 	opts->req_number = UAC2_DEF_REQ_NUM;
+	opts->fb_max = UAC2_DEF_FB_MAX;
 	return &opts->func_inst;
 }
 
diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c
index f637ebec80b0..b517beb17006 100644
--- a/drivers/usb/gadget/function/u_audio.c
+++ b/drivers/usb/gadget/function/u_audio.c
@@ -16,6 +16,7 @@
 #include <sound/core.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
+#include <sound/control.h>
 
 #include "u_audio.h"
 
@@ -35,12 +36,12 @@ struct uac_rtd_params {
 
 	void *rbuf;
 
+	unsigned int pitch;	/* Stream pitch ratio to 1000000 */
 	unsigned int max_psize;	/* MaxPacketSize of endpoint */
 
 	struct usb_request **reqs;
 
 	struct usb_request *req_fback; /* Feedback endpoint request */
-	unsigned int ffback; /* Real frequency reported by feedback endpoint */
 	bool fb_ep_enabled; /* if the ep is enabled */
 };
 
@@ -75,7 +76,9 @@ static const struct snd_pcm_hardware uac_pcm_hardware = {
 };
 
 static void u_audio_set_fback_frequency(enum usb_device_speed speed,
-					unsigned int freq, void *buf)
+					unsigned long long freq,
+					unsigned int pitch,
+					void *buf)
 {
 	u32 ff = 0;
 
@@ -86,7 +89,7 @@ static void u_audio_set_fback_frequency(enum usb_device_speed speed,
 		 * Format is encoded in Q10.10 left-justified in the 24 bits,
 		 * so that it has a Q10.14 format.
 		 */
-		ff = DIV_ROUND_UP((freq << 14), 1000);
+		freq <<= 14;
 	} else {
 		/*
 		 * High-speed feedback endpoints report frequency
@@ -95,8 +98,11 @@ static void u_audio_set_fback_frequency(enum usb_device_speed speed,
 		 * the binary point is located between the second and the third
 		 * byte fromat (that is Q16.16)
 		 */
-		ff = DIV_ROUND_UP((freq << 13), 1000);
+		freq <<= 13;
 	}
+
+	ff = DIV_ROUND_CLOSEST_ULL((freq * pitch), (1000 * 1000000));
+
 	*(__le32 *)buf = cpu_to_le32(ff);
 }
 
@@ -209,8 +215,8 @@ static void u_audio_iso_fback_complete(struct usb_ep *ep,
 	struct uac_rtd_params *prm = req->context;
 	struct snd_uac_chip *uac = prm->uac;
 	struct g_audio *audio_dev = uac->audio_dev;
+	struct uac_params *params = &audio_dev->params;
 	int status = req->status;
-	unsigned long flags;
 
 	/* i/f shutting down */
 	if (!prm->fb_ep_enabled || req->status == -ESHUTDOWN)
@@ -225,7 +231,8 @@ static void u_audio_iso_fback_complete(struct usb_ep *ep,
 			__func__, status, req->actual, req->length);
 
 	u_audio_set_fback_frequency(audio_dev->gadget->speed,
-				    prm->ffback, req->buf);
+				    params->c_srate, prm->pitch,
+				    req->buf);
 
 	if (usb_ep_queue(ep, req, GFP_ATOMIC))
 		dev_err(uac->card->dev, "%d Error!\n", __LINE__);
@@ -480,9 +487,10 @@ int u_audio_start_capture(struct g_audio *audio_dev)
 	 * Always start with original frequency since its deviation can't
 	 * be meauserd at start of playback
 	 */
-	prm->ffback = params->c_srate;
+	prm->pitch = 1000000;
 	u_audio_set_fback_frequency(audio_dev->gadget->speed,
-				    prm->ffback, req_fback->buf);
+				    params->c_srate, prm->pitch,
+				    req_fback->buf);
 
 	if (usb_ep_queue(ep_fback, req_fback, GFP_ATOMIC))
 		dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
@@ -578,12 +586,82 @@ void u_audio_stop_playback(struct g_audio *audio_dev)
 }
 EXPORT_SYMBOL_GPL(u_audio_stop_playback);
 
+static int u_audio_pitch_info(struct snd_kcontrol *kcontrol,
+				   struct snd_ctl_elem_info *uinfo)
+{
+	struct uac_rtd_params *prm = snd_kcontrol_chip(kcontrol);
+	struct snd_uac_chip *uac = prm->uac;
+	struct g_audio *audio_dev = uac->audio_dev;
+	struct uac_params *params = &audio_dev->params;
+	unsigned int pitch_min, pitch_max;
+
+	pitch_min = (1000 - FBACK_SLOW_MAX) * 1000;
+	pitch_max = (1000 + params->fb_max) * 1000;
+
+	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
+	uinfo->count = 1;
+	uinfo->value.integer.min = pitch_min;
+	uinfo->value.integer.max = pitch_max;
+	uinfo->value.integer.step = 1;
+	return 0;
+}
+
+static int u_audio_pitch_get(struct snd_kcontrol *kcontrol,
+				   struct snd_ctl_elem_value *ucontrol)
+{
+	struct uac_rtd_params *prm = snd_kcontrol_chip(kcontrol);
+
+	ucontrol->value.integer.value[0] = prm->pitch;
+
+	return 0;
+}
+
+static int u_audio_pitch_put(struct snd_kcontrol *kcontrol,
+				  struct snd_ctl_elem_value *ucontrol)
+{
+	struct uac_rtd_params *prm = snd_kcontrol_chip(kcontrol);
+	struct snd_uac_chip *uac = prm->uac;
+	struct g_audio *audio_dev = uac->audio_dev;
+	struct uac_params *params = &audio_dev->params;
+	unsigned int val;
+	unsigned int pitch_min, pitch_max;
+	int change = 0;
+
+	pitch_min = (1000 - FBACK_SLOW_MAX) * 1000;
+	pitch_max = (1000 + params->fb_max) * 1000;
+
+	val = ucontrol->value.integer.value[0];
+
+	if (val < pitch_min)
+		val = pitch_min;
+	if (val > pitch_max)
+		val = pitch_max;
+
+	if (prm->pitch != val) {
+		prm->pitch = val;
+		change = 1;
+	}
+
+	return change;
+}
+
+static const struct snd_kcontrol_new u_audio_controls[]  = {
+{
+	.iface =        SNDRV_CTL_ELEM_IFACE_PCM,
+	.name =         "Capture Pitch 1000000",
+	.info =         u_audio_pitch_info,
+	.get =          u_audio_pitch_get,
+	.put =          u_audio_pitch_put,
+},
+};
+
 int g_audio_setup(struct g_audio *g_audio, const char *pcm_name,
 					const char *card_name)
 {
 	struct snd_uac_chip *uac;
 	struct snd_card *card;
 	struct snd_pcm *pcm;
+	struct snd_kcontrol *kctl;
 	struct uac_params *params;
 	int p_chmask, c_chmask;
 	int err;
@@ -671,6 +749,23 @@ int g_audio_setup(struct g_audio *g_audio, const char *pcm_name,
 	snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &uac_pcm_ops);
 	snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, &uac_pcm_ops);
 
+	if (c_chmask && g_audio->in_ep_fback) {
+		strscpy(card->mixername, card_name, sizeof(card->driver));
+
+		kctl = snd_ctl_new1(&u_audio_controls[0], &uac->c_prm);
+		if (!kctl) {
+			err = -ENOMEM;
+			goto snd_fail;
+		}
+
+		kctl->id.device = pcm->device;
+		kctl->id.subdevice = 0;
+
+		err = snd_ctl_add(card, kctl);
+		if (err < 0)
+			goto snd_fail;
+	}
+
 	strscpy(card->driver, card_name, sizeof(card->driver));
 	strscpy(card->shortname, card_name, sizeof(card->shortname));
 	sprintf(card->longname, "%s %i", card_name, card->dev->id);
diff --git a/drivers/usb/gadget/function/u_audio.h b/drivers/usb/gadget/function/u_audio.h
index 53e6baf55cbf..a218cdf771fe 100644
--- a/drivers/usb/gadget/function/u_audio.h
+++ b/drivers/usb/gadget/function/u_audio.h
@@ -11,6 +11,14 @@
 
 #include <linux/usb/composite.h>
 
+/*
+ * Same maximum frequency deviation on the slower side as in
+ * sound/usb/endpoint.c. Value is expressed in per-mil deviation.
+ * The maximum deviation on the faster side will be provided as
+ * parameter, as it impacts the endpoint required bandwidth.
+ */
+#define FBACK_SLOW_MAX	250
+
 struct uac_params {
 	/* playback */
 	int p_chmask;	/* channel mask */
@@ -23,6 +31,7 @@ struct uac_params {
 	int c_ssize;	/* sample size */
 
 	int req_number; /* number of preallocated requests */
+	int fb_max;	/* upper frequency drift feedback limit per-mil */
 };
 
 struct g_audio {
diff --git a/drivers/usb/gadget/function/u_uac2.h b/drivers/usb/gadget/function/u_uac2.h
index 13589c3c805c..179d3ef6a195 100644
--- a/drivers/usb/gadget/function/u_uac2.h
+++ b/drivers/usb/gadget/function/u_uac2.h
@@ -23,6 +23,7 @@
 #define UAC2_DEF_CSSIZE 2
 #define UAC2_DEF_CSYNC		USB_ENDPOINT_SYNC_ASYNC
 #define UAC2_DEF_REQ_NUM 2
+#define UAC2_DEF_FB_MAX 5
 
 struct f_uac2_opts {
 	struct usb_function_instance	func_inst;
@@ -34,6 +35,7 @@ struct f_uac2_opts {
 	int				c_ssize;
 	int				c_sync;
 	int				req_number;
+	int				fb_max;
 	bool				bound;
 
 	struct mutex			lock;
-- 
2.31.1

Re: [RFC PATCH v2 1/3] usb: gadget: f_uac2/u_audio: add feedback endpoint support

[Pavel Hofman]

Dne 30. 04. 21 v 16:26 Jerome Brunet napsal(a):
> From: Ruslan Bilovol <ruslan.bilovol@gmail.com>
> 
> As per USB and UAC2 specs, asynchronous audio sink endpoint
> requires explicit synchronization mechanism (Isochronous
> Feedback Endpoint)
> 
> Implement feedback companion endpoint for ISO OUT endpoint
> 
> This patch adds all required infrastructure and USB requests
> handling for feedback endpoint. Syncrhonization itself is
> still dummy (feedback ep always reports 'nomimal frequency'
>  e.g. no adjustement is needed). This satisfies hosts that
> require feedback endpoint (like Win10) and poll it periodically
> 
> Actual synchronization mechanism should be implemented
> separately
> 
> Signed-off-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>

Hi,

The HS calculation of Q16.16 feedback value
overflows at some 524kHz, disallowing use of larger samplerates (e.g.
768kHz or higher).

I tested the formula used in alsa USB driver
https://github.com/torvalds/linux/blob/d99676af540c2dc829999928fb81c58c80a1dce4/sound/usb/endpoint.c#L80
which uses only 10bit shift. The feedback control in UAC2 gadget now
works up to 4M samplerate with 1Hz precision (tested on RPi4 with
bInterval = 1, checked in stream0 proc file on linux host).

--- a/drivers/usb/gadget/function/u_audio.c
+++ b/drivers/usb/gadget/function/u_audio.c
@@ -118,7 +119,8 @@ static void u_audio_set_fback_frequency(enum
usb_device_speed speed,
                 * Prevent integer overflow by calculating in Q12.13
format and
                 * then shifting to Q16.16
                 */
-               ff = DIV_ROUND_UP((freq << 13), (8*1000)) << 3;
+               ff = ((freq << 10) + 62) / 125;
        }
        *(__le32 *)buf = cpu_to_le32(ff);
 }


Best regards,

Pavel.

Re: [RFC PATCH v2 1/3] usb: gadget: f_uac2/u_audio: add feedback endpoint support

[Jerome Brunet]

On Fri 30 Apr 2021 at 16:55, Pavel Hofman <pavel.hofman@ivitera.com> wrote:

> Dne 30. 04. 21 v 16:26 Jerome Brunet napsal(a):
>> From: Ruslan Bilovol <ruslan.bilovol@gmail.com>
>> 
>> As per USB and UAC2 specs, asynchronous audio sink endpoint
>> requires explicit synchronization mechanism (Isochronous
>> Feedback Endpoint)
>> 
>> Implement feedback companion endpoint for ISO OUT endpoint
>> 
>> This patch adds all required infrastructure and USB requests
>> handling for feedback endpoint. Syncrhonization itself is
>> still dummy (feedback ep always reports 'nomimal frequency'
>>  e.g. no adjustement is needed). This satisfies hosts that
>> require feedback endpoint (like Win10) and poll it periodically
>> 
>> Actual synchronization mechanism should be implemented
>> separately
>> 
>> Signed-off-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>
> Hi,
>
> The HS calculation of Q16.16 feedback value
> overflows at some 524kHz, disallowing use of larger samplerates (e.g.
> 768kHz or higher).
>
> I tested the formula used in alsa USB driver
> https://github.com/torvalds/linux/blob/d99676af540c2dc829999928fb81c58c80a1dce4/sound/usb/endpoint.c#L80
> which uses only 10bit shift. The feedback control in UAC2 gadget now
> works up to 4M samplerate with 1Hz precision (tested on RPi4 with
> bInterval = 1, checked in stream0 proc file on linux host).
>
> --- a/drivers/usb/gadget/function/u_audio.c
> +++ b/drivers/usb/gadget/function/u_audio.c
> @@ -118,7 +119,8 @@ static void u_audio_set_fback_frequency(enum
> usb_device_speed speed,
>                  * Prevent integer overflow by calculating in Q12.13
> format and
>                  * then shifting to Q16.16
>                  */
> -               ff = DIV_ROUND_UP((freq << 13), (8*1000)) << 3;
> +               ff = ((freq << 10) + 62) / 125;

Pavel, The code posted is a little different from snip here.
While I understand the "<< 10" and "/ 125", the "+ 62" would welcome a
comment.

Also in the final patch, the calculation is a bit different and moved to
"long long" ... but I'm sure the same type of improvement could be done.

>         }
>         *(__le32 *)buf = cpu_to_le32(ff);
>  }
>
>
> Best regards,
>
> Pavel.

Re: [RFC PATCH v2 1/3] usb: gadget: f_uac2/u_audio: add feedback endpoint support

[Jerome Brunet]

On Fri 30 Apr 2021 at 19:09, Jerome Brunet <jbrunet@baylibre.com> wrote:

> On Fri 30 Apr 2021 at 16:55, Pavel Hofman <pavel.hofman@ivitera.com> wrote:
>
>> Dne 30. 04. 21 v 16:26 Jerome Brunet napsal(a):
>>> From: Ruslan Bilovol <ruslan.bilovol@gmail.com>
>>> 
>>> As per USB and UAC2 specs, asynchronous audio sink endpoint
>>> requires explicit synchronization mechanism (Isochronous
>>> Feedback Endpoint)
>>> 
>>> Implement feedback companion endpoint for ISO OUT endpoint
>>> 
>>> This patch adds all required infrastructure and USB requests
>>> handling for feedback endpoint. Syncrhonization itself is
>>> still dummy (feedback ep always reports 'nomimal frequency'
>>>  e.g. no adjustement is needed). This satisfies hosts that
>>> require feedback endpoint (like Win10) and poll it periodically
>>> 
>>> Actual synchronization mechanism should be implemented
>>> separately
>>> 
>>> Signed-off-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>
>>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>>
>> Hi,
>>
>> The HS calculation of Q16.16 feedback value
>> overflows at some 524kHz, disallowing use of larger samplerates (e.g.
>> 768kHz or higher).
>>
>> I tested the formula used in alsa USB driver
>> https://github.com/torvalds/linux/blob/d99676af540c2dc829999928fb81c58c80a1dce4/sound/usb/endpoint.c#L80
>> which uses only 10bit shift. The feedback control in UAC2 gadget now
>> works up to 4M samplerate with 1Hz precision (tested on RPi4 with
>> bInterval = 1, checked in stream0 proc file on linux host).
>>
>> --- a/drivers/usb/gadget/function/u_audio.c
>> +++ b/drivers/usb/gadget/function/u_audio.c
>> @@ -118,7 +119,8 @@ static void u_audio_set_fback_frequency(enum
>> usb_device_speed speed,
>>                  * Prevent integer overflow by calculating in Q12.13
>> format and
>>                  * then shifting to Q16.16
>>                  */
>> -               ff = DIV_ROUND_UP((freq << 13), (8*1000)) << 3;
>> +               ff = ((freq << 10) + 62) / 125;
>
> Pavel, The code posted is a little different from snip here.
> While I understand the "<< 10" and "/ 125", the "+ 62" would welcome a
> comment.

OOhhh I got it now ... I think using ROUND_UP() is lot more readable
(and maintainable) 

>
> Also in the final patch, the calculation is a bit different and moved to
> "long long" ... but I'm sure the same type of improvement could be done.
>
>>         }
>>         *(__le32 *)buf = cpu_to_le32(ff);
>>  }
>>
>>
>> Best regards,
>>
>> Pavel.

Re: [RFC PATCH v2 1/3] usb: gadget: f_uac2/u_audio: add feedback endpoint support

[Pavel Hofman]

Dne 30. 04. 21 v 19:11 Jerome Brunet napsal(a):
> 
> On Fri 30 Apr 2021 at 19:09, Jerome Brunet <jbrunet@baylibre.com> wrote:
> 
>> On Fri 30 Apr 2021 at 16:55, Pavel Hofman <pavel.hofman@ivitera.com> wrote:
>>
>>> Dne 30. 04. 21 v 16:26 Jerome Brunet napsal(a):
>>>> From: Ruslan Bilovol <ruslan.bilovol@gmail.com>
>>>>
>>>> As per USB and UAC2 specs, asynchronous audio sink endpoint
>>>> requires explicit synchronization mechanism (Isochronous
>>>> Feedback Endpoint)
>>>>
>>>> Implement feedback companion endpoint for ISO OUT endpoint
>>>>
>>>> This patch adds all required infrastructure and USB requests
>>>> handling for feedback endpoint. Syncrhonization itself is
>>>> still dummy (feedback ep always reports 'nomimal frequency'
>>>>   e.g. no adjustement is needed). This satisfies hosts that
>>>> require feedback endpoint (like Win10) and poll it periodically
>>>>
>>>> Actual synchronization mechanism should be implemented
>>>> separately
>>>>
>>>> Signed-off-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>
>>>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>>>
>>> Hi,
>>>
>>> The HS calculation of Q16.16 feedback value
>>> overflows at some 524kHz, disallowing use of larger samplerates (e.g.
>>> 768kHz or higher).
>>>
>>> I tested the formula used in alsa USB driver
>>> https://github.com/torvalds/linux/blob/d99676af540c2dc829999928fb81c58c80a1dce4/sound/usb/endpoint.c#L80
>>> which uses only 10bit shift. The feedback control in UAC2 gadget now
>>> works up to 4M samplerate with 1Hz precision (tested on RPi4 with
>>> bInterval = 1, checked in stream0 proc file on linux host).
>>>
>>> --- a/drivers/usb/gadget/function/u_audio.c
>>> +++ b/drivers/usb/gadget/function/u_audio.c
>>> @@ -118,7 +119,8 @@ static void u_audio_set_fback_frequency(enum
>>> usb_device_speed speed,
>>>                   * Prevent integer overflow by calculating in Q12.13
>>> format and
>>>                   * then shifting to Q16.16
>>>                   */
>>> -               ff = DIV_ROUND_UP((freq << 13), (8*1000)) << 3;
>>> +               ff = ((freq << 10) + 62) / 125;
>>
>> Pavel, The code posted is a little different from snip here.
>> While I understand the "<< 10" and "/ 125", the "+ 62" would welcome a
>> comment.
> 
> OOhhh I got it now ... I think using ROUND_UP() is lot more readable
> (and maintainable)
> 
>>
>> Also in the final patch, the calculation is a bit different and moved to
>> "long long" ... but I'm sure the same type of improvement could be done.
>>
>>>          }
>>>          *(__le32 *)buf = cpu_to_le32(ff);
>>>   }

I copied the code from the very same calculation in the current-version 
alsa usb host driver, as linked above. Hence I assumed it should have no 
problems in the gadget code either. All I can confirm is that it works 
in the gadget properly and does not overflow with ff up to 4M. The 
gadget can easily run at megahertz samplerates, there are standard 
USB-audio devices with 768kHz samplerates.

Thanks a lot. Best regards,

Pavel.

Re: [RFC PATCH v2 1/3] usb: gadget: f_uac2/u_audio: add feedback endpoint support

[Pavel Hofman]

Dne 30. 04. 21 v 19:11 Jerome Brunet napsal(a):
> 
> On Fri 30 Apr 2021 at 19:09, Jerome Brunet <jbrunet@baylibre.com> wrote:
> 
>> On Fri 30 Apr 2021 at 16:55, Pavel Hofman <pavel.hofman@ivitera.com> wrote:
>>
>>> Dne 30. 04. 21 v 16:26 Jerome Brunet napsal(a):
>>>> From: Ruslan Bilovol <ruslan.bilovol@gmail.com>
>>>>
>>>> As per USB and UAC2 specs, asynchronous audio sink endpoint
>>>> requires explicit synchronization mechanism (Isochronous
>>>> Feedback Endpoint)
>>>>
>>>> Implement feedback companion endpoint for ISO OUT endpoint
>>>>
>>>> This patch adds all required infrastructure and USB requests
>>>> handling for feedback endpoint. Syncrhonization itself is
>>>> still dummy (feedback ep always reports 'nomimal frequency'
>>>>  e.g. no adjustement is needed). This satisfies hosts that
>>>> require feedback endpoint (like Win10) and poll it periodically
>>>>
>>>> Actual synchronization mechanism should be implemented
>>>> separately
>>>>
>>>> Signed-off-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>
>>>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>>>
>>> Hi,
>>>
>>> The HS calculation of Q16.16 feedback value
>>> overflows at some 524kHz, disallowing use of larger samplerates (e.g.
>>> 768kHz or higher).
>>>
>>> I tested the formula used in alsa USB driver
>>> https://github.com/torvalds/linux/blob/d99676af540c2dc829999928fb81c58c80a1dce4/sound/usb/endpoint.c#L80
>>> which uses only 10bit shift. The feedback control in UAC2 gadget now
>>> works up to 4M samplerate with 1Hz precision (tested on RPi4 with
>>> bInterval = 1, checked in stream0 proc file on linux host).
>>>
>>> --- a/drivers/usb/gadget/function/u_audio.c
>>> +++ b/drivers/usb/gadget/function/u_audio.c
>>> @@ -118,7 +119,8 @@ static void u_audio_set_fback_frequency(enum
>>> usb_device_speed speed,
>>>                  * Prevent integer overflow by calculating in Q12.13
>>> format and
>>>                  * then shifting to Q16.16
>>>                  */
>>> -               ff = DIV_ROUND_UP((freq << 13), (8*1000)) << 3;
>>> +               ff = ((freq << 10) + 62) / 125;
>>
>> Pavel, The code posted is a little different from snip here.
>> While I understand the "<< 10" and "/ 125", the "+ 62" would welcome a
>> comment.
> 
> OOhhh I got it now ... I think using ROUND_UP() is lot more readable
> (and maintainable) 
> 
>>
>> Also in the final patch, the calculation is a bit different and moved to
>> "long long" ... but I'm sure the same type of improvement could be done.

Jerome, I see (sorry for the misunderstanding) that you have modified
original Ruslan's patch and the freq variable being shifted by 13 is
long long. Your code is definitely better, thanks.

Pavel.

Re: [RFC PATCH v2 3/3] usb: gadget: u_audio: .... PCM Rate Shift for playback too?

[Pavel Hofman]

Dne 30. 04. 21 v 16:26 Jerome Brunet napsal(a):
> From: Ruslan Bilovol <ruslan.bilovol@gmail.com>
> 
> This adds interface between userspace and feedback endpoint to report real
> feedback frequency to the Host.
> 
> Current implementation adds new userspace interface ALSA mixer control
> "Capture Pitch 1000000" (similar to aloop driver's "PCM Rate Shift 100000"
> mixer control)
> 
> Value in PPM is chosen to have correction value agnostic of the actual HW
> rate, which the application is not necessarily dealing with, while still
> retaining a good enough precision to allow smooth clock correction on the
> playback side, if necessary.
> 
> Similar to sound/usb/endpoint.c, a slow down is allowed up to 25%. This
> has no impact on the required bandwidth. Speedup correction has an impact
> on the bandwidth reserved for the isochronous endpoint. The default
> allowed speedup is 500ppm. This seems to be more than enough but, if
> necessary, this is configurable through a module parameter. The reserved
> bandwidth is rounded up to the next packet size.
> 
> Usage of this new control is easy to implement in existing userspace tools
> like alsaloop from alsa-utils.
> 
> Signed-off-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>


Hi, the existing patches solve the Host -> Gadget -> capturing
application direction, controlling the host playback rate. The other
direction (playback app -> gadget -> capturing host) is still paced by
the host USB controller. Packet size is pre-calculated in
u_audio_start_playback  as rate/p_interval
https://github.com/pavhofman/linux-rpi/blob/rpi-5.10.y/drivers/usb/gadget/function/u_audio.c#L441
and this fixed value is used for copying the audio data in
u_audio_iso_complete
https://github.com/pavhofman/linux-rpi/blob/rpi-5.10.y/drivers/usb/gadget/function/u_audio.c#L124
.

That means if the gadget has a physical duplex audio device with single
clock and runs a duplex operation, the path gadget-> host  will not run
synchronously with the physical audio device (the host -> gadget has
already the feedback control implemented).

How about "duplicating" the existing ALSA mixer control
"Capture Pitch 1000000" to "Playback Pitch 1000000" and using
pitch-adjusted p_srate in the above-linked calculations? That should
make the playback side run at the playback pitch requested by gadget
userspace, IIUC.

For the duplex operation with single clock, the capture pitch value
determined by the userspace chain (alsaloop, CamillaDSP, etc.) would be
used for setting both the capture and playback pitch controls, making
both directions run synchronously.

I can prepare patches based on Jerome's patchset should you find this
solution acceptable.

Thanks a lot,

Pavel.

Re: [RFC PATCH v2 3/3] usb: gadget: u_audio: .... PCM Rate Shift for playback too?

[Jerome Brunet]

On Mon 24 May 2021 at 14:29, Pavel Hofman <pavel.hofman@ivitera.com> wrote:

> Dne 30. 04. 21 v 16:26 Jerome Brunet napsal(a):
>> From: Ruslan Bilovol <ruslan.bilovol@gmail.com>
>> 
>> This adds interface between userspace and feedback endpoint to report real
>> feedback frequency to the Host.
>> 
>> Current implementation adds new userspace interface ALSA mixer control
>> "Capture Pitch 1000000" (similar to aloop driver's "PCM Rate Shift 100000"
>> mixer control)
>> 
>> Value in PPM is chosen to have correction value agnostic of the actual HW
>> rate, which the application is not necessarily dealing with, while still
>> retaining a good enough precision to allow smooth clock correction on the
>> playback side, if necessary.
>> 
>> Similar to sound/usb/endpoint.c, a slow down is allowed up to 25%. This
>> has no impact on the required bandwidth. Speedup correction has an impact
>> on the bandwidth reserved for the isochronous endpoint. The default
>> allowed speedup is 500ppm. This seems to be more than enough but, if
>> necessary, this is configurable through a module parameter. The reserved
>> bandwidth is rounded up to the next packet size.
>> 
>> Usage of this new control is easy to implement in existing userspace tools
>> like alsaloop from alsa-utils.
>> 
>> Signed-off-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>
>
> Hi, the existing patches solve the Host -> Gadget -> capturing
> application direction, controlling the host playback rate. The other
> direction (playback app -> gadget -> capturing host) is still paced by
> the host USB controller. Packet size is pre-calculated in
> u_audio_start_playback  as rate/p_interval
> https://github.com/pavhofman/linux-rpi/blob/rpi-5.10.y/drivers/usb/gadget/function/u_audio.c#L441
> and this fixed value is used for copying the audio data in
> u_audio_iso_complete
> https://github.com/pavhofman/linux-rpi/blob/rpi-5.10.y/drivers/usb/gadget/function/u_audio.c#L124
> .
>
> That means if the gadget has a physical duplex audio device with single
> clock and runs a duplex operation, the path gadget-> host  will not run
> synchronously with the physical audio device (the host -> gadget has
> already the feedback control implemented).
>
> How about "duplicating" the existing ALSA mixer control
> "Capture Pitch 1000000" to "Playback Pitch 1000000" and using
> pitch-adjusted p_srate in the above-linked calculations? That should
> make the playback side run at the playback pitch requested by gadget
> userspace, IIUC.
>
> For the duplex operation with single clock, the capture pitch value
> determined by the userspace chain (alsaloop, CamillaDSP, etc.) would be
> used for setting both the capture and playback pitch controls, making
> both directions run synchronously.
>
> I can prepare patches based on Jerome's patchset should you find this
> solution acceptable.

Well I have experimented with pitch on the playback path.
It does work but I'm not sure if it actually makes sense which is why I
have not not included it in RFC

If you need the playback and capture to run synchronously, you'd be
better off with implicit feedback (In which the host will provide the
same number of samples it received from the device)

With explicit feedback, we are (supposed) to tell the host to speed up
or slow down to match the device clock. This means the device run
asynchronously, and the host does the adaptation (if necessary). In such
case, I'm not sure adding pitch control on the playback path is a good
idea.

Having pitch control on the playback side allows to forward the audio
captured by the physical interface of the device (like I2S) to USB
without performing any resampling between the two (when USB and I2S are
not in sync). It's nice and convenient ... but I wonder if this is a
feature or a hack ;)

>
> Thanks a lot,
>
> Pavel.

Re: [RFC PATCH v2 3/3] usb: gadget: u_audio: .... PCM Rate Shift for playback too?

[Pavel Hofman]

Dne 24. 05. 21 v 17:40 Jerome Brunet napsal(a):
> 
> On Mon 24 May 2021 at 14:29, Pavel Hofman <pavel.hofman@ivitera.com> wrote:
> 
>> Dne 30. 04. 21 v 16:26 Jerome Brunet napsal(a):
>>> From: Ruslan Bilovol <ruslan.bilovol@gmail.com>
>>>
>>> This adds interface between userspace and feedback endpoint to report real
>>> feedback frequency to the Host.
>>>
>>> Current implementation adds new userspace interface ALSA mixer control
>>> "Capture Pitch 1000000" (similar to aloop driver's "PCM Rate Shift 100000"
>>> mixer control)
>>>
>>> Value in PPM is chosen to have correction value agnostic of the actual HW
>>> rate, which the application is not necessarily dealing with, while still
>>> retaining a good enough precision to allow smooth clock correction on the
>>> playback side, if necessary.
>>>
>>> Similar to sound/usb/endpoint.c, a slow down is allowed up to 25%. This
>>> has no impact on the required bandwidth. Speedup correction has an impact
>>> on the bandwidth reserved for the isochronous endpoint. The default
>>> allowed speedup is 500ppm. This seems to be more than enough but, if
>>> necessary, this is configurable through a module parameter. The reserved
>>> bandwidth is rounded up to the next packet size.
>>>
>>> Usage of this new control is easy to implement in existing userspace tools
>>> like alsaloop from alsa-utils.
>>>
>>> Signed-off-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>
>>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>>
>>
>> Hi, the existing patches solve the Host -> Gadget -> capturing
>> application direction, controlling the host playback rate. The other
>> direction (playback app -> gadget -> capturing host) is still paced by
>> the host USB controller. Packet size is pre-calculated in
>> u_audio_start_playback  as rate/p_interval
>> https://github.com/pavhofman/linux-rpi/blob/rpi-5.10.y/drivers/usb/gadget/function/u_audio.c#L441
>> and this fixed value is used for copying the audio data in
>> u_audio_iso_complete
>> https://github.com/pavhofman/linux-rpi/blob/rpi-5.10.y/drivers/usb/gadget/function/u_audio.c#L124
>> .
>>
>> That means if the gadget has a physical duplex audio device with single
>> clock and runs a duplex operation, the path gadget-> host  will not run
>> synchronously with the physical audio device (the host -> gadget has
>> already the feedback control implemented).
>>
>> How about "duplicating" the existing ALSA mixer control
>> "Capture Pitch 1000000" to "Playback Pitch 1000000" and using
>> pitch-adjusted p_srate in the above-linked calculations? That should
>> make the playback side run at the playback pitch requested by gadget
>> userspace, IIUC.
>>
>> For the duplex operation with single clock, the capture pitch value
>> determined by the userspace chain (alsaloop, CamillaDSP, etc.) would be
>> used for setting both the capture and playback pitch controls, making
>> both directions run synchronously.
>>
>> I can prepare patches based on Jerome's patchset should you find this
>> solution acceptable.
> 
> Well I have experimented with pitch on the playback path.
> It does work but I'm not sure if it actually makes sense which is why I
> have not not included it in RFC
> 
> If you need the playback and capture to run synchronously, you'd be
> better off with implicit feedback (In which the host will provide the
> same number of samples it received from the device)
> 
> With explicit feedback, we are (supposed) to tell the host to speed up
> or slow down to match the device clock. This means the device run
> asynchronously, and the host does the adaptation (if necessary). In such
> case, I'm not sure adding pitch control on the playback path is a good
> idea.
> 
> Having pitch control on the playback side allows to forward the audio
> captured by the physical interface of the device (like I2S) to USB
> without performing any resampling between the two (when USB and I2S are
> not in sync). It's nice and convenient ... but I wonder if this is a
> feature or a hack ;)
> 

Jerome, thanks a lot for your reply. The current implementation of the 
EP IN audio direction is timed by the USB host controller. Let's 
consider 48Khz samplerate and bInterval=1 fullspeed (8k packets per 
second). Every IN packet will always carry 6 audio frames. No matter how 
fast the real master clock (i.e. e.g. I2S of the gadget) runs. Until an 
xrun occurs, unfortunately. Even if the gadget configuration used 
implicit feedback, the incoming stream would still provide no 
synchronization information from the I2S hardware clock as the data 
stream is clocked by the USB host which controls the timing on the USB 
bus (and thus the moment of iso completion). Plus the stock usb-audio 
driver in Windows 10 does not support implicit feedback, according to 
https://docs.microsoft.com/en-us/windows-hardware/drivers/audio/usb-2-0-audio-drivers#audio-streaming 
.


The problem is the fixed "slicing" of the samples into the packets which 
cannot be controlled. The same situation was on the host side before the 
feedback EP was introduced. Here the one preparing the slices (the 
"transmitter") is the gadget now. And the slicing pace must be 
controllable just like the slicing pace on the host is via the feedback EP.

Because the gadget supports different playback and capture rates (which 
I find nice), I suggest a separate control element (Playback Pitch, 
Capture Pitch).

Of course the motivation is to avoid adaptive resampling in the gadget 
in the direction I2S -> gadget interface -> USB host. But the very same 
motivation lead to implementation of the async feedback EP in the 
opposite direction. IMO it is not a hack, but an indispensable feature 
making the gadget usable for duplex operation with hardware (i.e. the 
real master) clock at the backend (all the other "clocks" are just 
software-generated slices/blocks of audio frames).

With regards,

Pavel.

Re: [RFC PATCH v2 3/3] usb: gadget: u_audio: .... PCM Rate Shift for playback too?

[Pavel Hofman]

Dne 24. 05. 21 v 18:09 Pavel Hofman napsal(a):
> 
> Dne 24. 05. 21 v 17:40 Jerome Brunet napsal(a):
>>
>> On Mon 24 May 2021 at 14:29, Pavel Hofman <pavel.hofman@ivitera.com> 
>> wrote:
>>
>>> Dne 30. 04. 21 v 16:26 Jerome Brunet napsal(a):
>>>> From: Ruslan Bilovol <ruslan.bilovol@gmail.com>
>>>>
>>>> This adds interface between userspace and feedback endpoint to 
>>>> report real
>>>> feedback frequency to the Host.
>>>>
>>>> Current implementation adds new userspace interface ALSA mixer control
>>>> "Capture Pitch 1000000" (similar to aloop driver's "PCM Rate Shift 
>>>> 100000"
>>>> mixer control)
>>>>
>>>> Value in PPM is chosen to have correction value agnostic of the 
>>>> actual HW
>>>> rate, which the application is not necessarily dealing with, while 
>>>> still
>>>> retaining a good enough precision to allow smooth clock correction 
>>>> on the
>>>> playback side, if necessary.
>>>>
>>>> Similar to sound/usb/endpoint.c, a slow down is allowed up to 25%. This
>>>> has no impact on the required bandwidth. Speedup correction has an 
>>>> impact
>>>> on the bandwidth reserved for the isochronous endpoint. The default
>>>> allowed speedup is 500ppm. This seems to be more than enough but, if
>>>> necessary, this is configurable through a module parameter. The 
>>>> reserved
>>>> bandwidth is rounded up to the next packet size.
>>>>
>>>> Usage of this new control is easy to implement in existing userspace 
>>>> tools
>>>> like alsaloop from alsa-utils.
>>>>
>>>> Signed-off-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>
>>>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>>>
>>>
>>> Hi, the existing patches solve the Host -> Gadget -> capturing
>>> application direction, controlling the host playback rate. The other
>>> direction (playback app -> gadget -> capturing host) is still paced by
>>> the host USB controller. Packet size is pre-calculated in
>>> u_audio_start_playback  as rate/p_interval
>>> https://github.com/pavhofman/linux-rpi/blob/rpi-5.10.y/drivers/usb/gadget/function/u_audio.c#L441 
>>>
>>> and this fixed value is used for copying the audio data in
>>> u_audio_iso_complete
>>> https://github.com/pavhofman/linux-rpi/blob/rpi-5.10.y/drivers/usb/gadget/function/u_audio.c#L124 
>>>
>>> .
>>>
>>> That means if the gadget has a physical duplex audio device with single
>>> clock and runs a duplex operation, the path gadget-> host  will not run
>>> synchronously with the physical audio device (the host -> gadget has
>>> already the feedback control implemented).
>>>
>>> How about "duplicating" the existing ALSA mixer control
>>> "Capture Pitch 1000000" to "Playback Pitch 1000000" and using
>>> pitch-adjusted p_srate in the above-linked calculations? That should
>>> make the playback side run at the playback pitch requested by gadget
>>> userspace, IIUC.
>>>
>>> For the duplex operation with single clock, the capture pitch value
>>> determined by the userspace chain (alsaloop, CamillaDSP, etc.) would be
>>> used for setting both the capture and playback pitch controls, making
>>> both directions run synchronously.
>>>
>>> I can prepare patches based on Jerome's patchset should you find this
>>> solution acceptable.
>>
>> Well I have experimented with pitch on the playback path.
>> It does work but I'm not sure if it actually makes sense which is why I
>> have not not included it in RFC
>>
>> If you need the playback and capture to run synchronously, you'd be
>> better off with implicit feedback (In which the host will provide the
>> same number of samples it received from the device)
>>
>> With explicit feedback, we are (supposed) to tell the host to speed up
>> or slow down to match the device clock. This means the device run
>> asynchronously, and the host does the adaptation (if necessary). In such
>> case, I'm not sure adding pitch control on the playback path is a good
>> idea.
>>
>> Having pitch control on the playback side allows to forward the audio
>> captured by the physical interface of the device (like I2S) to USB
>> without performing any resampling between the two (when USB and I2S are
>> not in sync). It's nice and convenient ... but I wonder if this is a
>> feature or a hack ;)
>>
> 
> Jerome, thanks a lot for your reply. The current implementation of the 
> EP IN audio direction is timed by the USB host controller. Let's 
> consider 48Khz samplerate and bInterval=1 fullspeed (8k packets per 
> second). Every IN packet will always carry 6 audio frames. No matter how 
> fast the real master clock (i.e. e.g. I2S of the gadget) runs. Until an 
> xrun occurs, unfortunately. Even if the gadget configuration used 
> implicit feedback, the incoming stream would still provide no 
> synchronization information from the I2S hardware clock as the data 
> stream is clocked by the USB host which controls the timing on the USB 
> bus (and thus the moment of iso completion). Plus the stock usb-audio 
> driver in Windows 10 does not support implicit feedback, according to 
> https://docs.microsoft.com/en-us/windows-hardware/drivers/audio/usb-2-0-audio-drivers#audio-streaming 
> .
> 
> 
> The problem is the fixed "slicing" of the samples into the packets which 
> cannot be controlled. The same situation was on the host side before the 
> feedback EP was introduced. Here the one preparing the slices (the 
> "transmitter") is the gadget now. And the slicing pace must be 
> controllable just like the slicing pace on the host is via the feedback EP.
> 
> Because the gadget supports different playback and capture rates (which 
> I find nice), I suggest a separate control element (Playback Pitch, 
> Capture Pitch).
> 
> Of course the motivation is to avoid adaptive resampling in the gadget 
> in the direction I2S -> gadget interface -> USB host. But the very same 
> motivation lead to implementation of the async feedback EP in the 
> opposite direction. IMO it is not a hack, but an indispensable feature 
> making the gadget usable for duplex operation with hardware (i.e. the 
> real master) clock at the backend (all the other "clocks" are just 
> software-generated slices/blocks of audio frames).
> 
> With regards,
> 
> Pavel.

Jerome, please do you still have your playback-side patches available or 
should I prepare them? Thanks a lot,

Pavel.

Re: [RFC PATCH v2 3/3] usb: gadget: u_audio: .... PCM Rate Shift for playback too?

[Jerome Brunet]

On Thu 27 May 2021 at 08:52, Pavel Hofman <pavel.hofman@ivitera.com> wrote:

> Dne 24. 05. 21 v 18:09 Pavel Hofman napsal(a):
>> Dne 24. 05. 21 v 17:40 Jerome Brunet napsal(a):
>>>
>>> On Mon 24 May 2021 at 14:29, Pavel Hofman <pavel.hofman@ivitera.com>
>>> wrote:
>>>
>>>> Dne 30. 04. 21 v 16:26 Jerome Brunet napsal(a):
>>>>> From: Ruslan Bilovol <ruslan.bilovol@gmail.com>
>>>>>
>>>>> This adds interface between userspace and feedback endpoint to report
>>>>> real
>>>>> feedback frequency to the Host.
>>>>>
>>>>> Current implementation adds new userspace interface ALSA mixer control
>>>>> "Capture Pitch 1000000" (similar to aloop driver's "PCM Rate Shift
>>>>> 100000"
>>>>> mixer control)
>>>>>
>>>>> Value in PPM is chosen to have correction value agnostic of the actual
>>>>> HW
>>>>> rate, which the application is not necessarily dealing with, while
>>>>> still
>>>>> retaining a good enough precision to allow smooth clock correction on
>>>>> the
>>>>> playback side, if necessary.
>>>>>
>>>>> Similar to sound/usb/endpoint.c, a slow down is allowed up to 25%. This
>>>>> has no impact on the required bandwidth. Speedup correction has an
>>>>> impact
>>>>> on the bandwidth reserved for the isochronous endpoint. The default
>>>>> allowed speedup is 500ppm. This seems to be more than enough but, if
>>>>> necessary, this is configurable through a module parameter. The
>>>>> reserved
>>>>> bandwidth is rounded up to the next packet size.
>>>>>
>>>>> Usage of this new control is easy to implement in existing userspace
>>>>> tools
>>>>> like alsaloop from alsa-utils.
>>>>>
>>>>> Signed-off-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>
>>>>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>>>>
>>>>
>>>> Hi, the existing patches solve the Host -> Gadget -> capturing
>>>> application direction, controlling the host playback rate. The other
>>>> direction (playback app -> gadget -> capturing host) is still paced by
>>>> the host USB controller. Packet size is pre-calculated in
>>>> u_audio_start_playback  as rate/p_interval
>>>> https://github.com/pavhofman/linux-rpi/blob/rpi-5.10.y/drivers/usb/gadget/function/u_audio.c#L441 
>>>> and this fixed value is used for copying the audio data in
>>>> u_audio_iso_complete
>>>> https://github.com/pavhofman/linux-rpi/blob/rpi-5.10.y/drivers/usb/gadget/function/u_audio.c#L124 
>>>> .
>>>>
>>>> That means if the gadget has a physical duplex audio device with single
>>>> clock and runs a duplex operation, the path gadget-> host  will not run
>>>> synchronously with the physical audio device (the host -> gadget has
>>>> already the feedback control implemented).
>>>>
>>>> How about "duplicating" the existing ALSA mixer control
>>>> "Capture Pitch 1000000" to "Playback Pitch 1000000" and using
>>>> pitch-adjusted p_srate in the above-linked calculations? That should
>>>> make the playback side run at the playback pitch requested by gadget
>>>> userspace, IIUC.
>>>>
>>>> For the duplex operation with single clock, the capture pitch value
>>>> determined by the userspace chain (alsaloop, CamillaDSP, etc.) would be
>>>> used for setting both the capture and playback pitch controls, making
>>>> both directions run synchronously.
>>>>
>>>> I can prepare patches based on Jerome's patchset should you find this
>>>> solution acceptable.
>>>
>>> Well I have experimented with pitch on the playback path.
>>> It does work but I'm not sure if it actually makes sense which is why I
>>> have not not included it in RFC
>>>
>>> If you need the playback and capture to run synchronously, you'd be
>>> better off with implicit feedback (In which the host will provide the
>>> same number of samples it received from the device)
>>>
>>> With explicit feedback, we are (supposed) to tell the host to speed up
>>> or slow down to match the device clock. This means the device run
>>> asynchronously, and the host does the adaptation (if necessary). In such
>>> case, I'm not sure adding pitch control on the playback path is a good
>>> idea.
>>>
>>> Having pitch control on the playback side allows to forward the audio
>>> captured by the physical interface of the device (like I2S) to USB
>>> without performing any resampling between the two (when USB and I2S are
>>> not in sync). It's nice and convenient ... but I wonder if this is a
>>> feature or a hack ;)
>>>
>> Jerome, thanks a lot for your reply. The current implementation of the 
>> EP IN audio direction is timed by the USB host controller. Let's consider
>> 48Khz samplerate and bInterval=1 fullspeed (8k packets per second). Every
>> IN packet will always carry 6 audio frames. No matter how fast the real
>> master clock (i.e. e.g. I2S of the gadget) runs. Until an xrun occurs,
>> unfortunately. Even if the gadget configuration used implicit feedback,
>> the incoming stream would still provide no synchronization information
>> from the I2S hardware clock as the data stream is clocked by the USB host
>> which controls the timing on the USB bus (and thus the moment of iso
>> completion). Plus the stock usb-audio driver in Windows 10 does not
>> support implicit feedback, according to
>> https://docs.microsoft.com/en-us/windows-hardware/drivers/audio/usb-2-0-audio-drivers#audio-streaming 
>> .
>> 
>> The problem is the fixed "slicing" of the samples into the packets which
>> cannot be controlled. The same situation was on the host side before the 
>> feedback EP was introduced. Here the one preparing the slices (the
>> "transmitter") is the gadget now. And the slicing pace must be 
>> controllable just like the slicing pace on the host is via the feedback EP.
>> Because the gadget supports different playback and capture rates (which 
>> I find nice), I suggest a separate control element (Playback Pitch,
>> Capture Pitch).
>> Of course the motivation is to avoid adaptive resampling in the gadget 
>> in the direction I2S -> gadget interface -> USB host. But the very same
>> motivation lead to implementation of the async feedback EP in the 
>> opposite direction. IMO it is not a hack, but an indispensable feature
>> making the gadget usable for duplex operation with hardware (i.e. the 
>> real master) clock at the backend (all the other "clocks" are just
>> software-generated slices/blocks of audio frames).
>> With regards,
>> Pavel.
>
> Jerome, please do you still have your playback-side patches available or
> should I prepare them? Thanks a lot,
>

Yes, this is what I have tested:

https://gitlab.com/jbrunet/linux/-/commit/43f1930ba548e137a5d20b1801790fae83eaa1e0
https://gitlab.com/jbrunet/linux/-/commit/acb2ed9d9219488184cd2eb592fcdbf78b042a9e

It probably requires a rebase and cleaning before it is actually ready
for prime time but it does work.

For now, I'd like to focus on getting this initial explicit feedback
support in. There was no major complain on it, so I just have a minor
tweak to do before I send the patchset again. Hopefully soon ...

> Pavel.

Re: [RFC PATCH v2 3/3] usb: gadget: u_audio: .... PCM Rate Shift for playback too?

[Pavel Hofman]

Dne 27. 05. 21 v 11:52 Jerome Brunet napsal(a):
> 
> On Thu 27 May 2021 at 08:52, Pavel Hofman <pavel.hofman@ivitera.com> wrote:
> 
>> Dne 24. 05. 21 v 18:09 Pavel Hofman napsal(a):
>>> Dne 24. 05. 21 v 17:40 Jerome Brunet napsal(a):
>>>>
>>>> On Mon 24 May 2021 at 14:29, Pavel Hofman <pavel.hofman@ivitera.com>
>>>> wrote:
>>>>
>>>>> Dne 30. 04. 21 v 16:26 Jerome Brunet napsal(a):
>>>>>> From: Ruslan Bilovol <ruslan.bilovol@gmail.com>
>>>>>>
>>>>>> This adds interface between userspace and feedback endpoint to report
>>>>>> real
>>>>>> feedback frequency to the Host.
>>>>>>
>>>>>> Current implementation adds new userspace interface ALSA mixer control
>>>>>> "Capture Pitch 1000000" (similar to aloop driver's "PCM Rate Shift
>>>>>> 100000"
>>>>>> mixer control)
>>>>>>
>>>>>> Value in PPM is chosen to have correction value agnostic of the actual
>>>>>> HW
>>>>>> rate, which the application is not necessarily dealing with, while
>>>>>> still
>>>>>> retaining a good enough precision to allow smooth clock correction on
>>>>>> the
>>>>>> playback side, if necessary.
>>>>>>
>>>>>> Similar to sound/usb/endpoint.c, a slow down is allowed up to 25%. This
>>>>>> has no impact on the required bandwidth. Speedup correction has an
>>>>>> impact
>>>>>> on the bandwidth reserved for the isochronous endpoint. The default
>>>>>> allowed speedup is 500ppm. This seems to be more than enough but, if
>>>>>> necessary, this is configurable through a module parameter. The
>>>>>> reserved
>>>>>> bandwidth is rounded up to the next packet size.
>>>>>>
>>>>>> Usage of this new control is easy to implement in existing userspace
>>>>>> tools
>>>>>> like alsaloop from alsa-utils.
>>>>>>
>>>>>> Signed-off-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>
>>>>>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>>>>>
>>>>>
>>>>> Hi, the existing patches solve the Host -> Gadget -> capturing
>>>>> application direction, controlling the host playback rate. The other
>>>>> direction (playback app -> gadget -> capturing host) is still paced by
>>>>> the host USB controller. Packet size is pre-calculated in
>>>>> u_audio_start_playback  as rate/p_interval
>>>>> https://github.com/pavhofman/linux-rpi/blob/rpi-5.10.y/drivers/usb/gadget/function/u_audio.c#L441 
>>>>> and this fixed value is used for copying the audio data in
>>>>> u_audio_iso_complete
>>>>> https://github.com/pavhofman/linux-rpi/blob/rpi-5.10.y/drivers/usb/gadget/function/u_audio.c#L124 
>>>>> .
>>>>>
>>>>> That means if the gadget has a physical duplex audio device with single
>>>>> clock and runs a duplex operation, the path gadget-> host  will not run
>>>>> synchronously with the physical audio device (the host -> gadget has
>>>>> already the feedback control implemented).
>>>>>
>>>>> How about "duplicating" the existing ALSA mixer control
>>>>> "Capture Pitch 1000000" to "Playback Pitch 1000000" and using
>>>>> pitch-adjusted p_srate in the above-linked calculations? That should
>>>>> make the playback side run at the playback pitch requested by gadget
>>>>> userspace, IIUC.
>>>>>
>>>>> For the duplex operation with single clock, the capture pitch value
>>>>> determined by the userspace chain (alsaloop, CamillaDSP, etc.) would be
>>>>> used for setting both the capture and playback pitch controls, making
>>>>> both directions run synchronously.
>>>>>
>>>>> I can prepare patches based on Jerome's patchset should you find this
>>>>> solution acceptable.
>>>>
>>>> Well I have experimented with pitch on the playback path.
>>>> It does work but I'm not sure if it actually makes sense which is why I
>>>> have not not included it in RFC
>>>>
>>>> If you need the playback and capture to run synchronously, you'd be
>>>> better off with implicit feedback (In which the host will provide the
>>>> same number of samples it received from the device)
>>>>
>>>> With explicit feedback, we are (supposed) to tell the host to speed up
>>>> or slow down to match the device clock. This means the device run
>>>> asynchronously, and the host does the adaptation (if necessary). In such
>>>> case, I'm not sure adding pitch control on the playback path is a good
>>>> idea.
>>>>
>>>> Having pitch control on the playback side allows to forward the audio
>>>> captured by the physical interface of the device (like I2S) to USB
>>>> without performing any resampling between the two (when USB and I2S are
>>>> not in sync). It's nice and convenient ... but I wonder if this is a
>>>> feature or a hack ;)
>>>>
>>> Jerome, thanks a lot for your reply. The current implementation of the 
>>> EP IN audio direction is timed by the USB host controller. Let's consider
>>> 48Khz samplerate and bInterval=1 fullspeed (8k packets per second). Every
>>> IN packet will always carry 6 audio frames. No matter how fast the real
>>> master clock (i.e. e.g. I2S of the gadget) runs. Until an xrun occurs,
>>> unfortunately. Even if the gadget configuration used implicit feedback,
>>> the incoming stream would still provide no synchronization information
>>> from the I2S hardware clock as the data stream is clocked by the USB host
>>> which controls the timing on the USB bus (and thus the moment of iso
>>> completion). Plus the stock usb-audio driver in Windows 10 does not
>>> support implicit feedback, according to
>>> https://docs.microsoft.com/en-us/windows-hardware/drivers/audio/usb-2-0-audio-drivers#audio-streaming 
>>> .
>>>
>>> The problem is the fixed "slicing" of the samples into the packets which
>>> cannot be controlled. The same situation was on the host side before the 
>>> feedback EP was introduced. Here the one preparing the slices (the
>>> "transmitter") is the gadget now. And the slicing pace must be 
>>> controllable just like the slicing pace on the host is via the feedback EP.
>>> Because the gadget supports different playback and capture rates (which 
>>> I find nice), I suggest a separate control element (Playback Pitch,
>>> Capture Pitch).
>>> Of course the motivation is to avoid adaptive resampling in the gadget 
>>> in the direction I2S -> gadget interface -> USB host. But the very same
>>> motivation lead to implementation of the async feedback EP in the 
>>> opposite direction. IMO it is not a hack, but an indispensable feature
>>> making the gadget usable for duplex operation with hardware (i.e. the 
>>> real master) clock at the backend (all the other "clocks" are just
>>> software-generated slices/blocks of audio frames).
>>> With regards,
>>> Pavel.
>>
>> Jerome, please do you still have your playback-side patches available or
>> should I prepare them? Thanks a lot,
>>
> 
> Yes, this is what I have tested:
> 
> https://gitlab.com/jbrunet/linux/-/commit/43f1930ba548e137a5d20b1801790fae83eaa1e0
> https://gitlab.com/jbrunet/linux/-/commit/acb2ed9d9219488184cd2eb592fcdbf78b042a9e
> 
> It probably requires a rebase and cleaning before it is actually ready
> for prime time but it does work.
> 
> For now, I'd like to focus on getting this initial explicit feedback
> support in. There was no major complain on it, so I just have a minor
> tweak to do before I send the patchset again. Hopefully soon ...

Thanks for the patches, they look good. When you send your final
explicit FB patchset, I will rebase your two playback patches and put
all my changes (mainly rebased Julian's multiple samplerates patches) on
top to test the whole suite.

Pavel.