On Mon, Aug 25, 2014 at 11:40 PM, Daniel Mack <daniel@xxxxxxxxxx> wrote:
> Hi,
>
> On 08/25/2014 07:22 PM, Jassi Brar wrote:
>> On Mon, Aug 25, 2014 at 9:30 PM, Daniel Mack <zonque@xxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
