Re: [PATCH 6/9] ALSA: usb: caiaq: use usb_fill_int_urb()
On Thursday, June 21, 2018 11:16 PM, Sebastian Andrzej Siewior wrote: On 2018-06-21 22:19:32 [+0200], Daniel Mack wrote: On Tuesday, June 19, 2018 11:55 PM, Sebastian Andrzej Siewior wrote: Using usb_fill_int_urb() helps to find code which initializes an URB. A grep for members of the struct (like ->complete) reveal lots of other things, too. Acked-by: Daniel Mack nope, please don't. Takashi, please ignore the usb_fill_.* patches. I will be doing another spin with usb_fill_iso_urb() instead. Hmm, there is no such thing as usb_fill_iso_urb() in my tree, are you going to add that? The only part that needs attention is the interval parameter, which is passed to usb_fill_int_urb() as 1 now, and hence urb->interval will also be 1, just like the open-coded version before. Unless I miss anything, your conversion should work, but I haven't tested it yet. But I agree the function name is misleading, so we should probably get a usb_fill_iso_urb() and use it where appropriate. AFAICS, it will be identical to usb_fill_bulk_urb(), as the endpoint type is encoded in the pipe. Maybe it's worth adding a check? Thanks, Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/9] ALSA: usb: caiaq: use usb_fill_int_urb()
On Tuesday, June 19, 2018 11:55 PM, Sebastian Andrzej Siewior wrote:
Using usb_fill_int_urb() helps to find code which initializes an
URB. A grep for members of the struct (like ->complete) reveal lots
of other things, too.
Acked-by: Daniel Mack
Cc: Jaroslav Kysela
Cc: Takashi Iwai
Signed-off-by: Sebastian Andrzej Siewior
---
sound/usb/caiaq/audio.c | 23 +++
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/sound/usb/caiaq/audio.c b/sound/usb/caiaq/audio.c
index 15344d39a6cd..e10d5790099f 100644
--- a/sound/usb/caiaq/audio.c
+++ b/sound/usb/caiaq/audio.c
@@ -736,16 +736,17 @@ static struct urb **alloc_urbs(struct snd_usb_caiaqdev
*cdev, int dir, int *ret)
}
for (i = 0; i < N_URBS; i++) {
+ void *buf;
+
urbs[i] = usb_alloc_urb(FRAMES_PER_URB, GFP_KERNEL);
if (!urbs[i]) {
*ret = -ENOMEM;
return urbs;
}
- urbs[i]->transfer_buffer =
- kmalloc_array(BYTES_PER_FRAME, FRAMES_PER_URB,
- GFP_KERNEL);
- if (!urbs[i]->transfer_buffer) {
+ buf = kmalloc_array(BYTES_PER_FRAME, FRAMES_PER_URB,
+ GFP_KERNEL);
+ if (!buf) {
*ret = -ENOMEM;
return urbs;
}
@@ -758,15 +759,13 @@ static struct urb **alloc_urbs(struct snd_usb_caiaqdev
*cdev, int dir, int *ret)
iso->length = BYTES_PER_FRAME;
}
- urbs[i]->dev = usb_dev;
- urbs[i]->pipe = pipe;
- urbs[i]->transfer_buffer_length = FRAMES_PER_URB
- * BYTES_PER_FRAME;
- urbs[i]->context = >data_cb_info[i];
- urbs[i]->interval = 1;
+ usb_fill_int_urb(urbs[i], usb_dev, pipe, buf,
+FRAMES_PER_URB * BYTES_PER_FRAME,
+(dir == SNDRV_PCM_STREAM_CAPTURE) ?
+read_completed : write_completed,
+>data_cb_info[i], 1);
+
urbs[i]->number_of_packets = FRAMES_PER_URB;
- urbs[i]->complete = (dir == SNDRV_PCM_STREAM_CAPTURE) ?
- read_completed : write_completed;
}
*ret = 0;
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/9] ALSA: usb: caiaq: audio: use irqsave() in USB's complete callback
On Tuesday, June 19, 2018 11:55 PM, Sebastian Andrzej Siewior wrote:
From: John Ogness
The USB completion callback does not disable interrupts while acquiring
the lock. We want to remove the local_irq_disable() invocation from
__usb_hcd_giveback_urb() and therefore it is required for the callback
handler to disable the interrupts while acquiring the lock.
The callback may be invoked either in IRQ or BH context depending on the
USB host controller.
Use the _irqsave() variant of the locking primitives.
Acked-by: Daniel Mack
Cc: Jaroslav Kysela
Cc: Takashi Iwai
Signed-off-by: John Ogness
Signed-off-by: Sebastian Andrzej Siewior
---
sound/usb/caiaq/audio.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/sound/usb/caiaq/audio.c b/sound/usb/caiaq/audio.c
index f35d29f49ffe..15344d39a6cd 100644
--- a/sound/usb/caiaq/audio.c
+++ b/sound/usb/caiaq/audio.c
@@ -636,6 +636,7 @@ static void read_completed(struct urb *urb)
struct device *dev;
struct urb *out = NULL;
int i, frame, len, send_it = 0, outframe = 0;
+ unsigned long flags;
size_t offset = 0;
if (urb->status || !info)
@@ -672,10 +673,10 @@ static void read_completed(struct urb *urb)
offset += len;
if (len > 0) {
- spin_lock(>spinlock);
+ spin_lock_irqsave(>spinlock, flags);
fill_out_urb(cdev, out, >iso_frame_desc[outframe]);
read_in_urb(cdev, urb, >iso_frame_desc[frame]);
- spin_unlock(>spinlock);
+ spin_unlock_irqrestore(>spinlock, flags);
check_for_elapsed_periods(cdev, cdev->sub_playback);
check_for_elapsed_periods(cdev, cdev->sub_capture);
send_it = 1;
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/1] usb: gadget: f_uac2: finalize wMaxPacketSize according to bandwidth
On 07/28/2015 03:38 AM, Peter Chen wrote:
+static void set_ep_max_packet_size (struct f_uac2_opts *uac2_opts,
+ struct usb_endpoint_descriptor *ep_desc, unsigned int factor)
+{
+ int chmask;
+ int srate;
+ int ssize;
+ u16 max_packet_size;
+
+ if (ep_desc == fs_epin_desc || ep_desc == hs_epin_desc) {
+ chmask = uac2_opts-p_chmask;
+ srate = uac2_opts-p_srate;
+ ssize = uac2_opts-p_ssize;
+ } else {
+ WARN_ON (ep_desc != fs_epout_desc ep_desc !=
hs_epout_desc);
+ chmask = uac2_opts-c_chmask;
+ srate = uac2_opts-c_srate;
+ ssize = uac2_opts-c_ssize;
+ }
+
+ max_packet_size = num_channels(chmask) * ssize *
+ DIV_ROUND_UP(srate, factor / (1 (ep_desc-bInterval - 1)));
+ ep_desc-wMaxPacketSize = min(cpu_to_le16(max_packet_size),
+ ep_desc-wMaxPacketSize);
+}
Thinking about it again, it would probably be nicer to make this
function return wMaxPacketSize directly:
fs_epin_desc.wMaxPacketSize =
calc_ep_max_packet_size(uac2_opts-p_chmask,
uac2_opts-p_srate,
uac2_opts-p_ssize,
1000);
hs_epin_desc.wMaxPacketSize =
calc_ep_max_packet_size(uac2_opts-p_chmask,
uac2_opts-p_srate,
uac2_opts-p_ssize,
8000);
...
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/1] usb: gadget: f_uac2: finalize wMaxPacketSize according to bandwidth
On 07/28/2015 11:30 AM, Daniel Mack wrote:
On 07/28/2015 03:38 AM, Peter Chen wrote:
+static void set_ep_max_packet_size (struct f_uac2_opts *uac2_opts,
+struct usb_endpoint_descriptor *ep_desc, unsigned int factor)
+{
+int chmask;
+int srate;
+int ssize;
+u16 max_packet_size;
+
+if (ep_desc == fs_epin_desc || ep_desc == hs_epin_desc) {
+chmask = uac2_opts-p_chmask;
+srate = uac2_opts-p_srate;
+ssize = uac2_opts-p_ssize;
+} else {
+WARN_ON (ep_desc != fs_epout_desc ep_desc !=
hs_epout_desc);
+chmask = uac2_opts-c_chmask;
+srate = uac2_opts-c_srate;
+ssize = uac2_opts-c_ssize;
+}
+
+max_packet_size = num_channels(chmask) * ssize *
+DIV_ROUND_UP(srate, factor / (1 (ep_desc-bInterval - 1)));
+ep_desc-wMaxPacketSize = min(cpu_to_le16(max_packet_size),
+ep_desc-wMaxPacketSize);
+}
Thinking about it again, it would probably be nicer to make this
function return wMaxPacketSize directly:
Ah, no, that doesn't make sense, sorry. We need bInterval and the
current value of wMaxPacketSize as well. Just stick with your current
approach then :)
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/1] usb: gadget: f_uac2: finalize wMaxPacketSize according to bandwidth
On 07/28/2015 11:17 AM, Peter Chen wrote:
According to USB Audio Device 2.0 Spec, Ch4.10.1.1:
wMaxPacketSize is defined as follows:
Maximum packet size this endpoint is capable of sending or receiving
when this configuration is selected.
This is determined by the audio bandwidth constraints of the endpoint.
In current code, the wMaxPacketSize is defined as the maximum packet size
for ISO endpoint, and it will let the host reserve much more space than
it really needs, so that we can't let more endpoints work together at
one frame.
We find this issue when we try to let 4 f_uac2 gadgets work together [1]
at FS connection.
[1]http://www.spinics.net/lists/linux-usb/msg123478.html
Cc: andrze...@samsung.com
Cc: Daniel Mack zon...@gmail.com
Cc: ti...@suse.de
Cc: sta...@vger.kernel.org #v3.18+
Cc: Alan Stern st...@rowland.harvard.edu
Signed-off-by: Peter Chen peter.c...@freescale.com
Looks good to me now! I currently don't have hardware to test this on,
though.
Acked-by: Daniel Mack zon...@gmail.com
Thanks,
Daniel
---
Changes for v5:
- Add additional parameters 'is_playback' for helper
- Add const for parameter 'struct f_uac2_opts *uac2_opts'
- Put all three int variables at one line
- Fix the CodeStyle problem
Changes for v4:
- Add helper set_ep_max_packet_size to calculate max packet size
Changes for v3:
- Consider 'bInterval' to calculate wMaxPacketSize
- playback endpoint is IN ep, and capture endpoint is OUT ep
Changes for v2:
- Using DIV_ROUND_UP to calculate max packet size
drivers/usb/gadget/function/f_uac2.c | 31 +--
1 file changed, 29 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uac2.c
b/drivers/usb/gadget/function/f_uac2.c
index 5318615..fa93d79 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -975,6 +975,29 @@ free_ep(struct uac2_rtd_params *prm, struct usb_ep *ep)
%s:%d Error!\n, __func__, __LINE__);
}
+static void set_ep_max_packet_size(const struct f_uac2_opts *uac2_opts,
+ struct usb_endpoint_descriptor *ep_desc,
+ unsigned int factor, bool is_playback)
+{
+ int chmask, srate, ssize;
+ u16 max_packet_size;
+
+ if (is_playback) {
+ chmask = uac2_opts-p_chmask;
+ srate = uac2_opts-p_srate;
+ ssize = uac2_opts-p_ssize;
+ } else {
+ chmask = uac2_opts-c_chmask;
+ srate = uac2_opts-c_srate;
+ ssize = uac2_opts-c_ssize;
+ }
+
+ max_packet_size = num_channels(chmask) * ssize *
+ DIV_ROUND_UP(srate, factor / (1 (ep_desc-bInterval - 1)));
+ ep_desc-wMaxPacketSize = min(cpu_to_le16(max_packet_size),
+ ep_desc-wMaxPacketSize);
+}
+
static int
afunc_bind(struct usb_configuration *cfg, struct usb_function *fn)
{
@@ -1070,10 +1093,14 @@ afunc_bind(struct usb_configuration *cfg, struct
usb_function *fn)
uac2-p_prm.uac2 = uac2;
uac2-c_prm.uac2 = uac2;
+ /* Calculate wMaxPacketSize according to audio bandwidth */
+ set_ep_max_packet_size(uac2_opts, fs_epin_desc, 1000, true);
+ set_ep_max_packet_size(uac2_opts, fs_epout_desc, 1000, false);
+ set_ep_max_packet_size(uac2_opts, hs_epin_desc, 8000, true);
+ set_ep_max_packet_size(uac2_opts, hs_epout_desc, 8000, false);
+
hs_epout_desc.bEndpointAddress = fs_epout_desc.bEndpointAddress;
- hs_epout_desc.wMaxPacketSize = fs_epout_desc.wMaxPacketSize;
hs_epin_desc.bEndpointAddress = fs_epin_desc.bEndpointAddress;
- hs_epin_desc.wMaxPacketSize = fs_epin_desc.wMaxPacketSize;
ret = usb_assign_descriptors(fn, fs_audio_desc, hs_audio_desc, NULL);
if (ret)
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/1] usb: gadget: f_uac2: finalize wMaxPacketSize according to bandwidth
On 07/28/2015 03:38 AM, Peter Chen wrote:
diff --git a/drivers/usb/gadget/function/f_uac2.c
b/drivers/usb/gadget/function/f_uac2.c
index 5318615..6a8e0d2 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -975,6 +975,31 @@ free_ep(struct uac2_rtd_params *prm, struct usb_ep *ep)
%s:%d Error!\n, __func__, __LINE__);
}
+static void set_ep_max_packet_size (struct f_uac2_opts *uac2_opts,
+ struct usb_endpoint_descriptor *ep_desc, unsigned int factor)
scripts/checkpatch.pl will complain about a stray space before '(' and
wrong indentation. Also, uac2_opts can be const.
+{
+ int chmask;
+ int srate;
+ int ssize;
These can be put in one line.
+ u16 max_packet_size;
+
+ if (ep_desc == fs_epin_desc || ep_desc == hs_epin_desc) {
+ chmask = uac2_opts-p_chmask;
+ srate = uac2_opts-p_srate;
+ ssize = uac2_opts-p_ssize;
+ } else {
+ WARN_ON (ep_desc != fs_epout_desc ep_desc !=
hs_epout_desc);
I would rather pass a boolean flag called 'is_playback' than checking
the input parameter like this. But I forgot this detail in my proposal,
I admit.
Apart from that, I like the patch.
Thanks,
Daniel
+ chmask = uac2_opts-c_chmask;
+ srate = uac2_opts-c_srate;
+ ssize = uac2_opts-c_ssize;
+ }
+
+ max_packet_size = num_channels(chmask) * ssize *
+ DIV_ROUND_UP(srate, factor / (1 (ep_desc-bInterval - 1)));
+ ep_desc-wMaxPacketSize = min(cpu_to_le16(max_packet_size),
+ ep_desc-wMaxPacketSize);
+}
+
static int
afunc_bind(struct usb_configuration *cfg, struct usb_function *fn)
{
@@ -1070,10 +1095,14 @@ afunc_bind(struct usb_configuration *cfg, struct
usb_function *fn)
uac2-p_prm.uac2 = uac2;
uac2-c_prm.uac2 = uac2;
+ /* Calculate wMaxPacketSize according to audio bandwidth */
+ set_ep_max_packet_size(uac2_opts, fs_epin_desc, 1000);
+ set_ep_max_packet_size(uac2_opts, fs_epout_desc, 1000);
+ set_ep_max_packet_size(uac2_opts, hs_epin_desc, 8000);
+ set_ep_max_packet_size(uac2_opts, hs_epout_desc, 8000);
+
hs_epout_desc.bEndpointAddress = fs_epout_desc.bEndpointAddress;
- hs_epout_desc.wMaxPacketSize = fs_epout_desc.wMaxPacketSize;
hs_epin_desc.bEndpointAddress = fs_epin_desc.bEndpointAddress;
- hs_epin_desc.wMaxPacketSize = fs_epin_desc.wMaxPacketSize;
ret = usb_assign_descriptors(fn, fs_audio_desc, hs_audio_desc, NULL);
if (ret)
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/2] usb: gadget: f_uac2: finalize wMaxPacketSize according to bandwidth
On 07/27/2015 08:51 AM, Peter Chen wrote: According to USB Audio Device 2.0 Spec, Ch4.10.1.1: wMaxPacketSize is defined as follows: Maximum packet size this endpoint is capable of sending or receiving when this configuration is selected. This is determined by the audio bandwidth constraints of the endpoint. In current code, the wMaxPacketSize is defined as the maximum packet size for ISO endpoint, and it will let the host reserve much more space than it really needs, so that we can't let more endpoints work together at one frame. We find this issue when we try to let 4 f_uac2 gadgets work together [1] at FS connection. [1]http://www.spinics.net/lists/linux-usb/msg123478.html Cc: andrze...@samsung.com Cc: Daniel Mack zon...@gmail.com Cc: ti...@suse.de Cc: sta...@vger.kernel.org #v3.18+ Cc: Alan Stern st...@rowland.harvard.edu Signed-off-by: Peter Chen peter.c...@freescale.com --- Changes for v3: - Consider 'bInterval' to calculate wMaxPacketSize - playback endpoint is IN ep, and capture endpoint is OUT ep Changes for v2: - Using DIV_ROUND_UP to calculate max packet size drivers/usb/gadget/function/f_uac2.c | 33 +++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c index 6d3eb8b..51ca32d 100644 --- a/drivers/usb/gadget/function/f_uac2.c +++ b/drivers/usb/gadget/function/f_uac2.c @@ -987,6 +987,8 @@ afunc_bind(struct usb_configuration *cfg, struct usb_function *fn) struct f_uac2_opts *uac2_opts; struct usb_string *us; int ret; + u16 max_packet_size; + struct usb_endpoint_descriptor *ep_desc; uac2_opts = container_of(fn-fi, struct f_uac2_opts, func_inst); @@ -1070,10 +1072,37 @@ afunc_bind(struct usb_configuration *cfg, struct usb_function *fn) uac2-p_prm.uac2 = uac2; uac2-c_prm.uac2 = uac2; + /* Calculate wMaxPacketSize according to audio bandwidth */ + ep_desc = fs_epin_desc; + max_packet_size = num_channels(uac2_opts-p_chmask) * + uac2_opts-p_ssize * DIV_ROUND_UP(uac2_opts-p_srate, + 1000 / (1 (ep_desc-bInterval - 1))); + ep_desc-wMaxPacketSize = min(cpu_to_le16(max_packet_size), + ep_desc-wMaxPacketSize); + + ep_desc = fs_epout_desc; + max_packet_size = num_channels(uac2_opts-c_chmask) * + uac2_opts-c_ssize * DIV_ROUND_UP(uac2_opts-c_srate, + 1000 / (1 (ep_desc-bInterval - 1))); + ep_desc-wMaxPacketSize = min(cpu_to_le16(max_packet_size), + ep_desc-wMaxPacketSize); + + ep_desc = hs_epin_desc; + max_packet_size = num_channels(uac2_opts-p_chmask) * + uac2_opts-p_ssize * DIV_ROUND_UP(uac2_opts-p_srate, + 8000 / (1 (ep_desc-bInterval - 1))); + ep_desc-wMaxPacketSize = min(cpu_to_le16(max_packet_size), + ep_desc-wMaxPacketSize); + + ep_desc = hs_epout_desc; + max_packet_size = num_channels(uac2_opts-c_chmask) * + uac2_opts-c_ssize * DIV_ROUND_UP(uac2_opts-c_srate, + 8000 / (1 (ep_desc-bInterval - 1))); + ep_desc-wMaxPacketSize = min(cpu_to_le16(max_packet_size), + ep_desc-wMaxPacketSize); Basically, the same operation is repeated 4 times here. What about factoring the code out to a helper function, so that the call site looks like this: set_ep_max_packet_size(fs_epin_desc, uac2_opts, 1000); set_ep_max_packet_size(fs_epout_desc, uac2_opts, 1000); set_ep_max_packet_size(hs_epin_desc, uac2_opts, 8000); set_ep_max_packet_size(hs_epout_desc, uac2_opts, 8000); Other than that, the patch now looks good to me! Thanks, Daniel + hs_epout_desc.bEndpointAddress = fs_epout_desc.bEndpointAddress; - hs_epout_desc.wMaxPacketSize = fs_epout_desc.wMaxPacketSize; hs_epin_desc.bEndpointAddress = fs_epin_desc.bEndpointAddress; - hs_epin_desc.wMaxPacketSize = fs_epin_desc.wMaxPacketSize; ret = usb_assign_descriptors(fn, fs_audio_desc, hs_audio_desc, NULL); if (ret) -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] usb: gadget: f_uac2: fix calculation of uac2-p_interval
On 07/27/2015 08:51 AM, Peter Chen wrote:
The p_interval should be less if the 'bInterval' at the descriptor
is larger, eg, if 'bInterval' is 5 for HS, the p_interval should be
8000 / 16 = 500.
It fixes the patch 9bb87f16 usb: gadget: f_uac2: send reasonably
sized packets
Acked-by: Daniel Mack zon...@gmail.com
Thanks for spotting this!
Daniel
Cc: Daniel Mack zon...@gmail.com
Cc: sta...@vger.kernel.org #v3.18+
Signed-off-by: Peter Chen peter.c...@freescale.com
---
drivers/usb/gadget/function/f_uac2.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uac2.c
b/drivers/usb/gadget/function/f_uac2.c
index 51ca32d..4bd9a8e 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -1191,14 +1191,14 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
factor = 1000;
} else {
ep_desc = hs_epin_desc;
- factor = 125;
+ factor = 8000;
}
/* pre-compute some values for iso_complete() */
uac2-p_framesize = opts-p_ssize *
num_channels(opts-p_chmask);
rate = opts-p_srate * uac2-p_framesize;
- uac2-p_interval = (1 (ep_desc-bInterval - 1)) * factor;
+ uac2-p_interval = factor / (1 (ep_desc-bInterval - 1));
uac2-p_pktsize = min_t(unsigned int, rate / uac2-p_interval,
prm-max_psize);
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/1] usb: gadget: f_uac2: finalize wMaxPacketSize according to bandwidth
On 07/23/2015 03:00 AM, Peter Chen wrote:
Thanks, it is correct. But looking the code at afunc_set_alt:
the method of calculating uac2-p_pktsize seems incorrect, it
may need to change like below:
Ok, sorry. I just read the code again an figured you're right here, this
needs fixing. It doesn't really matter for the currently configured
values of bInterval though, as p_interval will always be 1000 for both
HS and FS.
@@ -1176,15 +1188,16 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
factor = 1000;
} else {
ep_desc = hs_epin_desc;
- factor = 125;
+ factor = 8000;
}
/* pre-compute some values for iso_complete() */
uac2-p_framesize = opts-p_ssize *
num_channels(opts-p_chmask);
rate = opts-p_srate * uac2-p_framesize;
- uac2-p_interval = (1 (ep_desc-bInterval - 1)) * factor;
- uac2-p_pktsize = min_t(unsigned int, rate / uac2-p_interval,
+ uac2-p_interval = factor / (1 (ep_desc-bInterval - 1));
Your version is correct. b_interval needs to get larger when bInterval
decreases of course.
+ uac2-p_pktsize = min_t(unsigned int, DIV_ROUND_UP(rate,
+ uac2-p_interval),
prm-max_psize);
This change, however, is not needed. uac2-p_pktsize needs to be rounded
down, so an extra frame can be added when the residue accumulator
overflows. The reason is simply that we can only send packets that
contain full sample frames, so we have to evenly distribute those
left-over samples that accumulate in one go once we have enough to fill
a complete frame.
Could you put the above change in an extra patch, as it's not directly
related to your wMaxPacketSize change?
Thanks,
Daniel
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/1] usb: gadget: f_uac2: finalize wMaxPacketSize according to bandwidth
On 07/23/2015 03:00 AM, Peter Chen wrote:
That detail is merely about completeness. The code that calculates the
value of wMaxPacketSize should take into account what is configured in
bInterval of the endpoint, so if users change one thing, they don't have
to tweak the other as well.
bInterval denotes how many packets an endpoint can serve per second, and
wMaxPacketSize defines how large each packet can be. So in an
application that knows how many bytes/s are to be transferred,
wMaxPacketSize depends on bInterval.
On HS endpoints, we have 8 microframes per USB frame, so the divisor is
8000, not 1000. However, I just figured the descriptors in f_uac2 set
.bInterval to 4, which means a period of 8 (2^(4-1)), and that
compensates the factor again.
So, to conclude - your calculation indeed comes up with the correct
value, but it should still take the configured endpoint details into
account so the code makes clear how the numbers are determined.
Something like the following should work:
/* for FS */
div = 1000 / (1 (fs_epout_desc-bInterval - 1));
/* for HS */
div = 8000 / (1 (hs_epout_desc-bInterval - 1));
c_max_packet_size = uac2_opts-c_chmask * uac2_opts-c_ssize
* DIV_ROUND_UP(uac2_opts-c_srate, div);
Makes sense?
Thanks, it is correct. But looking the code at afunc_set_alt:
the method of calculating uac2-p_pktsize seems incorrect, it
may need to change like below:
@@ -1176,15 +1188,16 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
factor = 1000;
} else {
ep_desc = hs_epin_desc;
- factor = 125;
+ factor = 8000;
}
/* pre-compute some values for iso_complete() */
uac2-p_framesize = opts-p_ssize *
num_channels(opts-p_chmask);
rate = opts-p_srate * uac2-p_framesize;
- uac2-p_interval = (1 (ep_desc-bInterval - 1)) * factor;
- uac2-p_pktsize = min_t(unsigned int, rate / uac2-p_interval,
+ uac2-p_interval = factor / (1 (ep_desc-bInterval - 1));
+ uac2-p_pktsize = min_t(unsigned int, DIV_ROUND_UP(rate,
+ uac2-p_interval),
prm-max_psize);
Your p_interval calculation is equivalent in both cases:
FS: 1 * 1000 == 1000 / 1
HS: 8 * 125 == 8000 / 8
And no, p_pktsize is intentionally set to the minimum packet size that a
packet will usually have. The actual size might be higher due to the
accumulated residue (see below).
Two more questions:
1. If the wMaxPacketSize is calculated correctly at afunc_bind, could we use
it
directly at afunc_set_alt?
All code that sets up runtime parameters should live in afunc_set_alt(),
while code that cares for preparation of descriptor parameters remains
in afunc_setup(). At least in theory, the driver could support multiple
alternate settings which operate on different parameters.
2. If we use DIV_ROUND_UP to calculate packet size, do we still need
p_pktsize_residue?
The packets the audio driver sends can only contain full sample frames,
and the residue cares about accumulated left-overs that are smaller than
such frames. Setting p_pktsize with DIV_ROUND_UP() would cause every
packet to be slightly too large in certain setups, which will make the
audio run slightly too fast. So yes, we do need the residue logic in
order to provide exact timing.
Note that this is different from the wMaxPacketSize calculation, which
is a value that's stored in the descriptors, transfered to the host and
cached there, so it and cannot be changed at runtime. Hence, it has to
prepare for the 'worst' case, while the determination of actual packet
sizes at runtime might come up with smaller values than the maximum.
Thanks,
Daniel
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/1] usb: gadget: f_uac2: finalize wMaxPacketSize according to bandwidth
On 07/23/2015 10:35 AM, Peter Chen wrote:
Assume the interval for each packet is 2ms, the rate is 192 kbytes
for both FS HS:
uac2-p_interval = 2000;
uac2-p_pktsize = 192 kbytes / 2000 = 96 bytes
And the uac2-p_pktsize is real usb packet length, it means hardware
would expect there are 96 bytes per 2ms, but the real frame rate is 192k,
and it needs to 192 * 2 bytes per 2ms in the bus, am I missing
something?
Hmm? The numerator in that division ('rate') includes the actual frame size:
rate = opts-p_srate * uac2-p_framesize;
uac2-p_pktsize = rate / uac2-p_interval;
Which means for 192KHz, stereo 16bit, rate is 768000. In this case,
bInterval = 4 doesn't work, as the packets would become 512 bytes, so
the bandwidth wouldn't suffice.
Another think I am not understand is why playback uses IN endpoint?
Don't this playback mean the data is from host, and play at device side?
That's a little confusing on first sight, but actually quite logical:
The ALSA capture stream is the one that allows applications on the
gadget to record audio, which is data that is sent *from* the host
(playback on their side) to the device, using OUT tokens.
The ALSA playback stream is the one that allows applications on the
gadget to playback audio, which is data that is sent *to* the host
(capture on their side) by the device, using IN tokens.
Thanks,
Daniel
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/1] usb: gadget: f_uac2: finalize wMaxPacketSize according to bandwidth
On 07/22/2015 08:45 AM, Peter Chen wrote:
According to USB Audio Device 2.0 Spec, Ch4.10.1.1:
wMaxPacketSize is defined as follows:
Maximum packet size this endpoint is capable of sending or receiving
when this configuration is selected.
This is determined by the audio bandwidth constraints of the endpoint.
In current code, the wMaxPacketSize is defined as the maximum packet size
for ISO endpoint, and it will let the host reserve much more space than
it really needs, so that we can't let more endpoints work together at
one frame.
We find this issue when we try to let 4 f_uac2 gadgets work together [1]
at FS connection.
[1]http://www.spinics.net/lists/linux-usb/msg123478.html
Cc: andrze...@samsung.com
Cc: zon...@gmail.com
Cc: ti...@suse.de
Cc: sta...@vger.kernel.org #v3.18+
Cc: Alan Stern st...@rowland.harvard.edu
Signed-off-by: Peter Chen peter.c...@freescale.com
---
Changes for v2:
- Using DIV_ROUND_UP to calculate max packet size
drivers/usb/gadget/function/f_uac2.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/drivers/usb/gadget/function/f_uac2.c
b/drivers/usb/gadget/function/f_uac2.c
index 6d3eb8b..6eaa4c4 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -987,6 +987,7 @@ afunc_bind(struct usb_configuration *cfg, struct
usb_function *fn)
struct f_uac2_opts *uac2_opts;
struct usb_string *us;
int ret;
+ u16 c_max_packet_size, p_max_packet_size;
uac2_opts = container_of(fn-fi, struct f_uac2_opts, func_inst);
@@ -1070,6 +1071,19 @@ afunc_bind(struct usb_configuration *cfg, struct
usb_function *fn)
uac2-p_prm.uac2 = uac2;
uac2-c_prm.uac2 = uac2;
+ /* Calculate wMaxPacketSize according to audio bandwidth */
+ c_max_packet_size = uac2_opts-c_chmask * uac2_opts-c_ssize
+ * DIV_ROUND_UP(uac2_opts-c_srate, 1000);
+ p_max_packet_size = uac2_opts-p_chmask * uac2_opts-p_ssize
+ * DIV_ROUND_UP(uac2_opts-p_srate, 1000);
+ if ((c_max_packet_size fs_epout_desc.wMaxPacketSize) ||
+ (p_max_packet_size fs_epin_desc.wMaxPacketSize)) {
+ dev_err(dev, parameters are incorrect\n);
+ goto err;
+ }
+ fs_epin_desc.wMaxPacketSize = cpu_to_le16(c_max_packet_size);
+ fs_epout_desc.wMaxPacketSize = cpu_to_le16(p_max_packet_size);
+
hs_epout_desc.bEndpointAddress = fs_epout_desc.bEndpointAddress;
hs_epout_desc.wMaxPacketSize = fs_epout_desc.wMaxPacketSize;
Your calculation still doesn't take into account the endpoint's
'bInterval', and for HS, the value is still wrong.
Daniel
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/1] usb: gadget: f_uac2: finalize wMaxPacketSize according to bandwidth
On 07/22/2015 10:23 AM, Peter Chen wrote:
On Wed, Jul 22, 2015 at 10:04:30AM +0200, Daniel Mack wrote:
On 07/22/2015 08:45 AM, Peter Chen wrote:
According to USB Audio Device 2.0 Spec, Ch4.10.1.1:
wMaxPacketSize is defined as follows:
Maximum packet size this endpoint is capable of sending or receiving
when this configuration is selected.
This is determined by the audio bandwidth constraints of the endpoint.
In current code, the wMaxPacketSize is defined as the maximum packet size
for ISO endpoint, and it will let the host reserve much more space than
it really needs, so that we can't let more endpoints work together at
one frame.
We find this issue when we try to let 4 f_uac2 gadgets work together [1]
at FS connection.
[1]http://www.spinics.net/lists/linux-usb/msg123478.html
Cc: andrze...@samsung.com
Cc: zon...@gmail.com
Cc: ti...@suse.de
Cc: sta...@vger.kernel.org #v3.18+
Cc: Alan Stern st...@rowland.harvard.edu
Signed-off-by: Peter Chen peter.c...@freescale.com
---
Changes for v2:
- Using DIV_ROUND_UP to calculate max packet size
drivers/usb/gadget/function/f_uac2.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/drivers/usb/gadget/function/f_uac2.c
b/drivers/usb/gadget/function/f_uac2.c
index 6d3eb8b..6eaa4c4 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -987,6 +987,7 @@ afunc_bind(struct usb_configuration *cfg, struct
usb_function *fn)
struct f_uac2_opts *uac2_opts;
struct usb_string *us;
int ret;
+ u16 c_max_packet_size, p_max_packet_size;
uac2_opts = container_of(fn-fi, struct f_uac2_opts, func_inst);
@@ -1070,6 +1071,19 @@ afunc_bind(struct usb_configuration *cfg, struct
usb_function *fn)
uac2-p_prm.uac2 = uac2;
uac2-c_prm.uac2 = uac2;
+ /* Calculate wMaxPacketSize according to audio bandwidth */
+ c_max_packet_size = uac2_opts-c_chmask * uac2_opts-c_ssize
+ * DIV_ROUND_UP(uac2_opts-c_srate, 1000);
+ p_max_packet_size = uac2_opts-p_chmask * uac2_opts-p_ssize
+ * DIV_ROUND_UP(uac2_opts-p_srate, 1000);
+ if ((c_max_packet_size fs_epout_desc.wMaxPacketSize) ||
+ (p_max_packet_size fs_epin_desc.wMaxPacketSize)) {
+ dev_err(dev, parameters are incorrect\n);
+ goto err;
+ }
+ fs_epin_desc.wMaxPacketSize = cpu_to_le16(c_max_packet_size);
+ fs_epout_desc.wMaxPacketSize = cpu_to_le16(p_max_packet_size);
+
hs_epout_desc.bEndpointAddress = fs_epout_desc.bEndpointAddress;
hs_epout_desc.wMaxPacketSize = fs_epout_desc.wMaxPacketSize;
Your calculation still doesn't take into account the endpoint's
'bInterval', and for HS, the value is still wrong.
I still not understand why I need to consider 'bInterval' for packet
size, per my understanding, 'bInterval' is the interval time for sending
each packet. At current code, it defines wMaxPacketSize as max value
(1023/1024) for one packet, it may cause problem for audio driver,
so you have the patch (9bb87f168931 usb: gadget: f_uac2: send reasonably
sized packets) for reducing packet size according to its 'bInterval', but
with my change, the wMaxPacketSize will be smaller than its max value,
do we still need to reduce packet size for each transfer?
That detail is merely about completeness. The code that calculates the
value of wMaxPacketSize should take into account what is configured in
bInterval of the endpoint, so if users change one thing, they don't have
to tweak the other as well.
bInterval denotes how many packets an endpoint can serve per second, and
wMaxPacketSize defines how large each packet can be. So in an
application that knows how many bytes/s are to be transferred,
wMaxPacketSize depends on bInterval.
On HS endpoints, we have 8 microframes per USB frame, so the divisor is
8000, not 1000. However, I just figured the descriptors in f_uac2 set
.bInterval to 4, which means a period of 8 (2^(4-1)), and that
compensates the factor again.
So, to conclude - your calculation indeed comes up with the correct
value, but it should still take the configured endpoint details into
account so the code makes clear how the numbers are determined.
Something like the following should work:
/* for FS */
div = 1000 / (1 (fs_epout_desc-bInterval - 1));
/* for HS */
div = 8000 / (1 (hs_epout_desc-bInterval - 1));
c_max_packet_size = uac2_opts-c_chmask * uac2_opts-c_ssize
* DIV_ROUND_UP(uac2_opts-c_srate, div);
Makes sense?
Thanks,
Daniel
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] usb: gadget: f_uac2: finalize wMaxPacketSize according to bandwidth
Hi,
On 07/14/2015 04:29 AM, Peter Chen wrote:
According to USB Audio Device 2.0 Spec, Ch4.10.1.1:
wMaxPacketSize is defined as follows:
Maximum packet size this endpoint is capable of sending or receiving
when this configuration is selected.
This is determined by the audio bandwidth constraints of the endpoint.
In current code, the wMaxPacketSize is defined as the maximum packet size
for ISO endpoint, and it will let the host reserve much more space than
it really needs, so that we can't let more endpoints work together at
one frame.
...
diff --git a/drivers/usb/gadget/function/f_uac2.c
b/drivers/usb/gadget/function/f_uac2.c
index 6d3eb8b..0ed6f0e 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -987,6 +987,7 @@ afunc_bind(struct usb_configuration *cfg, struct
usb_function *fn)
struct f_uac2_opts *uac2_opts;
struct usb_string *us;
int ret;
+ u16 c_max_packet_size, p_max_packet_size;
uac2_opts = container_of(fn-fi, struct f_uac2_opts, func_inst);
@@ -1070,6 +1071,29 @@ afunc_bind(struct usb_configuration *cfg, struct
usb_function *fn)
uac2-p_prm.uac2 = uac2;
uac2-c_prm.uac2 = uac2;
+ /* Calculate wMaxPacketSize according to audio bandwidth */
+ c_max_packet_size = uac2_opts-c_chmask * uac2_opts-c_ssize
+ * uac2_opts-c_srate / 1000;
+ p_max_packet_size = uac2_opts-p_chmask * uac2_opts-p_ssize
+ * uac2_opts-p_srate / 1000;
For high-speed endpoints, we in fact have 8 microframes per frame, and
the factor also depends on the endpoint's polling interval. See what
afunc_set_alt() does here.
Did you test this in HS or FS setups or both?
+ if ((c_max_packet_size fs_epout_desc.wMaxPacketSize) ||
+ (p_max_packet_size fs_epin_desc.wMaxPacketSize)) {
+ dev_err(dev, parameters are incorrect\n);
+ goto err;
+ }
+ /*
+ * Keep max packet size is multiplier of 4, and
+ * a little larger than bandwidth.
+ * Eg, for frame rate 44100, 1 channel, and 16 bits data
+ * we need to reserve more than 44 * 2 bytes.
+ */
+ c_max_packet_size += (4 - (c_max_packet_size % 4));
I guess DIV_ROUND_UP() allows you to account for the packets that are
sent when the devision residue accumulator overflows. See
agdev_iso_complete().
+ fs_epin_desc.wMaxPacketSize = min(cpu_to_le16(c_max_packet_size),
+ fs_epin_desc.wMaxPacketSize);
+ p_max_packet_size += (4 - (p_max_packet_size % 4));
+ fs_epout_desc.wMaxPacketSize = min(cpu_to_le16(p_max_packet_size),
+ fs_epout_desc.wMaxPacketSize);
+
hs_epout_desc.bEndpointAddress = fs_epout_desc.bEndpointAddress;
hs_epout_desc.wMaxPacketSize = fs_epout_desc.wMaxPacketSize;
We probably have to pass different values for the two settings, as they
can't be the same once they contain pre-calculated values.
Thanks,
Daniel
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gagdet: f_midi: fix parameter assignment
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 11/20/2014 08:49 PM, Felipe Balbi wrote: On Tue, Nov 18, 2014 at 11:21:17PM +0100, Daniel Mack wrote: f_midi_register_card() uses midi-id and midi-index, so they need to be assigned before the function is called from f_midi_bind_config(). Move the assigment of midi-buflen and midi-qlen as well so they are all grouped. Compile tested only. Signed-off-by: Daniel Mack dan...@zonque.org doesn't apply: checking file drivers/usb/gadget/function/f_midi.c Hunk #1 FAILED at 954. Hunk #2 FAILED at 965. 2 out of 2 hunks FAILED can you rebase on my testing/next ? Ah, I thought for a fix like this, your 'fixes' branch is enough. In your testing/next, the code looks quite different, and if I read that correctly, the bug is already fixed there. Guess you can just drop the patch then :) Thanks, Daniel -BEGIN PGP SIGNATURE- Version: GnuPG v1 iQIcBAEBAgAGBQJUboCeAAoJELphMRr8Y1QkFD4QAKZB+gwXgEiEqueazfiSj/LP WfVJiw09JcIBsupserd7ILYJyQXOuOEvBxAP/Jak0iOG1Vc8NaR1a4g3bDYGBEGU PWd+8WkV45hj/tZbAJwoFp92gIvWyXIVYJZc2B/EqpT+eyq653zvuy2oQl3x5jpo pc80eGBD3/DkzQEUe7U/lB05RnlOle+7+I0yOPg3W1X2Bc7OJFbSVekg5fiMCAee wbmnYKDp7F923PQCs99zVNCS8o8c4lPuSZH4VSzJ5gyctLPcGv33O4ECPx3qYDWQ L9iwgczD4UQ2yzxbXVwAi1yXUm120xE5ktdYO4Ry+d/VMP/dq22QNhy2e9SiydaQ SBfen1bW7bs4+C7WUiv5TEKl2SXKdXes/TIqqii75YiNuv8L/GAJQ6KWYghIGZ+K c9K4FHIpKsigRQtCOkd4finA1gaWYrGMarFXM7UlGoAJ2UuqLE1vm/LOj3I5lUhs d0hUvGe6jegLRcCTnQj1nxc4ejf2RHKsM9H1TNcTN2LwSg0wrV+n5UM54vfVbZFy hPVjEsV+XYdjGsWgycVNN+nHN4SUk91JSzFwtA7lPgvNasN/866ZCnx3bkJ1ThUJ 8+z19LKTPgOzVUjaobf6LEjeQKve48xLFrd0bYDB+orN6+SiI32jVfu+/dyFdMjr 5BErg/WaF+G3kdmVcq89 =P8sF -END PGP SIGNATURE- -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: gagdet: f_midi: fix parameter assignment
f_midi_register_card() uses midi-id and midi-index, so they need to be assigned before the function is called from f_midi_bind_config(). Move the assigment of midi-buflen and midi-qlen as well so they are all grouped. Compile tested only. Signed-off-by: Daniel Mack dan...@zonque.org --- drivers/usb/gadget/function/f_midi.c | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c index 807b31c..a83f129 100644 --- a/drivers/usb/gadget/function/f_midi.c +++ b/drivers/usb/gadget/function/f_midi.c @@ -954,6 +954,11 @@ int __init f_midi_bind_config(struct usb_configuration *c, /* set up ALSA midi devices */ midi-in_ports = in_ports; midi-out_ports = out_ports; + midi-id = kstrdup(id, GFP_KERNEL); + midi-index = index; + midi-buflen = buflen; + midi-qlen = qlen; + status = f_midi_register_card(midi); if (status 0) goto setup_fail; @@ -965,11 +970,6 @@ int __init f_midi_bind_config(struct usb_configuration *c, midi-func.set_alt = f_midi_set_alt; midi-func.disable = f_midi_disable; - midi-id = kstrdup(id, GFP_KERNEL); - midi-index = index; - midi-buflen = buflen; - midi-qlen = qlen; - status = usb_add_function(c, midi-func); if (status) goto setup_fail; -- 2.1.0 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] usb: musb: musb_cppi41: revert to old timer poll intervals
On 11/05/2014 08:21 PM, Sebastian Andrzej Siewior wrote: On 11/05/2014 08:20 PM, Felipe Balbi wrote: could you just resend these two aptches ? I think about waiting for Daniel for some feedback. What about I resend them next week if I don't hear from him until then? Sorry for the delay - I'm currently traveling and won't be back before early next week. I hope to have a chance to test the patches then! Sebastian (Reimers), given that you have a good test setup too, any chance you can as well do some testing here? Thanks, Daniel -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: udc: pxa27x: fix build warning when !OF
On 11/06/2014 05:55 AM, Felipe Balbi wrote:
in case kernel is built without CONFIG_OF there
will be a build warning (see below) due to of_match_ptr()
being defined to NULL.
Because of_match_ptr() is pretty pointless anyway,
let's just remove it and fix the warning.
The alternative, of course, would be to wrap the udc_pxa_dt_ids
declaration in IS_ENABLED(CONFIG_OF), but I'm not sure whether it's
worth it.
Daniel
drivers/usb/gadget/udc/pxa27x_udc.c:2403:28: warning: ‘udc_pxa_dt_ids’
defined but not used [-Wunused-variable]
static struct of_device_id udc_pxa_dt_ids[] = {
^
Signed-off-by: Felipe Balbi ba...@ti.com
---
drivers/usb/gadget/udc/pxa27x_udc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/udc/pxa27x_udc.c
b/drivers/usb/gadget/udc/pxa27x_udc.c
index 69e7b816..d1d8a9f 100644
--- a/drivers/usb/gadget/udc/pxa27x_udc.c
+++ b/drivers/usb/gadget/udc/pxa27x_udc.c
@@ -2590,7 +2590,7 @@ static struct platform_driver udc_driver = {
.driver = {
.name = pxa27x-udc,
.owner = THIS_MODULE,
- .of_match_table = of_match_ptr(udc_pxa_dt_ids),
+ .of_match_table = udc_pxa_dt_ids,
},
.probe = pxa_udc_probe,
.remove = pxa_udc_remove,
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] usb: musb: musb_cppi41: revert to old timer poll intervals
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 11/06/2014 08:38 AM, Sebastian Reimers wrote: Sebastian (Reimers), given that you have a good test setup too, any chance you can as well do some testing here? Of course. I can try to reproduce your bug and then retry it with Sebastian's patch, anything special with the audio setup (sampling rate, format)? Thanks! You should check two things: a) The data throughput on mass storage media should improve by ~15%, according to Sebastian's measurements, and b) Both FS and HS audio devices should still function as before, with no regression regarding CPU load while streaming. Thanks, Daniel -BEGIN PGP SIGNATURE- Version: GnuPG v1 iQIcBAEBAgAGBQJUWykmAAoJELphMRr8Y1QkaaAP/3Cm0UEMclHdz1vIOOjkBfql QiNwBhrlG3CuWMhpgR6i7YWHBj/gjM9U9y+ovepuKQI7Ee1vFClyVu8fECejy3Ml n2bErBVbFUID7/1rB0x3lLUXwG3kgwLNBzi+xVfRnUnmKRHPDU2fvOeTkpjHqcqg 4Ix/HNxyPIbAl0dFC+yVpqsJgxBXj7sGFhzRjffuSYYUdNzlXOprrDRIIIJZiuzo z4jUYqEXB19/TqnbbT0NgVncVTpvE0g/bvV+s0KZRVVEDZ5zhoEx3YvGr1ZnCuH+ rA1Q/aML8BAQBSfPbccPy7S1nDNEGwNrXcWzo4loPRRdm/gNHdSblafm0PacJbBL G7AhxP/qTKzlwDHpz4LkuX8NLjR09E0RJxAVmwiszlmHAJ39mVkWVtz96T82Z/1I JCilh2wL/Bl6XvYwHqt7EMheHhiNVo57a2s0sBb/9tfAU1PLAVYqSm7nqCWPQLtk TP9UrMj2dBB3Qqxs4o0UOM80raoRJCdm2xeA4pzfqE0dYa8HV2p/sJP40hzANFWu aidhKfoLM043e2Nk+RMipRpOjm+vq5UKP1Hnesw2VaQD17oPwjC+S5BiqxwmtkNX n4WZkQP7kJLKHStKkN+F1fTS8P8194djRulZAMR/1VFEGs5Bga/TO2+GyWC1YOTU U82blEUHAMFrvmcUP2Ul =hu6S -END PGP SIGNATURE- -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: musb: cppi41: tweak hrtimer values
Intensive tests with USB audio devices connected to a musb host port
have shown reproducible pops and clicks in both the playback and the
capture stream.
These are related to how the early_tx hrtimer is set up, and it turns
out they can be fixed by reducing the timer's slack value from 40 to
25 us. Also, when the callback is ran without taking action, it should
be rescheduled 20 us later instead of 50 us.
Reported-and-tested-by: Sven Neumann neum...@teufel.de
Signed-off-by: Daniel Mack dan...@zonque.org
---
drivers/usb/musb/musb_cppi41.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c
index 47ae645..ecdd632 100644
--- a/drivers/usb/musb/musb_cppi41.c
+++ b/drivers/usb/musb/musb_cppi41.c
@@ -200,7 +200,7 @@ static enum hrtimer_restart cppi41_recheck_tx_req(struct
hrtimer *timer)
if (!list_empty(controller-early_tx_list)) {
ret = HRTIMER_RESTART;
hrtimer_forward_now(controller-early_tx,
- ktime_set(0, 50 * NSEC_PER_USEC));
+ ktime_set(0, 20 * NSEC_PER_USEC));
}
spin_unlock_irqrestore(musb-lock, flags);
@@ -278,7 +278,7 @@ static void cppi41_dma_callback(void *private_data)
hrtimer_start_range_ns(controller-early_tx,
ktime_set(0, usecs * NSEC_PER_USEC),
- 40 * NSEC_PER_USEC,
+ 20 * NSEC_PER_USEC,
HRTIMER_MODE_REL);
}
}
--
2.1.0
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/5] usb: gadget: f_uac2: restructure some code in afunc_set_alt()
Hi, On 09/02/2014 02:41 PM, Jassi Brar wrote: On Sat, Aug 30, 2014 at 2:16 AM, Felipe Balbi ba...@ti.com wrote: Hi, On Wed, Aug 27, 2014 at 07:09:03PM +0200, Daniel Mack wrote: Restructure some code to make it easier to read. While at it, return -ENOMEM instead of -EINVAL if usb_ep_alloc_request() fails, and omit the logging in such cases (the mm core will complain loud enough). Signed-off-by: Daniel Mack zon...@gmail.com --- does this depend on anything ? It doesn't apply to my testing/next There's v6 of the patchset here http://www.spinics.net/lists/linux-usb/msg112769.html though the 1-4 patches are probably same. This is what Felipe used, but I in fact based my patches on the wrong branch. There's both 'testing/next' and 'next' remote branches, and I picked the latter. I'll quickly rebase and resend. For Patchv6-5/5 we need either http://www.spinics.net/lists/linux-usb/msg112773.html or http://www.spinics.net/lists/linux-usb/msg112913.html I and Daniel feel strongly about how we implement data rate control. Please share your decision making. Yeah, I'll describe that in the updated cover letter. I'm sure we'll find a good solution for that :) Thanks, Daniel -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/5] usb: gadget: f_uac2: restructure some code in afunc_set_alt()
Hi Felipe, On 08/29/2014 10:46 PM, Felipe Balbi wrote: On Wed, Aug 27, 2014 at 07:09:03PM +0200, Daniel Mack wrote: Restructure some code to make it easier to read. While at it, return -ENOMEM instead of -EINVAL if usb_ep_alloc_request() fails, and omit the logging in such cases (the mm core will complain loud enough). Signed-off-by: Daniel Mack zon...@gmail.com --- does this depend on anything ? It doesn't apply to my testing/next The reason it doesn't apply is that you already applied v6 of my series to your testing/next tree: https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?h=testing/nextid=c52425b The currently pending discussion is about how to determine the size of the packets sent down to the host, and it only affects the last patch in the series. To summarize, here are the two approaches to do this: 1) We pre-calculate a pattern of lengths which is then followed when sending the packets. This is what Jassi implemented in his alternative approach to my version here: http://www.spinics.net/lists/linux-usb/msg112913.html This idea comes at the price of storing the pre-calculated sequence and track its state, which currently means adding 163 unsigned ints to the device's private structs. This, however could be reduced by only storing the pattern for the capture side and by using u8 or u16 instead of unsigned int for the pattern store. I haven't yet tested this version, though. 2) We keep track of the number of sent bytes and do an ad-hoc determination on how big the next packet should be, each time we send one to the host. This is what my v6 does, which you already have in your tree. This comes at the price of storing and maintaining 5 unsigned ints for keeping track of the current state and some pre-calculated numbers (so we don't have to do that for every packet), plus some calculations to be done for every packet. If we want to go for this solution, we don't have to add any patch on top. So, ultimately, it's a matter of trade-off, memory footprint vs. maintenance of 3 more state variables vs. some calculation done for each of the packets (~1000 per second). I really don't feel like arguing over this much, but my impression is that the currently applied version is more comprehensible, and the overhead caused by the calculation is not measurable (we do a lot more of such arithmetic in the snd-usb driver for each packet). So, decide for yourself which way to go, and let us know if you want any further action to be taken. Thanks, Daniel -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: f_uac2: modulate playback data rate
Hi,
On 08/29/2014 08:13 AM, Jassi Brar wrote:
+/*
+ * 5512.5Hz is going to need the maximum number of elements (80),
+ * in the length-pattern loop, among standard ALSA supported rates.
+ */
+#define MAX_LOOP_LEN 80
+
struct uac2_rtd_params {
struct snd_uac2_chip *uac2; /* parent chip */
bool ep_enabled; /* if the ep is enabled */
@@ -80,6 +87,9 @@ struct uac2_rtd_params {
unsigned max_psize;
struct uac2_req ureq[USB_XFERS];
+ unsigned pattern[MAX_LOOP_LEN];
+ unsigned plen; /* valid entries in pattern[] */
+
spinlock_t lock;
};
You're doing this for both directions, while only the capture side needs
such treatment.
+/*
+ * Find optimal pattern of payloads for a given number
+ * of samples and maximum sync period (in ms) over which
+ * we have to distribute them uniformly.
+ */
+static unsigned
+get_pattern(unsigned samples, unsigned sync, unsigned *pt)
+{
+ unsigned n, x = 0, i = 0, p = samples % sync;
+
+ do {
+ x += p;
+ n = samples / sync;
+ if (x = sync) {
+ n += 1;
+ x -= sync;
+ }
+ if (pt)
+ pt[i] = n;
+ i++;
+ } while (x);
+
+ return i;
+}
static int
afunc_set_alt(struct usb_function *fn, unsigned intf, unsigned alt)
{
@@ -1097,11 +1136,35 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
if (intf == agdev-as_out_intf) {
ep = agdev-out_ep;
prm = uac2-c_prm;
+ prm-plen = 1;
+ prm-pattern[0] = prm-max_psize;
config_ep_by_speed(gadget, fn, ep);
agdev-as_out_alt = alt;
} else if (intf == agdev-as_in_intf) {
+ struct f_uac2_opts *opts = agdev_to_uac2_opts(agdev);
+ unsigned intvl, rate;
+
+ if (gadget-speed == USB_SPEED_FULL)
+ intvl = (1 (fs_epin_desc.bInterval - 1)) * 1000;
+ else
+ intvl = (1 (hs_epin_desc.bInterval - 1)) * 125;
+
+ rate = opts-p_srate;
+ if (rate == 5512) { /* which implies 5512.5 practically */
+ rate = 55125;
+ intvl *= 10;
+ }
+
Well, I'd say that Alan's approach as implemented in my v6 is still more
comprehensible for anyone who'll try to understand this later, and it
doesn't make any assumption on run time values. Plus, it only adds 5
unsigned ints to struct snd_uac2_chip, while your version blows up
struct uac2_rtd_params by 81 ints.
But it's also a matter of taste, so I guess it's ultimately up to Felipe
now.
Thanks,
Daniel
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 5/5] usb: gadget: f_uac2: send reasonably sized packets
On 08/28/2014 11:33 AM, Jassi Brar wrote:
On Wed, Aug 27, 2014 at 10:39 PM, Daniel Mack zon...@gmail.com wrote:
diff --git a/drivers/usb/gadget/function/f_uac2.c
b/drivers/usb/gadget/function/f_uac2.c
index 246a778..a5a27a5 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -92,6 +92,15 @@ struct snd_uac2_chip {
struct snd_card *card;
struct snd_pcm *pcm;
+
+ /* timekeeping for the playback endpoint */
+ unsigned int p_interval;
+ unsigned int p_residue;
+
+ /* pre-calculated values for playback iso completion */
+ unsigned int p_pktsize;
+ unsigned int p_pktsize_residue;
+ unsigned int p_framesize;
};
I admire Alan's simple algo. I couldn't have matched that.
However I am not sure how worth is the implementation if it requires 3
more members to avoid calculations simple enough to cause no
noticeable overhead. Once every millisecond is perfectly bearable IMO.
Alan is right. If we can avoid that, we should.
5 members in uac2 structure for only playback, look ugly.
It's still superior to a number of unnecessary calculations done every
millisecond which will come up with the same result every time. So I
clearly favor that approach. Three more ints in a struct don't hurt
really for something that's usually not instantiated more than once per
system.
Anyway, I'll leave it to Felipe whether to merge v5 or v6 :)
Thanks,
Daniel
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 5/5] usb: gadget: f_uac2: send reasonably sized packets
On 08/28/2014 12:10 PM, Jassi Brar wrote: On Thu, Aug 28, 2014 at 3:33 PM, Daniel Mack dan...@zonque.org wrote: It's still superior to a number of unnecessary calculations done every millisecond which will come up with the same result every time. So I clearly favor that approach. Three more ints in a struct don't hurt really for something that's usually not instantiated more than once per system. Anyway, I'll leave it to Felipe whether to merge v5 or v6 :) Please wait, let me cook up a patch that uses (a) Alan's algo, (b) cause lesser churn and (c) is even 'faster'. Alright. Please note, however, that you can't do the divison 'uac2-p_residue / uac2-p_interval' in a pre-calucated value, as that won't cover cases with a per-packet residual value that isn't a multiple of the frame size. Hence, the residue counter has to go in bytes, not frames, and for that reason, I left that division in the per-packet loop. Felipe, could you already merge patch 1-5 of the last series (v6)? Thanks, Daniel -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 5/5] usb: gadget: f_uac2: send reasonably sized packets
On 08/28/2014 12:17 PM, Daniel Mack wrote: Felipe, could you already merge patch 1-5 of the last series (v6)? 1-4, of course. Jassi's changes will only affect patch 5/5. Daniel -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 5/5] usb: gadget: f_uac2: send reasonably sized packets
On 08/27/2014 06:08 AM, Jassi Brar wrote:
On Wed, Aug 27, 2014 at 3:23 AM, Daniel Mack zon...@gmail.com wrote:
+ uac2-p_interval = (1 (ep_desc-bInterval - 1)) * factor;
+ req_len = rate / uac2-p_interval;
+ if (opts-p_srate % uac2-p_interval)
+ req_len += fsz;
.
+ uac2-p_residue = 0;
} else {
dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
return -EINVAL;
@@ -1128,7 +1188,7 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
req-zero = 0;
req-context = prm-ureq[i];
- req-length = prm-max_psize;
+ req-length = req_len;
req-complete = agdev_iso_complete;
req-buf = prm-rbuf + i * req-length;
otherwise req[0]-buf might overlap req[1]-buf's first frame
for when we need to send an extra frame.
Hmm? The first USB_XFERS packets will only contain zeros, and we're only
preparing those here. For every successive packet, the length is
recalculated and the audio material is copied in accordingly before the
requets is requeued. What buffers should overlap here?
Daniel
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 5/5] usb: gadget: f_uac2: send reasonably sized packets
On 08/27/2014 09:07 AM, Jassi Brar wrote: On Wed, Aug 27, 2014 at 12:20 PM, Daniel Mack dan...@zonque.org wrote: Hmm? The first USB_XFERS packets will only contain zeros, and we're only preparing those here. For every successive packet, the length is recalculated and the audio material is copied in accordingly before the requets is requeued. What buffers should overlap here? For 44100/2/S16, req_len is 176 or 44 frames. But we need to send 45 frames in a packet occasionally. req[0]-buf = rbuf + 0 and req[1]-buf = rbuf + 176. No. req[0]-buf = rbuf + 0 and req[1]-buf = rbuf + max_psize. prm-max_psize is still set to wMaxPacketSize of the endpoint. We just decide how much of it to really use dynamically, each time before a packet with real payload is commited. But what if we req[0] needed to carry the packet with 45 frames? Then we do so in iso_complete(). This is unrelated to the first USB_XFERS dummy packets. I just wanted to avoid sending queuing them with max_psize length. Daniel -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 5/5] usb: gadget: f_uac2: send reasonably sized packets
On 08/27/2014 09:15 AM, Daniel Mack wrote: On 08/27/2014 09:07 AM, Jassi Brar wrote: On Wed, Aug 27, 2014 at 12:20 PM, Daniel Mack dan...@zonque.org wrote: Hmm? The first USB_XFERS packets will only contain zeros, and we're only preparing those here. For every successive packet, the length is recalculated and the audio material is copied in accordingly before the requets is requeued. What buffers should overlap here? For 44100/2/S16, req_len is 176 or 44 frames. But we need to send 45 frames in a packet occasionally. req[0]-buf = rbuf + 0 and req[1]-buf = rbuf + 176. No. req[0]-buf = rbuf + 0 and req[1]-buf = rbuf + max_psize. prm-max_psize is still set to wMaxPacketSize of the endpoint. We just decide how much of it to really use dynamically, each time before a packet with real payload is commited. As I said earlier, I decided to do it like this so that we can eventually implement sample rate and format switches during runtime. We wouldn't want to touch the size of allocated buffers when this happens, so let's keep them at the largest packet size that might ever occur on the endpoint. The *actual* size of the each request's buffer is a different thing. Thanks, Daniel -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 5/5] usb: gadget: f_uac2: send reasonably sized packets
On 08/27/2014 09:28 AM, Jassi Brar wrote: On Wed, Aug 27, 2014 at 12:45 PM, Daniel Mack dan...@zonque.org wrote: On 08/27/2014 09:07 AM, Jassi Brar wrote: On Wed, Aug 27, 2014 at 12:20 PM, Daniel Mack dan...@zonque.org wrote: Hmm? The first USB_XFERS packets will only contain zeros, and we're only preparing those here. For every successive packet, the length is recalculated and the audio material is copied in accordingly before the requets is requeued. What buffers should overlap here? For 44100/2/S16, req_len is 176 or 44 frames. But we need to send 45 frames in a packet occasionally. req[0]-buf = rbuf + 0 and req[1]-buf = rbuf + 176. No. req[0]-buf = rbuf + 0 and req[1]-buf = rbuf + max_psize. Note that you are referring to the buffer pointer here. You patch does - req-length = prm-max_psize; + req-length = req_len; Yes, because we tell the udc driver we want to commit req_len bytes in this request. req-buf, however, it set to a buffer that can accommodate wMaxPacketSize, so when the packet completes, we can stuff more bytes into it and modify req-length again. req-length it the request length, not the max size of the buffer we point to. Clearly for 44.1/2/S16, req_len evaluates to 176. Yes, that's the start condition. Once both the USB and the ALSA side are running, the req-length calculation in iso_complete() kicks in and tweaks the buffer sizes. Does it make sense now? Or am I missing anything? Thanks, Daniel -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 5/5] usb: gadget: f_uac2: send reasonably sized packets
On 08/27/2014 09:35 AM, Jassi Brar wrote:
On Wed, Aug 27, 2014 at 3:23 AM, Daniel Mack zon...@gmail.com wrote:
+ uac2-p_residue = 0;
} else {
dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
return -EINVAL;
@@ -1128,7 +1188,7 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
req-zero = 0;
req-context = prm-ureq[i];
- req-length = prm-max_psize;
+ req-length = req_len;
(b)req-length = req_len or 176
req-complete = agdev_iso_complete;
req-buf = prm-rbuf + i * req-length;
Here req[0]-buf is req-length, which is 176 bytes from (b).
Ah. Sorry, now I see what you mean. Yes, that should be
req-buf = prm-rbuf + i * p_maxsize
Alright, let me spin v5 later.
Thanks,
Daniel
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 0/5] usb: gadget: f_uac2: cleanups and capture timing
Hi, this is v4 of the f_uac2 timing fixup series. Changes from v4: * Fix buffer setup in afunc_set_alt() Changes from v3: * add another patch (3/5) to introduce agdev_to_uac2_opts() which is also needed in 5/5 * patch 5/5 only: move from a pre-calculated sequence of packet lengths to an accumulator that sums up the residual values from the packet size calculation and adds an extra sample frame now and then when the accumulator has gathered enough bytes. Changes from v2: * swap path 3 and 4, so that the ALSA buffer wrap around fix comes in first. It's not actually a bug fix for the current code, but more a preparation to allow for smaller packets. * use the p_ssize, p_chmask and p_srate for the length calculations * prepare a sequence of packet sizes to send, and alternate over them when sending the the IN packets. The actual commit log and the comments yield some more details on that. Changes from v1: * drop UAC_EP_CS_ATTR_FILL_MAX approach and rather size the packets correctly * add a patch to fix buffer wrap problems in the ALSA buffer logic, which wasn't needed before The first two patches are just cleanups. Thanks to Alan Stern and Jassi Brar for the feedback! Daniel Daniel Mack (5): usb: gadget: f_uac2: restructure some code in afunc_set_alt() usb: gadget: f_uac2: add short-hand for 'dev' usb: gadget: f_uac2: introduce agdev_to_uac2_opts usb: gadget: f_uac2: handle partial dma area wrap usb: gadget: f_uac2: send reasonably sized packets drivers/usb/gadget/function/f_uac2.c | 164 --- 1 file changed, 115 insertions(+), 49 deletions(-) -- 2.1.0 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 1/5] usb: gadget: f_uac2: restructure some code in afunc_set_alt()
Restructure some code to make it easier to read.
While at it, return -ENOMEM instead of -EINVAL if
usb_ep_alloc_request() fails, and omit the logging in such cases
(the mm core will complain loud enough).
Signed-off-by: Daniel Mack zon...@gmail.com
---
drivers/usb/gadget/function/f_uac2.c | 39 +++-
1 file changed, 16 insertions(+), 23 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uac2.c
b/drivers/usb/gadget/function/f_uac2.c
index 0d65e7c..ab4652e 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -1104,31 +1104,24 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
usb_ep_enable(ep);
for (i = 0; i USB_XFERS; i++) {
- if (prm-ureq[i].req) {
- if (usb_ep_queue(ep, prm-ureq[i].req, GFP_ATOMIC))
- dev_err(uac2-pdev.dev, %d Error!\n,
- __LINE__);
- continue;
- }
-
- req = usb_ep_alloc_request(ep, GFP_ATOMIC);
- if (req == NULL) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
- return -EINVAL;
+ if (!prm-ureq[i].req) {
+ req = usb_ep_alloc_request(ep, GFP_ATOMIC);
+ if (req == NULL)
+ return -ENOMEM;
+
+ prm-ureq[i].req = req;
+ prm-ureq[i].pp = prm;
+
+ req-zero = 0;
+ req-context = prm-ureq[i];
+ req-length = prm-max_psize;
+ req-complete = agdev_iso_complete;
+ req-buf = prm-rbuf + i * req-length;
}
- prm-ureq[i].req = req;
- prm-ureq[i].pp = prm;
-
- req-zero = 0;
- req-context = prm-ureq[i];
- req-length = prm-max_psize;
- req-complete = agdev_iso_complete;
- req-buf = prm-rbuf + i * req-length;
-
- if (usb_ep_queue(ep, req, GFP_ATOMIC))
- dev_err(uac2-pdev.dev, %d Error!\n, __LINE__);
+ if (usb_ep_queue(ep, prm-ureq[i].req, GFP_ATOMIC))
+ dev_err(uac2-pdev.dev, %s:%d Error!\n,
+ __func__, __LINE__);
}
return 0;
--
2.1.0
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 2/5] usb: gadget: f_uac2: add short-hand for 'dev'
In afunc_bind() and afunc_set_alt(), uac2-pdev.dev are used multiple
times. Adding a short-hand for them makes lines shorter so we can
remove some line wraps.
No functional change.
Signed-off-by: Daniel Mack zon...@gmail.com
---
drivers/usb/gadget/function/f_uac2.c | 29 +++--
1 file changed, 11 insertions(+), 18 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uac2.c
b/drivers/usb/gadget/function/f_uac2.c
index ab4652e..efe8add 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -918,6 +918,7 @@ afunc_bind(struct usb_configuration *cfg, struct
usb_function *fn)
struct snd_uac2_chip *uac2 = agdev-uac2;
struct usb_composite_dev *cdev = cfg-cdev;
struct usb_gadget *gadget = cdev-gadget;
+ struct device *dev = uac2-pdev.dev;
struct uac2_rtd_params *prm;
struct f_uac2_opts *uac2_opts;
struct usb_string *us;
@@ -961,8 +962,7 @@ afunc_bind(struct usb_configuration *cfg, struct
usb_function *fn)
ret = usb_interface_id(cfg, fn);
if (ret 0) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
return ret;
}
std_ac_if_desc.bInterfaceNumber = ret;
@@ -971,8 +971,7 @@ afunc_bind(struct usb_configuration *cfg, struct
usb_function *fn)
ret = usb_interface_id(cfg, fn);
if (ret 0) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
return ret;
}
std_as_out_if0_desc.bInterfaceNumber = ret;
@@ -982,8 +981,7 @@ afunc_bind(struct usb_configuration *cfg, struct
usb_function *fn)
ret = usb_interface_id(cfg, fn);
if (ret 0) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
return ret;
}
std_as_in_if0_desc.bInterfaceNumber = ret;
@@ -993,16 +991,14 @@ afunc_bind(struct usb_configuration *cfg, struct
usb_function *fn)
agdev-out_ep = usb_ep_autoconfig(gadget, fs_epout_desc);
if (!agdev-out_ep) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
goto err;
}
agdev-out_ep-driver_data = agdev;
agdev-in_ep = usb_ep_autoconfig(gadget, fs_epin_desc);
if (!agdev-in_ep) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
goto err;
}
agdev-in_ep-driver_data = agdev;
@@ -1057,6 +1053,7 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
struct audio_dev *agdev = func_to_agdev(fn);
struct snd_uac2_chip *uac2 = agdev-uac2;
struct usb_gadget *gadget = cdev-gadget;
+ struct device *dev = uac2-pdev.dev;
struct usb_request *req;
struct usb_ep *ep;
struct uac2_rtd_params *prm;
@@ -1064,16 +1061,14 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
/* No i/f has more than 2 alt settings */
if (alt 1) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
return -EINVAL;
}
if (intf == agdev-ac_intf) {
/* Control I/f has only 1 AltSetting - 0 */
if (alt) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
return -EINVAL;
}
return 0;
@@ -1090,8 +1085,7 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
config_ep_by_speed(gadget, fn, ep);
agdev-as_in_alt = alt;
} else {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
return -EINVAL;
}
@@ -1120,8 +1114,7 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
}
if (usb_ep_queue(ep, prm-ureq[i].req, GFP_ATOMIC))
- dev_err(uac2-pdev.dev, %s:%d Error!\n,
- __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
}
return 0;
--
2.1.0
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body
[PATCH v5 3/5] usb: gadget: f_uac2: introduce agdev_to_uac2_opts
Add a simple container_of() wrapper to get a struct f_uac2_opts from a
struct struct audio_dev. Use it in two places where it is currently
open-coded.
Signed-off-by: Daniel Mack zon...@gmail.com
---
drivers/usb/gadget/function/f_uac2.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uac2.c
b/drivers/usb/gadget/function/f_uac2.c
index efe8add..9c8831d 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -140,6 +140,12 @@ struct snd_uac2_chip *pdev_to_uac2(struct platform_device
*p)
}
static inline
+struct f_uac2_opts *agdev_to_uac2_opts(struct audio_dev *agdev)
+{
+ return container_of(agdev-func.fi, struct f_uac2_opts, func_inst);
+}
+
+static inline
uint num_channels(uint chanmask)
{
uint num = 0;
@@ -1168,7 +1174,7 @@ in_rq_cur(struct usb_function *fn, const struct
usb_ctrlrequest *cr)
int value = -EOPNOTSUPP;
int p_srate, c_srate;
- opts = container_of(agdev-func.fi, struct f_uac2_opts, func_inst);
+ opts = agdev_to_uac2_opts(agdev);
p_srate = opts-p_srate;
c_srate = opts-c_srate;
@@ -1210,7 +1216,7 @@ in_rq_range(struct usb_function *fn, const struct
usb_ctrlrequest *cr)
int value = -EOPNOTSUPP;
int p_srate, c_srate;
- opts = container_of(agdev-func.fi, struct f_uac2_opts, func_inst);
+ opts = agdev_to_uac2_opts(agdev);
p_srate = opts-p_srate;
c_srate = opts-c_srate;
--
2.1.0
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 5/5] usb: gadget: f_uac2: send reasonably sized packets
The UAC2 function driver currently responds to all packets at all times
with wMaxPacketSize packets. That results in way too fast audio
playback as the function driver (which is in fact supposed to define
the audio stream pace) delivers as fast as it can.
Fix this by sizing each packet correctly with the following steps:
a) Set the packet's size by dividing the nominal data rate by the
playback endpoint's interval.q
b) If there is a residual value from the calculation in a), add
it to a accumulator to keep track of it across packets.
c) If the accumulator has gathered at least the number of bytes
that are needed for one sample frame, increase the packet size.
This way, the packet size calculation will get rid of any kind of
imprecision that would otherwise occur with a simple division over
time.
Signed-off-by: Daniel Mack zon...@gmail.com
---
drivers/usb/gadget/function/f_uac2.c | 68 +---
1 file changed, 64 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uac2.c
b/drivers/usb/gadget/function/f_uac2.c
index 246a778..f42c2ad 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -92,6 +92,10 @@ struct snd_uac2_chip {
struct snd_card *card;
struct snd_pcm *pcm;
+
+ /* timekeeping for the playback endpoint */
+ unsigned int p_interval;
+ unsigned int p_residue;
};
#define BUFF_SIZE_MAX (PAGE_SIZE * 16)
@@ -191,8 +195,43 @@ agdev_iso_complete(struct usb_ep *ep, struct usb_request
*req)
spin_lock_irqsave(prm-lock, flags);
- if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK)
+ if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK) {
+ struct audio_dev *agdev = uac2_to_agdev(uac2);
+ struct f_uac2_opts *opts = agdev_to_uac2_opts(agdev);
+ unsigned int fsz = opts-p_ssize * num_channels(opts-p_chmask);
+ unsigned int rate = opts-p_srate * fsz;
+
+ /*
+* For each IN packet, calculate the minimum packet size by
+* dividing the nominal bytes per second as required by the
+* current audio format by the endpoint's interval.
+*/
+ req-length = min_t(unsigned int,
+ rate / uac2-p_interval, prm-max_psize);
+
+ /*
+* If there is a residual value from the division, add it to
+* the residue accumulator.
+*/
+ uac2-p_residue += rate % uac2-p_interval;
+
+ /*
+* Whenever there are more bytes in the accumulator than we
+* need to add one more sample frame, increase this packet's
+* size and decrease the accumulator.
+*/
+ if (uac2-p_residue / uac2-p_interval = fsz) {
+ req-length += fsz;
+ uac2-p_residue -= fsz * uac2-p_interval;
+ }
+
+ /*
+* req-actual is used as copy length below. We can safely
+* override it here as it is not looked at when the packet
+* is resubmitted on an IN endpoint.
+*/
req-actual = req-length;
+ }
pending = prm-hw_ptr % prm-period_size;
pending += req-actual;
@@ -346,6 +385,7 @@ static int uac2_pcm_open(struct snd_pcm_substream
*substream)
c_srate = opts-c_srate;
p_chmask = opts-p_chmask;
c_chmask = opts-c_chmask;
+ uac2-p_residue = 0;
runtime-hw = uac2_pcm_hardware;
@@ -1077,7 +1117,7 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
struct usb_request *req;
struct usb_ep *ep;
struct uac2_rtd_params *prm;
- int i;
+ int req_len, i;
/* No i/f has more than 2 alt settings */
if (alt 1) {
@@ -1099,11 +1139,31 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
prm = uac2-c_prm;
config_ep_by_speed(gadget, fn, ep);
agdev-as_out_alt = alt;
+ req_len = prm-max_psize;
} else if (intf == agdev-as_in_intf) {
+ struct f_uac2_opts *opts = agdev_to_uac2_opts(agdev);
+ unsigned int rate = opts-p_srate * opts-p_ssize *
+ num_channels(opts-p_chmask);
+ struct usb_endpoint_descriptor *ep_desc;
+ unsigned int factor;
+
ep = agdev-in_ep;
prm = uac2-p_prm;
config_ep_by_speed(gadget, fn, ep);
agdev-as_in_alt = alt;
+
+ /* pre-calculate the playback endpoint's interval */
+ if (gadget-speed == USB_SPEED_FULL) {
+ ep_desc = fs_epin_desc;
+ factor = 1000;
+ } else
[PATCH v5 4/5] usb: gadget: f_uac2: handle partial dma area wrap
With packet sizes other than 512, payloads in the packets may wrap
around the ALSA dma buffer partially, which leads to memory corruption
and audible clicks and pops in the audio stream at the moment, because
there is no boundary check before the memcpy().
In preparation to an implementation for smaller and dynamically sized
packets, we have to address such cases, and copy the payload in two
steps conditionally.
The 'src' and 'dst' approach doesn't work here anymore, as different
behavior is necessary in playback and capture cases. Thus, this patch
open-codes the routine now.
Signed-off-by: Daniel Mack zon...@gmail.com
---
drivers/usb/gadget/function/f_uac2.c | 32 +++-
1 file changed, 23 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uac2.c
b/drivers/usb/gadget/function/f_uac2.c
index 9c8831d..246a778 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -163,8 +163,8 @@ agdev_iso_complete(struct usb_ep *ep, struct usb_request
*req)
{
unsigned pending;
unsigned long flags;
+ unsigned int hw_ptr;
bool update_alsa = false;
- unsigned char *src, *dst;
int status = req-status;
struct uac2_req *ur = req-context;
struct snd_pcm_substream *substream;
@@ -191,26 +191,40 @@ agdev_iso_complete(struct usb_ep *ep, struct usb_request
*req)
spin_lock_irqsave(prm-lock, flags);
- if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK) {
- src = prm-dma_area + prm-hw_ptr;
+ if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK)
req-actual = req-length;
- dst = req-buf;
- } else {
- dst = prm-dma_area + prm-hw_ptr;
- src = req-buf;
- }
pending = prm-hw_ptr % prm-period_size;
pending += req-actual;
if (pending = prm-period_size)
update_alsa = true;
+ hw_ptr = prm-hw_ptr;
prm-hw_ptr = (prm-hw_ptr + req-actual) % prm-dma_bytes;
spin_unlock_irqrestore(prm-lock, flags);
/* Pack USB load in ALSA ring buffer */
- memcpy(dst, src, req-actual);
+ pending = prm-dma_bytes - hw_ptr;
+
+ if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK) {
+ if (unlikely(pending req-actual)) {
+ memcpy(req-buf, prm-dma_area + hw_ptr, pending);
+ memcpy(req-buf + pending, prm-dma_area,
+ req-actual - pending);
+ } else {
+ memcpy(req-buf, prm-dma_area + hw_ptr, req-actual);
+ }
+ } else {
+ if (unlikely(pending req-actual)) {
+ memcpy(prm-dma_area + hw_ptr, req-buf, pending);
+ memcpy(prm-dma_area, req-buf + pending,
+ req-actual - pending);
+ } else {
+ memcpy(prm-dma_area + hw_ptr, req-buf, req-actual);
+ }
+ }
+
exit:
if (usb_ep_queue(ep, req, GFP_ATOMIC))
dev_err(uac2-pdev.dev, %d Error!\n, __LINE__);
--
2.1.0
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 0/5] usb: gadget: f_uac2: cleanups and capture timing
On 08/27/2014 10:38 AM, Jassi Brar wrote: On Wed, Aug 27, 2014 at 1:15 PM, Daniel Mack zon...@gmail.com wrote: Daniel Mack (5): usb: gadget: f_uac2: restructure some code in afunc_set_alt() usb: gadget: f_uac2: add short-hand for 'dev' usb: gadget: f_uac2: introduce agdev_to_uac2_opts usb: gadget: f_uac2: handle partial dma area wrap usb: gadget: f_uac2: send reasonably sized packets Acked-by: Jassi Brar jassisinghb...@gmail.com Thanks for your continuous review and your feedback! BTW, against which tree is this patchset based? Felipe's usb-next. Best regards, Daniel -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 5/5] usb: gadget: f_uac2: send reasonably sized packets
On 08/27/2014 04:18 PM, Alan Stern wrote:
On Wed, 27 Aug 2014, Daniel Mack wrote:
+/*
+ * Whenever there are more bytes in the accumulator than we
+ * need to add one more sample frame, increase this packet's
+ * size and decrease the accumulator.
+ */
+if (uac2-p_residue / uac2-p_interval = fsz) {
+req-length += fsz;
+uac2-p_residue -= fsz * uac2-p_interval;
+}
One thing I didn't mention about the algorithm I posted earlier: The
individual packet calculations don't contain any multiplications or
divisions.
The code here is in the hottest part of the driver, so you might want
to optimize it a little. For instance, several of the operations could
be precomputed and stored for later use.
Yeah, well. I did tests with that, and at least the difference in load
is not measureable on BBB. But you're right of course, and it even leads
to nicer code. Will send that as v6.
Thanks,
Daniel
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 0/5] usb: gadget: f_uac2: cleanups and capture timing
Hi, this is v6 of the f_uac2 timing fixup series. Changes from v5: * Pre-calculate some more variables so we have to do their computation at runtime for each frame Changes from v4: * Fix buffer setup in afunc_set_alt() Changes from v3: * add another patch (3/5) to introduce agdev_to_uac2_opts() which is also needed in 5/5 * patch 5/5 only: move from a pre-calculated sequence of packet lengths to an accumulator that sums up the residual values from the packet size calculation and adds an extra sample frame now and then when the accumulator has gathered enough bytes. Changes from v2: * swap path 3 and 4, so that the ALSA buffer wrap around fix comes in first. It's not actually a bug fix for the current code, but more a preparation to allow for smaller packets. * use the p_ssize, p_chmask and p_srate for the length calculations * prepare a sequence of packet sizes to send, and alternate over them when sending the the IN packets. The actual commit log and the comments yield some more details on that. Changes from v1: * drop UAC_EP_CS_ATTR_FILL_MAX approach and rather size the packets correctly * add a patch to fix buffer wrap problems in the ALSA buffer logic, which wasn't needed before The first two patches are just cleanups. Thanks to Alan Stern and Jassi Brar for the feedback! Daniel Subject: [PATCH v6 0/5] *** SUBJECT HERE *** *** BLURB HERE *** Daniel Mack (5): usb: gadget: f_uac2: restructure some code in afunc_set_alt() usb: gadget: f_uac2: add short-hand for 'dev' usb: gadget: f_uac2: introduce agdev_to_uac2_opts usb: gadget: f_uac2: handle partial dma area wrap usb: gadget: f_uac2: send reasonably sized packets drivers/usb/gadget/function/f_uac2.c | 167 +-- 1 file changed, 118 insertions(+), 49 deletions(-) -- 2.1.0 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 5/5] usb: gadget: f_uac2: send reasonably sized packets
The UAC2 function driver currently responds to all packets at all times
with wMaxPacketSize packets. That results in way too fast audio
playback as the function driver (which is in fact supposed to define
the audio stream pace) delivers as fast as it can.
Fix this by sizing each packet correctly with the following steps:
a) Set the packet's size by dividing the nominal data rate by the
playback endpoint's interval.
b) If there is a residual value from the calculation in a), add
it to a accumulator to keep track of it across packets.
c) If the accumulator has gathered at least the number of bytes
that are needed for one sample frame, increase the packet size.
This way, the packet size calculation will get rid of any kind of
imprecision that would otherwise occur with a simple division over
time.
Some of the variables that are needed while processing each packet
are pre-computed for performance reasons.
Signed-off-by: Daniel Mack zon...@gmail.com
---
drivers/usb/gadget/function/f_uac2.c | 69 +---
1 file changed, 65 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uac2.c
b/drivers/usb/gadget/function/f_uac2.c
index 246a778..a5a27a5 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -92,6 +92,15 @@ struct snd_uac2_chip {
struct snd_card *card;
struct snd_pcm *pcm;
+
+ /* timekeeping for the playback endpoint */
+ unsigned int p_interval;
+ unsigned int p_residue;
+
+ /* pre-calculated values for playback iso completion */
+ unsigned int p_pktsize;
+ unsigned int p_pktsize_residue;
+ unsigned int p_framesize;
};
#define BUFF_SIZE_MAX (PAGE_SIZE * 16)
@@ -191,8 +200,29 @@ agdev_iso_complete(struct usb_ep *ep, struct usb_request
*req)
spin_lock_irqsave(prm-lock, flags);
- if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK)
+ if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK) {
+ /*
+* For each IN packet, take the quotient of the current data
+* rate and the endpoint's interval as the base packet size.
+* If there is a residue from this division, add it to the
+* residue accumulator.
+*/
+ req-length = uac2-p_pktsize;
+ uac2-p_residue += uac2-p_pktsize_residue;
+
+ /*
+* Whenever there are more bytes in the accumulator than we
+* need to add one more sample frame, increase this packet's
+* size and decrease the accumulator.
+*/
+ if (uac2-p_residue / uac2-p_interval = uac2-p_framesize) {
+ req-length += uac2-p_framesize;
+ uac2-p_residue -= uac2-p_framesize *
+ uac2-p_interval;
+ }
+
req-actual = req-length;
+ }
pending = prm-hw_ptr % prm-period_size;
pending += req-actual;
@@ -346,6 +376,7 @@ static int uac2_pcm_open(struct snd_pcm_substream
*substream)
c_srate = opts-c_srate;
p_chmask = opts-p_chmask;
c_chmask = opts-c_chmask;
+ uac2-p_residue = 0;
runtime-hw = uac2_pcm_hardware;
@@ -1077,7 +1108,7 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
struct usb_request *req;
struct usb_ep *ep;
struct uac2_rtd_params *prm;
- int i;
+ int req_len, i;
/* No i/f has more than 2 alt settings */
if (alt 1) {
@@ -1099,11 +1130,41 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
prm = uac2-c_prm;
config_ep_by_speed(gadget, fn, ep);
agdev-as_out_alt = alt;
+ req_len = prm-max_psize;
} else if (intf == agdev-as_in_intf) {
+ struct f_uac2_opts *opts = agdev_to_uac2_opts(agdev);
+ unsigned int factor, rate;
+ struct usb_endpoint_descriptor *ep_desc;
+
ep = agdev-in_ep;
prm = uac2-p_prm;
config_ep_by_speed(gadget, fn, ep);
agdev-as_in_alt = alt;
+
+ /* pre-calculate the playback endpoint's interval */
+ if (gadget-speed == USB_SPEED_FULL) {
+ ep_desc = fs_epin_desc;
+ factor = 1000;
+ } else {
+ ep_desc = hs_epin_desc;
+ factor = 125;
+ }
+
+ /* pre-compute some values for iso_complete() */
+ uac2-p_framesize = opts-p_ssize *
+ num_channels(opts-p_chmask);
+ rate = opts-p_srate * uac2-p_framesize;
+ uac2-p_interval = (1 (ep_desc-bInterval - 1)) * factor;
+ uac2-p_pktsize = min_t(unsigned int, rate / uac2
[PATCH v6 2/5] usb: gadget: f_uac2: add short-hand for 'dev'
In afunc_bind() and afunc_set_alt(), uac2-pdev.dev are used multiple
times. Adding a short-hand for them makes lines shorter so we can
remove some line wraps.
No functional change.
Signed-off-by: Daniel Mack zon...@gmail.com
---
drivers/usb/gadget/function/f_uac2.c | 29 +++--
1 file changed, 11 insertions(+), 18 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uac2.c
b/drivers/usb/gadget/function/f_uac2.c
index ab4652e..efe8add 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -918,6 +918,7 @@ afunc_bind(struct usb_configuration *cfg, struct
usb_function *fn)
struct snd_uac2_chip *uac2 = agdev-uac2;
struct usb_composite_dev *cdev = cfg-cdev;
struct usb_gadget *gadget = cdev-gadget;
+ struct device *dev = uac2-pdev.dev;
struct uac2_rtd_params *prm;
struct f_uac2_opts *uac2_opts;
struct usb_string *us;
@@ -961,8 +962,7 @@ afunc_bind(struct usb_configuration *cfg, struct
usb_function *fn)
ret = usb_interface_id(cfg, fn);
if (ret 0) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
return ret;
}
std_ac_if_desc.bInterfaceNumber = ret;
@@ -971,8 +971,7 @@ afunc_bind(struct usb_configuration *cfg, struct
usb_function *fn)
ret = usb_interface_id(cfg, fn);
if (ret 0) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
return ret;
}
std_as_out_if0_desc.bInterfaceNumber = ret;
@@ -982,8 +981,7 @@ afunc_bind(struct usb_configuration *cfg, struct
usb_function *fn)
ret = usb_interface_id(cfg, fn);
if (ret 0) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
return ret;
}
std_as_in_if0_desc.bInterfaceNumber = ret;
@@ -993,16 +991,14 @@ afunc_bind(struct usb_configuration *cfg, struct
usb_function *fn)
agdev-out_ep = usb_ep_autoconfig(gadget, fs_epout_desc);
if (!agdev-out_ep) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
goto err;
}
agdev-out_ep-driver_data = agdev;
agdev-in_ep = usb_ep_autoconfig(gadget, fs_epin_desc);
if (!agdev-in_ep) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
goto err;
}
agdev-in_ep-driver_data = agdev;
@@ -1057,6 +1053,7 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
struct audio_dev *agdev = func_to_agdev(fn);
struct snd_uac2_chip *uac2 = agdev-uac2;
struct usb_gadget *gadget = cdev-gadget;
+ struct device *dev = uac2-pdev.dev;
struct usb_request *req;
struct usb_ep *ep;
struct uac2_rtd_params *prm;
@@ -1064,16 +1061,14 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
/* No i/f has more than 2 alt settings */
if (alt 1) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
return -EINVAL;
}
if (intf == agdev-ac_intf) {
/* Control I/f has only 1 AltSetting - 0 */
if (alt) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
return -EINVAL;
}
return 0;
@@ -1090,8 +1085,7 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
config_ep_by_speed(gadget, fn, ep);
agdev-as_in_alt = alt;
} else {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
return -EINVAL;
}
@@ -1120,8 +1114,7 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
}
if (usb_ep_queue(ep, prm-ureq[i].req, GFP_ATOMIC))
- dev_err(uac2-pdev.dev, %s:%d Error!\n,
- __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
}
return 0;
--
2.1.0
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body
[PATCH v6 4/5] usb: gadget: f_uac2: handle partial dma area wrap
With packet sizes other than 512, payloads in the packets may wrap
around the ALSA dma buffer partially, which leads to memory corruption
and audible clicks and pops in the audio stream at the moment, because
there is no boundary check before the memcpy().
In preparation to an implementation for smaller and dynamically sized
packets, we have to address such cases, and copy the payload in two
steps conditionally.
The 'src' and 'dst' approach doesn't work here anymore, as different
behavior is necessary in playback and capture cases. Thus, this patch
open-codes the routine now.
Signed-off-by: Daniel Mack zon...@gmail.com
---
drivers/usb/gadget/function/f_uac2.c | 32 +++-
1 file changed, 23 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uac2.c
b/drivers/usb/gadget/function/f_uac2.c
index 9c8831d..246a778 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -163,8 +163,8 @@ agdev_iso_complete(struct usb_ep *ep, struct usb_request
*req)
{
unsigned pending;
unsigned long flags;
+ unsigned int hw_ptr;
bool update_alsa = false;
- unsigned char *src, *dst;
int status = req-status;
struct uac2_req *ur = req-context;
struct snd_pcm_substream *substream;
@@ -191,26 +191,40 @@ agdev_iso_complete(struct usb_ep *ep, struct usb_request
*req)
spin_lock_irqsave(prm-lock, flags);
- if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK) {
- src = prm-dma_area + prm-hw_ptr;
+ if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK)
req-actual = req-length;
- dst = req-buf;
- } else {
- dst = prm-dma_area + prm-hw_ptr;
- src = req-buf;
- }
pending = prm-hw_ptr % prm-period_size;
pending += req-actual;
if (pending = prm-period_size)
update_alsa = true;
+ hw_ptr = prm-hw_ptr;
prm-hw_ptr = (prm-hw_ptr + req-actual) % prm-dma_bytes;
spin_unlock_irqrestore(prm-lock, flags);
/* Pack USB load in ALSA ring buffer */
- memcpy(dst, src, req-actual);
+ pending = prm-dma_bytes - hw_ptr;
+
+ if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK) {
+ if (unlikely(pending req-actual)) {
+ memcpy(req-buf, prm-dma_area + hw_ptr, pending);
+ memcpy(req-buf + pending, prm-dma_area,
+ req-actual - pending);
+ } else {
+ memcpy(req-buf, prm-dma_area + hw_ptr, req-actual);
+ }
+ } else {
+ if (unlikely(pending req-actual)) {
+ memcpy(prm-dma_area + hw_ptr, req-buf, pending);
+ memcpy(prm-dma_area, req-buf + pending,
+ req-actual - pending);
+ } else {
+ memcpy(prm-dma_area + hw_ptr, req-buf, req-actual);
+ }
+ }
+
exit:
if (usb_ep_queue(ep, req, GFP_ATOMIC))
dev_err(uac2-pdev.dev, %d Error!\n, __LINE__);
--
2.1.0
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 3/5] usb: gadget: f_uac2: introduce agdev_to_uac2_opts
Add a simple container_of() wrapper to get a struct f_uac2_opts from a
struct struct audio_dev. Use it in two places where it is currently
open-coded.
Signed-off-by: Daniel Mack zon...@gmail.com
---
drivers/usb/gadget/function/f_uac2.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uac2.c
b/drivers/usb/gadget/function/f_uac2.c
index efe8add..9c8831d 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -140,6 +140,12 @@ struct snd_uac2_chip *pdev_to_uac2(struct platform_device
*p)
}
static inline
+struct f_uac2_opts *agdev_to_uac2_opts(struct audio_dev *agdev)
+{
+ return container_of(agdev-func.fi, struct f_uac2_opts, func_inst);
+}
+
+static inline
uint num_channels(uint chanmask)
{
uint num = 0;
@@ -1168,7 +1174,7 @@ in_rq_cur(struct usb_function *fn, const struct
usb_ctrlrequest *cr)
int value = -EOPNOTSUPP;
int p_srate, c_srate;
- opts = container_of(agdev-func.fi, struct f_uac2_opts, func_inst);
+ opts = agdev_to_uac2_opts(agdev);
p_srate = opts-p_srate;
c_srate = opts-c_srate;
@@ -1210,7 +1216,7 @@ in_rq_range(struct usb_function *fn, const struct
usb_ctrlrequest *cr)
int value = -EOPNOTSUPP;
int p_srate, c_srate;
- opts = container_of(agdev-func.fi, struct f_uac2_opts, func_inst);
+ opts = agdev_to_uac2_opts(agdev);
p_srate = opts-p_srate;
c_srate = opts-c_srate;
--
2.1.0
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 1/5] usb: gadget: f_uac2: restructure some code in afunc_set_alt()
Restructure some code to make it easier to read.
While at it, return -ENOMEM instead of -EINVAL if
usb_ep_alloc_request() fails, and omit the logging in such cases
(the mm core will complain loud enough).
Signed-off-by: Daniel Mack zon...@gmail.com
---
drivers/usb/gadget/function/f_uac2.c | 39 +++-
1 file changed, 16 insertions(+), 23 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uac2.c
b/drivers/usb/gadget/function/f_uac2.c
index 0d65e7c..ab4652e 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -1104,31 +1104,24 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
usb_ep_enable(ep);
for (i = 0; i USB_XFERS; i++) {
- if (prm-ureq[i].req) {
- if (usb_ep_queue(ep, prm-ureq[i].req, GFP_ATOMIC))
- dev_err(uac2-pdev.dev, %d Error!\n,
- __LINE__);
- continue;
- }
-
- req = usb_ep_alloc_request(ep, GFP_ATOMIC);
- if (req == NULL) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
- return -EINVAL;
+ if (!prm-ureq[i].req) {
+ req = usb_ep_alloc_request(ep, GFP_ATOMIC);
+ if (req == NULL)
+ return -ENOMEM;
+
+ prm-ureq[i].req = req;
+ prm-ureq[i].pp = prm;
+
+ req-zero = 0;
+ req-context = prm-ureq[i];
+ req-length = prm-max_psize;
+ req-complete = agdev_iso_complete;
+ req-buf = prm-rbuf + i * req-length;
}
- prm-ureq[i].req = req;
- prm-ureq[i].pp = prm;
-
- req-zero = 0;
- req-context = prm-ureq[i];
- req-length = prm-max_psize;
- req-complete = agdev_iso_complete;
- req-buf = prm-rbuf + i * req-length;
-
- if (usb_ep_queue(ep, req, GFP_ATOMIC))
- dev_err(uac2-pdev.dev, %d Error!\n, __LINE__);
+ if (usb_ep_queue(ep, prm-ureq[i].req, GFP_ATOMIC))
+ dev_err(uac2-pdev.dev, %s:%d Error!\n,
+ __func__, __LINE__);
}
return 0;
--
2.1.0
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] usb: gadget: f_uac2: send reasonably sized packets
On 08/25/2014 07:22 PM, Jassi Brar wrote:
On Mon, Aug 25, 2014 at 9:30 PM, Daniel Mack zon...@gmail.com wrote:
@@ -187,7 +189,7 @@ agdev_iso_complete(struct usb_ep *ep, struct usb_request
*req)
if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK) {
src = prm-dma_area + prm-hw_ptr;
- req-actual = req-length;
+ req-length = req-actual = uac2-c_pktsize;
This doesn't seem right.
We asked req-length to be transmitted by the udc which shouldn't
return until that is done. So at this point setting 'length' doesn't
mean much. The original assignment to 'actual' is only because we want
to ignore any issue that caused the udc to transmit fewer bytes (we
drop that data).
Looking at this again, setting req-length is in fact the right thing to
do. We want to prepare a new packet of a specific length, so we have to
let the udc driver know how much data is contains before we call
usb_ep_queue() again.
Note that this is for SNDRV_PCM_STREAM_PLAYBACK, so for the IN endpoint
of the gadget. Reading your description again makes me believe you
actually wanted to do that for the *capture* side, because this is were
could possibly 'drop data', right?
Daniel
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 3/4] usb: gadget: f_uac2: handle partial dma area wrap
With packet sizes other than 512, payloads in the packets may wrap
around the ALSA dma buffer partially, which leads to memory corruption
and audible clicks and pops in the audio stream at the moment, because
there is no boundary check before the memcpy().
In preparation to an implementation for smaller and dynamically sized
packets, we have to address such cases, and copy the payload in two
steps conditionally.
The 'src' and 'dst' approach doesn't work here anymore, as different
behavior is necessary in playback and capture cases. Thus, this patch
open-codes the routine now.
Signed-off-by: Daniel Mack zon...@gmail.com
---
drivers/usb/gadget/function/f_uac2.c | 32 +++-
1 file changed, 23 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uac2.c
b/drivers/usb/gadget/function/f_uac2.c
index efe8add..a18f147 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -157,8 +157,8 @@ agdev_iso_complete(struct usb_ep *ep, struct usb_request
*req)
{
unsigned pending;
unsigned long flags;
+ unsigned int hw_ptr;
bool update_alsa = false;
- unsigned char *src, *dst;
int status = req-status;
struct uac2_req *ur = req-context;
struct snd_pcm_substream *substream;
@@ -185,26 +185,40 @@ agdev_iso_complete(struct usb_ep *ep, struct usb_request
*req)
spin_lock_irqsave(prm-lock, flags);
- if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK) {
- src = prm-dma_area + prm-hw_ptr;
+ if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK)
req-actual = req-length;
- dst = req-buf;
- } else {
- dst = prm-dma_area + prm-hw_ptr;
- src = req-buf;
- }
pending = prm-hw_ptr % prm-period_size;
pending += req-actual;
if (pending = prm-period_size)
update_alsa = true;
+ hw_ptr = prm-hw_ptr;
prm-hw_ptr = (prm-hw_ptr + req-actual) % prm-dma_bytes;
spin_unlock_irqrestore(prm-lock, flags);
/* Pack USB load in ALSA ring buffer */
- memcpy(dst, src, req-actual);
+ pending = prm-dma_bytes - hw_ptr;
+
+ if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK) {
+ if (unlikely(pending req-actual)) {
+ memcpy(req-buf, prm-dma_area + hw_ptr, pending);
+ memcpy(req-buf + pending, prm-dma_area,
+ req-actual - pending);
+ } else {
+ memcpy(req-buf, prm-dma_area + hw_ptr, req-actual);
+ }
+ } else {
+ if (unlikely(pending req-actual)) {
+ memcpy(prm-dma_area + hw_ptr, req-buf, pending);
+ memcpy(prm-dma_area, req-buf + pending,
+ req-actual - pending);
+ } else {
+ memcpy(prm-dma_area + hw_ptr, req-buf, req-actual);
+ }
+ }
+
exit:
if (usb_ep_queue(ep, req, GFP_ATOMIC))
dev_err(uac2-pdev.dev, %d Error!\n, __LINE__);
--
2.1.0
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 0/4] usb: gadget: f_uac2: cleanups and capture timing
Hi, this is v3 of the f_uac2 timing fixup series. Changes from v2: * swap path 3 and 4, so that the ALSA buffer wrap around fix comes in first. It's not actually a bug fix for the current code, but more a preparation to allow for smaller packets. * use the p_ssize, p_chmask and p_srate for the length calculations * prepare a sequence of packet sizes to send, and alternate over them when sending the the IN packets. The actual commit log and the comments yield some more details on that. Changes from v1: * drop UAC_EP_CS_ATTR_FILL_MAX approach and rather size the packets correctly * add a patch to fix buffer wrap problems in the ALSA buffer logic, which wasn't needed before The first two patches are just cleanups. Sebastian, could you give these patches a try? They seem to work well on a BBB setup here. Thanks, Daniel Daniel Mack (4): usb: gadget: f_uac2: restructure some code in afunc_set_alt() usb: gadget: f_uac2: add short-hand for 'dev' usb: gadget: f_uac2: handle partial dma area wrap usb: gadget: f_uac2: send reasonably sized packets drivers/usb/gadget/function/f_uac2.c | 158 --- 1 file changed, 111 insertions(+), 47 deletions(-) -- 2.1.0 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 4/4] usb: gadget: f_uac2: send reasonably sized packets
The UAC2 function driver currently responds to all packets at all times
with wMaxPacketSize packets. That results in way too fast audio
playback as the function driver (which is in fact supposed to define
the audio stream pace) delivers as fast as it can.
Fix this by pre-calculating the size of each packet to meet the
requested sample rate and format. As a simple division won't allow
for exact data rates, prepare a sequence of packet lengths, and
alternate over them when sending the IN packets.
For example, with 44100 Hz and stereo, 16-bit samples, the nominal
data rate is 176400 bytes/s. With a simple division by the number
of available frames (1000), we would be off by 400 bytes, or 100
samples per second. By preparing a sequence of lengths, we can make
each 10th packet accomodate one more frame, which results in 400 more
bytes/s.
Signed-off-by: Daniel Mack zon...@gmail.com
---
drivers/usb/gadget/function/f_uac2.c | 68 ++--
1 file changed, 66 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uac2.c
b/drivers/usb/gadget/function/f_uac2.c
index a18f147..40c501a 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -92,6 +92,10 @@ struct snd_uac2_chip {
struct snd_card *card;
struct snd_pcm *pcm;
+
+ u16 p_pktsize_seq[10];
+ unsigned int p_pktsize_seq_len;
+ unsigned int cur_p_pktsize;
};
#define BUFF_SIZE_MAX (PAGE_SIZE * 16)
@@ -185,8 +189,11 @@ agdev_iso_complete(struct usb_ep *ep, struct usb_request
*req)
spin_lock_irqsave(prm-lock, flags);
- if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK)
- req-actual = req-length;
+ if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK) {
+ /* alternate over the package size sequence */
+ req-length = uac2-p_pktsize_seq[uac2-cur_p_pktsize++];
+ uac2-cur_p_pktsize %= uac2-p_pktsize_seq_len;
+ }
pending = prm-hw_ptr % prm-period_size;
pending += req-actual;
@@ -340,6 +347,7 @@ static int uac2_pcm_open(struct snd_pcm_substream
*substream)
c_srate = opts-c_srate;
p_chmask = opts-p_chmask;
c_chmask = opts-c_chmask;
+ uac2-cur_p_pktsize = 0;
runtime-hw = uac2_pcm_hardware;
@@ -1060,6 +1068,61 @@ err:
return -EINVAL;
}
+static void
+afunc_set_p_pktsize(struct usb_gadget *gadget, struct audio_dev *agdev)
+{
+ unsigned i, residue, rate, factor, interval, framesize, pktsize, len;
+ struct snd_uac2_chip *uac2 = agdev-uac2;
+ struct usb_endpoint_descriptor *ep_desc;
+ struct f_uac2_opts *opts =
+ container_of(agdev-func.fi, struct f_uac2_opts, func_inst);
+
+ if (gadget-speed == USB_SPEED_FULL) {
+ ep_desc = fs_epin_desc;
+ factor = 1000;
+ } else {
+ ep_desc = hs_epin_desc;
+ factor = 125;
+ }
+
+ /*
+* Simply dividing the desired data rate by the number of available
+* packets per seconds give rounding errors. Hence, we prepare a
+* sequence of packet sizes that will be alternated over when sending
+* the packets to the IN endpoint.
+*/
+ framesize = opts-p_ssize * num_channels(opts-p_chmask);
+ interval = (1 (ep_desc-bInterval - 1)) * factor;
+ rate = opts-p_srate * framesize;
+
+ pktsize = min_t(unsigned, rate / interval, ep_desc-wMaxPacketSize);
+
+ /*
+* If there's a residue from the modulo operation, calculate the index
+* of the first packet that could be increased by one sample frame.
+* This will be our sequence length, and the last member carries one
+* frame more than the others. Hence, the shorter the sequence is, the
+* more bytes will be transferred.
+*/
+ residue = (rate % interval) / framesize;
+ if (residue)
+ len = min_t(unsigned, interval / residue,
+ ARRAY_SIZE(uac2-p_pktsize_seq));
+ else
+ len = 1;
+
+ /* Set all lengths in the sequence to the divided value ... */
+ for (i = 0; i len; i++)
+ uac2-p_pktsize_seq[i] = pktsize;
+
+ /* ... and add another frame to the last sequence member */
+ if (len 1)
+ uac2-p_pktsize_seq[len - 1] += framesize;
+
+ uac2-p_pktsize_seq_len = len;
+ uac2-cur_p_pktsize = 0;
+}
+
static int
afunc_set_alt(struct usb_function *fn, unsigned intf, unsigned alt)
{
@@ -1098,6 +1161,7 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
prm = uac2-p_prm;
config_ep_by_speed(gadget, fn, ep);
agdev-as_in_alt = alt;
+ afunc_set_p_pktsize(gadget, agdev);
} else {
dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
return -EINVAL;
--
2.1.0
[PATCH v3 2/4] usb: gadget: f_uac2: add short-hand for 'dev'
In afunc_bind() and afunc_set_alt(), uac2-pdev.dev are used multiple
times. Adding a short-hand for them makes lines shorter so we can
remove some line wraps.
No functional change.
Signed-off-by: Daniel Mack zon...@gmail.com
---
drivers/usb/gadget/function/f_uac2.c | 29 +++--
1 file changed, 11 insertions(+), 18 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uac2.c
b/drivers/usb/gadget/function/f_uac2.c
index ab4652e..efe8add 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -918,6 +918,7 @@ afunc_bind(struct usb_configuration *cfg, struct
usb_function *fn)
struct snd_uac2_chip *uac2 = agdev-uac2;
struct usb_composite_dev *cdev = cfg-cdev;
struct usb_gadget *gadget = cdev-gadget;
+ struct device *dev = uac2-pdev.dev;
struct uac2_rtd_params *prm;
struct f_uac2_opts *uac2_opts;
struct usb_string *us;
@@ -961,8 +962,7 @@ afunc_bind(struct usb_configuration *cfg, struct
usb_function *fn)
ret = usb_interface_id(cfg, fn);
if (ret 0) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
return ret;
}
std_ac_if_desc.bInterfaceNumber = ret;
@@ -971,8 +971,7 @@ afunc_bind(struct usb_configuration *cfg, struct
usb_function *fn)
ret = usb_interface_id(cfg, fn);
if (ret 0) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
return ret;
}
std_as_out_if0_desc.bInterfaceNumber = ret;
@@ -982,8 +981,7 @@ afunc_bind(struct usb_configuration *cfg, struct
usb_function *fn)
ret = usb_interface_id(cfg, fn);
if (ret 0) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
return ret;
}
std_as_in_if0_desc.bInterfaceNumber = ret;
@@ -993,16 +991,14 @@ afunc_bind(struct usb_configuration *cfg, struct
usb_function *fn)
agdev-out_ep = usb_ep_autoconfig(gadget, fs_epout_desc);
if (!agdev-out_ep) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
goto err;
}
agdev-out_ep-driver_data = agdev;
agdev-in_ep = usb_ep_autoconfig(gadget, fs_epin_desc);
if (!agdev-in_ep) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
goto err;
}
agdev-in_ep-driver_data = agdev;
@@ -1057,6 +1053,7 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
struct audio_dev *agdev = func_to_agdev(fn);
struct snd_uac2_chip *uac2 = agdev-uac2;
struct usb_gadget *gadget = cdev-gadget;
+ struct device *dev = uac2-pdev.dev;
struct usb_request *req;
struct usb_ep *ep;
struct uac2_rtd_params *prm;
@@ -1064,16 +1061,14 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
/* No i/f has more than 2 alt settings */
if (alt 1) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
return -EINVAL;
}
if (intf == agdev-ac_intf) {
/* Control I/f has only 1 AltSetting - 0 */
if (alt) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
return -EINVAL;
}
return 0;
@@ -1090,8 +1085,7 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
config_ep_by_speed(gadget, fn, ep);
agdev-as_in_alt = alt;
} else {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
return -EINVAL;
}
@@ -1120,8 +1114,7 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
}
if (usb_ep_queue(ep, prm-ureq[i].req, GFP_ATOMIC))
- dev_err(uac2-pdev.dev, %s:%d Error!\n,
- __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
}
return 0;
--
2.1.0
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body
[PATCH v3 1/4] usb: gadget: f_uac2: restructure some code in afunc_set_alt()
Restructure some code to make it easier to read.
While at it, return -ENOMEM instead of -EINVAL if
usb_ep_alloc_request() fails, and omit the logging in such cases
(the mm core will complain loud enough).
Signed-off-by: Daniel Mack zon...@gmail.com
---
drivers/usb/gadget/function/f_uac2.c | 39 +++-
1 file changed, 16 insertions(+), 23 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uac2.c
b/drivers/usb/gadget/function/f_uac2.c
index 0d65e7c..ab4652e 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -1104,31 +1104,24 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
usb_ep_enable(ep);
for (i = 0; i USB_XFERS; i++) {
- if (prm-ureq[i].req) {
- if (usb_ep_queue(ep, prm-ureq[i].req, GFP_ATOMIC))
- dev_err(uac2-pdev.dev, %d Error!\n,
- __LINE__);
- continue;
- }
-
- req = usb_ep_alloc_request(ep, GFP_ATOMIC);
- if (req == NULL) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
- return -EINVAL;
+ if (!prm-ureq[i].req) {
+ req = usb_ep_alloc_request(ep, GFP_ATOMIC);
+ if (req == NULL)
+ return -ENOMEM;
+
+ prm-ureq[i].req = req;
+ prm-ureq[i].pp = prm;
+
+ req-zero = 0;
+ req-context = prm-ureq[i];
+ req-length = prm-max_psize;
+ req-complete = agdev_iso_complete;
+ req-buf = prm-rbuf + i * req-length;
}
- prm-ureq[i].req = req;
- prm-ureq[i].pp = prm;
-
- req-zero = 0;
- req-context = prm-ureq[i];
- req-length = prm-max_psize;
- req-complete = agdev_iso_complete;
- req-buf = prm-rbuf + i * req-length;
-
- if (usb_ep_queue(ep, req, GFP_ATOMIC))
- dev_err(uac2-pdev.dev, %d Error!\n, __LINE__);
+ if (usb_ep_queue(ep, prm-ureq[i].req, GFP_ATOMIC))
+ dev_err(uac2-pdev.dev, %s:%d Error!\n,
+ __func__, __LINE__);
}
return 0;
--
2.1.0
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] usb: gadget: f_uac2: send reasonably sized packets
On 08/26/2014 05:08 PM, Alan Stern wrote: The normal approach is to perform a simple runtime calculation (no pre-allocated pattern). It's not complex. Let S be the number of samples per second at the nominal transfer rate (for example, S = 44100). Let R be the number of packets per second (1000 because you transfer one packet every millisecond). [...] Yes, I thought about that too. The pre-allocated approach is not much code either, and it also gives accurate values for all common audio sample rates, but maybe the runtime calculation nicer and easier to read in the end. I'll give that a try later. Thanks, Daniel -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 0/5] usb: gadget: f_uac2: cleanups and capture timing
Hi, this is v4 of the f_uac2 timing fixup series. Changes from v3: * add another patch (3/5) to introduce agdev_to_uac2_opts() which is also needed in 5/5 * patch 5/5 only: move from a pre-calculated sequence of packet lengths to an accumulator that sums up the residual values from the packet size calculation and adds an extra sample frame now and then when the accumulator has gathered enough bytes. Changes from v2: * swap path 3 and 4, so that the ALSA buffer wrap around fix comes in first. It's not actually a bug fix for the current code, but more a preparation to allow for smaller packets. * use the p_ssize, p_chmask and p_srate for the length calculations * prepare a sequence of packet sizes to send, and alternate over them when sending the the IN packets. The actual commit log and the comments yield some more details on that. Changes from v1: * drop UAC_EP_CS_ATTR_FILL_MAX approach and rather size the packets correctly * add a patch to fix buffer wrap problems in the ALSA buffer logic, which wasn't needed before The first two patches are just cleanups. Thanks to Alan Stern and Jassi Brar for the feedback! Daniel Daniel Mack (5): usb: gadget: f_uac2: restructure some code in afunc_set_alt() usb: gadget: f_uac2: add short-hand for 'dev' usb: gadget: f_uac2: introduce agdev_to_uac2_opts usb: gadget: f_uac2: handle partial dma area wrap usb: gadget: f_uac2: send reasonably sized packets drivers/usb/gadget/function/f_uac2.c | 157 --- 1 file changed, 109 insertions(+), 48 deletions(-) -- 2.1.0 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 1/5] usb: gadget: f_uac2: restructure some code in afunc_set_alt()
Restructure some code to make it easier to read.
While at it, return -ENOMEM instead of -EINVAL if
usb_ep_alloc_request() fails, and omit the logging in such cases
(the mm core will complain loud enough).
Signed-off-by: Daniel Mack zon...@gmail.com
---
drivers/usb/gadget/function/f_uac2.c | 39 +++-
1 file changed, 16 insertions(+), 23 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uac2.c
b/drivers/usb/gadget/function/f_uac2.c
index 0d65e7c..ab4652e 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -1104,31 +1104,24 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
usb_ep_enable(ep);
for (i = 0; i USB_XFERS; i++) {
- if (prm-ureq[i].req) {
- if (usb_ep_queue(ep, prm-ureq[i].req, GFP_ATOMIC))
- dev_err(uac2-pdev.dev, %d Error!\n,
- __LINE__);
- continue;
- }
-
- req = usb_ep_alloc_request(ep, GFP_ATOMIC);
- if (req == NULL) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
- return -EINVAL;
+ if (!prm-ureq[i].req) {
+ req = usb_ep_alloc_request(ep, GFP_ATOMIC);
+ if (req == NULL)
+ return -ENOMEM;
+
+ prm-ureq[i].req = req;
+ prm-ureq[i].pp = prm;
+
+ req-zero = 0;
+ req-context = prm-ureq[i];
+ req-length = prm-max_psize;
+ req-complete = agdev_iso_complete;
+ req-buf = prm-rbuf + i * req-length;
}
- prm-ureq[i].req = req;
- prm-ureq[i].pp = prm;
-
- req-zero = 0;
- req-context = prm-ureq[i];
- req-length = prm-max_psize;
- req-complete = agdev_iso_complete;
- req-buf = prm-rbuf + i * req-length;
-
- if (usb_ep_queue(ep, req, GFP_ATOMIC))
- dev_err(uac2-pdev.dev, %d Error!\n, __LINE__);
+ if (usb_ep_queue(ep, prm-ureq[i].req, GFP_ATOMIC))
+ dev_err(uac2-pdev.dev, %s:%d Error!\n,
+ __func__, __LINE__);
}
return 0;
--
2.1.0
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 2/5] usb: gadget: f_uac2: add short-hand for 'dev'
In afunc_bind() and afunc_set_alt(), uac2-pdev.dev are used multiple
times. Adding a short-hand for them makes lines shorter so we can
remove some line wraps.
No functional change.
Signed-off-by: Daniel Mack zon...@gmail.com
---
drivers/usb/gadget/function/f_uac2.c | 29 +++--
1 file changed, 11 insertions(+), 18 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uac2.c
b/drivers/usb/gadget/function/f_uac2.c
index ab4652e..efe8add 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -918,6 +918,7 @@ afunc_bind(struct usb_configuration *cfg, struct
usb_function *fn)
struct snd_uac2_chip *uac2 = agdev-uac2;
struct usb_composite_dev *cdev = cfg-cdev;
struct usb_gadget *gadget = cdev-gadget;
+ struct device *dev = uac2-pdev.dev;
struct uac2_rtd_params *prm;
struct f_uac2_opts *uac2_opts;
struct usb_string *us;
@@ -961,8 +962,7 @@ afunc_bind(struct usb_configuration *cfg, struct
usb_function *fn)
ret = usb_interface_id(cfg, fn);
if (ret 0) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
return ret;
}
std_ac_if_desc.bInterfaceNumber = ret;
@@ -971,8 +971,7 @@ afunc_bind(struct usb_configuration *cfg, struct
usb_function *fn)
ret = usb_interface_id(cfg, fn);
if (ret 0) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
return ret;
}
std_as_out_if0_desc.bInterfaceNumber = ret;
@@ -982,8 +981,7 @@ afunc_bind(struct usb_configuration *cfg, struct
usb_function *fn)
ret = usb_interface_id(cfg, fn);
if (ret 0) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
return ret;
}
std_as_in_if0_desc.bInterfaceNumber = ret;
@@ -993,16 +991,14 @@ afunc_bind(struct usb_configuration *cfg, struct
usb_function *fn)
agdev-out_ep = usb_ep_autoconfig(gadget, fs_epout_desc);
if (!agdev-out_ep) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
goto err;
}
agdev-out_ep-driver_data = agdev;
agdev-in_ep = usb_ep_autoconfig(gadget, fs_epin_desc);
if (!agdev-in_ep) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
goto err;
}
agdev-in_ep-driver_data = agdev;
@@ -1057,6 +1053,7 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
struct audio_dev *agdev = func_to_agdev(fn);
struct snd_uac2_chip *uac2 = agdev-uac2;
struct usb_gadget *gadget = cdev-gadget;
+ struct device *dev = uac2-pdev.dev;
struct usb_request *req;
struct usb_ep *ep;
struct uac2_rtd_params *prm;
@@ -1064,16 +1061,14 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
/* No i/f has more than 2 alt settings */
if (alt 1) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
return -EINVAL;
}
if (intf == agdev-ac_intf) {
/* Control I/f has only 1 AltSetting - 0 */
if (alt) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
return -EINVAL;
}
return 0;
@@ -1090,8 +1085,7 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
config_ep_by_speed(gadget, fn, ep);
agdev-as_in_alt = alt;
} else {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
return -EINVAL;
}
@@ -1120,8 +1114,7 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
}
if (usb_ep_queue(ep, prm-ureq[i].req, GFP_ATOMIC))
- dev_err(uac2-pdev.dev, %s:%d Error!\n,
- __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
}
return 0;
--
2.1.0
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body
[PATCH v4 3/5] usb: gadget: f_uac2: introduce agdev_to_uac2_opts
Add a simple container_of() wrapper to get a struct f_uac2_opts from a
struct struct audio_dev. Use it in two places where it is currently
open-coded.
Signed-off-by: Daniel Mack zon...@gmail.com
---
drivers/usb/gadget/function/f_uac2.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uac2.c
b/drivers/usb/gadget/function/f_uac2.c
index efe8add..9c8831d 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -140,6 +140,12 @@ struct snd_uac2_chip *pdev_to_uac2(struct platform_device
*p)
}
static inline
+struct f_uac2_opts *agdev_to_uac2_opts(struct audio_dev *agdev)
+{
+ return container_of(agdev-func.fi, struct f_uac2_opts, func_inst);
+}
+
+static inline
uint num_channels(uint chanmask)
{
uint num = 0;
@@ -1168,7 +1174,7 @@ in_rq_cur(struct usb_function *fn, const struct
usb_ctrlrequest *cr)
int value = -EOPNOTSUPP;
int p_srate, c_srate;
- opts = container_of(agdev-func.fi, struct f_uac2_opts, func_inst);
+ opts = agdev_to_uac2_opts(agdev);
p_srate = opts-p_srate;
c_srate = opts-c_srate;
@@ -1210,7 +1216,7 @@ in_rq_range(struct usb_function *fn, const struct
usb_ctrlrequest *cr)
int value = -EOPNOTSUPP;
int p_srate, c_srate;
- opts = container_of(agdev-func.fi, struct f_uac2_opts, func_inst);
+ opts = agdev_to_uac2_opts(agdev);
p_srate = opts-p_srate;
c_srate = opts-c_srate;
--
2.1.0
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 5/5] usb: gadget: f_uac2: send reasonably sized packets
The UAC2 function driver currently responds to all packets at all times
with wMaxPacketSize packets. That results in way too fast audio
playback as the function driver (which is in fact supposed to define
the audio stream pace) delivers as fast as it can.
Fix this by sizing each packet correctly with the following steps:
a) Set the packet's size by dividing the nominal data rate by the
playback endpoint's interval.q
b) If there is a residual value from the calculation in a), add
it to a accumulator to keep track of it across packets.
c) If the accumulator has gathered at least the number of bytes
that are needed for one sample frame, increase the packet size.
This way, the packet size calculation will get rid of any kind of
imprecision that would otherwise occur with a simple division over
time.
Signed-off-by: Daniel Mack zon...@gmail.com
---
drivers/usb/gadget/function/f_uac2.c | 66 ++--
1 file changed, 63 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uac2.c
b/drivers/usb/gadget/function/f_uac2.c
index 246a778..06c364a 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -92,6 +92,10 @@ struct snd_uac2_chip {
struct snd_card *card;
struct snd_pcm *pcm;
+
+ /* timekeeping for the playback endpoint */
+ unsigned int p_interval;
+ unsigned int p_residue;
};
#define BUFF_SIZE_MAX (PAGE_SIZE * 16)
@@ -191,8 +195,43 @@ agdev_iso_complete(struct usb_ep *ep, struct usb_request
*req)
spin_lock_irqsave(prm-lock, flags);
- if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK)
+ if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK) {
+ struct audio_dev *agdev = uac2_to_agdev(uac2);
+ struct f_uac2_opts *opts = agdev_to_uac2_opts(agdev);
+ unsigned int fsz = opts-p_ssize * num_channels(opts-p_chmask);
+ unsigned int rate = opts-p_srate * fsz;
+
+ /*
+* For each IN packet, calculate the minimum packet size by
+* dividing the nominal bytes per second as required by the
+* current audio format by the endpoint's interval.
+*/
+ req-length = min_t(unsigned int,
+ rate / uac2-p_interval, prm-max_psize);
+
+ /*
+* If there is a residual value from the division, add it to
+* the residue accumulator.
+*/
+ uac2-p_residue += rate % uac2-p_interval;
+
+ /*
+* Whenever there are more bytes in the accumulator than we
+* need to add one more sample frame, increase this packet's
+* size and decrease the accumulator.
+*/
+ if (uac2-p_residue / uac2-p_interval = fsz) {
+ req-length += fsz;
+ uac2-p_residue -= fsz * uac2-p_interval;
+ }
+
+ /*
+* req-actual is used as copy length below. We can safely
+* override it here as it is not looked at when the packet
+* is resubmitted on an IN endpoint.
+*/
req-actual = req-length;
+ }
pending = prm-hw_ptr % prm-period_size;
pending += req-actual;
@@ -346,6 +385,7 @@ static int uac2_pcm_open(struct snd_pcm_substream
*substream)
c_srate = opts-c_srate;
p_chmask = opts-p_chmask;
c_chmask = opts-c_chmask;
+ uac2-p_residue = 0;
runtime-hw = uac2_pcm_hardware;
@@ -1077,7 +1117,7 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
struct usb_request *req;
struct usb_ep *ep;
struct uac2_rtd_params *prm;
- int i;
+ int req_len, i;
/* No i/f has more than 2 alt settings */
if (alt 1) {
@@ -1099,11 +1139,31 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
prm = uac2-c_prm;
config_ep_by_speed(gadget, fn, ep);
agdev-as_out_alt = alt;
+ req_len = prm-max_psize;
} else if (intf == agdev-as_in_intf) {
+ struct f_uac2_opts *opts = agdev_to_uac2_opts(agdev);
+ unsigned int rate = opts-p_srate * opts-p_ssize *
+ num_channels(opts-p_chmask);
+ struct usb_endpoint_descriptor *ep_desc;
+ unsigned int factor;
+
ep = agdev-in_ep;
prm = uac2-p_prm;
config_ep_by_speed(gadget, fn, ep);
agdev-as_in_alt = alt;
+
+ /* pre-calculate the playback endpoint's interval */
+ if (gadget-speed == USB_SPEED_FULL) {
+ ep_desc = fs_epin_desc;
+ factor = 1000;
+ } else
[PATCH v4 4/5] usb: gadget: f_uac2: handle partial dma area wrap
With packet sizes other than 512, payloads in the packets may wrap
around the ALSA dma buffer partially, which leads to memory corruption
and audible clicks and pops in the audio stream at the moment, because
there is no boundary check before the memcpy().
In preparation to an implementation for smaller and dynamically sized
packets, we have to address such cases, and copy the payload in two
steps conditionally.
The 'src' and 'dst' approach doesn't work here anymore, as different
behavior is necessary in playback and capture cases. Thus, this patch
open-codes the routine now.
Signed-off-by: Daniel Mack zon...@gmail.com
---
drivers/usb/gadget/function/f_uac2.c | 32 +++-
1 file changed, 23 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uac2.c
b/drivers/usb/gadget/function/f_uac2.c
index 9c8831d..246a778 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -163,8 +163,8 @@ agdev_iso_complete(struct usb_ep *ep, struct usb_request
*req)
{
unsigned pending;
unsigned long flags;
+ unsigned int hw_ptr;
bool update_alsa = false;
- unsigned char *src, *dst;
int status = req-status;
struct uac2_req *ur = req-context;
struct snd_pcm_substream *substream;
@@ -191,26 +191,40 @@ agdev_iso_complete(struct usb_ep *ep, struct usb_request
*req)
spin_lock_irqsave(prm-lock, flags);
- if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK) {
- src = prm-dma_area + prm-hw_ptr;
+ if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK)
req-actual = req-length;
- dst = req-buf;
- } else {
- dst = prm-dma_area + prm-hw_ptr;
- src = req-buf;
- }
pending = prm-hw_ptr % prm-period_size;
pending += req-actual;
if (pending = prm-period_size)
update_alsa = true;
+ hw_ptr = prm-hw_ptr;
prm-hw_ptr = (prm-hw_ptr + req-actual) % prm-dma_bytes;
spin_unlock_irqrestore(prm-lock, flags);
/* Pack USB load in ALSA ring buffer */
- memcpy(dst, src, req-actual);
+ pending = prm-dma_bytes - hw_ptr;
+
+ if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK) {
+ if (unlikely(pending req-actual)) {
+ memcpy(req-buf, prm-dma_area + hw_ptr, pending);
+ memcpy(req-buf + pending, prm-dma_area,
+ req-actual - pending);
+ } else {
+ memcpy(req-buf, prm-dma_area + hw_ptr, req-actual);
+ }
+ } else {
+ if (unlikely(pending req-actual)) {
+ memcpy(prm-dma_area + hw_ptr, req-buf, pending);
+ memcpy(prm-dma_area, req-buf + pending,
+ req-actual - pending);
+ } else {
+ memcpy(prm-dma_area + hw_ptr, req-buf, req-actual);
+ }
+ }
+
exit:
if (usb_ep_queue(ep, req, GFP_ATOMIC))
dev_err(uac2-pdev.dev, %d Error!\n, __LINE__);
--
2.1.0
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] usb: gadget: f_uac2: cleanups and capture timing
Hi Clemens, On 08/25/2014 09:15 AM, Clemens Ladisch wrote: Daniel Mack wrote: a) Linux snd-usb-audio currently pre-calculates the estimated packet sizes to expect from a USB device, and will only receive packets that have the expected size or are smaller. snd-usb-audio allows packets to be 25 % larger. Yes, but packets can't be larger than *that*. This can be worked around by setting the UAC_EP_CS_ATTR_FILL_MAX in the UAC2 endpoint descriptor The spec says about this flag (4.10.1.2): | when receiving data from an IN endpoint, the Host software must be | prepared to receive wMaxPacketSize bytes and discard any superfluous | zero bytes in the packet. | Note: This bit can only be used for Type II formatted data. The data |stream must contain enough information (in a header) to |separate the actual data from the padded zero bytes. Right, I've read this too, and we're not using Type II, so we don't have header information to tell us the real payload. The idea was to just use an 0 or 512 bytes approach. send 0-byte packets from agdev_iso_complete() if the time passed since the last completed buffer does not allow for another one to be sent. Audio Formats, 2.1: | To indicate a temporary stop in the isochronous data stream ..., an | in-band Transfer Delimiter needs to be defined. This specification | considers two situations to be a Transfer Delimiter. The first is | a zero-length data packet and the second is the absence of an | isochronous transfer 2.3.1.1: | The goal must be to keep the instantaneous number of audio slots per | virtual frame, ni as close as possible to the average number of audio | slots per virtual frame, nav. [...] If the sampling rate is a constant, | the allowable variation on ni is limited to one audio slot, that is, | ∆ni = 1. This implies that all virtual frame packets must either | contain INT(nav) audio slots (small VFP) or INT(nav) + 1 (large VFP) | audio slots. This results in very stable timing behavior in my tests. But it increases jitter, and might not work with any other driver. You're right, that's also my concern. f_uac(2) *must* implement some mechanism to control the clock, i.e., the packet sizes. (And this is not part of the standard ALSA API.) The easiest is probably really to just calculate correct packet sizes and stick to them. After all, the actual clock is really arbitrary, we just have to pick something that is in the range of the sample rate. I'll cook up an alternative patch and do some tests with Sebastian off-list. Thanks, Daniel -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] usb: gadget: f_uac2: cleanups and capture timing
On 08/25/2014 11:23 AM, Jassi Brar wrote:
On Mon, Aug 25, 2014 at 2:14 PM, Daniel Mack dan...@zonque.org wrote:
The easiest is probably really to just calculate correct packet sizes
and stick to them. After all, the actual clock is really arbitrary, we
just have to pick something that is in the range of the sample rate.
I'll cook up an alternative patch and do some tests with Sebastian off-list.
How about configuring bInterval and wMaxPacketSize to get the desired rate?
For ex, 48KHz/2/S16, we need 192bytes/millisec. So we set bInterval=1
(or 4 for HS) and wMaxPacketSize=192 for that configuration. For
44.1KHz/2/S16 we need 176.4bytes/millisec, so we set
wMaxPacketSize=178 and send packets of length in {176, 176, 176,
176,178} pattern.
Yes, something like that. But you can't modify wMaxPacketSize in
accordance to the sample rate and format, but just send short packets.
I'm currently testing a patch which does that.
Daniel
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] usb: gadget: f_uac2: cleanups and capture timing
On 08/25/2014 11:30 AM, Jassi Brar wrote:
On Mon, Aug 25, 2014 at 2:57 PM, Daniel Mack dan...@zonque.org wrote:
On 08/25/2014 11:23 AM, Jassi Brar wrote:
On Mon, Aug 25, 2014 at 2:14 PM, Daniel Mack dan...@zonque.org wrote:
The easiest is probably really to just calculate correct packet sizes
and stick to them. After all, the actual clock is really arbitrary, we
just have to pick something that is in the range of the sample rate.
I'll cook up an alternative patch and do some tests with Sebastian
off-list.
How about configuring bInterval and wMaxPacketSize to get the desired rate?
For ex, 48KHz/2/S16, we need 192bytes/millisec. So we set bInterval=1
(or 4 for HS) and wMaxPacketSize=192 for that configuration. For
44.1KHz/2/S16 we need 176.4bytes/millisec, so we set
wMaxPacketSize=178 and send packets of length in {176, 176, 176,
176,178} pattern.
Yes, something like that. But you can't modify wMaxPacketSize in
accordance to the sample rate and format, but just send short packets.
We can't change rate once the f_uac2 module is loaded. So
wMaxPacketSize changes only across module loads.
Yes, but we shouldn't rely on wMaxPacketSize but really just send
packets of the right size. This is what other USB audio devices do as well.
And btw - we could also change the logic of f_uac2 so the sample rate
can be changed at runtime. The only constaint is that the counterpart
device on the gadget side must not be active when this happens. Whatever
part of the system comes up first (USB or ALSA) defines the sample rate
and the format. But I'll save that for later :)
Daniel
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/4] usb: gadget: f_uac2: send reasonably sized packets
The UAC2 function driver currently responds to all packets at all times
with wMaxPacketSize packets. That results in way too fast audio
playback as the function driver (which is in fact supposed to define
the audio stream pace) delivers as fast as it can.
Fix this by pre-calculating the size of each packet to meet the
requested sample rate and format. This won't be 100% accurate, but
that's acceptable. Audio applications have to adopt to the stream's
rate anyway. The important thing here is to make f_uac2 operate at
least rougly in the range of speed that is expected by the host.
Signed-off-by: Daniel Mack zon...@gmail.com
---
drivers/usb/gadget/function/f_uac2.c | 27 ++-
1 file changed, 26 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/function/f_uac2.c
b/drivers/usb/gadget/function/f_uac2.c
index efe8add..610a2f1 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -92,6 +92,8 @@ struct snd_uac2_chip {
struct snd_card *card;
struct snd_pcm *pcm;
+
+ unsigned int c_pktsize;
};
#define BUFF_SIZE_MAX (PAGE_SIZE * 16)
@@ -187,7 +189,7 @@ agdev_iso_complete(struct usb_ep *ep, struct usb_request
*req)
if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK) {
src = prm-dma_area + prm-hw_ptr;
- req-actual = req-length;
+ req-length = req-actual = uac2-c_pktsize;
dst = req-buf;
} else {
dst = prm-dma_area + prm-hw_ptr;
@@ -1046,6 +1048,28 @@ err:
return -EINVAL;
}
+static void
+afunc_set_c_pktsize(struct usb_gadget *gadget, struct audio_dev *agdev)
+{
+ struct usb_endpoint_descriptor *ep_desc;
+ unsigned int rate, factor, interval;
+ struct f_uac2_opts *opts =
+ container_of(agdev-func.fi, struct f_uac2_opts, func_inst);
+
+ if (gadget-speed == USB_SPEED_FULL) {
+ ep_desc = fs_epin_desc;
+ factor = 1000;
+ } else {
+ ep_desc = hs_epin_desc;
+ factor = 125;
+ }
+
+ interval = (1 (ep_desc-bInterval - 1)) * factor;
+ rate = opts-c_srate * opts-c_ssize * num_channels(opts-c_chmask);
+ agdev-uac2.c_pktsize =
+ min_t(unsigned int, rate / interval, ep_desc-wMaxPacketSize);
+}
+
static int
afunc_set_alt(struct usb_function *fn, unsigned intf, unsigned alt)
{
@@ -1084,6 +1108,7 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
prm = uac2-p_prm;
config_ep_by_speed(gadget, fn, ep);
agdev-as_in_alt = alt;
+ afunc_set_c_pktsize(gadget, agdev);
} else {
dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
return -EINVAL;
--
2.1.0
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/4] usb: gadget: f_uac2: add short-hand for 'dev'
In afunc_bind() and afunc_set_alt(), uac2-pdev.dev are used multiple
times. Adding a short-hand for them makes lines shorter so we can
remove some line wraps.
No functional change.
Signed-off-by: Daniel Mack zon...@gmail.com
---
drivers/usb/gadget/function/f_uac2.c | 29 +++--
1 file changed, 11 insertions(+), 18 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uac2.c
b/drivers/usb/gadget/function/f_uac2.c
index ab4652e..efe8add 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -918,6 +918,7 @@ afunc_bind(struct usb_configuration *cfg, struct
usb_function *fn)
struct snd_uac2_chip *uac2 = agdev-uac2;
struct usb_composite_dev *cdev = cfg-cdev;
struct usb_gadget *gadget = cdev-gadget;
+ struct device *dev = uac2-pdev.dev;
struct uac2_rtd_params *prm;
struct f_uac2_opts *uac2_opts;
struct usb_string *us;
@@ -961,8 +962,7 @@ afunc_bind(struct usb_configuration *cfg, struct
usb_function *fn)
ret = usb_interface_id(cfg, fn);
if (ret 0) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
return ret;
}
std_ac_if_desc.bInterfaceNumber = ret;
@@ -971,8 +971,7 @@ afunc_bind(struct usb_configuration *cfg, struct
usb_function *fn)
ret = usb_interface_id(cfg, fn);
if (ret 0) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
return ret;
}
std_as_out_if0_desc.bInterfaceNumber = ret;
@@ -982,8 +981,7 @@ afunc_bind(struct usb_configuration *cfg, struct
usb_function *fn)
ret = usb_interface_id(cfg, fn);
if (ret 0) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
return ret;
}
std_as_in_if0_desc.bInterfaceNumber = ret;
@@ -993,16 +991,14 @@ afunc_bind(struct usb_configuration *cfg, struct
usb_function *fn)
agdev-out_ep = usb_ep_autoconfig(gadget, fs_epout_desc);
if (!agdev-out_ep) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
goto err;
}
agdev-out_ep-driver_data = agdev;
agdev-in_ep = usb_ep_autoconfig(gadget, fs_epin_desc);
if (!agdev-in_ep) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
goto err;
}
agdev-in_ep-driver_data = agdev;
@@ -1057,6 +1053,7 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
struct audio_dev *agdev = func_to_agdev(fn);
struct snd_uac2_chip *uac2 = agdev-uac2;
struct usb_gadget *gadget = cdev-gadget;
+ struct device *dev = uac2-pdev.dev;
struct usb_request *req;
struct usb_ep *ep;
struct uac2_rtd_params *prm;
@@ -1064,16 +1061,14 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
/* No i/f has more than 2 alt settings */
if (alt 1) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
return -EINVAL;
}
if (intf == agdev-ac_intf) {
/* Control I/f has only 1 AltSetting - 0 */
if (alt) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
return -EINVAL;
}
return 0;
@@ -1090,8 +1085,7 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
config_ep_by_speed(gadget, fn, ep);
agdev-as_in_alt = alt;
} else {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
return -EINVAL;
}
@@ -1120,8 +1114,7 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
}
if (usb_ep_queue(ep, prm-ureq[i].req, GFP_ATOMIC))
- dev_err(uac2-pdev.dev, %s:%d Error!\n,
- __func__, __LINE__);
+ dev_err(dev, %s:%d Error!\n, __func__, __LINE__);
}
return 0;
--
2.1.0
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body
[PATCH v2 1/4] usb: gadget: f_uac2: restructure some code in afunc_set_alt()
Restructure some code to make it easier to read.
While at it, return -ENOMEM instead of -EINVAL if
usb_ep_alloc_request() fails, and omit the logging in such cases
(the mm core will complain loud enough).
Signed-off-by: Daniel Mack zon...@gmail.com
---
drivers/usb/gadget/function/f_uac2.c | 39 +++-
1 file changed, 16 insertions(+), 23 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uac2.c
b/drivers/usb/gadget/function/f_uac2.c
index 0d65e7c..ab4652e 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -1104,31 +1104,24 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
unsigned alt)
usb_ep_enable(ep);
for (i = 0; i USB_XFERS; i++) {
- if (prm-ureq[i].req) {
- if (usb_ep_queue(ep, prm-ureq[i].req, GFP_ATOMIC))
- dev_err(uac2-pdev.dev, %d Error!\n,
- __LINE__);
- continue;
- }
-
- req = usb_ep_alloc_request(ep, GFP_ATOMIC);
- if (req == NULL) {
- dev_err(uac2-pdev.dev,
- %s:%d Error!\n, __func__, __LINE__);
- return -EINVAL;
+ if (!prm-ureq[i].req) {
+ req = usb_ep_alloc_request(ep, GFP_ATOMIC);
+ if (req == NULL)
+ return -ENOMEM;
+
+ prm-ureq[i].req = req;
+ prm-ureq[i].pp = prm;
+
+ req-zero = 0;
+ req-context = prm-ureq[i];
+ req-length = prm-max_psize;
+ req-complete = agdev_iso_complete;
+ req-buf = prm-rbuf + i * req-length;
}
- prm-ureq[i].req = req;
- prm-ureq[i].pp = prm;
-
- req-zero = 0;
- req-context = prm-ureq[i];
- req-length = prm-max_psize;
- req-complete = agdev_iso_complete;
- req-buf = prm-rbuf + i * req-length;
-
- if (usb_ep_queue(ep, req, GFP_ATOMIC))
- dev_err(uac2-pdev.dev, %d Error!\n, __LINE__);
+ if (usb_ep_queue(ep, prm-ureq[i].req, GFP_ATOMIC))
+ dev_err(uac2-pdev.dev, %s:%d Error!\n,
+ __func__, __LINE__);
}
return 0;
--
2.1.0
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 4/4] usb: gadget: f_uac2: handle partial dma area wrap
With packet sizes other than 512, payloads in the packets may wrap
around the ALSA dma buffer partially, which leads to memory corruption
and audible clicks and pops in the audio stream at the moment, because
there is no boundary check before the memcpy().
Now that we have smaller and dynamically sized packets, we have to
address such cases, and copy the payload in two steps conditionally.
The 'src' and 'dst' approach doesn't work here anymore, as different
behavior is necessary in playback and capture cases. Thus, this patch
open-codes the routine now.
Signed-off-by: Daniel Mack zon...@gmail.com
---
drivers/usb/gadget/function/f_uac2.c | 32 +++-
1 file changed, 23 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uac2.c
b/drivers/usb/gadget/function/f_uac2.c
index 610a2f1..06446cc 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -159,8 +159,8 @@ agdev_iso_complete(struct usb_ep *ep, struct usb_request
*req)
{
unsigned pending;
unsigned long flags;
+ unsigned int hw_ptr;
bool update_alsa = false;
- unsigned char *src, *dst;
int status = req-status;
struct uac2_req *ur = req-context;
struct snd_pcm_substream *substream;
@@ -187,26 +187,40 @@ agdev_iso_complete(struct usb_ep *ep, struct usb_request
*req)
spin_lock_irqsave(prm-lock, flags);
- if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK) {
- src = prm-dma_area + prm-hw_ptr;
+ if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK)
req-length = req-actual = uac2-c_pktsize;
- dst = req-buf;
- } else {
- dst = prm-dma_area + prm-hw_ptr;
- src = req-buf;
- }
pending = prm-hw_ptr % prm-period_size;
pending += req-actual;
if (pending = prm-period_size)
update_alsa = true;
+ hw_ptr = prm-hw_ptr;
prm-hw_ptr = (prm-hw_ptr + req-actual) % prm-dma_bytes;
spin_unlock_irqrestore(prm-lock, flags);
/* Pack USB load in ALSA ring buffer */
- memcpy(dst, src, req-actual);
+ pending = prm-dma_bytes - hw_ptr;
+
+ if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK) {
+ if (unlikely(pending req-actual)) {
+ memcpy(req-buf, prm-dma_area + hw_ptr, pending);
+ memcpy(req-buf + pending, prm-dma_area,
+ req-actual - pending);
+ } else {
+ memcpy(req-buf, prm-dma_area + hw_ptr, req-actual);
+ }
+ } else {
+ if (unlikely(pending req-actual)) {
+ memcpy(prm-dma_area + hw_ptr, req-buf, pending);
+ memcpy(prm-dma_area, req-buf + pending,
+ req-actual - pending);
+ } else {
+ memcpy(prm-dma_area + hw_ptr, req-buf, req-actual);
+ }
+ }
+
exit:
if (usb_ep_queue(ep, req, GFP_ATOMIC))
dev_err(uac2-pdev.dev, %d Error!\n, __LINE__);
--
2.1.0
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/4] usb: gadget: f_uac2: cleanups and capture timing
Hi, this is v2 of the f_uac2 timing fixup series. Changes from v1: * drop UAC_EP_CS_ATTR_FILL_MAX approach and rather size the packets correctly * add a patch to fix buffer wrap problems in the ALSA buffer logic, which wasn't needed before The first two patches are just cleanups. Sebastian, could you give these patches a try? They seem to work well on a BBB setup here. Thanks, Daniel Daniel Mack (4): usb: gadget: f_uac2: restructure some code in afunc_set_alt() usb: gadget: f_uac2: add short-hand for 'dev' usb: gadget: f_uac2: send reasonably sized packets usb: gadget: f_uac2: handle partial dma area wrap drivers/usb/gadget/function/f_uac2.c | 123 +-- 1 file changed, 74 insertions(+), 49 deletions(-) -- 2.1.0 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] usb: gadget: f_uac2: send reasonably sized packets
Hi,
On 08/25/2014 07:22 PM, Jassi Brar wrote:
On Mon, Aug 25, 2014 at 9:30 PM, Daniel Mack zon...@gmail.com wrote:
The UAC2 function driver currently responds to all packets at all times
with wMaxPacketSize packets. That results in way too fast audio
playback as the function driver (which is in fact supposed to define
the audio stream pace) delivers as fast as it can.
Fix this by pre-calculating the size of each packet to meet the
requested sample rate and format. This won't be 100% accurate, but
that's acceptable.
For rates like 44100 we will always hear clicks because we can not
transfer 176400bytes by packets of equal size over duration of 1
second.
How do you test to hear those clicks? If you do arecord | aplay on the
host, you will have underruns or overruns at some point, because the
internal sound interface of the host runs at a different speed than the
gadget. This, however, also happens when you use any other USB sound
card, and is hence it not surprising.
It doesn't really matter if we transfer 176000 or 176400 bytes per
second, measured by the host's time base. After all, the internal sound
card may also be off by some percentage, depending on how it is built.
We shouldn't be too far off though, as we currently are.
The inaccuracy here is due to the way we program, and not due
to system/bus load.
Sure, but rates across devices will never match, so it doesn't matter
really. Two clocks on two devices will always drift, even if they're
totally accurate by their own means. You have the same situation the
other way around on the playback endpoint: the host plays at whatever
speed it considers to be 176400 bytes/sec, which will never be exactly
176400 bytes/s measured by the gadget's clock, because there is no
feedback endpoint. Audio applications have to be aware of that, and if
they need to synchronize two devices with their own clock each, they
have to implement some sort of resampler.
Have you tried the approach I suggested - {4x176, 1x178} pattern
packets, and does that not work? Please let me know if I am
overlooking something. Otherwise let us do the best we can (If you
want me to give that a try, I can in a day or two).
That would only be necessary if you want the gadget's playback device to
run absolutely in sync with its system clock. And there's no need for
that IMO.
@@ -187,7 +189,7 @@ agdev_iso_complete(struct usb_ep *ep, struct usb_request
*req)
if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK) {
src = prm-dma_area + prm-hw_ptr;
- req-actual = req-length;
+ req-length = req-actual = uac2-c_pktsize;
This doesn't seem right.
We asked req-length to be transmitted by the udc which shouldn't
return until that is done. So at this point setting 'length' doesn't
mean much.
That's right. I had it fixed already. Seems I staged the wrong version.
Will fix in v3, thanks!
which should render the patch-4/4 needless, I hope because there is
nowhere 512 in the code and neither did I assume any such fixed value.
The maxsize variables are initialized to the endpoint's wMaxPacketSize,
which is 512. So your audio packets will go in slices of 512, and so
they'll always fit nicely into the dma buffer, which has 64k.
We simply alloc 2 usb requests of wMaxPacketSize each and copy data
to/from the ALSA ring buffer. For you the wMaxPacketSize might be 512,
but the code should work for any value.
Exactly, but with 65536 bytes in the DMA buffer, and 176 bytes in each
packet, you will have an uneven wrap around around each 372th packet, so
we need to address these cases.
Best regards,
Daniel
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] usb: gadget: f_uac2: send reasonably sized packets
Hi, On 08/25/2014 07:43 PM, Jassi Brar wrote: On Mon, Aug 25, 2014 at 10:52 PM, Jassi Brar jassisinghb...@gmail.com wrote: I believe you want to do the following in afunc_set_alt(). - req-length = prm-max_psize; + req-length = uac2-c_pktsize; Sorry I intended... - prm-max_psize = hs_epin_desc.wMaxPacketSize; + prm-max_psize = agdev-uac2.c_pktsize; Yes, that would be another way, but as we might have sample rates and formats that can be configured at runtime, I opted for another variable and leave max_psize = wMaxPacketSize. That should work equally well but it's more flexible in the future, right? Also USB-IN is capture for host, but in f_uac2 it is playback. So you may want to do - rate = opts-c_srate * opts-c_ssize * num_channels(opts-c_chmask); - rate = opts-p_srate * opts-p_ssize * num_channels(opts-p_chmask); Ah, right. Will also fix in v3. BTW, why not do the same for USB-OUT as well? it shouldn't hurt. Not sure if I'm following, but on the OUT side (capture on the gadget), the model is entirely different. We don't have to estimate the packet sizes, and we're not really interested whether the data rate matches our system clock. The host will start sending in whatever it believes to be the correct rate. With other sound interfaces, it will normally be notified on a regular base if it should speed up or slow down. But as we don't have a feedback endpoint in f_uac2, the host will keep sending at its original data rate, and we have to sink away whatever we get, and feed it to the virtual ALSA device. As I've described in my previous mail, this is intended for audio streaming. The virtual capture interface on the gadget will be in sync with the host's clock, not with its own system clock. If we wanted to change that, we'd need to implement a feedback endpoint, but I don't currently see any need for that. I'll fix the two things you pointed out, and resend v3. Probably tomorrow. Best regards, and thanks for your feedback, Daniel -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] usb: gadget: f_uac2: send reasonably sized packets
On 08/25/2014 09:00 PM, Jassi Brar wrote: On Mon, Aug 25, 2014 at 11:40 PM, Daniel Mack dan...@zonque.org wrote: Sure, but rates across devices will never match, so it doesn't matter really. Two clocks on two devices will always drift, even if they're totally accurate by their own means. You have the same situation the other way around on the playback endpoint: the host plays at whatever speed it considers to be 176400 bytes/sec, which will never be exactly 176400 bytes/s measured by the gadget's clock, because there is no feedback endpoint. Audio applications have to be aware of that, and if they need to synchronize two devices with their own clock each, they have to implement some sort of resampler. A high-end device, that consumes exactly 176400 bytes per second, on host is piped data captured from f_uac2. However we write the code so that f_uac2 can send only 176000 bytes every second. Theoretically ruing out 'perfection' unsettles me. We can do better and is not much trouble. Alright, you're right. I'll cook something up to alternate the sizes in order to get closer. Will be part of v3. Exactly, but with 65536 bytes in the DMA buffer, and 176 bytes in each packet, you will have an uneven wrap around around each 372th packet, so we need to address these cases. I see, thanks. That is a bug fix then, and probably should have been patch-3/4 instead. It hasn't been a problem since, but only after the packet size change. But I can swap them around, no problem :) Thanks, Daniel -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What is the command line commands to use UAC2 at USB client side?
Hi Jassi, On 08/19/2014 11:52 AM, Jassi Brar wrote: Its been quite some time now, but I think we designed the uac2 to rely on USB's ISO packets' rate control to send and receive audio data at announced sampling rate. I'm still thinking how that setup should have ever been possible with snd-usb-audio on the host side. snd-usb-audio prepares its capture USB ISO frames with a pre-calculated value of how many bytes per packet to expect on the capture endpoint. That value is derived from the currently configured sample rate, and if the sound card fills each of its buffers with that number of bytes or less, everything's fine. Also, each urb is resubmitted immediately after reception, and there is no delay or timing or anything. f_uac2, however, currently always completes its buffers with 512 bytes packets, which causes two problems: a) it leads to -EOVERFLOW errors on the host side, as the host doesn't expect such big packets, and b) audio is transported as fast as possible, and nothing ties the actual rate to any clock on either side. In my tests, audio was transported roughly at 3x the actual sample rate. While this works fine if only files are in the game on both sides, but once any part of the system expects the actual rate to be at least approximately in the configured range, things go wrong of course. Also, the maximum number of bytes that the host expects to receive for a packet is not part of the request communication on the USB wire. Even if we wanted, we wouldn't be able to adopt to it in order to prevent overflows on the host side. My current fix is comprised of two parts: a) set UAC_EP_CS_ATTR_FILL_MAX in the capture UAC2 endpoint, which allows the gadget to actually send packet with wMaxPacketSize bytes, and b) implement a simple timing mechanism to tie the gadget's capture stream to the configured sample rate, and send 0 bytes packets if the timing doesn't allow a full-sized packet to be sent again. These two changes fixed the functionality on a BBB/musb gadget setup, but I'd still like to understand how this could have ever worked the way it's implemented at the moment. Which OS did you test with on the host side, and what type of gadget hardware was in use? I'll send out the patches once I have confidence that I'm not missing anything essential :) Thanks, Daniel -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-usb] USB Gadget drivers Windows 7/8 support and .bAlternateSetting in interface descriptor
On 08/22/2014 04:43 AM, Xuebing Wang wrote:
static inline bool gadget_supports_altsettings(struct usb_gadget *gadget)
{
/* PXA 21x/25x/26x has no altsettings at all */
if (gadget_is_pxa(gadget))
return false;
/* PXA 27x and 3xx have *broken* altsetting support */
if (gadget_is_pxa27x(gadget))
return false;
/* Everything else is *presumably* fine ... */
return true;
}
Hmm. On hardware without altsetting support, isochronous endpoints
cannot be used. I wonder whether the gadget core should loudly complain
if it detects such a situation.
This is what the USB spec says in 5.6.3:
All device default interface settings must not include any isochronous
endpoints with non-zero data payload sizes (specified via
wMaxPacketSize in the endpoint descriptor). Alternate interface
settings may specify non-zero data payload sizes for isochronous
endpoints.
Daniel
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What is the command line commands to use UAC2 at USB client side?
On 08/19/2014 02:01 AM, Xuebing Wang wrote: On 08/19/2014 01:11 AM, Daniel Mack wrote: On 08/15/2014 05:49 AM, Xuebing Wang wrote: root@imx6slevk:~# root@imx6slevk:~# arecord -f dat -t wav -D hw:2,0 | aplay -D hw:0,0 Recording WAVE 'stdin' : Signed 16 bit Little Endian, Rate 48000 Hz, Stereo Playing WAVE 'stdin' : Signed 16 bit Little Endian, Rate 48000 Hz, Stereo Such a setup should work, I recently tried it myself. The other direction (capturing from host, playback on device), however, has a major problem as the device interface has no timing mechanism, and hence 'arecord | aplay' on the gadget side fails. I've prepared patches and a more comprehensive description for this, but I'm waiting for Andrzej's patches to be reviewed, as mine are based upon his. Thanks. You were trying UAC2, right? Yes. Does it work with Windows 7/8 host? I have no idea, and no Windows box to test on. Did you try UAC1 too? Not in this case, no. Daniel -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What is the command line commands to use UAC2 at USB client side?
On 08/19/2014 10:54 AM, Xuebing Wang wrote: Thanks. You were trying UAC2, right? Yes. Thanks Daniel. Did you hear noise (hear with ear phone, not speaker) especially when playing music from Ubuntu host? I am hearing slight noise here. Not that I noticed. I'd suggest recording audio from the host into a file, then copy it over to your host for examination. Once you build a chain of audio components, it's hard to tell where your problem is otherwise. Daniel -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What is the command line commands to use UAC2 at USB client side?
On 08/19/2014 11:01 AM, Jassi Brar wrote: On Tue, Aug 19, 2014 at 2:15 PM, Daniel Mack dan...@zonque.org wrote: On 08/19/2014 02:01 AM, Xuebing Wang wrote: root@imx6slevk:~# root@imx6slevk:~# arecord -f dat -t wav -D hw:2,0 | aplay -D hw:0,0 Recording WAVE 'stdin' : Signed 16 bit Little Endian, Rate 48000 Hz, Stereo Playing WAVE 'stdin' : Signed 16 bit Little Endian, Rate 48000 Hz, Stereo Such a setup should work, I recently tried it myself. The other direction (capturing from host, playback on device), however, has a major problem as the device interface has no timing mechanism, and hence 'arecord | aplay' on the gadget side fails. I've prepared patches and a more comprehensive description for this, but I'm waiting for Andrzej's patches to be reviewed, as mine are based upon his. Hmm... I tested 48KHz USB-IN without noise, 44.1KHz did show noise though ... iirc With USB-IN, you're referring to arecord on the host side, and aplay on the gadget? Playing/ recording wave files on both sides worked fine for me. The only problem here is that once you link one side to a sink or source that expects audio to be transported at least roughly with the announces sample rate, things break because there is nothing that controls the timing. It's easy to fix, and as I said, I have patches for this that I'll send out shortly. However, I thought Xuebing's setup is the other way around, right? Does it work with Windows 7/8 host? I have no idea, and no Windows box to test on. IIRC Windows doesn't have native support for UAC2. That's true, you need a third party driver for that. I was referring to UAC1, but I didn't test this either. Daniel -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/2] usb: gadget: Add xilinx usb2 device support
Hi,
On 07/22/2014 11:08 AM, Subbaraya Sundeep Bhatta wrote:
This patch adds xilinx usb2 device driver support
Add some more information here, please. Copying the text from the
Kconfig option is already a good start.
drivers/usb/gadget/Kconfig | 14 +
drivers/usb/gadget/Makefile |1 +
drivers/usb/gadget/udc-xilinx.c | 2261
+++
3 files changed, 2276 insertions(+), 0 deletions(-)
create mode 100644 drivers/usb/gadget/udc-xilinx.c
As your driver has a DT binding, you have to describe it in
Documentation/devicetree/bindings.
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -459,6 +459,20 @@ config USB_EG20T
ML7213/ML7831 is companion chip for Intel Atom E6xx series.
ML7213/ML7831 is completely compatible for Intel EG20T PCH.
+config USB_GADGET_XILINX
+ tristate Xilinx USB Driver
+ depends on COMPILE_TEST
Why would you depend on that?
Also, your code uses device tree functions unconditionally, which is
fine, but it must hence depend on 'OF'.
+struct xusb_ep {
+ struct usb_ep ep_usb;
+ struct list_head queue;
+ struct xusb_udc *udc;
+ const struct usb_endpoint_descriptor *desc;
+ u32 rambase;
+ u32 offset;
+ char name[4];
+ u16 epnumber;
+ u16 maxpacket;
+ u16 buffer0count;
+ u16 buffer1count;
+ u8 buffer0ready;
+ u8 buffer1ready;
+ u8 eptype;
+ u8 curbufnum;
+ u8 is_in;
+ u8 is_iso;
Some of those could probably become booleans.
+struct xusb_udc {
+ struct usb_gadget gadget;
+ struct xusb_ep ep[8];
+ struct usb_gadget_driver *driver;
+ struct usb_ctrlrequest setup;
+ struct xusb_req *req;
+ struct device *dev;
+ u32 usb_state;
+ u32 remote_wkp;
+ u32 setupseqtx;
+ u32 setupseqrx;
+ void __iomem *base_address;
+ spinlock_t lock;
+ bool dma_enabled;
+
+ unsigned int (*read_fn)(void __iomem *);
+ void (*write_fn)(void __iomem *, u32, u32);
+};
+
+/* Endpoint buffer start addresses in the core */
+static u32 rambase[8] = { 0x22, 0x1000, 0x1100, 0x1200, 0x1300, 0x1400,
0x1500,
+ 0x1600 };
As stated by Peter, indenting such lines to match the position of the
line before makes such code much prettier and more readable. It's also
done in the majority of drivers.
This counts for many locations in your code.
+/* Control endpoint configuration.*/
+static const struct usb_endpoint_descriptor config_bulk_out_desc = {
+ .bLength= USB_DT_ENDPOINT_SIZE,
+ .bDescriptorType= USB_DT_ENDPOINT,
+ .bEndpointAddress = USB_DIR_OUT,
+ .bmAttributes = USB_ENDPOINT_XFER_BULK,
+ .wMaxPacketSize = cpu_to_le16(64),
Why not use EP0_MAX_PACKET here?
+static void xudc_wrstatus(struct xusb_udc *udc)
+{
+ struct xusb_ep *ep0 = udc-ep[XUSB_EP_NUMBER_ZERO];
+ u32 epcfgreg;
+
+ epcfgreg = udc-read_fn(udc-base_address + ep0-offset)|
+ XUSB_EP_CFG_DATA_TOGGLE_MASK;
+ udc-write_fn(udc-base_address, ep0-offset, epcfgreg);
+ udc-write_fn(udc-base_address, ep0-offset + XUSB_EP_BUF0COUNT_OFFSET,
+ 0);
Just a nit, but renaming 'base_address' to something like 'base' or
'addr' would help you get around or maximum line constraint here and in
some other places.
+static int xudc_dma_send(struct xusb_ep *ep, struct xusb_req *req,
+ u8 *buffer, u32 length)
+{
+ u32 *eprambase;
+ dma_addr_t src;
+ dma_addr_t dst;
+ int ret;
+ struct xusb_udc *udc = ep-udc;
+
+ src = req-usb_req.dma + req-usb_req.actual;
+ if (req-usb_req.length)
+ dma_sync_single_for_device(udc-dev, src,
+ length, DMA_TO_DEVICE);
+ if (!ep-curbufnum !ep-buffer0ready) {
+ /* Get the Buffer address and copy the transmit data.*/
+ eprambase = (u32 __force *)(udc-base_address +
+ ep-rambase);
+ dst = virt_to_phys(eprambase);
+ udc-write_fn(udc-base_address, ep-offset +
+ XUSB_EP_BUF0COUNT_OFFSET, length);
+ udc-write_fn(udc-base_address, XUSB_DMA_CONTROL_OFFSET,
+ XUSB_DMA_BRR_CTRL | (1 ep-epnumber));
+ ep-buffer0ready = 1;
+ ep-curbufnum = 1;
+ } else if (ep-curbufnum !ep-buffer1ready) {
+ /* Get the Buffer address and copy the transmit data.*/
+ eprambase = (u32 __force *)(udc-base_address +
+ ep-rambase + ep-ep_usb.maxpacket);
+ dst = virt_to_phys(eprambase);
+ udc-write_fn(udc-base_address, ep-offset +
+ XUSB_EP_BUF1COUNT_OFFSET, length);
+ udc-write_fn(udc-base_address, XUSB_DMA_CONTROL_OFFSET,
+
Re: [PATCH v4 2/2] usb: gadget: Add xilinx usb2 device support
On 08/19/2014 11:56 AM, Daniel Mack wrote: On 07/22/2014 11:08 AM, Subbaraya Sundeep Bhatta wrote: drivers/usb/gadget/Kconfig | 14 + drivers/usb/gadget/Makefile |1 + drivers/usb/gadget/udc-xilinx.c | 2261 +++ 3 files changed, 2276 insertions(+), 0 deletions(-) create mode 100644 drivers/usb/gadget/udc-xilinx.c As your driver has a DT binding, you have to describe it in Documentation/devicetree/bindings. Ah, sorry. I missed patch 1/2 which does that. Thanks, Daniel -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What is the command line commands to use UAC2 at USB client side?
On 08/15/2014 05:49 AM, Xuebing Wang wrote: Jassi, thanks for your help. Without knowing what is the best way to implement UAC2 at the device side, I managed to simply pipe is as below: Note: When I am playing music from Ubuntu host, I can hear light noise from the device (via UAC2) root@imx6slevk:~# modprobe g_audio c_srate=48000 g_audio gadget: Linux USB Audio Gadget, version: Feb 2, 2012 g_audio gadget: g_audio ready root@imx6slevk:~# g_audio gadget: high-speed config #1: Linux USB Audio Gadget root@imx6slevk:~# root@imx6slevk:~# arecord -f dat -t wav -D hw:2,0 | aplay -D hw:0,0 Recording WAVE 'stdin' : Signed 16 bit Little Endian, Rate 48000 Hz, Stereo Playing WAVE 'stdin' : Signed 16 bit Little Endian, Rate 48000 Hz, Stereo Such a setup should work, I recently tried it myself. The other direction (capturing from host, playback on device), however, has a major problem as the device interface has no timing mechanism, and hence 'arecord | aplay' on the gadget side fails. I've prepared patches and a more comprehensive description for this, but I'm waiting for Andrzej's patches to be reviewed, as mine are based upon his. Thanks, Daniel -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Alsa-user] linux does not recognize my midi controller samson carbon 61
On 08/15/2014 12:21 AM, Lion Bino wrote: I have a problem a midi controller, a Samson Carbon 61. When I connect via usb, dmesg tells me the following. [ 1480.097123] usb 2-1.1: new full-speed USB device number 9 using ehci_hcd [ 1480.118082] usb 2-1.1: no configurations [ 1480.118087] usb 2-1.1: can't read configurations, error -22 [ 1480.118206] hub 2-1:1.0: unable to enumerate USB device on port 1 That's not an ALSA problem, but most probably a firmware bug. A device with bNumConfigurations == 0 is invalid. Copied the linux-usb list. I use ubuntu 12.04 and 14.04 and nothing, I have tested in windows and works well. Could you follow the instructions in this document and generate a usbmon trace when connecting the device? I'd like to know whether the entire device descriptor is garbled. https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/usb/usbmon.txt Thanks, Daniel -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Alsa-user] linux does not recognize my midi controller samson carbon 61
Hi, On 08/15/2014 04:00 PM, Lionel wrote: Hi Daniel. This is trace when i connect the midi controller. Thanks. This one here is the setup packet followed by the answer, which is the device descriptor: 8801cbae2a80 1534946150 S Ci:2:000:0 s 80 06 0100 0040 64 8801cbae2a80 1534946409 C Ci:2:000:0 0 18 = 12010002 0040 a0174b34 01020301 Which decodes to (some fields omitted): bcdUSB = 0x0200 idVendor = 0x17a0 idProduct = 0x344b bcdDevice = 0x0201 bNumConfigurations = 0 So this is clearly a bug in the firmware of your device, as bNumConfigurations = 0 doesn't make any sense. You basically have two options: ask the vendor to fix the bug, and send you instructions on how to update the firmware. Or we add a quirk to the USB layer in the kernel which sets bNumConfigurations to 1 for your device, hoping that that's the only bug in the firmware, which is not certain. If you want to go for the latter, you'd first need to build a kernel from the sources. As you said earlier you're on Ubuntu, some guidance is provided here: https://wiki.ubuntu.com/KernelTeam/GitKernelBuild Let me know when you managed to boot your own kernel. I'll send you a patch to test then. Best regards, Daniel -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Alsa-user] linux does not recognize my midi controller samson carbon 61
On 08/15/2014 05:25 PM, Lionel wrote: Thanks Daniel! You're welcome! I decided to ask the vendor to fix the bug in the firmware. Do I have any expectation of success? Probably not. Most audio hardware vendors don't care about Linux. What should be the best way to request support for linux? Well, I already offered to provide a patch to at least make the device be recognized by the USB subsystem layer. After that has happened, we can look at the other decriptors and see what they yield. It's likely, however, that the device is not compatible to any class compliant driver. I'd even assume the vendor set bNumConfigurations to 1 on purpose so that Windows doesn't look at the device. But that's just guessing. Let me know if you want to give such a patch a try. Daniel -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: musb: cppi41: fix not transmitting zero length packet issue
Hi Bin,
On 08/12/2014 06:46 PM, Bin Liu wrote:
drivers/usb/musb/musb_cppi41.c | 15 +--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c
index 5989def..e9a0e54 100644
--- a/drivers/usb/musb/musb_cppi41.c
+++ b/drivers/usb/musb/musb_cppi41.c
@@ -39,6 +39,7 @@ struct cppi41_dma_channel {
u32 transferred;
u32 packet_sz;
struct list_head tx_check;
+ u32 tx_zlp;
'bool' might be easier to read later.
};
#define MUSB_DMA_NUM_CHANNELS 15
@@ -122,6 +123,8 @@ static void cppi41_trans_done(struct cppi41_dma_channel
*cppi41_channel)
{
struct musb_hw_ep *hw_ep = cppi41_channel-hw_ep;
struct musb *musb = hw_ep-musb;
+ void __iomem *epio = hw_ep-regs;
+ u16 csr;
if (!cppi41_channel-prog_len) {
@@ -130,15 +133,22 @@ static void cppi41_trans_done(struct cppi41_dma_channel
*cppi41_channel)
cppi41_channel-transferred;
cppi41_channel-channel.status = MUSB_DMA_STATUS_FREE;
cppi41_channel-channel.rx_packet_done = true;
+
+ /* transmit ZLP using PIO mode for transfers which size is
+ * multiple of EP packet size. */
I'd prefer this block to follow the kernel-style of comments, even
though this driver is full of other flavors.
+ if (cppi41_channel-tx_zlp (cppi41_channel-transferred %
+ cppi41_channel-packet_sz) == 0) {
+ musb_ep_select(musb-mregs, hw_ep-epnum);
+ csr = MUSB_TXCSR_MODE | MUSB_TXCSR_TXPKTRDY;
+ musb_writew(epio, MUSB_TXCSR, csr);
+ }
musb_dma_completion(musb, hw_ep-epnum, cppi41_channel-is_tx);
} else {
/* next iteration, reload */
struct dma_chan *dc = cppi41_channel-dc;
struct dma_async_tx_descriptor *dma_desc;
enum dma_transfer_direction direction;
- u16 csr;
u32 remain_bytes;
- void __iomem *epio = cppi41_channel-hw_ep-regs;
cppi41_channel-buf_addr += cppi41_channel-packet_sz;
@@ -362,6 +372,7 @@ static bool cppi41_configure_channel(struct dma_channel
*channel,
cppi41_channel-total_len = len;
cppi41_channel-transferred = 0;
cppi41_channel-packet_sz = packet_sz;
+ cppi41_channel-tx_zlp = (cppi41_channel-is_tx mode) ? 1 : 0;
Don't you need to check for (urb-transfer_flags URB_ZERO_PACKET)
somewhere here? I'd not expect an extra 0-byte packet at the end of a
transfer unless this flag is set.
Thanks,
Daniel
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: musb: cppi41: fix not transmitting zero length packet issue
On 08/12/2014 07:05 PM, Bin Liu wrote: Don't you need to check for (urb-transfer_flags URB_ZERO_PACKET) somewhere here? I'd not expect an extra 0-byte packet at the end of a transfer unless this flag is set. mode is set to 1 when (urb-transfer_flags URB_ZERO_PACKET), in musb_tx_dma_program in musb_host.c. Hmm, yes, but only for (!CONFIG_USB_INVENTRA_DMA !CONFIG_USB_UX500_DMA). Otherwise, mode will be 1 'if (length pkt_size)', so on those platforms, the driver will now send terminating ZLPs whenever a transfer spans across multiple packets. Or am I missing anything? You might need to pass a pointer to the urb or its transfer_flags down to channel_program(). Thanks, Daniel -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: musb: cppi41: fix not transmitting zero length packet issue
On 08/12/2014 07:32 PM, Bin Liu wrote: 633 #if defined(CONFIG_USB_INVENTRA_DMA) || defined(CONFIG_USB_UX500_DMA) ... 658 #else 659 if (!is_cppi_enabled() !tusb_dma_omap()) 660 return false; ... 668 mode = (urb-transfer_flags URB_ZERO_PACKET) ? 1 : 0; 669 #endif So mode is only set to 1 for CPPI DMA when URB_ZERO_PACKET is set, then musb_cppi41 driver will transmit the ZLP. For other DMA engine (INVERNTRA or UX500), its driver should have its own channel_program() callback, cppi41 driver should not be called. Ah, ok. That's the detail I missed - the channel_program() is a different callback in that case. Alright then. Thanks, Daniel -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [alsa-devel] [PATCH 1/1] usb: gadget: f_uac2: Fix pcm sample size selection
On 07/04/2014 07:43 AM, Takashi Iwai wrote:
At Thu, 3 Jul 2014 20:15:28 +0200,
Sebastian Reimers wrote:
The pcm playback and capture sample size format was fixed
SNDRV_PCM_FMTBIT_S16_LE.
This patch respects also 16, 24 and 32 bit p_ssize and c_ssize values.
Signed-off-by: Sebastian Reimers sebastian.reim...@gmail.com
In general, this driver should check more on p_ssize. This can be any
integer value passed as a module parameter. It should be cut to a
sane value or give an error in the probe function when an invalid
value is given. Ditto for p_srate and other parameters.
Also, I don't understand the logic of the code
runtime-hw.period_bytes_min = 2 * uac2-p_prm.max_psize
/ runtime-hw.periods_min;
But this is mostly irrelevant with this patch, so for this patch:
Reviewed-by: Takashi Iwai ti...@suse.de
Felipe, are you fine with this one?
Thanks,
Daniel
---
drivers/usb/gadget/f_uac2.c | 24 ++--
1 file changed, 22 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/f_uac2.c b/drivers/usb/gadget/f_uac2.c
index 6261db4a..3ed89ec 100644
--- a/drivers/usb/gadget/f_uac2.c
+++ b/drivers/usb/gadget/f_uac2.c
@@ -348,14 +348,34 @@ static int uac2_pcm_open(struct snd_pcm_substream
*substream)
if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK) {
spin_lock_init(uac2-p_prm.lock);
runtime-hw.rate_min = p_srate;
-runtime-hw.formats = SNDRV_PCM_FMTBIT_S16_LE; /* ! p_ssize ! */
+switch (p_ssize) {
+case 3:
+runtime-hw.formats = SNDRV_PCM_FMTBIT_S24_3LE;
+break;
+case 4:
+runtime-hw.formats = SNDRV_PCM_FMTBIT_S32_LE;
+break;
+default:
+runtime-hw.formats = SNDRV_PCM_FMTBIT_S16_LE;
+break;
+}
runtime-hw.channels_min = num_channels(p_chmask);
runtime-hw.period_bytes_min = 2 * uac2-p_prm.max_psize
/ runtime-hw.periods_min;
} else {
spin_lock_init(uac2-c_prm.lock);
runtime-hw.rate_min = c_srate;
-runtime-hw.formats = SNDRV_PCM_FMTBIT_S16_LE; /* ! c_ssize ! */
+switch (c_ssize) {
+case 3:
+runtime-hw.formats = SNDRV_PCM_FMTBIT_S24_3LE;
+break;
+case 4:
+runtime-hw.formats = SNDRV_PCM_FMTBIT_S32_LE;
+break;
+default:
+runtime-hw.formats = SNDRV_PCM_FMTBIT_S16_LE;
+break;
+}
runtime-hw.channels_min = num_channels(c_chmask);
runtime-hw.period_bytes_min = 2 * uac2-c_prm.max_psize
/ runtime-hw.periods_min;
--
2.0.1
___
Alsa-devel mailing list
alsa-de...@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [alsa-devel] [PATCH 1/1] usb: gadget: f_uac2: Fix pcm sample size selection
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 07/22/2014 07:44 PM, Felipe Balbi wrote: On Tue, Jul 22, 2014 at 07:09:51PM +0200, Daniel Mack wrote: On 07/04/2014 07:43 AM, Takashi Iwai wrote: At Thu, 3 Jul 2014 20:15:28 +0200, Sebastian Reimers wrote: The pcm playback and capture sample size format was fixed SNDRV_PCM_FMTBIT_S16_LE. This patch respects also 16, 24 and 32 bit p_ssize and c_ssize values. Signed-off-by: Sebastian Reimers sebastian.reim...@gmail.com In general, this driver should check more on p_ssize. This can be any integer value passed as a module parameter. It should be cut to a sane value or give an error in the probe function when an invalid value is given. Ditto for p_srate and other parameters. Also, I don't understand the logic of the code runtime-hw.period_bytes_min = 2 * uac2-p_prm.max_psize / runtime-hw.periods_min; But this is mostly irrelevant with this patch, so for this patch: Reviewed-by: Takashi Iwai ti...@suse.de Felipe, are you fine with this one? seems like I missed this one, do you mind if we take it during v3.17-rc and add Cc: stable ? I guess that was what Sebastian intended :) Thanks for taking care of it! Daniel -BEGIN PGP SIGNATURE- Version: GnuPG v1 Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJTzqckAAoJELphMRr8Y1QkzXYP/iWtHkczgDurT5s/6JNGJSv1 UsVTueRVepMLfVVFRW0mMiHHVfZVD/JoLMTAl0LkODBf95Vxgd0OR9WmHm1FVYdW oJ80LTCkGEEjeZlZTcHJFIqfgwB6ryUsPoC+BAWvWpVSjvp2uHME8JmipYiNrwLS fJNJSwL4lT5PFESQ27seonkDneAUk8FQObZCSOnqJsqqZpAWHmRQOPTQSyRsJ/fA wOj/jo9gWJXqMtow2mfDteM/doiblBJaquKoQDIlj1UuEv4Ta97a1f/0N/O0q8kP +4VqynEgCOhr8TNR8x7a6YTlzYQkonddFUGZuuJLLf5//Z5SA1zGdBl+bnZ3OPVV 7oVJZPakVaG/1ImkkPh+yUmPg6jRjaEbW6f2Y9p1t2iitnEGsi/doQpBS7xEf3+n kHehgcKBUXCGzrJ25vxu9Q1/wUQXDXqsb3Q+PorcbhSF86OOe9pll9GvGjJbc7VY zEitPPPKMmmLVUgyfFcsyz94NkZeAYMOCPZIBIZXtshoQGMvujUKB/qRbK5tO7x3 Y7cUnb5rSlfSuNCuRz4kKGIVK+ULr8XtdT6sSzm5kmNnqwYlxh2zt4XEPflIiRg9 4Z0MMHW4mRN0rUikfJofjsHObx/B6BvbFj7EBhyFipRZZW32b5jEn7kL5pOQRq1K pANkTdVOnKGmgJ1VI6x8 =3cTI -END PGP SIGNATURE- -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/7] dma: cppi41: handle 0-length packets
On 06/30/2014 09:40 PM, Daniel Mack wrote: #3 fixes a reproducible kernel crash when unplugging streaming audio devices, and #5 causes data stream starvation which makes using many s/causes/fixes/, of course. -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/2] usb musb/cppi41: Address issues with isochronous audio endpoints
On 06/30/2014 07:54 PM, Felipe Balbi wrote: Hi, On Fri, Jun 20, 2014 at 09:14:34AM +0530, George Cherian wrote: On 6/20/2014 3:50 AM, Daniel Mack wrote: Hi, I've been debugging issues with musb in host mode and both full-speed and high-speed USB audio devices with cppi41 DMA mode enabled. The effect that was observed with full-speed devices was that CPU load went up to 100% due to the dma channels dma_completion work struct. For FS devices, the MUSB_TXCSR_TXPKTRDY bit that signals the FIFO's emptyness takes a long time to be cleared, and if the worker function determines that it's still set, it will re-queue the work immediately, which effectively results in a busy poll that renders the system unusable. There are audible crackles on the output, and every bit of extra load will distort the audio stream even more. The work struct was introduced by 1af54b7a40 (usb: musb: musb_cppi41: Handle ISOCH differently and not use the hrtimer.), apparantly in order to mitigate an unreliable behaviour of the driver. Geroge, do you recall which problems you saw, which device you tested with and what the effect of this patch was for you? I'm asking because I suspect the issue in fact lies in the hrtimer interval setup and can hence be fixed well without the extra worker. Sebastian's patch to implement that timer (a655f481d: usb: musb: musb_cppi41: handle pre-mature TX complete interrupt) expressed some uncertainty about the chosen value of 140us, and suspected that there's a relation between the dma burst size and the minimum time value. Hence, I'm sending two patches. The first one reverts 1af54b7a40 as it causes trouble with FS audio devices, and the seconds addresses the issue by tweaking the hrtimer settings instead. This seems to work fine with both FS and HS audio devices now. George, could you give this a try with your original test case? Yes, I would do by this weekend and update. Sorry that am lil busy with some trainings. Do we have any new developments here ? FWIW, I now got back positive results from Sebastian, who tests these patches extensively. Daniel signature.asc Description: OpenPGP digital signature
Re: [PATCH v4 1/7] dma: cppi41: handle 0-length packets
Hi Felipe, Glad to see you find some time for this patch set :) On 06/30/2014 07:19 PM, Felipe Balbi wrote: On Mon, May 26, 2014 at 02:52:34PM +0200, Daniel Mack wrote: When a 0-length packet is received on the bus, desc-pd0 yields 1, which confuses the driver's users. This information is clearly wrong and not in accordance to the datasheet, but it's been observed on an AM335x board, very reproducible. Fix this by looking at bit 19 in PD2 of the completed packet. This bit will tell us if a zero-length packet was received on a queue. If it's set, ignore the value in PD0 and report a total length of 0 instead. Signed-off-by: Daniel Mack zon...@gmail.com Does this create dependency on the following patches ? Since they're in the same series, I suppose it does... Please clarify so we don't cause regressions upstream due to ordering. No, there's no dependency. I sent them in one series as they all contribute to the fact that multiple USB devices would behave incorrectly on musb. All of these patches have to be applied to fix different details, but for this particular one, the order doesn't matter. I also asked Vinod to pick this single patch for his tree, but there was no answer yet. Thanks, Daniel signature.asc Description: OpenPGP digital signature
Re: [PATCH v4 1/7] dma: cppi41: handle 0-length packets
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 06/30/2014 09:26 PM, Felipe Balbi wrote: On Mon, Jun 30, 2014 at 09:19:08PM +0200, Daniel Mack wrote: I also asked Vinod to pick this single patch for his tree, but there was no answer yet. cool. hopefully we can get a better v3.17. Ultimately it's your decision of course, but I really think the patches sould make it for v3.16, as they fix real bugs. Thanks, Daniel -BEGIN PGP SIGNATURE- Version: GnuPG v1 Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJTsbqGAAoJELphMRr8Y1QkjBUP/1V8FYuPpNgapV7wbvUND8fE cXckUSPkH9n1M9v6+7Jz/aRJ6KJGy5GBmyJU29v26wq1JlkvrD7o3iOtwPU5JzZr NjaHVkaBF4/8JKiCvOokKf+NiN07BBQdxJjbUaX2Wx0qaBR/2vaI98jFIRLQ8fe0 uTvUpJ1wUTHEppcMfPGEWtLVYJRN9ygQHwZ2XvfAo6wnRHu25/q0skMsy1hL40m6 eAR/dkl6eCC+e6HMR16OEjFizHbeZ1lHOeN95oTwznpIXafboPsap4/ZwePl/jOn 9Bw6q0nEZIE/QoTnWSRnRPo5JXNmGBA4xbB0pzHVA1eodd4GkkXlPQzWRy0UfdWP ouw+806aLIBBNEfXgvXSxzDkkWaVFf0jhYkQt3SNGnbrLClwmGSuownhhR6bYTih FOQoCenU3sR+4VNGvFDjConso2KxHIlxVkS4Zsc2hgpRNiEnrrxJaIGBmPGJgW6W x5MEp1446uegxHdzrkAXvo7Bqkh7SbsncdrymQbFgs/+PiT5hmL3LC8zfaOrqcR+ yOe848gZ2+qASEIxH9XypTGZWLCjvLrKaq0U1NvQRYCUdjNB2qHu5aR0ZHxCFJBT CMiwovnCzRDfbaSwjROj8QLsslAItXPm64i/ubuJYRVz+CZdIj327n/mo1tBzQfN KGxhT1iDNEgxkiiGwDnh =KIqc -END PGP SIGNATURE- -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/7] dma: cppi41: handle 0-length packets
On 06/30/2014 09:31 PM, Felipe Balbi wrote: On Mon, Jun 30, 2014 at 09:29:10PM +0200, Daniel Mack wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 06/30/2014 09:26 PM, Felipe Balbi wrote: On Mon, Jun 30, 2014 at 09:19:08PM +0200, Daniel Mack wrote: I also asked Vinod to pick this single patch for his tree, but there was no answer yet. cool. hopefully we can get a better v3.17. Ultimately it's your decision of course, but I really think the patches sould make it for v3.16, as they fix real bugs. only patchs 4 and 6 seem to be -rc material. Will it work if I apply only those during this -rc ? Hmm, I'd say #3 and #5 are -rc material, too. #3 fixes a reproducible kernel crash when unplugging streaming audio devices, and #5 causes data stream starvation which makes using many Wifi adapters with musb impossible, #1 should better go through Vinod's tree, and #2 and #7 are just cosmetic things. Daniel -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Gadget regression with enabling of MUSB babble interrupt handling
(+ George) On 06/19/2014 11:56 AM, Tony Lindgren wrote: Looks like commit ca88fc2ef0d7 (usb: musb: add a work_struct to recover from babble errors) causes MUSB gadgets to stop enumerating at least on omap3. Reverting the the commit fixes the issue. Hmm, so do you see babble errors occuring? Also, there are some more patches for musb and babble error recovery on the usb list, namely the ones starting here in v6: http://marc.info/?l=linux-usbm=140109627505065w=4 Care to give them a try? Thanks, Daniel -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Gadget regression with enabling of MUSB babble interrupt handling
Hi Tony, On 06/19/2014 12:31 PM, Tony Lindgren wrote: * Daniel Mack dan...@zonque.org [140619 03:10]: On 06/19/2014 11:56 AM, Tony Lindgren wrote: Looks like commit ca88fc2ef0d7 (usb: musb: add a work_struct to recover from babble errors) causes MUSB gadgets to stop enumerating at least on omap3. Reverting the the commit fixes the issue. Hmm, so do you see babble errors occuring? Not that I noticed of. Also, there are some more patches for musb and babble error recovery on the usb list, namely the ones starting here in v6: http://marc.info/?l=linux-usbm=140109627505065w=4 Care to give them a try? I can confirm that [PATCH v6 1/5] usb: musb: core: Handle Babble condition only in HOST mode fixes the issue for me. Also the explanation in the patch description explains why it breaks, and probably for all gadgets too because BABBLE and RESET share the same interrupt so it's RESET in peripheral mode. Ok, thanks for testing. I was told Felipe was out for some weeks, so I don't know when these patches will be reviewed and merged. But that also raises a question: Were these patches merged for v3.16 ever even tested in peripheral mode? At the time, I had no such hardware to test this on, so I was hoping for more testers to give them a try in different environments, which apparently didn't happen. It fixed a dead USB port condition on host-mode enabled hardware, though. We should make sure at least patch 1/5 from the series mentioned above makes it to 3.16. Thanks, Daniel -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Gadget regression with enabling of MUSB babble interrupt handling
On 06/19/2014 12:43 PM, Tony Lindgren wrote: * Daniel Mack dan...@zonque.org [140619 03:38]: On 06/19/2014 12:31 PM, Tony Lindgren wrote: * Daniel Mack dan...@zonque.org [140619 03:10]: On 06/19/2014 11:56 AM, Tony Lindgren wrote: But that also raises a question: Were these patches merged for v3.16 ever even tested in peripheral mode? At the time, I had no such hardware to test this on, so I was hoping for more testers to give them a try in different environments, which apparently didn't happen. It fixed a dead USB port condition on host-mode enabled hardware, though. Well we probably should not merge patches without proper acks and tested-by:s in general as things just seem to keep breaking constantly otherwise. And things not working will keep people from using linux next which will lead into even less testing.. I'm fairly sure the patch causing your trouble has been in linux-next for a while before they hit the merge window, so people with gadget enabled musb could have noticed the breakage early enough. The feedback rate for patches to this driver posted to linux-usb is also usually low, unfortunately. Anyway, breaking things is certainly not good, and I'm sorry for that. I'm just uncertain what detail in the procedure should be tweaked in order to prevent that from happening in the future. Thanks, Daniel -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] usb: musb: cppi41: fire hrtimer according to programmed channel length
On 06/19/2014 05:07 PM, Felipe Balbi wrote: On Wed, Jun 18, 2014 at 11:36:43AM +0200, Daniel Mack wrote: On 06/18/2014 11:32 AM, David Laight wrote: From: Of Daniel Mack Sent: 18 June 2014 10:28 To: ba...@ti.com; george.cher...@ti.com; bige...@linutronix.de Cc: sebastian.reim...@googlemail.com; linux-usb@vger.kernel.org; Daniel Mack Subject: [PATCH 2/2] usb: musb: cppi41: fire hrtimer according to programmed channel length The musb/cppi41 code installs a hrtimer to work around DMA completion interrupts that have fired too early on AM335x hardware. This timer is currently programmed to first fire 140 nanoseconds after the DMA completion callback. According to the commit which introduced it (a655f481d83, usb: musb: musb_cppi41: handle pre-mature TX complete interrupt), that value is is considered a 'rule of thumb' that worked well with the test case described in the commit log. Test show, however, that for USB audio devices and much smaller packet sizes, the timer has to fire earlier in order to correctly handle the audio stream. The original test case had output transfer sizes of 1514 bytes, and a delay of 140 nanoseconds. For audio devices with 24 bytes channel size, 3 nanoseconds seem to work well. You can't really mean nanoseconds? Microseconds of course. Thanks for the heads-up. Fixed locally. Are you sending another one or want me to fix the commit log when applying here ? I was actually waiting for feedback on the patch before resending another version. In particular, I'd like to know whether George's test case still works with this patch applied. If nobody has objections, you fixing the commit log would of course be easiest, yes :) Thanks, Daniel -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] usb: musb: cppi41: fire hrtimer according to programmed channel length
On 06/19/2014 05:53 PM, David Laight wrote: From: Daniel Mack [mailto:dan...@zonque.org] On 06/19/2014 05:07 PM, Felipe Balbi wrote: On Wed, Jun 18, 2014 at 11:36:43AM +0200, Daniel Mack wrote: On 06/18/2014 11:32 AM, David Laight wrote: You can't really mean nanoseconds? Microseconds of course. Thanks for the heads-up. Fixed locally. Are you sending another one or want me to fix the commit log when applying here ? I was actually waiting for feedback on the patch before resending another version. In particular, I'd like to know whether George's test case still works with this patch applied. If nobody has objections, you fixing the commit log would of course be easiest, yes :) IIRC there was a reference to nanoseconds inside the patch as well. Possibly a variable name. Indeed. Nevermind, I'll send another version soon. Thanks! Daniel -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/2] usb musb/cppi41: Address issues with isochronous audio endpoints
Hi, I've been debugging issues with musb in host mode and both full-speed and high-speed USB audio devices with cppi41 DMA mode enabled. The effect that was observed with full-speed devices was that CPU load went up to 100% due to the dma channels dma_completion work struct. For FS devices, the MUSB_TXCSR_TXPKTRDY bit that signals the FIFO's emptyness takes a long time to be cleared, and if the worker function determines that it's still set, it will re-queue the work immediately, which effectively results in a busy poll that renders the system unusable. There are audible crackles on the output, and every bit of extra load will distort the audio stream even more. The work struct was introduced by 1af54b7a40 (usb: musb: musb_cppi41: Handle ISOCH differently and not use the hrtimer.), apparantly in order to mitigate an unreliable behaviour of the driver. Geroge, do you recall which problems you saw, which device you tested with and what the effect of this patch was for you? I'm asking because I suspect the issue in fact lies in the hrtimer interval setup and can hence be fixed well without the extra worker. Sebastian's patch to implement that timer (a655f481d: usb: musb: musb_cppi41: handle pre-mature TX complete interrupt) expressed some uncertainty about the chosen value of 140us, and suspected that there's a relation between the dma burst size and the minimum time value. Hence, I'm sending two patches. The first one reverts 1af54b7a40 as it causes trouble with FS audio devices, and the seconds addresses the issue by tweaking the hrtimer settings instead. This seems to work fine with both FS and HS audio devices now. George, could you give this a try with your original test case? Note that these patches are to be applied on top of my musb/cppi41 cleanup series that I sent ~3 weeks ago. Thanks, Daniel v1 - v2: * clean up nanosecond/microsecond confusion in patch #2 Daniel Mack (2): Revert usb: musb: musb_cppi41: Handle ISOCH differently and not use the hrtimer. usb: musb: cppi41: fire hrtimer according to programmed channel length drivers/usb/musb/musb_cppi41.c | 59 +++--- 1 file changed, 4 insertions(+), 55 deletions(-) -- 1.9.3 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/2] Revert usb: musb: musb_cppi41: Handle ISOCH differently and not use the hrtimer.
This reverts commit 1af54b7a4.
The commit tried to address cases in which isochronous transfers are 'not
reliable', most probably in the tests conducted, polling for the
MUSB_TXCSR_TXPKTRDY bit in MUSB_TXCSR is done too late.
Hence, it installs a work struct which basically busy-polls for the bit in a
rather agressive way by rescheduling the work if the FIFO is not empty. With
USB audio devices, tests have shown that it takes approximately 100
iterations of the asynchronous worker until the FIFO signals completion,
which leads to 100% CPU loads when streaming audio.
The issue the patch tried to address can be handled differently, which is
what the next patch does.
Signed-off-by: Daniel Mack zon...@gmail.com
Reported-by: Sebastian Reimers sebastian.reim...@googlemail.com
---
drivers/usb/musb/musb_cppi41.c | 53 --
1 file changed, 53 deletions(-)
diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c
index 932464f..a2c4456 100644
--- a/drivers/usb/musb/musb_cppi41.c
+++ b/drivers/usb/musb/musb_cppi41.c
@@ -39,7 +39,6 @@ struct cppi41_dma_channel {
u32 transferred;
u32 packet_sz;
struct list_head tx_check;
- struct work_struct dma_completion;
};
#define MUSB_DMA_NUM_CHANNELS 15
@@ -117,18 +116,6 @@ static bool musb_is_tx_fifo_empty(struct musb_hw_ep *hw_ep)
return true;
}
-static bool is_isoc(struct musb_hw_ep *hw_ep, bool in)
-{
- if (in hw_ep-in_qh) {
- if (hw_ep-in_qh-type == USB_ENDPOINT_XFER_ISOC)
- return true;
- } else if (hw_ep-out_qh) {
- if (hw_ep-out_qh-type == USB_ENDPOINT_XFER_ISOC)
- return true;
- }
- return false;
-}
-
static void cppi41_dma_callback(void *private_data);
static void cppi41_trans_done(struct cppi41_dma_channel *cppi41_channel)
@@ -185,32 +172,6 @@ static void cppi41_trans_done(struct cppi41_dma_channel
*cppi41_channel)
}
}
-static void cppi_trans_done_work(struct work_struct *work)
-{
- unsigned long flags;
- struct cppi41_dma_channel *cppi41_channel =
- container_of(work, struct cppi41_dma_channel, dma_completion);
- struct cppi41_dma_controller *controller = cppi41_channel-controller;
- struct musb *musb = controller-musb;
- struct musb_hw_ep *hw_ep = cppi41_channel-hw_ep;
- bool empty;
-
- if (!cppi41_channel-is_tx is_isoc(hw_ep, 1)) {
- spin_lock_irqsave(musb-lock, flags);
- cppi41_trans_done(cppi41_channel);
- spin_unlock_irqrestore(musb-lock, flags);
- } else {
- empty = musb_is_tx_fifo_empty(hw_ep);
- if (empty) {
- spin_lock_irqsave(musb-lock, flags);
- cppi41_trans_done(cppi41_channel);
- spin_unlock_irqrestore(musb-lock, flags);
- } else {
- schedule_work(cppi41_channel-dma_completion);
- }
- }
-}
-
static enum hrtimer_restart cppi41_recheck_tx_req(struct hrtimer *timer)
{
struct cppi41_dma_controller *controller;
@@ -274,14 +235,6 @@ static void cppi41_dma_callback(void *private_data)
transferred cppi41_channel-packet_sz)
cppi41_channel-prog_len = 0;
- if (!cppi41_channel-is_tx) {
- if (is_isoc(hw_ep, 1))
- schedule_work(cppi41_channel-dma_completion);
- else
- cppi41_trans_done(cppi41_channel);
- goto out;
- }
-
empty = musb_is_tx_fifo_empty(hw_ep);
if (empty) {
cppi41_trans_done(cppi41_channel);
@@ -318,10 +271,6 @@ static void cppi41_dma_callback(void *private_data)
goto out;
}
}
- if (is_isoc(hw_ep, 0)) {
- schedule_work(cppi41_channel-dma_completion);
- goto out;
- }
list_add_tail(cppi41_channel-tx_check,
controller-early_tx_list);
if (!hrtimer_active(controller-early_tx)) {
@@ -679,8 +628,6 @@ static int cppi41_dma_controller_start(struct
cppi41_dma_controller *controller)
cppi41_channel-port_num = port;
cppi41_channel-is_tx = is_tx;
INIT_LIST_HEAD(cppi41_channel-tx_check);
- INIT_WORK(cppi41_channel-dma_completion,
- cppi_trans_done_work);
musb_dma = cppi41_channel-channel;
musb_dma-private_data = cppi41_channel;
--
1.9.3
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/2] usb: musb: cppi41: fire hrtimer according to programmed channel length
The musb/cppi41 code installs a hrtimer to work around DMA completion
interrupts that have fired too early on AM335x hardware. This timer
is currently programmed to first fire 140 microseconds after the DMA
completion callback. According to the commit which introduced it
(a655f481d83, usb: musb: musb_cppi41: handle pre-mature TX complete
interrupt), that value is is considered a 'rule of thumb' that worked
well with the test case described in the commit log.
Test show, however, that for USB audio devices and much smaller packet
sizes, the timer has to fire earlier in order to correctly handle the audio
stream. The original test case had output transfer sizes of 1514 bytes, and
a delay of 140 microseconds. For audio devices with 24 bytes channel size, 3
microseconds seem to work well.
Hence, let's assume that the time it takes to clear the bit correlates with
the number of bytes transferred. The referenced commit log mentions such a
suspicion as well. Let the timer fire in cppi41_channel-total_len/10
microseconds to correctly handle both cases.
Also, shorten the interval in which the timer fires again in case of
a non-empty early_tx list.
With these changes in place, both FS and HS audio devices appear to work
well on AM335x hardware.
Signed-off-by: Daniel Mack zon...@gmail.com
Reported-by: Sebastian Reimers sebastian.reim...@googlemail.com
---
drivers/usb/musb/musb_cppi41.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c
index a2c4456..adfffe8 100644
--- a/drivers/usb/musb/musb_cppi41.c
+++ b/drivers/usb/musb/musb_cppi41.c
@@ -200,7 +200,7 @@ static enum hrtimer_restart cppi41_recheck_tx_req(struct
hrtimer *timer)
if (!list_empty(controller-early_tx_list)) {
ret = HRTIMER_RESTART;
hrtimer_forward_now(controller-early_tx,
- ktime_set(0, 150 * NSEC_PER_USEC));
+ ktime_set(0, 50 * NSEC_PER_USEC));
}
spin_unlock_irqrestore(musb-lock, flags);
@@ -274,8 +274,10 @@ static void cppi41_dma_callback(void *private_data)
list_add_tail(cppi41_channel-tx_check,
controller-early_tx_list);
if (!hrtimer_active(controller-early_tx)) {
+ unsigned long usecs = cppi41_channel-total_len / 10;
+
hrtimer_start_range_ns(controller-early_tx,
- ktime_set(0, 140 * NSEC_PER_USEC),
+ ktime_set(0, usecs * NSEC_PER_USEC),
40 * NSEC_PER_USEC,
HRTIMER_MODE_REL);
}
--
1.9.3
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] Revert usb: musb: musb_cppi41: Handle ISOCH differently and not use the hrtimer.
This reverts commit 1af54b7a4.
The commit tried to address cases in which isochronous transfers are 'not
reliable', most probably in the tests conducted, polling for the
MUSB_TXCSR_TXPKTRDY bit in MUSB_TXCSR is done too late.
Hence, it installs a work struct which basically busy-polls for the bit in a
rather agressive way by rescheduling the work if the FIFO is not empty. With
USB audio devices, tests have shown that it takes approximately 100
iterations of the asynchronous worker until the FIFO signals completion,
which leads to 100% CPU loads when streaming audio.
The issue the patch tried to address can be handled differently, which is
what the next patch does.
Signed-off-by: Daniel Mack zon...@gmail.com
Reported-by: Sebastian Reimers sebastian.reim...@googlemail.com
---
drivers/usb/musb/musb_cppi41.c | 53 --
1 file changed, 53 deletions(-)
diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c
index 932464f..a2c4456 100644
--- a/drivers/usb/musb/musb_cppi41.c
+++ b/drivers/usb/musb/musb_cppi41.c
@@ -39,7 +39,6 @@ struct cppi41_dma_channel {
u32 transferred;
u32 packet_sz;
struct list_head tx_check;
- struct work_struct dma_completion;
};
#define MUSB_DMA_NUM_CHANNELS 15
@@ -117,18 +116,6 @@ static bool musb_is_tx_fifo_empty(struct musb_hw_ep *hw_ep)
return true;
}
-static bool is_isoc(struct musb_hw_ep *hw_ep, bool in)
-{
- if (in hw_ep-in_qh) {
- if (hw_ep-in_qh-type == USB_ENDPOINT_XFER_ISOC)
- return true;
- } else if (hw_ep-out_qh) {
- if (hw_ep-out_qh-type == USB_ENDPOINT_XFER_ISOC)
- return true;
- }
- return false;
-}
-
static void cppi41_dma_callback(void *private_data);
static void cppi41_trans_done(struct cppi41_dma_channel *cppi41_channel)
@@ -185,32 +172,6 @@ static void cppi41_trans_done(struct cppi41_dma_channel
*cppi41_channel)
}
}
-static void cppi_trans_done_work(struct work_struct *work)
-{
- unsigned long flags;
- struct cppi41_dma_channel *cppi41_channel =
- container_of(work, struct cppi41_dma_channel, dma_completion);
- struct cppi41_dma_controller *controller = cppi41_channel-controller;
- struct musb *musb = controller-musb;
- struct musb_hw_ep *hw_ep = cppi41_channel-hw_ep;
- bool empty;
-
- if (!cppi41_channel-is_tx is_isoc(hw_ep, 1)) {
- spin_lock_irqsave(musb-lock, flags);
- cppi41_trans_done(cppi41_channel);
- spin_unlock_irqrestore(musb-lock, flags);
- } else {
- empty = musb_is_tx_fifo_empty(hw_ep);
- if (empty) {
- spin_lock_irqsave(musb-lock, flags);
- cppi41_trans_done(cppi41_channel);
- spin_unlock_irqrestore(musb-lock, flags);
- } else {
- schedule_work(cppi41_channel-dma_completion);
- }
- }
-}
-
static enum hrtimer_restart cppi41_recheck_tx_req(struct hrtimer *timer)
{
struct cppi41_dma_controller *controller;
@@ -274,14 +235,6 @@ static void cppi41_dma_callback(void *private_data)
transferred cppi41_channel-packet_sz)
cppi41_channel-prog_len = 0;
- if (!cppi41_channel-is_tx) {
- if (is_isoc(hw_ep, 1))
- schedule_work(cppi41_channel-dma_completion);
- else
- cppi41_trans_done(cppi41_channel);
- goto out;
- }
-
empty = musb_is_tx_fifo_empty(hw_ep);
if (empty) {
cppi41_trans_done(cppi41_channel);
@@ -318,10 +271,6 @@ static void cppi41_dma_callback(void *private_data)
goto out;
}
}
- if (is_isoc(hw_ep, 0)) {
- schedule_work(cppi41_channel-dma_completion);
- goto out;
- }
list_add_tail(cppi41_channel-tx_check,
controller-early_tx_list);
if (!hrtimer_active(controller-early_tx)) {
@@ -679,8 +628,6 @@ static int cppi41_dma_controller_start(struct
cppi41_dma_controller *controller)
cppi41_channel-port_num = port;
cppi41_channel-is_tx = is_tx;
INIT_LIST_HEAD(cppi41_channel-tx_check);
- INIT_WORK(cppi41_channel-dma_completion,
- cppi_trans_done_work);
musb_dma = cppi41_channel-channel;
musb_dma-private_data = cppi41_channel;
--
1.9.3
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
