Re:Re: [PATCH V2] usb: gadget: f_uac1: add adaptive sync support for capture

From: be286
Date: Wed Oct 25 2023 - 22:14:07 EST

Next message: Mi, Dapeng: "Re: [kvm-unit-tests Patch 2/5] x86: pmu: Change the minimum value of llc_misses event to 0"
Previous message: Vincent MAILHOL: "Re: [PATCH net-next 4/5] net: phy: broadcom: Add support for WAKE_FILTER"
In reply to: Pavel Hofman: "Re: [PATCH V2] usb: gadget: f_uac1: add adaptive sync support for capture"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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 */

Next message: Mi, Dapeng: "Re: [kvm-unit-tests Patch 2/5] x86: pmu: Change the minimum value of llc_misses event to 0"
Previous message: Vincent MAILHOL: "Re: [PATCH net-next 4/5] net: phy: broadcom: Add support for WAKE_FILTER"
In reply to: Pavel Hofman: "Re: [PATCH V2] usb: gadget: f_uac1: add adaptive sync support for capture"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]