Re: [RFC PATCH v2 3/3] usb: gadget: u_audio: .... PCM Rate Shift for playback too?

[Pavel Hofman <pavel.hofman@ivitera.com>]

Dne 27. 05. 21 v 11:52 Jerome Brunet napsal(a):
> 
> On Thu 27 May 2021 at 08:52, Pavel Hofman <pavel.hofman@ivitera.com> wrote:
> 
>> Dne 24. 05. 21 v 18:09 Pavel Hofman napsal(a):
>>> Dne 24. 05. 21 v 17:40 Jerome Brunet napsal(a):
>>>>
>>>> On Mon 24 May 2021 at 14:29, Pavel Hofman <pavel.hofman@ivitera.com>
>>>> wrote:
>>>>
>>>>> Dne 30. 04. 21 v 16:26 Jerome Brunet napsal(a):
>>>>>> From: Ruslan Bilovol <ruslan.bilovol@gmail.com>
>>>>>>
>>>>>> This adds interface between userspace and feedback endpoint to report
>>>>>> real
>>>>>> feedback frequency to the Host.
>>>>>>
>>>>>> Current implementation adds new userspace interface ALSA mixer control
>>>>>> "Capture Pitch 1000000" (similar to aloop driver's "PCM Rate Shift
>>>>>> 100000"
>>>>>> mixer control)
>>>>>>
>>>>>> Value in PPM is chosen to have correction value agnostic of the actual
>>>>>> HW
>>>>>> rate, which the application is not necessarily dealing with, while
>>>>>> still
>>>>>> retaining a good enough precision to allow smooth clock correction on
>>>>>> the
>>>>>> playback side, if necessary.
>>>>>>
>>>>>> Similar to sound/usb/endpoint.c, a slow down is allowed up to 25%. This
>>>>>> has no impact on the required bandwidth. Speedup correction has an
>>>>>> impact
>>>>>> on the bandwidth reserved for the isochronous endpoint. The default
>>>>>> allowed speedup is 500ppm. This seems to be more than enough but, if
>>>>>> necessary, this is configurable through a module parameter. The
>>>>>> reserved
>>>>>> bandwidth is rounded up to the next packet size.
>>>>>>
>>>>>> Usage of this new control is easy to implement in existing userspace
>>>>>> tools
>>>>>> like alsaloop from alsa-utils.
>>>>>>
>>>>>> Signed-off-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>
>>>>>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>>>>>
>>>>>
>>>>> Hi, the existing patches solve the Host -> Gadget -> capturing
>>>>> application direction, controlling the host playback rate. The other
>>>>> direction (playback app -> gadget -> capturing host) is still paced by
>>>>> the host USB controller. Packet size is pre-calculated in
>>>>> u_audio_start_playback  as rate/p_interval
>>>>> https://github.com/pavhofman/linux-rpi/blob/rpi-5.10.y/drivers/usb/gadget/function/u_audio.c#L441 
>>>>> and this fixed value is used for copying the audio data in
>>>>> u_audio_iso_complete
>>>>> https://github.com/pavhofman/linux-rpi/blob/rpi-5.10.y/drivers/usb/gadget/function/u_audio.c#L124 
>>>>> .
>>>>>
>>>>> That means if the gadget has a physical duplex audio device with single
>>>>> clock and runs a duplex operation, the path gadget-> host  will not run
>>>>> synchronously with the physical audio device (the host -> gadget has
>>>>> already the feedback control implemented).
>>>>>
>>>>> How about "duplicating" the existing ALSA mixer control
>>>>> "Capture Pitch 1000000" to "Playback Pitch 1000000" and using
>>>>> pitch-adjusted p_srate in the above-linked calculations? That should
>>>>> make the playback side run at the playback pitch requested by gadget
>>>>> userspace, IIUC.
>>>>>
>>>>> For the duplex operation with single clock, the capture pitch value
>>>>> determined by the userspace chain (alsaloop, CamillaDSP, etc.) would be
>>>>> used for setting both the capture and playback pitch controls, making
>>>>> both directions run synchronously.
>>>>>
>>>>> I can prepare patches based on Jerome's patchset should you find this
>>>>> solution acceptable.
>>>>
>>>> Well I have experimented with pitch on the playback path.
>>>> It does work but I'm not sure if it actually makes sense which is why I
>>>> have not not included it in RFC
>>>>
>>>> If you need the playback and capture to run synchronously, you'd be
>>>> better off with implicit feedback (In which the host will provide the
>>>> same number of samples it received from the device)
>>>>
>>>> With explicit feedback, we are (supposed) to tell the host to speed up
>>>> or slow down to match the device clock. This means the device run
>>>> asynchronously, and the host does the adaptation (if necessary). In such
>>>> case, I'm not sure adding pitch control on the playback path is a good
>>>> idea.
>>>>
>>>> Having pitch control on the playback side allows to forward the audio
>>>> captured by the physical interface of the device (like I2S) to USB
>>>> without performing any resampling between the two (when USB and I2S are
>>>> not in sync). It's nice and convenient ... but I wonder if this is a
>>>> feature or a hack ;)
>>>>
>>> Jerome, thanks a lot for your reply. The current implementation of the 
>>> EP IN audio direction is timed by the USB host controller. Let's consider
>>> 48Khz samplerate and bInterval=1 fullspeed (8k packets per second). Every
>>> IN packet will always carry 6 audio frames. No matter how fast the real
>>> master clock (i.e. e.g. I2S of the gadget) runs. Until an xrun occurs,
>>> unfortunately. Even if the gadget configuration used implicit feedback,
>>> the incoming stream would still provide no synchronization information
>>> from the I2S hardware clock as the data stream is clocked by the USB host
>>> which controls the timing on the USB bus (and thus the moment of iso
>>> completion). Plus the stock usb-audio driver in Windows 10 does not
>>> support implicit feedback, according to
>>> https://docs.microsoft.com/en-us/windows-hardware/drivers/audio/usb-2-0-audio-drivers#audio-streaming 
>>> .
>>>
>>> The problem is the fixed "slicing" of the samples into the packets which
>>> cannot be controlled. The same situation was on the host side before the 
>>> feedback EP was introduced. Here the one preparing the slices (the
>>> "transmitter") is the gadget now. And the slicing pace must be 
>>> controllable just like the slicing pace on the host is via the feedback EP.
>>> Because the gadget supports different playback and capture rates (which 
>>> I find nice), I suggest a separate control element (Playback Pitch,
>>> Capture Pitch).
>>> Of course the motivation is to avoid adaptive resampling in the gadget 
>>> in the direction I2S -> gadget interface -> USB host. But the very same
>>> motivation lead to implementation of the async feedback EP in the 
>>> opposite direction. IMO it is not a hack, but an indispensable feature
>>> making the gadget usable for duplex operation with hardware (i.e. the 
>>> real master) clock at the backend (all the other "clocks" are just
>>> software-generated slices/blocks of audio frames).
>>> With regards,
>>> Pavel.
>>
>> Jerome, please do you still have your playback-side patches available or
>> should I prepare them? Thanks a lot,
>>
> 
> Yes, this is what I have tested:
> 
> https://gitlab.com/jbrunet/linux/-/commit/43f1930ba548e137a5d20b1801790fae83eaa1e0
> https://gitlab.com/jbrunet/linux/-/commit/acb2ed9d9219488184cd2eb592fcdbf78b042a9e
> 
> It probably requires a rebase and cleaning before it is actually ready
> for prime time but it does work.
> 
> For now, I'd like to focus on getting this initial explicit feedback
> support in. There was no major complain on it, so I just have a minor
> tweak to do before I send the patchset again. Hopefully soon ...

Thanks for the patches, they look good. When you send your final
explicit FB patchset, I will rebase your two playback patches and put
all my changes (mainly rebased Julian's multiple samplerates patches) on
top to test the whole suite.

Pavel.