Re: [PATCH v2 03/11] usb: gadget: f_uac2: Support multiple sampling rates

[John Keeping <john@metanate.com>]

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@jusst.de>
> > > > > 
> > > > > 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@jusst.de>
> > > > > Signed-off-by: Pavel Hofman <pavel.hofman@ivitera.com>
> > > > > ---
> > [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)?


John