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
Re: [PATCH v2 3/4] usb: gadget: f_uac2: send reasonably sized packets
On Tue, 26 Aug 2014, Jassi Brar wrote:
Cool... we can avoid runtime calculations by maybe picking the
pre-defined 'length pattern' at module load time to match the rate
selected. And have those many usb requests allocated and their
'length' initialized to the pattern. Then the rest of code would
remain unchanged. Though I won't be surprised if you have some better
idea.
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).
Precompute M = DIV_ROUND_UP(S,R); that is the maximum number of samples
you ever have to send. So the maxpacket value is M * (number of bytes
per sample), for example, M * 4 for 16-bit stereo. That value should
be stored in the endpoint descriptor.
Also precompute n = S / R and p = S % R (the truncated quotient and the
remainder). Each packet will contain either n or n + 1 samples.
Here's how you decide which:
When the data stream begins, set x = 0. When preparing each packet,
do:
if (x R) {
put n samples in the next packet;
} else {
x = x - R;
put n + 1 samples in the next packet;
}
x = x + p;
That always gives exactly what you want.
Alan Stern
--
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
Re: [PATCH v2 3/4] usb: gadget: f_uac2: send reasonably sized packets
On Tue, 26 Aug 2014, Daniel Mack wrote: 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. It has the advantage of working for all audio rates, not just the common ones. And it doesn't require preallocation of quite so many transfer buffers (although they are generally small enough not to matter much). Alan Stern -- 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 Tue, Aug 26, 2014 at 9:18 PM, Alan Stern st...@rowland.harvard.edu wrote: On Tue, 26 Aug 2014, Daniel Mack wrote: 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. It has the advantage of working for all audio rates, not just the common ones. And it doesn't require preallocation of quite so many transfer buffers (although they are generally small enough not to matter much). I was indeed aware we could do the calculations in runtime, but I am not sure one formula can get us optimal result for all scenarios. I think we want to play with bInterval as well. For ex, on extreme end, for 5512Hz we may want to double packet size (from 22 bytes) in favor of halving ISO polls or even better. Similarly for 192000Hz, we don't want to reject only because wMaxPacketSize is 512... because we are likely running HS, so we could set bInterval to 0.5ms and do 384 bytes packet. I am still aware this all could be calculated and decided in runtime, but I don't think many readers will find that easier to understand than a static table. BTW, we can do without increasing allocated usb requests. Regards, Jassi -- 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 Tue, Aug 26, 2014 at 10:00 PM, Jassi Brar jassisinghb...@gmail.com wrote:
On Tue, Aug 26, 2014 at 9:18 PM, Alan Stern st...@rowland.harvard.edu wrote:
On Tue, 26 Aug 2014, Daniel Mack wrote:
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.
It has the advantage of working for all audio rates, not just the
common ones. And it doesn't require preallocation of quite so many
transfer buffers (although they are generally small enough not to
matter much).
I was indeed aware we could do the calculations in runtime, but I am
not sure one formula can get us optimal result for all scenarios. I
think we want to play with bInterval as well.
For ex, on extreme end, for 5512Hz we may want to double packet size
(from 22 bytes) in favor of halving ISO polls or even better.
Similarly for 192000Hz, we don't want to reject only because
wMaxPacketSize is 512... because we are likely running HS, so we could
set bInterval to 0.5ms and do 384 bytes packet.
I am still aware this all could be calculated and decided in runtime,
but I don't think many readers will find that easier to understand
than a static table.
Another example, for 44100Hz, calculations suggest synchronizing over
10ms as {9x176 + 1x180}, however we can keep better sync over 5ms as
{4x176+1x178}. Again we can calculate for even such optimizations but
only at the cost of readability.
Regards,
Jassi
--
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 Tue, Aug 26, 2014 at 1:38 PM, Daniel Mack dan...@zonque.org wrote:
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.
I was coming from increasing usb requests to number of elements in the
pattern array. In which case we set length of each req just once in
afunc_set_alt() and leave iso_complete() *untouched*.
However, if we do want to keep using only 2 usb requests, then yes we
do have to reset the req-length for the next packet. But still that
should ideally be done at the end of iso_complete(). The
initialization of 2 requests' length in afunc_set_alt() should be done
to first 2 elements of pattern (though nothing observable changes).
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?
By 'drop data' I meant we don't retry sending length-actual bytes.
Regards,
Jassi
--
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 Tue, 26 Aug 2014, Jassi Brar wrote:
On Tue, Aug 26, 2014 at 10:00 PM, Jassi Brar jassisinghb...@gmail.com wrote:
On Tue, Aug 26, 2014 at 9:18 PM, Alan Stern st...@rowland.harvard.edu
wrote:
On Tue, 26 Aug 2014, Daniel Mack wrote:
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.
It has the advantage of working for all audio rates, not just the
common ones. And it doesn't require preallocation of quite so many
transfer buffers (although they are generally small enough not to
matter much).
I was indeed aware we could do the calculations in runtime, but I am
not sure one formula can get us optimal result for all scenarios. I
think we want to play with bInterval as well.
For ex, on extreme end, for 5512Hz we may want to double packet size
(from 22 bytes) in favor of halving ISO polls or even better.
Similarly for 192000Hz, we don't want to reject only because
wMaxPacketSize is 512... because we are likely running HS, so we could
set bInterval to 0.5ms and do 384 bytes packet.
That's a separate matter. Selecting the sample rate and the polling
interval... You can put those things in a static table or decide them
at runtime. Either way, once you have settled on those parameters, the
computation I outlined earlier shows how to pack the samples into
packets.
I am still aware this all could be calculated and decided in runtime,
but I don't think many readers will find that easier to understand
than a static table.
Another example, for 44100Hz, calculations suggest synchronizing over
10ms as {9x176 + 1x180}, however we can keep better sync over 5ms as
{4x176+1x178}.
No, that's not good because it ends up splitting samples between
packets. You'd put 44 samples in the first four packets, then 44+(1/2)
in the fifth, then (1/2)+43+(1/2) in the next four, and (1/2)+44 in the
tenth (as opposed to 44 in nine packets and 45 in the tenth).
Maybe UAC2 allows such things (I haven't read the spec), but in general
it seems like a bad idea. And IIRC, the USB-2 spec disallows it.
Again we can calculate for even such optimizations but
only at the cost of readability.
Anybody accustomed to audio programming will immediately recognize the
pattern of the computation I outlined.
Alan Stern
--
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 Tue, Aug 26, 2014 at 11:07 PM, Alan Stern st...@rowland.harvard.edu wrote:
On Tue, 26 Aug 2014, Jassi Brar wrote:
On Tue, Aug 26, 2014 at 10:00 PM, Jassi Brar jassisinghb...@gmail.com
wrote:
On Tue, Aug 26, 2014 at 9:18 PM, Alan Stern st...@rowland.harvard.edu
wrote:
On Tue, 26 Aug 2014, Daniel Mack wrote:
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.
It has the advantage of working for all audio rates, not just the
common ones. And it doesn't require preallocation of quite so many
transfer buffers (although they are generally small enough not to
matter much).
I was indeed aware we could do the calculations in runtime, but I am
not sure one formula can get us optimal result for all scenarios. I
think we want to play with bInterval as well.
For ex, on extreme end, for 5512Hz we may want to double packet size
(from 22 bytes) in favor of halving ISO polls or even better.
Similarly for 192000Hz, we don't want to reject only because
wMaxPacketSize is 512... because we are likely running HS, so we could
set bInterval to 0.5ms and do 384 bytes packet.
That's a separate matter. Selecting the sample rate and the polling
interval... You can put those things in a static table or decide them
at runtime. Either way, once you have settled on those parameters, the
computation I outlined earlier shows how to pack the samples into
packets.
OK.
I am still aware this all could be calculated and decided in runtime,
but I don't think many readers will find that easier to understand
than a static table.
Another example, for 44100Hz, calculations suggest synchronizing over
10ms as {9x176 + 1x180}, however we can keep better sync over 5ms as
{4x176+1x178}.
No, that's not good because it ends up splitting samples between
packets. You'd put 44 samples in the first four packets, then 44+(1/2)
in the fifth, then (1/2)+43+(1/2) in the next four, and (1/2)+44 in the
tenth (as opposed to 44 in nine packets and 45 in the tenth).
Maybe UAC2 allows such things (I haven't read the spec), but in general
it seems like a bad idea. And IIRC, the USB-2 spec disallows it.
Hmm... yeah, frame-unaligned packets may mess up audio in case of
packet drops. Thanks.
Again we can calculate for even such optimizations but
only at the cost of readability.
Anybody accustomed to audio programming will immediately recognize the
pattern of the computation I outlined.
:) that is applicable against argument for readability for any subsystem.
Anyways, Daniel is doing the patch, I think he has the final call as
long we get best functionality.
Thanks
Jassi
--
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
Re: [PATCH v2 3/4] usb: gadget: f_uac2: send reasonably sized packets
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. The inaccuracy here is due to the way we program, and not due
to system/bus load.
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).
@@ -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).
I believe you want to do the following in afunc_set_alt().
- req-length = prm-max_psize;
+ req-length = uac2-c_pktsize;
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.
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.
Regards,
Jassi
--
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 Mon, Aug 25, 2014 at 10:52 PM, Jassi Brar jassisinghb...@gmail.com 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. The inaccuracy here is due to the way we program, and not due
to system/bus load.
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).
@@ -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).
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;
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);
BTW, why not do the same for USB-OUT as well? it shouldn't hurt.
Thanks
Jassi
--
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 Mon, Aug 25, 2014 at 11:40 PM, Daniel Mack dan...@zonque.org wrote:
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.
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.
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.
And if we want to provide exactly 176400 bytes of audio data every
second to the host.
@@ -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.
I see, thanks. That is a bug fix then, and probably should have been
patch-3/4 instead.
Regards,
Jassi
--
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: [PATCH v2 3/4] usb: gadget: f_uac2: send reasonably sized packets
On Tue, Aug 26, 2014 at 1:38 AM, Daniel Mack dan...@zonque.org wrote: 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. Cool... we can avoid runtime calculations by maybe picking the pre-defined 'length pattern' at module load time to match the rate selected. And have those many usb requests allocated and their 'length' initialized to the pattern. Then the rest of code would remain unchanged. Though I won't be surprised if you have some better idea. 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, Its just the we wouldn't want the fix to get delayed with the data-rate fix patch. Also maybe stable should be CC'ed on that patch? Regards, Jassi -- 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
