Hi Pavel, Keep the adaptive mode as default is better. Thanks. Charles Yi At 2023-10-25 14:44:49, "Pavel Hofman" <pavel.hofman@xxxxxxxxxxx> wrote: >Dne 24. 10. 23 v 12:14 be286 napsal(a): >> >> Hi Pavel, >> >> Feedback endpoint works for uca1 capture, means "EPOUT_EN". >> >> Charles Yi >> >Hi Charles, > >Sorry for my mistake, I thought you were implementing adaptive mode for >EP-IN. > >IIUC now your patch adds asynchronous mode (not adaptive sync) to UAC1 >EP OUT, in the same way as implemented in UAC2. > >IIUC your patch also changes the UAC1 EP-OUT default mode from adaptive >to asynchronous via > > +#define UAC1_DEF_CSYNC USB_ENDPOINT_SYNC_ASYNC > >The current (the only supported) mode is adaptive >https://elixir.bootlin.com/linux/latest/source/drivers/usb/gadget/function/f_uac1.c#L214: > >/* Standard ISO OUT Endpoint Descriptor */ >static struct usb_endpoint_descriptor as_out_ep_desc = { > .bLength = USB_DT_ENDPOINT_AUDIO_SIZE, > .bDescriptorType = USB_DT_ENDPOINT, > .bEndpointAddress = USB_DIR_OUT, > .bmAttributes = USB_ENDPOINT_SYNC_ADAPTIVE > | USB_ENDPOINT_XFER_ISOC, > .wMaxPacketSize = cpu_to_le16(UAC1_OUT_EP_MAX_PACKET_SIZE), > .bInterval = 4, >}; > >IMO the patch should keep the adaptive mode as default, to maintain >compatibility with user setups. > >With regards, > >Pavel. > >> >> >> >> >> >> >> >> At 2023-10-18 17:52:20, "Pavel Hofman" <pavel.hofman@xxxxxxxxxxx> wrote: >>> >>> >>> Dne 18. 10. 23 v 9:47 Charles Yi napsal(a): >>>> UAC1 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 UAC1 to prevent overruns/underruns. >>>> >>>> Change UAC1 driver to make it configurable through additional >>>> 'c_sync' configfs file. >>>> >>>> Default remains 'asynchronous' with possibility to switch it >>>> to 'adaptive'. >>> >>> >>> Hi Charles, >>> >>> Please can you clarify more the adaptive EP IN scenario? I am aware that >>> the f_uac2.c also allows defining c_sync type (that's what your patch is >>> based on). >>> >>> IIUC the data production rate of adaptive source endpoint (i.e. EP IN) >>> is controlled by feed forward messages from the host >>> Quoting http://sdpha2.ucsd.edu/Lab_Equip_Manuals/usb_20.pdf page 73: >>> >>> "Adaptive source endpoints produce data at a rate that is controlled by >>> the data sink. The sink provides feedback (refer to Section 5.12.4.2) to >>> the source, which allows the source to know the desired data rate of the >>> sink." >>> >>> While the current f_uac2 implementation generates feedback for EP OUT >>> async (unlike f_uac1), I cannot find any support for incoming >>> feed-forward messages from the host for EP IN adaptive case. Neither in >>> f_uac1, of course. >>> >>> I am not sure if linux supports IN EP adaptive, but the MS UAC2 driver >>> does not >>> https://learn.microsoft.com/en-us/windows-hardware/drivers/audio/usb-2-0-audio-drivers#audio-streaming: >>> >>> "For the Adaptive IN case the driver doesn't support a feed forward >>> endpoint. If such an endpoint is present in the alternate setting, it >>> will be ignored. The driver handles the Adaptive IN stream in the same >>> way as an Asynchronous IN stream." >>> >>> IIUC (and I may be wrong) all the c_sync param does in f_uac2 (and >>> f_uac1 in your patch) is just changing the EP IN configuration flag, but >>> the actual support for truly adaptive EP IN is not implemented. IMO >>> there is no code which would accept the feed-forward message from the >>> host and adjust the rate at which samples are consumed from the alsa >>> buffer to EP IN packets (method u_audio_iso_complete >>> https://elixir.bootlin.com/linux/latest/source/drivers/usb/gadget/function/u_audio.c#L193 >>> ) >>> >>> That pertains a bit to the first sentence of your patch - IMO it >>> describes EP OUT async, but not EP IN adaptive. >>> >>> Thanks a lot for a bit of clarification. >>> >>> Pavel. >>> >>> >>>> >>>> Changes in V2: >>>> - Updated the indentation of commit message. >>>> >>>> Signed-off-by: Charles Yi <be286@xxxxxxx> >>>> --- >>>> drivers/usb/gadget/function/f_uac1.c | 30 ++++++++++++++++++++++++++++ >>>> drivers/usb/gadget/function/u_uac1.h | 2 ++ >>>> 2 files changed, 32 insertions(+) >>>> >>>> diff --git a/drivers/usb/gadget/function/f_uac1.c b/drivers/usb/gadget/function/f_uac1.c >>>> index 6f0e1d803dc2..7a6fcb40bb46 100644 >>>> --- a/drivers/usb/gadget/function/f_uac1.c >>>> +++ b/drivers/usb/gadget/function/f_uac1.c >>>> @@ -33,6 +33,8 @@ >>>> #define FUOUT_EN(_opts) ((_opts)->c_mute_present \ >>>> || (_opts)->c_volume_present) >>>> >>>> +#define EPOUT_FBACK_IN_EN(_opts) ((_opts)->c_sync == USB_ENDPOINT_SYNC_ASYNC) >>>> + >>>> struct f_uac1 { >>>> struct g_audio g_audio; >>>> u8 ac_intf, as_in_intf, as_out_intf; >>>> @@ -227,6 +229,16 @@ static struct uac_iso_endpoint_descriptor as_iso_out_desc = { >>>> .wLockDelay = cpu_to_le16(1), >>>> }; >>>> >>>> +static struct usb_endpoint_descriptor as_fback_ep_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 uac_format_type_i_discrete_descriptor as_in_type_i_desc = { >>>> .bLength = 0, /* filled on rate setup */ >>>> .bDescriptorType = USB_DT_CS_INTERFACE, >>>> @@ -280,6 +292,7 @@ static struct usb_descriptor_header *f_audio_desc[] = { >>>> >>>> (struct usb_descriptor_header *)&as_out_ep_desc, >>>> (struct usb_descriptor_header *)&as_iso_out_desc, >>>> + (struct usb_descriptor_header *)&as_fback_ep_desc, >>>> >>>> (struct usb_descriptor_header *)&as_in_interface_alt_0_desc, >>>> (struct usb_descriptor_header *)&as_in_interface_alt_1_desc, >>>> @@ -1107,6 +1120,9 @@ static void setup_descriptor(struct f_uac1_opts *opts) >>>> f_audio_desc[i++] = USBDHDR(&as_out_type_i_desc); >>>> f_audio_desc[i++] = USBDHDR(&as_out_ep_desc); >>>> f_audio_desc[i++] = USBDHDR(&as_iso_out_desc); >>>> + if (EPOUT_FBACK_IN_EN(opts)) { >>>> + f_audio_desc[i++] = USBDHDR(&as_fback_ep_desc); >>>> + } >>>> } >>>> if (EPIN_EN(opts)) { >>>> f_audio_desc[i++] = USBDHDR(&as_in_interface_alt_0_desc); >>>> @@ -1317,6 +1333,12 @@ static int f_audio_bind(struct usb_configuration *c, struct usb_function *f) >>>> ac_header_desc->baInterfaceNr[ba_iface_id++] = status; >>>> uac1->as_out_intf = status; >>>> uac1->as_out_alt = 0; >>>> + >>>> + if (EPOUT_FBACK_IN_EN(audio_opts)) { >>>> + as_out_ep_desc.bmAttributes = >>>> + USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ASYNC; >>>> + as_out_interface_alt_1_desc.bNumEndpoints++; >>>> + } >>>> } >>>> >>>> if (EPIN_EN(audio_opts)) { >>>> @@ -1354,6 +1376,12 @@ static int f_audio_bind(struct usb_configuration *c, struct usb_function *f) >>>> goto err_free_fu; >>>> audio->out_ep = ep; >>>> audio->out_ep->desc = &as_out_ep_desc; >>>> + if (EPOUT_FBACK_IN_EN(audio_opts)) { >>>> + audio->in_ep_fback = usb_ep_autoconfig(gadget, &as_fback_ep_desc); >>>> + if (!audio->in_ep_fback) { >>>> + goto err_free_fu; >>>> + } >>>> + } >>>> } >>>> >>>> if (EPIN_EN(audio_opts)) { >>>> @@ -1685,6 +1713,8 @@ static struct usb_function_instance *f_audio_alloc_inst(void) >>>> >>>> opts->req_number = UAC1_DEF_REQ_NUM; >>>> >>>> + opts->c_sync = UAC1_DEF_CSYNC; >>>> + >>>> snprintf(opts->function_name, sizeof(opts->function_name), "AC Interface"); >>>> >>>> return &opts->func_inst; >>>> diff --git a/drivers/usb/gadget/function/u_uac1.h b/drivers/usb/gadget/function/u_uac1.h >>>> index f7a616760e31..c6e2271e8cdd 100644 >>>> --- a/drivers/usb/gadget/function/u_uac1.h >>>> +++ b/drivers/usb/gadget/function/u_uac1.h >>>> @@ -27,6 +27,7 @@ >>>> #define UAC1_DEF_MAX_DB 0 /* 0 dB */ >>>> #define UAC1_DEF_RES_DB (1*256) /* 1 dB */ >>>> >>>> +#define UAC1_DEF_CSYNC USB_ENDPOINT_SYNC_ASYNC >>>> >>>> struct f_uac1_opts { >>>> struct usb_function_instance func_inst; >>>> @@ -56,6 +57,7 @@ struct f_uac1_opts { >>>> >>>> struct mutex lock; >>>> int refcnt; >>>> + int c_sync; >>>> }; >>>> >>>> #endif /* __U_UAC1_H */
