On Thu, Nov 26, 2020 at 1:13 PM Jerome Brunet <jbrunet@xxxxxxxxxxxx> wrote:
>
>
> 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 */
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;
>
