| Message ID | 20210430142625.357152-4-jbrunet@baylibre.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | UAC2 Gadget: feedback endpoint support | expand |
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. Thanks a lot, Pavel.
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 ;) > > Thanks a lot, > > Pavel.
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.
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, Pavel.
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 ... > Pavel.
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.
diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uac2 b/Documentation/ABI/testing/configfs-usb-gadget-uac2 index e7e59d7fb12f..26fb8e9b4e61 100644 --- a/Documentation/ABI/testing/configfs-usb-gadget-uac2 +++ b/Documentation/ABI/testing/configfs-usb-gadget-uac2 @@ -9,6 +9,7 @@ Description: c_srate capture sampling rate c_ssize capture sample size (bytes) c_sync capture synchronization type (async/adaptive) + fb_max maximum extra bandwidth in async mode p_chmask playback channel mask p_srate playback sampling rate p_ssize playback sample size (bytes) diff --git a/Documentation/usb/gadget-testing.rst b/Documentation/usb/gadget-testing.rst index f5a12667bd41..9d6276f82774 100644 --- a/Documentation/usb/gadget-testing.rst +++ b/Documentation/usb/gadget-testing.rst @@ -729,6 +729,7 @@ The uac2 function provides these attributes in its function directory: c_srate capture sampling rate c_ssize capture sample size (bytes) c_sync capture synchronization type (async/adaptive) + fb_max maximum extra bandwidth in async mode p_chmask playback channel mask p_srate playback sampling rate p_ssize playback sample size (bytes) diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c index 321e6c05ba93..ae29ff2b2b68 100644 --- a/drivers/usb/gadget/function/f_uac2.c +++ b/drivers/usb/gadget/function/f_uac2.c @@ -584,8 +584,11 @@ static int set_ep_max_packet_size(const struct f_uac2_opts *uac2_opts, ssize = uac2_opts->c_ssize; } + if (!is_playback && (uac2_opts->c_sync == USB_ENDPOINT_SYNC_ASYNC)) + srate = srate * (1000 + uac2_opts->fb_max) / 1000; + max_size_bw = num_channels(chmask) * ssize * - ((srate / (factor / (1 << (ep_desc->bInterval - 1)))) + 1); + DIV_ROUND_UP(srate, factor / (1 << (ep_desc->bInterval - 1))); ep_desc->wMaxPacketSize = cpu_to_le16(min_t(u16, max_size_bw, max_size_ep)); @@ -957,6 +960,7 @@ afunc_bind(struct usb_configuration *cfg, struct usb_function *fn) agdev->params.c_srate = uac2_opts->c_srate; agdev->params.c_ssize = uac2_opts->c_ssize; agdev->params.req_number = uac2_opts->req_number; + agdev->params.fb_max = uac2_opts->fb_max; ret = g_audio_setup(agdev, "UAC2 PCM", "UAC2_Gadget"); if (ret) goto err_free_descs; @@ -1329,6 +1333,7 @@ UAC2_ATTRIBUTE(c_srate); UAC2_ATTRIBUTE_SYNC(c_sync); UAC2_ATTRIBUTE(c_ssize); UAC2_ATTRIBUTE(req_number); +UAC2_ATTRIBUTE(fb_max); static struct configfs_attribute *f_uac2_attrs[] = { &f_uac2_opts_attr_p_chmask, @@ -1339,6 +1344,7 @@ static struct configfs_attribute *f_uac2_attrs[] = { &f_uac2_opts_attr_c_ssize, &f_uac2_opts_attr_c_sync, &f_uac2_opts_attr_req_number, + &f_uac2_opts_attr_fb_max, NULL, }; @@ -1378,6 +1384,7 @@ static struct usb_function_instance *afunc_alloc_inst(void) opts->c_ssize = UAC2_DEF_CSSIZE; opts->c_sync = UAC2_DEF_CSYNC; opts->req_number = UAC2_DEF_REQ_NUM; + opts->fb_max = UAC2_DEF_FB_MAX; return &opts->func_inst; } diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c index f637ebec80b0..b517beb17006 100644 --- a/drivers/usb/gadget/function/u_audio.c +++ b/drivers/usb/gadget/function/u_audio.c @@ -16,6 +16,7 @@ #include <sound/core.h> #include <sound/pcm.h> #include <sound/pcm_params.h> +#include <sound/control.h> #include "u_audio.h" @@ -35,12 +36,12 @@ struct uac_rtd_params { void *rbuf; + unsigned int pitch; /* Stream pitch ratio to 1000000 */ unsigned int max_psize; /* MaxPacketSize of endpoint */ struct usb_request **reqs; struct usb_request *req_fback; /* Feedback endpoint request */ - unsigned int ffback; /* Real frequency reported by feedback endpoint */ bool fb_ep_enabled; /* if the ep is enabled */ }; @@ -75,7 +76,9 @@ static const struct snd_pcm_hardware uac_pcm_hardware = { }; static void u_audio_set_fback_frequency(enum usb_device_speed speed, - unsigned int freq, void *buf) + unsigned long long freq, + unsigned int pitch, + void *buf) { u32 ff = 0; @@ -86,7 +89,7 @@ static void u_audio_set_fback_frequency(enum usb_device_speed speed, * Format is encoded in Q10.10 left-justified in the 24 bits, * so that it has a Q10.14 format. */ - ff = DIV_ROUND_UP((freq << 14), 1000); + freq <<= 14; } else { /* * High-speed feedback endpoints report frequency @@ -95,8 +98,11 @@ static void u_audio_set_fback_frequency(enum usb_device_speed speed, * the binary point is located between the second and the third * byte fromat (that is Q16.16) */ - ff = DIV_ROUND_UP((freq << 13), 1000); + freq <<= 13; } + + ff = DIV_ROUND_CLOSEST_ULL((freq * pitch), (1000 * 1000000)); + *(__le32 *)buf = cpu_to_le32(ff); } @@ -209,8 +215,8 @@ static void u_audio_iso_fback_complete(struct usb_ep *ep, struct uac_rtd_params *prm = req->context; struct snd_uac_chip *uac = prm->uac; struct g_audio *audio_dev = uac->audio_dev; + struct uac_params *params = &audio_dev->params; int status = req->status; - unsigned long flags; /* i/f shutting down */ if (!prm->fb_ep_enabled || req->status == -ESHUTDOWN) @@ -225,7 +231,8 @@ static void u_audio_iso_fback_complete(struct usb_ep *ep, __func__, status, req->actual, req->length); u_audio_set_fback_frequency(audio_dev->gadget->speed, - prm->ffback, req->buf); + params->c_srate, prm->pitch, + req->buf); if (usb_ep_queue(ep, req, GFP_ATOMIC)) dev_err(uac->card->dev, "%d Error!\n", __LINE__); @@ -480,9 +487,10 @@ int u_audio_start_capture(struct g_audio *audio_dev) * Always start with original frequency since its deviation can't * be meauserd at start of playback */ - prm->ffback = params->c_srate; + prm->pitch = 1000000; u_audio_set_fback_frequency(audio_dev->gadget->speed, - prm->ffback, req_fback->buf); + params->c_srate, prm->pitch, + req_fback->buf); if (usb_ep_queue(ep_fback, req_fback, GFP_ATOMIC)) dev_err(dev, "%s:%d Error!\n", __func__, __LINE__); @@ -578,12 +586,82 @@ void u_audio_stop_playback(struct g_audio *audio_dev) } EXPORT_SYMBOL_GPL(u_audio_stop_playback); +static int u_audio_pitch_info(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_info *uinfo) +{ + struct uac_rtd_params *prm = snd_kcontrol_chip(kcontrol); + struct snd_uac_chip *uac = prm->uac; + struct g_audio *audio_dev = uac->audio_dev; + struct uac_params *params = &audio_dev->params; + unsigned int pitch_min, pitch_max; + + pitch_min = (1000 - FBACK_SLOW_MAX) * 1000; + pitch_max = (1000 + params->fb_max) * 1000; + + uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER; + uinfo->count = 1; + uinfo->value.integer.min = pitch_min; + uinfo->value.integer.max = pitch_max; + uinfo->value.integer.step = 1; + return 0; +} + +static int u_audio_pitch_get(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct uac_rtd_params *prm = snd_kcontrol_chip(kcontrol); + + ucontrol->value.integer.value[0] = prm->pitch; + + return 0; +} + +static int u_audio_pitch_put(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct uac_rtd_params *prm = snd_kcontrol_chip(kcontrol); + struct snd_uac_chip *uac = prm->uac; + struct g_audio *audio_dev = uac->audio_dev; + struct uac_params *params = &audio_dev->params; + unsigned int val; + unsigned int pitch_min, pitch_max; + int change = 0; + + pitch_min = (1000 - FBACK_SLOW_MAX) * 1000; + pitch_max = (1000 + params->fb_max) * 1000; + + val = ucontrol->value.integer.value[0]; + + if (val < pitch_min) + val = pitch_min; + if (val > pitch_max) + val = pitch_max; + + if (prm->pitch != val) { + prm->pitch = val; + change = 1; + } + + return change; +} + +static const struct snd_kcontrol_new u_audio_controls[] = { +{ + .iface = SNDRV_CTL_ELEM_IFACE_PCM, + .name = "Capture Pitch 1000000", + .info = u_audio_pitch_info, + .get = u_audio_pitch_get, + .put = u_audio_pitch_put, +}, +}; + int g_audio_setup(struct g_audio *g_audio, const char *pcm_name, const char *card_name) { struct snd_uac_chip *uac; struct snd_card *card; struct snd_pcm *pcm; + struct snd_kcontrol *kctl; struct uac_params *params; int p_chmask, c_chmask; int err; @@ -671,6 +749,23 @@ int g_audio_setup(struct g_audio *g_audio, const char *pcm_name, snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &uac_pcm_ops); snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, &uac_pcm_ops); + if (c_chmask && g_audio->in_ep_fback) { + strscpy(card->mixername, card_name, sizeof(card->driver)); + + kctl = snd_ctl_new1(&u_audio_controls[0], &uac->c_prm); + if (!kctl) { + err = -ENOMEM; + goto snd_fail; + } + + kctl->id.device = pcm->device; + kctl->id.subdevice = 0; + + err = snd_ctl_add(card, kctl); + if (err < 0) + goto snd_fail; + } + strscpy(card->driver, card_name, sizeof(card->driver)); strscpy(card->shortname, card_name, sizeof(card->shortname)); sprintf(card->longname, "%s %i", card_name, card->dev->id); diff --git a/drivers/usb/gadget/function/u_audio.h b/drivers/usb/gadget/function/u_audio.h index 53e6baf55cbf..a218cdf771fe 100644 --- a/drivers/usb/gadget/function/u_audio.h +++ b/drivers/usb/gadget/function/u_audio.h @@ -11,6 +11,14 @@ #include <linux/usb/composite.h> +/* + * Same maximum frequency deviation on the slower side as in + * sound/usb/endpoint.c. Value is expressed in per-mil deviation. + * The maximum deviation on the faster side will be provided as + * parameter, as it impacts the endpoint required bandwidth. + */ +#define FBACK_SLOW_MAX 250 + struct uac_params { /* playback */ int p_chmask; /* channel mask */ @@ -23,6 +31,7 @@ struct uac_params { int c_ssize; /* sample size */ int req_number; /* number of preallocated requests */ + int fb_max; /* upper frequency drift feedback limit per-mil */ }; struct g_audio { diff --git a/drivers/usb/gadget/function/u_uac2.h b/drivers/usb/gadget/function/u_uac2.h index 13589c3c805c..179d3ef6a195 100644 --- a/drivers/usb/gadget/function/u_uac2.h +++ b/drivers/usb/gadget/function/u_uac2.h @@ -23,6 +23,7 @@ #define UAC2_DEF_CSSIZE 2 #define UAC2_DEF_CSYNC USB_ENDPOINT_SYNC_ASYNC #define UAC2_DEF_REQ_NUM 2 +#define UAC2_DEF_FB_MAX 5 struct f_uac2_opts { struct usb_function_instance func_inst; @@ -34,6 +35,7 @@ struct f_uac2_opts { int c_ssize; int c_sync; int req_number; + int fb_max; bool bound; struct mutex lock;