On Sun 08 Nov 2020 at 01:18, Ruslan Bilovol <ruslan.bilovol@xxxxxxxxx> wrote:
> 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@xxxxxxxxx>
> ---
> Documentation/ABI/testing/configfs-usb-gadget-uac2 | 1 +
> Documentation/usb/gadget-testing.rst | 1 +
> drivers/usb/gadget/function/f_uac2.c | 96 ++++++++++++++++++++--
> drivers/usb/gadget/function/u_uac2.h | 2 +
> 4 files changed, 91 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uac2 b/Documentation/ABI/testing/configfs-usb-gadget-uac2
> index 2bfdd4e..4fbff96 100644
> --- a/Documentation/ABI/testing/configfs-usb-gadget-uac2
> +++ b/Documentation/ABI/testing/configfs-usb-gadget-uac2
> @@ -7,6 +7,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 2eeb3e9..360a7ca 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 a57bf77..3187ad3 100644
> --- a/drivers/usb/gadget/function/f_uac2.c
> +++ b/drivers/usb/gadget/function/f_uac2.c
> @@ -41,6 +41,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;
> @@ -237,7 +238,7 @@ enum {
> .bDescriptorType = USB_DT_INTERFACE,
>
> .bAlternateSetting = 1,
> - .bNumEndpoints = 2,
> + .bNumEndpoints = 1,
> .bInterfaceClass = USB_CLASS_AUDIO,
> .bInterfaceSubClass = USB_SUBCLASS_AUDIOSTREAMING,
> .bInterfaceProtocol = UAC_VERSION_2,
> @@ -270,7 +271,7 @@ enum {
> .bDescriptorType = USB_DT_ENDPOINT,
>
> .bEndpointAddress = USB_DIR_OUT,
> - .bmAttributes = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ASYNC,
> + .bmAttributes = USB_ENDPOINT_XFER_ISOC,
> .wMaxPacketSize = cpu_to_le16(1023),
> .bInterval = 1,
> };
> @@ -279,7 +280,7 @@ enum {
> .bLength = USB_DT_ENDPOINT_SIZE,
> .bDescriptorType = USB_DT_ENDPOINT,
>
> - .bmAttributes = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ASYNC,
> + .bmAttributes = USB_ENDPOINT_XFER_ISOC,
> .wMaxPacketSize = cpu_to_le16(1024),
> .bInterval = 1,
> };
> @@ -540,6 +541,19 @@ static void setup_descriptor(struct f_uac2_opts *opts)
> len += sizeof(io_out_ot_desc);
> ac_hdr_desc.wTotalLength = cpu_to_le16(len);
> iad_desc.bInterfaceCount++;
> +
> + if (EPOUT_FBACK_IN_EN(opts)) {
> + fs_epout_desc.bmAttributes |=
> + USB_ENDPOINT_SYNC_ASYNC;
> + hs_epout_desc.bmAttributes |=
> + USB_ENDPOINT_SYNC_ASYNC;
> + std_as_out_if1_desc.bNumEndpoints++;
> + } else {
> + fs_epout_desc.bmAttributes |=
> + USB_ENDPOINT_SYNC_ADAPTIVE;
> + hs_epout_desc.bmAttributes |=
> + USB_ENDPOINT_SYNC_ADAPTIVE;
> + }
Hi Ruslan,
First, thanks a lot for this series, it is very helpful
Instead of amending the descriptors, could you consider using comments like
/* .bNumEndpoints = DYNAMIC */
or
/* .bmAttributes = DYNAMIC */
It would help understand that the actual value of these parameters will be
determine at runtime, making the code easier to follow and maintain.
Also, I wonder if it would be difficult to add implicit feedback support
while at it ?
I'm asking this now because it could (possibly) change the driver params
(implicit is async as well)
> }
>
> i = 0;
> @@ -564,7 +578,8 @@ static void setup_descriptor(struct f_uac2_opts *opts)
> fs_audio_desc[i++] = USBDHDR(&as_out_fmt1_desc);
> fs_audio_desc[i++] = USBDHDR(&fs_epout_desc);
> fs_audio_desc[i++] = USBDHDR(&as_iso_out_desc);
> - fs_audio_desc[i++] = USBDHDR(&fs_epin_fback_desc);
> + if (EPOUT_FBACK_IN_EN(opts))
> + fs_audio_desc[i++] = USBDHDR(&fs_epin_fback_desc);
> }
> if (EPIN_EN(opts)) {
> fs_audio_desc[i++] = USBDHDR(&std_as_in_if0_desc);
> @@ -598,7 +613,8 @@ static void setup_descriptor(struct f_uac2_opts *opts)
> hs_audio_desc[i++] = USBDHDR(&as_out_fmt1_desc);
> hs_audio_desc[i++] = USBDHDR(&hs_epout_desc);
> hs_audio_desc[i++] = USBDHDR(&as_iso_out_desc);
> - hs_audio_desc[i++] = USBDHDR(&hs_epin_fback_desc);
> + if (EPOUT_FBACK_IN_EN(opts))
> + hs_audio_desc[i++] = USBDHDR(&hs_epin_fback_desc);
> }
> if (EPIN_EN(opts)) {
> hs_audio_desc[i++] = USBDHDR(&std_as_in_if0_desc);
> @@ -706,11 +722,14 @@ static void setup_descriptor(struct f_uac2_opts *opts)
> 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;
> + }
> }
> }
>
> @@ -1057,11 +1076,68 @@ static void f_uac2_attr_release(struct config_item *item)
> \
> 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);
>
> @@ -1072,6 +1148,7 @@ static void f_uac2_attr_release(struct config_item *item)
> &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,
> };
> @@ -1110,6 +1187,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 b503571..13589c3 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;
