Dne 05. 01. 22 v 13:44 John Keeping napsal(a):
On Wed, Jan 05, 2022 at 01:20:01PM +0100, Pavel Hofman wrote:Dne 04. 01. 22 v 16:33 John Keeping napsal(a):On Wed, Dec 22, 2021 at 11:01:16AM +0100, Pavel Hofman wrote:Dne 21. 12. 21 v 12:59 John Keeping napsal(a):On Mon, Dec 20, 2021 at 10:11:22PM +0100, Pavel Hofman wrote:From: Julian Scheel <julian@xxxxxxxx> A list of sampling rates can be specified via configfs. All enabled sampling rates are sent to the USB host on request. When the host selects a sampling rate the internal active rate is updated. Config strings with single value stay compatible with the previous version. Multiple samplerates passed as configuration arrays to g_audio module when built for f_uac2. Signed-off-by: Julian Scheel <julian@xxxxxxxx> Signed-off-by: Pavel Hofman <pavel.hofman@xxxxxxxxxxx> ---[snip]}; static inline struct f_uac2 *func_to_uac2(struct usb_function *f) @@ -166,7 +167,7 @@ static struct uac_clock_source_descriptor in_clk_src_desc = { .bDescriptorSubtype = UAC2_CLOCK_SOURCE, /* .bClockID = DYNAMIC */ .bmAttributes = UAC_CLOCK_SOURCE_TYPE_INT_FIXED, - .bmControls = (CONTROL_RDONLY << CLK_FREQ_CTRL), + .bmControls = (CONTROL_RDWR << CLK_FREQ_CTRL), .bAssocTerminal = 0, }; @@ -178,7 +179,7 @@ static struct uac_clock_source_descriptor out_clk_src_desc = { .bDescriptorSubtype = UAC2_CLOCK_SOURCE, /* .bClockID = DYNAMIC */ .bmAttributes = UAC_CLOCK_SOURCE_TYPE_INT_FIXED, - .bmControls = (CONTROL_RDONLY << CLK_FREQ_CTRL), + .bmControls = (CONTROL_RDWR << CLK_FREQ_CTRL), .bAssocTerminal = 0, }; @@ -635,12 +636,32 @@ struct cntrl_cur_lay3 { }; struct cntrl_range_lay3 { - __le16 wNumSubRanges; __le32 dMIN; __le32 dMAX; __le32 dRES; } __packed; +#define ranges_size(c) (sizeof(c.wNumSubRanges) + c.wNumSubRanges \ + * sizeof(struct cntrl_ranges_lay3)) + +struct cntrl_ranges_lay3 { + __u16 wNumSubRanges; + struct cntrl_range_lay3 r[UAC_MAX_RATES]; +} __packed;These structures are now inconsistent between cntrl_range_lay2 and cntrl_range_lay3. Would it be better to make these flex arrays? I guess that will make the code that uses it more complicated, but at the moment it looks like these are trying to be generic while in reality being quite specific to the one place that uses them at the moment.I am afraid I do not know exactly how to do that. Please can you post an example? The rate control requires u32 (u16 is too small). Thanks a lot.After the change in this patch, we end up with: struct cntrl_range_lay2 { __le16 wNumSubRanges; __le16 wMIN; __le16 wMAX; __le16 wRES; } __packed; struct cntrl_range_lay3 { __le32 dMIN; __le32 dMAX; __le32 dRES; } __packed; so there are two structures with similar names but totally different structure, which I think risks confusion in the future. I wonder if DECLARE_UAC2_FEATURE_UNIT_DESCRIPTOR in linux/usb/audio-v2.h provides inspiration here, so potentially something like: #define DECLARE_UAC2_CNTRL_RANGE_LAY3(n) \ struct uac2_cntrl_range_lay3_##n { \ __le16 wNumSubRanges; \ struct cntrl_range_le32 r[n]; \ } __packed; DECLARE_UAC2_CNTRL_RANGE_LAY3(UAC_MAX_RATES);Thanks, I will try to follow your suggestion in the next patchset version.+static int get_max_srate(const int *srates) +{ + int i, max_srate = 0; + + for (i = 0; i < UAC_MAX_RATES; i++) { + if (srates[i] == 0) + break; + if (srates[i] > max_srate) + max_srate = srates[i]; + } + return max_srate; +} + static int set_ep_max_packet_size(const struct f_uac2_opts *uac2_opts, struct usb_endpoint_descriptor *ep_desc, enum usb_device_speed speed, bool is_playback) @@ -667,11 +688,11 @@ static int set_ep_max_packet_size(const struct f_uac2_opts *uac2_opts, if (is_playback) { chmask = uac2_opts->p_chmask; - srate = uac2_opts->p_srate; + srate = get_max_srate(uac2_opts->p_srates); ssize = uac2_opts->p_ssize; } else { chmask = uac2_opts->c_chmask; - srate = uac2_opts->c_srate; + srate = get_max_srate(uac2_opts->c_srates); ssize = uac2_opts->c_ssize; } @@ -912,10 +933,10 @@ static int afunc_validate_opts(struct g_audio *agdev, struct device *dev) } else if ((opts->c_ssize < 1) || (opts->c_ssize > 4)) { dev_err(dev, "Error: incorrect capture sample size\n"); return -EINVAL; - } else if (!opts->p_srate) { + } else if (!opts->p_srates[0]) { dev_err(dev, "Error: incorrect playback sampling rate\n"); return -EINVAL; - } else if (!opts->c_srate) { + } else if (!opts->c_srates[0]) { dev_err(dev, "Error: incorrect capture sampling rate\n"); return -EINVAL; } @@ -1210,7 +1231,8 @@ afunc_bind(struct usb_configuration *cfg, struct usb_function *fn) agdev->params.p_chmask = uac2_opts->p_chmask; agdev->params.p_srate = uac2_opts->p_srate; - agdev->params.p_srates[0] = uac2_opts->p_srate; + memcpy(agdev->params.p_srates, uac2_opts->p_srates, + sizeof(agdev->params.p_srates)); agdev->params.p_ssize = uac2_opts->p_ssize; if (FUIN_EN(uac2_opts)) { agdev->params.p_fu.id = USB_IN_FU_ID; @@ -1222,7 +1244,8 @@ afunc_bind(struct usb_configuration *cfg, struct usb_function *fn) } agdev->params.c_chmask = uac2_opts->c_chmask; agdev->params.c_srate = uac2_opts->c_srate; - agdev->params.c_srates[0] = uac2_opts->c_srate; + memcpy(agdev->params.c_srates, uac2_opts->c_srates, + sizeof(agdev->params.c_srates)); agdev->params.c_ssize = uac2_opts->c_ssize; if (FUOUT_EN(uac2_opts)) { agdev->params.c_fu.id = USB_OUT_FU_ID; @@ -1502,28 +1525,39 @@ in_rq_range(struct usb_function *fn, const struct usb_ctrlrequest *cr) u8 entity_id = (w_index >> 8) & 0xff; u8 control_selector = w_value >> 8; int value = -EOPNOTSUPP; - int p_srate, c_srate; - - p_srate = opts->p_srate; - c_srate = opts->c_srate; if ((entity_id == USB_IN_CLK_ID) || (entity_id == USB_OUT_CLK_ID)) { if (control_selector == UAC2_CS_CONTROL_SAM_FREQ) { - struct cntrl_range_lay3 r; + struct cntrl_ranges_lay3 rs; + int i; + int wNumSubRanges = 0; + int srate; + int *srates; if (entity_id == USB_IN_CLK_ID) - r.dMIN = cpu_to_le32(p_srate); + srates = opts->p_srates; else if (entity_id == USB_OUT_CLK_ID) - r.dMIN = cpu_to_le32(c_srate); + srates = opts->c_srates; else return -EOPNOTSUPP; - - r.dMAX = r.dMIN; - r.dRES = 0; - r.wNumSubRanges = cpu_to_le16(1); - - value = min_t(unsigned int, w_length, sizeof(r)); - memcpy(req->buf, &r, value); + for (i = 0; i < UAC_MAX_RATES; i++) { + srate = srates[i]; + if (srate == 0) + break; + + rs.r[wNumSubRanges].dMIN = cpu_to_le32(srate); + rs.r[wNumSubRanges].dMAX = cpu_to_le32(srate); + rs.r[wNumSubRanges].dRES = 0; + wNumSubRanges++; + dev_dbg(&agdev->gadget->dev, + "%s(): clk %d: rate ID %d: %d\n", + __func__, entity_id, wNumSubRanges, srate); + } + rs.wNumSubRanges = cpu_to_le16(wNumSubRanges); + value = min_t(unsigned int, w_length, ranges_size(rs)); + dev_dbg(&agdev->gadget->dev, "%s(): sending %d rates, size %d\n", + __func__, rs.wNumSubRanges, value); + memcpy(req->buf, &rs, value); } else { dev_err(&agdev->gadget->dev, "%s:%d control_selector=%d TODO!\n", @@ -1582,6 +1616,28 @@ ac_rq_in(struct usb_function *fn, const struct usb_ctrlrequest *cr) return -EOPNOTSUPP; } +static void uac2_cs_control_sam_freq(struct usb_ep *ep, struct usb_request *req) +{ + struct usb_function *fn = ep->driver_data; + struct g_audio *agdev = func_to_g_audio(fn); + struct f_uac2 *uac2 = func_to_uac2(fn); + struct f_uac2_opts *opts = g_audio_to_uac2_opts(agdev); + u32 val; + + if (req->actual != 4) + return; + + val = le32_to_cpu(*((u32 *)req->buf)); + dev_dbg(&agdev->gadget->dev, "%s val: %d.\n", __func__, val); + if (uac2->ctl_id == USB_IN_CLK_ID) { + opts->p_srate = val;Don't you need to hold opts->lock to change this? I'm not sure opts should be changed here though - that's the setup phase and this is "current state", so shouldn't it move to struct f_uac2?OK. I moved the current p_srate/c_srate from struct opts to f_uac2, initialized with first value of opts->p_srates/c_srates[0] in afunc_bind. The struct f_uac2 has no lock yet. Should I add the lock mutex to f_uac2 and be locking f_uac2 access here in uac2_cs_control_sam_freq?Could we move this into struct uac_rtd_params and use the existing lock there to guard it? It would need accessor functions as that structure's local to u_audio.c, but there's already u_audio_set_playback_srate() so that isn't a big change.I have already moved p_/c_srate from uac_params to uac_rtd_params in u_audio.c in the next version of the patchset. But IIUC the currently selected playback/capture rate is required within f_uac2 too, in in_rq_cur() in: if (control_selector == UAC2_CS_CONTROL_SAM_FREQ) { ... if (entity_id == USB_IN_CLK_ID) c.dCUR = cpu_to_le32(p_srate); else if (entity_id == USB_OUT_CLK_ID) c.dCUR = cpu_to_le32(c_srate); ... }Can this can be u_audio_get_playback_srate(agdev) (and equivalent for capture)?
We certainly can add the rate getter API call. But I do not know whether master should store and query slave for data required for proper operation of the master. IMO f_uac2 should be in control of the params requested by the real master - the USB host.
Currently the gadget-side app can open the gadget alsa device at the last-used rate (or channel count if/when multiple channels config is implemented) at any time, without knowing what params the user will actually request. IMO in some use cases the gadget alsa device should not allow opening before the host starts playback/capture, i.e. before the user actually requests what params to use. This would require more patches, nothing critical now and not in my pipeline. I (will) handle this issue with the side-channel gaudio_ctl application controller which receives the currently required params (srate as of now) via ctl notifications.
The question is whether it's correct for f_uac2 to ask u_audio about current rate at any time, even when u_audio is inactive and the only party doing something with the rate is actually f_uac2.
Maybe the ultimate data flow should be the other way round. But IIUC u_audio currently does not have any hooks back to f_uac1/2. The USB functions store all the params to uac_param for u_audio to use.
But I can add the u_audio_get_playback/capture_srate API method (in a separate patch, I guess) to avoid keeping the same values in two places, if found convenient.
I think we all should discuss our use cases in order to delineate the path along which the audio gadget should evolve.
Thanks, Pavel.
