| Message ID | 1604794711-8661-3-git-send-email-ruslan.bilovol@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | UAC2 Gadget: feedback endpoint support | expand |
On 20-11-08 02:18:30, Ruslan Bilovol 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@gmail.com> > --- > 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) Can't apply it based on the latest code, this file has changed. > 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; > + } > } > > 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; > > -- > 1.9.1 >
On Wed, Nov 11, 2020 at 11:18 AM Peter Chen <peter.chen@nxp.com> wrote: > > On 20-11-08 02:18:30, Ruslan Bilovol 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@gmail.com> > > --- > > 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) > > Can't apply it based on the latest code, this file has changed. This patch set is based on Felipe's -next branch which seems to be a little bit outdated. Thanks, Ruslan
On Sun 08 Nov 2020 at 01:18, Ruslan Bilovol <ruslan.bilovol@gmail.com> 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@gmail.com> > --- > 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;
On Thu, Nov 26, 2020 at 1:13 PM Jerome Brunet <jbrunet@baylibre.com> wrote: > > > On Sun 08 Nov 2020 at 01:18, Ruslan Bilovol <ruslan.bilovol@gmail.com> 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@gmail.com> > > --- > > 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 */ Agree, I just forgot to do that > > 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) It seems it should be quite easy to add it, so at least worth a try. Will experiment this in next patch set Thanks, Ruslan > > > } > > > > 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; >
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; + } } 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;
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> --- 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(-)